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

[Kaduk, Ben via RT]

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

Strictly speaking, it is not a matter of "is the check left as-is" vs.
"is the check compiled away".  C's undefined behavior rules are pretty
open-ended, and the compiler is free to generate code such that, if
inputs that would trigger that check were supplied, does absolutely
anything at all.  Absolutely anything at all means just that; it does
not need to be limited to the local scope and could include exiting from
the program or also reading from /etc/ssh/ssh_host_rsa_key and sending
it over the network.  Now, the compiler is unlikely to do something
"interesting" like that, since it would be at odds with the compiler's
goal of producing fast code, but relying on that does not exactly make
me comfortable.

(N.B. this is not the common case of signed integer overflow that's easy
to reason about; pointer arithmetic has its own rules for undefined
behavior that get invoked when the resulting pointer would not point to
inside (or one past) the same array object that the starting pointer
pointed inside.  This happens in many, many, many more cases than the
current check would catch.  Section 6.5.6 of n1256.pdf covers this topic.)

-Ben

> - There could be a good argument for adding an additional maximum length
> check
> - I do not believe the function should be made void
>