As of 2015-10-14, the function PACKET_buf_init in ssl/packet_locl.h reads:
static inline int PACKET_buf_init(PACKET *pkt, unsigned char *buf, size_t len)
{
/* Sanity check for negative values. */
if (buf + len < buf)
return 0;
pkt->curr = buf;
pkt->remaining = len;
return 1;
}
Link:
https://github.com/openssl/openssl/blob/310115448188415e270bb0bef958c7c130939838/ssl/packet_locl.h#L113
The rules of pointer arithmetics are such that the condition (buf + len < buf)
is always false when it is defined. A modern compiler, or even an old compiler,
could suppress the entire conditional from the generated code without a second
thought and without a warning. Please read https://lwn.net/Articles/278137/ .
Note that in that very similar instance, the GCC developers did not acknowledge
any bug in their compiler, did not change the compiler's behavior, and simply
regretted that "their project has been singled out in an unfair way".
That LWN story is not a about a compiler bug, or in any case, it is about a
compiler bug that is here to stay. And according to GCC developers, to be
common to several C compilers.
As far as I can tell, no current compiler recognizes that the very same
reasoning as in that old LWN story justifies the suppression of the
conditional. I tested the compilers currently available on
https://gcc.godbolt.org . However, any compiler willing to miscompile the
examples in the LWN article would gladly miscompile the function
PACKET_buf_init given the chance.
If the function is intended to return 0 for large values of len, then the test
should look like:
if (len > (size_t)(SIZE_MAX / 2)) ...
Here I have chosen a constant that happens to give a behavior close to the one
obtained by luck with current compilers. If another constant makes sense, then
that other constant can be used. The current implementation is an invitation
for the compiler to generate code that, even when len is above the limit, sets
pkt->curr to buf, sets pkt->remaining to len, and returns 1, which is not what
is intended, and could have serious security-related consequences latter on.
If, as the comment implies, the function is not intended to be called with a
len so large that it causes (uintptr_t)buf + len to be lower than
(uintptr_t)buf, then please, please, please simplify the function by removing
this nonsensical "sanity check", and make this function, which cannot fail,
return void-as the history of the rest of OpenSSL shows, remembering of testing
all error codes returned by called functions is difficult enough, even when
only functions that have reason to fail return such codes.
PACKET_buf_init is called with (size_t)-1 for len in test/packettest.c:
https://github.com/openssl/openssl/blob/310115448188415e270bb0bef958c7c130939838/test/packettest.c#L341
Since PACKET_buf_init could no longer fail, that test could be simplified,
which is good.
If a sanity check is indispensable in PACKET_buf_init, but is really a check
for something not supposed to happen, then in order to keep the type returned
by the function as void, the check could be expressed as:
if (len > (size_t)(SIZE_MAX / 2)) abort();
or
assert (len <= (size_t)(SIZE_MAX / 2));
_______________________________________________
openssl-bugs-mod mailing list
openssl-bugs-...@openssl.org
https://mta.openssl.org/mailman/listinfo/openssl-bugs-mod
_______________________________________________
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev
