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

[Matt Caswell via RT]

On 15/10/15 04:11, Pascal Cuoq via RT wrote:
> 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.

The LWN story is about secure coding, and a specific pitfall to be aware
of which may compile away a test for buffer underflow. Specifically they
are talking about a length value received from an untrusted source and
testing that it falls within the bounds of a buffer.

That is *not* the case here. This test is not about adding a critical
security check. The contract that PACKET_buf_init provides to code using
it requires that the buffer provided must be (at least) |len| bytes
long. It is for the calling code to perform this necessary security
checks prior to calling PACKET_buf_init. This code can assume that |len|
is from a trusted source.

The purpose of the sanity check is not then for security, but to guard
against programmer error. For a correctly functioning program this test
should never fail. For an incorrectly functioning program it may do. It
is not guaranteed to fail because the test could be compiled away but,
most likely, it will. We can have some degree of confidence that the
test works and does not get compiled away in most instances because, as
you point out, an explicit check for it appears in packettest.c and, to
date, we have had no reported failures.


> 
> 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.

The biggest packet that I can think of that we will ever have to deal
with within libssl would be a handshake message. This has a maximum
length of 0xffffff plus 5 bytes of message header, i.e. 0x1000004. There
could be an argument to say we should test against this to ensure that
|len| is not too big.

However that does not necessarily guard against the pointer overflowing.
In an incorrect program where len is just a bit bigger than the actual
size of buf this could, theoretically, be sufficient to overflow the
pointer.


> 
> 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.

You are correct that this has historically been a problem. All the dev
team use the "--strict-warnings" parameter to config. One of the effects
of this is to treat warnings as errors and this will therefore fail on
any function declared with "__owur" where the return value is unused. We
should ensure that PACKET_buf_init is declared with this (and I believe
Emilia is making that change). Therefore this should not be an issue for
this code.

> 
> PACKET_buf_init is called with (size_t)-1 for len in
> test/packettest.c:

The issue you are raising is actually problematic for this test. We've
not had a reported issue with it, but technically it is relying on
undefined behaviour. If we additionally added the maximum length check
then this would still pass, even if the underflow check got compiled away.


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

I do not agree that this is a good approach. Opinions on this differ
within the dev team, however IMO we should never call abort in a
library. I have seen too many instances where "should never happen"
events which resulted in an abort did actually happen, and could be
forced to happen under attacker control thus leading to a DoS. So whilst
we do not rely on the test in question for adding security, aborting if
it fails could lead to a CVE.

In summary my opinion is:
- I believe the sanity check does have some value in guarding against
programmer error
- If it were to be compiled away this does not have a detrimental impact
on security (it just increases the likelihood of a crash in the event of
a programmer error)
- There could be a good argument for adding an additional maximum length
check
- I do not believe the function should be made void

Matt