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.
