Re: Remote Pre-Auth Buffer Overflow in GNU Inetutils telnetd (LINEMODE SLC)

[Collin Funk <collin.funk1 () gmail com>]

Solar Designer <solar () openwall com> writes:

> Thank you, Justin!
>
> In cases like this, we should be bringing the entire report to
> oss-security, not just a link.  So I'll include it below.
>
> Further in the above thread, there's a link to a fix pull request by
> Collin Funk.  I didn't review it in full context, but even within the
> patch context it fails my review:
>
> add_slc (char func, char flag, cc_t val)
> {
>
>   /* Do nothing if the entire triplet cannot fit in the buffer.  */
>   if (slcbuf + sizeof slcbuf <= slcptr + 6)
>     return;
>
>   if ((*slcptr++ = (unsigned char) func) == 0xff)
>     *slcptr++ = 0xff;
>
>   if ((*slcptr++ = (unsigned char) flag) == 0xff)
>     *slcptr++ = 0xff;
>
>   if ((*slcptr++ = (unsigned char) val) == 0xff)
>     *slcptr++ = 0xff;
>
> }                             /* end of add_slc */
>
> In "slcptr + 6", it appears to rely on pointer math working outside of
> the object, but that's UB in C.  If the C compiler concludes that the
> "if" condition cannot be true within defined behavior, it is free to
> optimize the entire "if" and "return" out.

CC'ing bug-gnulib. Do we make any assumptions about this behavior in
Gnulib? I know we generally assume systems are more sane than ISO C
requires. E.g. no holes in integers, flat address space, etc. Perhaps it
is worth another bullet point in our documentation [1].

> A proper check may be:
>
>   if (slcbuf + sizeof slcbuf - 6 <= slcptr)
>
> or perhaps with "<" in place of "<=", unless we need an extra element
> for some reason.

Yeah, that works. Thanks.

Collin

[1] https://www.gnu.org/software/gnulib/manual/gnulib.html#Other-portability-assumptions-made-by-Gnulib