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

[Matt Caswell via RT]

On 16/10/15 17:32, Viktor Dukhovni wrote:
> On Fri, Oct 16, 2015 at 04:09:57PM +0000, Kaduk, Ben via RT wrote:
> 
>> I hope I am not dragging this thread on too long, but with all due
>> respect, we are not asking the compiler/optimizer to detect overflow --
>> we are asking the compiler to instantiate undefined behavior in a way
>> that is convenient for us.  This will only happen by chance, as a side
>> effect of some other decisions made by the compiler authors, in the
>> present state of compiler development.
> 
> My take is that we should generally stay clear of relying on any
> remotely sensible outcome for undefined behaviour.  If this thread
> is about such a situation, then we may have to code around the
> issue.
> 

We are *not* relying on that. Or at least we are only to the extent that
we are talking about a scenario where something has gone wrong already
and undefined behaviour is inevitable. We are hoping that our undefined
behaviour is likely to be slightly less bad than the undefined behaviour
that we would get otherwise. We cannot know that it will be...but in
reality there is a reasonable chance that it will.

In a well-behaved program there is no undefined behaviour. The "buf +
len < buf" check will always evaluate to false, so in that sense is
useless but it *is* well defined.

In a non well-behaved program if we remove the check then undefined
behaviour is almost certainly inevitable. Who don't know what it will do
(it could do anything), but there is a reasonable chance that it could
result in the disclosure or use of memory it shouldn't be touching. A
CVE is quite a possible result of such undefined behaviour.

In a non well-behaved program if we keep the check then we still have
undefined behaviour. The check could do what we kind of expect that it
might, it might do nothing at all, or it could do something entirely
different. But without the check undefined behaviour is inevitable
anyway, so in that sense we are no better off one way or the other. In
reality however we have a reasonable hope that the check will do
something like what we hope it might, in which case we will prevent a
possible security issue.

That said the packettest test where we deliberately use -1 for a len, is
actually deliberately relying on undefined behaviour so perhaps should
be changed. It may also be reasonable to add an additional max length check.

Matt