On Sat, Nov 10, 2018 at 6:55 AM Paul Mackerras <paulus@xxxxxxxxxx> wrote:
>
> 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.
>
I'll specify in the help message that the string must be in hex format
> 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.
Right. I'll add a check against PPPoETag.payload size.
I'm sending a v4
--
Matteo Croce
per aspera ad upstream
