[openssl-dev] [openssl.org #4094] Nonsensical pointer comparison in PACKET_buf_init

[Pascal Cuoq via RT]

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));



-------------- next part --------------
_______________________________________________
openssl-bugs-mod mailing list
openssl-bugs-mod at openssl.org
https://mta.openssl.org/mailman/listinfo/openssl-bugs-mod