From: Colin Percival (cpercivafreebsd.org)
Date: Thu Mar 31 2005 - 16:36:56 CST

• Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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
_______________________________________________
freebsd-securityfreebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-security
To unsubscribe, send any mail to "freebsd-security-unsubscribefreebsd.org"

• Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]