FreeBSD Security Advisory FreeBSD-SA-05:01.telnet

[Roberto]

> Steve Kiernan wrote:
>> I was looking at this patch, but there seems to be an error in it:
>>
>>  unsigned char slc_reply[128];
>> +unsigned char const * const slc_reply_eom =
>> &slc_reply[sizeof(slc_reply)];
>>  unsigned char *slc_replyp;
>>
>> Should the value for slc_reply_eom not be this instead?
>>
>> unsigned char const * const slc_reply_eom = &slc_reply[sizeof(slc_reply)
>> - 1];
>
> No.
>
>> Considering the conditionals are the following:
>>
>> +       if (&slc_replyp[6+2] > slc_reply_eom)
>> +               return;
>>
>> .. and ..
>>
>> +    /* The end of negotiation command requires 2 bytes. */
>> +    if (&slc_replyp[2] > slc_reply_eom)
>> +            return;
>>
>> If you don't subtract 1 from the sizeof(slc_reply) or change the
>> conditional operators to >=, then you could try to write one byte past
>> the end of the buffer.
>
> The tests are written a bit oddly, but I'm fairly certain that they
> are correct.  &slc_replyp[6+2] and &slc_replyp[2] are not the
> addresses of the last bytes which will be written; rather, they are
> the addresses of the byte after the last byte which will be written.
>
> Taking the second example, if slc_replyp == slc_reply + 126, then we
> will have &slc_replyp[2] == slc_reply_eom, but (looking at the code)
> the two final bytes will be written into slc_reply[126] and
> slc_reply[127].
>
> Colin Percival

Actually I've not read the code, but from these email it seems to me that
someone could be confused by this code (at least Steve and I); for example
refer to the address "&slc_reply[128];" when slc_reply[127] is the last
element.

I do not want to be offensive in any way, what I want to say is that this
code is clear to you (and the person who wrote it) but the next programmer
that will reuse the code (because this is a open source) could make a
mistake.

I think many bugs can derive from code not easy to understand.

This is only my opinion.

Kind Regards,
Roberto