On Fri, 1 Apr 2005, Roberto wrote:
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.
Actually, they are wrong. Pointers can only be compared (without the
comparision giving undefined behaviour) iff they are into the same
array (or one is null). There is a special case for the element one past
the end of the array (slc_reply_eom here). This is a valid pointer,
and some comparisons with it are valid, but ones like
(&slc_replyp[2] > slc_reply_eom) are nonsense since if &slc_replyp[2]
is valid then the result of the comparison is 0. The undefined behaviour
just happens to be to work on most machines, because there are usually
some bytes in the same address space beyond the end of the array.
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].
Then both slc_replyp and &slp_replyp[2] are valid, so there is no problem.
If slc_replyp == slc_reply + 127, then &slc_replyp[2] is garbage and
referencing it gives undefined behaviour (slightly before the comparison
gives it).
This is essentially an example of how not to do error overflow checking:
check for it after it may have occurred.
The correct code is something like:
/* The end of negotiation command requires 2 bytes. */
if (slc_reply_eom - slp_replyp < 2)
return;
This depends on slc_replyp being valid. Previous code should have ensured
this. To be valid, it must point into some array of unsigned chars, and
that array must be slc_reply. If it points into a differentn array of
unsigned chars, then undefined behaviour doesn't occur until we compare it.
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.
Having an "end" address of 1 after the last element is normal and good.
This address is a bit hard to compare with correctly, but comparing with
the address of the last element is harder. The fixed version of the
above would be:
/*
* The end of negotiation command requires 2 bytes. */
*/
if (slc_reply_lastp - slp_replyp < 2 - 1)
return;
Note that the pointer difference is -1 if slp_replyp happens to be the
"end" address, so it is important that the size in the right hand operand
is a signed integer. If the amount of space required was sizeof(foo),
then we would have to convert it to a signed integer; if it was #defined,
the we would have to convert it similarly and worry more about the conversion
overflowing.
Bruce
_______________________________________________
freebsd-security@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-security
To unsubscribe, send any mail to "[EMAIL PROTECTED]"
