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

[Alexander Cherepanov via RT]

On 2015-10-15 15:41, Matt Caswell via RT wrote:
> 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.

What was not entirely clear from the original bug report is that, while 
the check is not compiled away, it's compiled into something completely 
different from what is written in the source. Specifically, the check 
"buf + len < buf" is optimized into "len >> 63" on 64-bit platform, i.e. 
"(ssize_t)len < 0" or "len > SIZE_MAX / 2". This is not a check for 
overflow at all, it doesn't even depend on the value of "buf".

If this is what was intended then it's better to write it explicitly. If 
this is not what was intended then some other approach is required.

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

Right. That's exactly one of the problem with the current code the way 
it is compiled by optimizing compilers.

A couple of other remarks.

1. The mere fact that the (sub)expression "buf + len" is evaluated 
unconditionally permits a compiler to assume that this is a valid 
pointer (it's UB otherwise) and hence that it points inside an array. 
This, in turn, permits the compiler to remove some other checks against 
"buf + len" or "len" even if they are well-written. I'm not saying that 
any of current compilers can do it, I'm just saying that undefined 
behavior is a very delicate thing and it's better not to have it in your 
program.

2. If you want to simplify checking openssl for undefined behavior it's 
better to fix even non-critical cases of it. Otherwise everybody have to 
research it, to maintain their blacklists of things to ignore etc. It's 
similar to compiler warnings: if you want to get more value from 
compiler warnings you have to keep your project warnings-free fixing 
even minor and false ones.

HTH

-- 
Alexander Cherepanov