On Thu, Nov 08, 2018 at 12:52:43AM +0100, Matteo Croce wrote: > Add pppoe 'host-uniq' option to set an arbitrary > host-uniq tag instead of the pppd pid. > Some ISPs use such tag to authenticate the CPE, > so it must be set to a proper value to connect. In the absence of a man page for rp-pppoe, I'd like to see a description of the new option and the syntax for its argument in the patch description. And unfortunately, I think there is a user-triggerable buffer overrun that you've added: > +static inline int parseHostUniq(const char *uniq, PPPoETag *tag) > +{ > + unsigned i, len = strlen(uniq); > + > +#define hex(x) \ > + (((x) <= '9') ? ((x) - '0') : \ > + (((x) <= 'F') ? ((x) - 'A' + 10) : \ > + ((x) - 'a' + 10))) > + > + if (!len || len & 1) > + return 0; > + > + for (i = 0; i < len; i += 2) > + { > + if (!isxdigit(uniq[i]) || !isxdigit(uniq[i+1])) > + return 0; > + > + tag->payload[i / 2] = (char)(hex(uniq[i]) << 4 | hex(uniq[i+1])); > + } > + > +#undef hex > + > + tag->type = htons(TAG_HOST_UNIQ); > + tag->length = htons(len / 2); > + return 1; > +} > + If I give a sufficiently long argument to the host-uniq option, I can overflow conn->hostUniq and write whatever I want into whatever follows it in the heap, can't I? Paul.