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

[Kaduk, Ben via RT]

On 10/16/2015 11:50 AM, Matt Caswell via RT wrote:
>
> On 16/10/15 17:32, Viktor Dukhovni wrote:
>> 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.

I think we can have a check in the function that blocks (most of) the
cases we are worried about encountering undefined behavior for, without
actually involving undefined behavior, which at least resembles the best
of both worlds.

Does anyone object to changing the sanity check to be comparing len
against (size_1)-1?  Alternately, checking the range (size_t)-255 to
(size_t)-1?

My personal preference would be to make the function return void and
have this sanity check be an assert() or explicit abort(), but I would
not object to the above given your preference to retain a sanity check.

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

It would not rely on undefined behavior with my proposal above.  Of
course, a max length check would supersede my proposal; however, I think
that the PACKET structure may well eventually be used for processing
things other than TLS wire protocol packets, so I would suggest a limit
at least somewhat higher than 2^14+2048.

-Ben