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

Hi, Wouldn't if ( UINTPTR_MAX - (uintptr_t) buffer < len) be closer to the intention of the original check? Or is this undefined behaviour as well and I stupidly missed that fact? Regards, Stefan ___ openssl-dev mailing

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

On 10/22/2015 01:02 PM, stefan.n...@t-online.de via RT wrote: > Hi, > > Wouldn't > if ( UINTPTR_MAX - (uintptr_t) buffer < len) > be closer to the intention of the original check? > Or is this undefined behaviour as well and I > stupidly missed that fact? > That appears to be defined

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

There seems a strong consensus to change this so: https://github.com/openssl/openssl/commit/3fde6c9276c9cd6e56e8e06e756350a4fbdd7031 Closing this ticket. Matt ___ openssl-dev mailing list To unsubscribe:

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

On Sat, 17 Oct 2015 at 06:35 Kurt Roeckx via RT wrote: > On Fri, Oct 16, 2015 at 09:44:22PM +, Kaduk, Ben via RT wrote: > > On 10/16/2015 04:35 PM, Kurt Roeckx via RT wrote: > > > On Fri, Oct 16, 2015 at 06:50:36PM +, Kurt Roeckx via RT wrote: > > >> On Fri, Oct 16,

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

On Sat, 17 Oct 2015 at 06:35 Kurt Roeckx via RT wrote: > On Fri, Oct 16, 2015 at 09:44:22PM +, Kaduk, Ben via RT wrote: > > On 10/16/2015 04:35 PM, Kurt Roeckx via RT wrote: > > > On Fri, Oct 16, 2015 at 06:50:36PM +, Kurt Roeckx via RT wrote: > > >> On Fri, Oct 16,

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

On Fri, Oct 16, 2015 at 04:50:59PM +, Matt Caswell via RT wrote: > 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. The defined behaviour for the "buf + len" part is

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

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

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

On Fri, Oct 16, 2015 at 06:50:36PM +, Kurt Roeckx via RT wrote: > On Fri, Oct 16, 2015 at 04:50:59PM +, Matt Caswell via RT wrote: > > 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

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

On 15/10/15 20:53, Alexander Cherepanov via RT wrote: > 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

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

On 10/16/2015 03:32 AM, Matt Caswell via RT wrote: > > On 15/10/15 20:53, Alexander Cherepanov via RT wrote: >> 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

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

On Fri, 16 Oct 2015 at 01:32 Matt Caswell via RT wrote: > > > On 15/10/15 20:53, Alexander Cherepanov via RT wrote: > > 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

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

On Fri, 16 Oct 2015 at 01:32 Matt Caswell via RT wrote: > > > On 15/10/15 20:53, Alexander Cherepanov via RT wrote: > > 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

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

On 2015-10-17 01:46, Ben Laurie via RT wrote: > On Fri, 16 Oct 2015 at 01:32 Matt Caswell via RT wrote: >> On 15/10/15 20:53, Alexander Cherepanov via RT wrote: >>> On 2015-10-15 15:41, Matt Caswell via RT wrote: The purpose of the sanity check is not then for security, but

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

On Fri, Oct 16, 2015 at 09:44:22PM +, Kaduk, Ben via RT wrote: > On 10/16/2015 04:35 PM, Kurt Roeckx via RT wrote: > > On Fri, Oct 16, 2015 at 06:50:36PM +, Kurt Roeckx via RT wrote: > >> On Fri, Oct 16, 2015 at 04:50:59PM +, Matt Caswell via RT wrote: > >>> In a well-behaved program

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

On Fri, Oct 16, 2015 at 04:09:57PM +, 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

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

On 16/10/15 17:32, Viktor Dukhovni wrote: > On Fri, Oct 16, 2015 at 04:09:57PM +, 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

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

Given OpenSSL's eternal type confusion, this check is meant to trap callers that get an error return (typically -1) from some API returning signed values and pass that on to PACKET_buf_init as a size_t. For example, ssl3_get_message returns a long to signal buffer length, and that makes me

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

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

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

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

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

> 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. I would say that the combination of these two

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

On 15/10/15 14:35, Salz, Rich via RT wrote: > >> 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 >>

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

> 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. I would say that the combination of these two

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

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

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

On 10/15/2015 05:44 AM, Emilia Käsper via RT wrote: > Given OpenSSL's eternal type confusion, this check is meant to trap callers > that get an error return (typically -1) from some API returning signed values > Hmm, do we have a sense for how typically "typically" is? Maybe just adding a check

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

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;