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

[stefan.n...@t-online.de via RT]

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 list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev

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

[Kaduk, Ben via RT]

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 behavior, but the intention of the original
check is not particularly well-specified.  The committed version should
be sufficient; there does not seem to be a reason to change it again.

-Ben


___
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev

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

[Matt Caswell via RT]

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: https://mta.openssl.org/mailman/listinfo/openssl-dev

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

[Ben Laurie]

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, 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 as far as I know
> > >> that you're that the pointer should point inside the allocated
> > >> object or 1 byte after it.  So as long as "len" is in the valid
> > >> range, the "buf + len" part should be well defined.  The test with
> > >> -1 is clearly undefined.
> > >>
> > >> As far as I know in the comparison pointers they should point
> > >> to the same object.  But the check seems to imply that they might
> > >> not point to the same object or that buf is not the base of the
> > >> object.  But since len is unsigned only the option that they don't
> > >> point to the same object seems to be left.
> > >>
> > >> So it's unclear to me if this is defined behaviour or not.
> > > So thinking about this some more, it seems to be a check for
> > > undefined behaviour that probably itself is still defined.
> > >
> > > That probably also means the compiler can assume that it's always
> > > false and eliminate the check, it's probably just not smart enough
> > > yet.
> > >
> >
> > I think it is unambiguous that there are values of unsigned char *buf
> > and size_t len for which evaluating (bug + len < buf) invokes undefined
> > behavior -- the sheer act of performing the addition buf + len can
> > produce undefined behavior, even before any comparison.  I am as-yet
> > uncertain whether the case when buf points into the middle of an object
> > and len is (size_t)-1 invokes undefined behavior, but I am inclined to
> > believe that it is also undefined behavior.
>
> Just to clarify what I mean, buf + len can both have defined and
> undefined meaning, it depends on the value of len, so the compiler
> can probably not say anything about that part without knowing all
> the callers of that code.  As long as it has a defined meaning the
> compiler will probably do it.  buf + len < buf also seems to have
> a defined meaning, but in all defined meanings it's false, and so
> the compiler can just optimize that part away.
>
> Anyway, I would really like this to be changed.
>

+1


>
>
> Kurt
>
>
> ___
> openssl-dev mailing list
> To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev
>
___
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev

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

[Ben Laurie via RT]

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, 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 as far as I know
> > >> that you're that the pointer should point inside the allocated
> > >> object or 1 byte after it.  So as long as "len" is in the valid
> > >> range, the "buf + len" part should be well defined.  The test with
> > >> -1 is clearly undefined.
> > >>
> > >> As far as I know in the comparison pointers they should point
> > >> to the same object.  But the check seems to imply that they might
> > >> not point to the same object or that buf is not the base of the
> > >> object.  But since len is unsigned only the option that they don't
> > >> point to the same object seems to be left.
> > >>
> > >> So it's unclear to me if this is defined behaviour or not.
> > > So thinking about this some more, it seems to be a check for
> > > undefined behaviour that probably itself is still defined.
> > >
> > > That probably also means the compiler can assume that it's always
> > > false and eliminate the check, it's probably just not smart enough
> > > yet.
> > >
> >
> > I think it is unambiguous that there are values of unsigned char *buf
> > and size_t len for which evaluating (bug + len < buf) invokes undefined
> > behavior -- the sheer act of performing the addition buf + len can
> > produce undefined behavior, even before any comparison.  I am as-yet
> > uncertain whether the case when buf points into the middle of an object
> > and len is (size_t)-1 invokes undefined behavior, but I am inclined to
> > believe that it is also undefined behavior.
>
> Just to clarify what I mean, buf + len can both have defined and
> undefined meaning, it depends on the value of len, so the compiler
> can probably not say anything about that part without knowing all
> the callers of that code.  As long as it has a defined meaning the
> compiler will probably do it.  buf + len < buf also seems to have
> a defined meaning, but in all defined meanings it's false, and so
> the compiler can just optimize that part away.
>
> Anyway, I would really like this to be changed.
>

+1


>
>
> Kurt
>
>
> ___
> openssl-dev mailing list
> To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev
>

___
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev

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

[Kurt Roeckx]

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 as far as I know
that you're that the pointer should point inside the allocated
object or 1 byte after it.  So as long as "len" is in the valid
range, the "buf + len" part should be well defined.  The test with
-1 is clearly undefined.

As far as I know in the comparison pointers they should point
to the same object.  But the check seems to imply that they might
not point to the same object or that buf is not the base of the
object.  But since len is unsigned only the option that they don't
point to the same object seems to be left.

So it's unclear to me if this is defined behaviour or not.


Kurt

___
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev

Re: [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


___
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev

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

[Kurt Roeckx via RT]

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 but it *is* well defined.
> 
> The defined behaviour for the "buf + len" part is as far as I know
> that you're that the pointer should point inside the allocated
> object or 1 byte after it.  So as long as "len" is in the valid
> range, the "buf + len" part should be well defined.  The test with
> -1 is clearly undefined.
> 
> As far as I know in the comparison pointers they should point
> to the same object.  But the check seems to imply that they might
> not point to the same object or that buf is not the base of the
> object.  But since len is unsigned only the option that they don't
> point to the same object seems to be left.
> 
> So it's unclear to me if this is defined behaviour or not.

So thinking about this some more, it seems to be a check for
undefined behaviour that probably itself is still defined.

That probably also means the compiler can assume that it's always
false and eliminate the check, it's probably just not smart enough
yet.


Kurt


___
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev

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

[Matt Caswell via RT]

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

I'd say that is an instance of the compiler knowing better than us how
big |len| would have to be in order to trigger an overflow. Those rules
are going to be platform specific so we should not attempt to second
guess them, but instead let the optimiser do its job.

Matt


___
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev

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

[Kaduk, Ben via RT]

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 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.
> I'd say that is an instance of the compiler knowing better than us how
> big |len| would have to be in order to trigger an overflow. Those rules
> are going to be platform specific so we should not attempt to second
> guess them, but instead let the optimiser do its job.
>

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.

-Ben

P.S. If you haven't encountered it yet,
http://blog.regehr.org/archives/213 et. seq. make for fun reading.


___
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev

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

[Ben Laurie]

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 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.
>
> I'd say that is an instance of the compiler knowing better than us how
> big |len| would have to be in order to trigger an overflow. Those rules
> are going to be platform specific so we should not attempt to second
> guess them, but instead let the optimiser do its job.
>

If it is, then the compiler is wrong, surely? e.g. if buf is 0xfff...fff,
and len is 1, you get an overflow, which the optimised version does not
catch.

What I'm not understanding from this thread is what the argument is against
avoiding undefined behaviour?
___
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev

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

[Ben Laurie via RT]

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 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.
>
> I'd say that is an instance of the compiler knowing better than us how
> big |len| would have to be in order to trigger an overflow. Those rules
> are going to be platform specific so we should not attempt to second
> guess them, but instead let the optimiser do its job.
>

If it is, then the compiler is wrong, surely? e.g. if buf is 0xfff...fff,
and len is 1, you get an overflow, which the optimised version does not
catch.

What I'm not understanding from this thread is what the argument is against
avoiding undefined behaviour?

___
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev

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

[Alexander Cherepanov via RT]

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 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.
>>
>> I'd say that is an instance of the compiler knowing better than us how
>> big |len| would have to be in order to trigger an overflow. Those rules
>> are going to be platform specific so we should not attempt to second
>> guess them, but instead let the optimiser do its job.

Matt, I'm confused. In your previous email you yourself (correctly) 
explained why this check does not guard against the pointer overflowing.

AIUI this check is not some clever trick, it's just ordinary 
simplification of "a + b < a" into "b < 0" by subtracting a common term 
from both sides (which is correct only if there is no overflow) with an 
additional twist that an unsigned integer are treated as signed. (IMHO 
this is a bug in compilers and I've just reported it in gcc -- 
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67999. But it doesn't 
really matter for our discussion.)

On 2015-10-17 01:46, Ben Laurie via RT wrote:
> If it is, then the compiler is wrong, surely? e.g. if buf is 0xfff...fff,
> and len is 1, you get an overflow, which the optimised version does not
> catch.

Right.

> What I'm not understanding from this thread is what the argument is against
> avoiding undefined behaviour?

I guess it's not clear what is the best way to fix it.

You cannot check for a pointer overflow directly. There is no such 
notion in the C standards. Perhaps it's possible with casts to uintptr_t


-- 
Alexander Cherepanov


___
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev

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

[Kurt Roeckx via RT]

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 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 as far as I know
> >> that you're that the pointer should point inside the allocated
> >> object or 1 byte after it.  So as long as "len" is in the valid
> >> range, the "buf + len" part should be well defined.  The test with
> >> -1 is clearly undefined.
> >>
> >> As far as I know in the comparison pointers they should point
> >> to the same object.  But the check seems to imply that they might
> >> not point to the same object or that buf is not the base of the
> >> object.  But since len is unsigned only the option that they don't
> >> point to the same object seems to be left.
> >>
> >> So it's unclear to me if this is defined behaviour or not.
> > So thinking about this some more, it seems to be a check for
> > undefined behaviour that probably itself is still defined.
> >
> > That probably also means the compiler can assume that it's always
> > false and eliminate the check, it's probably just not smart enough
> > yet.
> >
> 
> I think it is unambiguous that there are values of unsigned char *buf
> and size_t len for which evaluating (bug + len < buf) invokes undefined
> behavior -- the sheer act of performing the addition buf + len can
> produce undefined behavior, even before any comparison.  I am as-yet
> uncertain whether the case when buf points into the middle of an object
> and len is (size_t)-1 invokes undefined behavior, but I am inclined to
> believe that it is also undefined behavior.

Just to clarify what I mean, buf + len can both have defined and
undefined meaning, it depends on the value of len, so the compiler
can probably not say anything about that part without knowing all
the callers of that code.  As long as it has a defined meaning the
compiler will probably do it.  buf + len < buf also seems to have
a defined meaning, but in all defined meanings it's false, and so
the compiler can just optimize that part away.

Anyway, I would really like this to be changed.


Kurt


___
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev

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

[Viktor Dukhovni]

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

-- 
Viktor.
___
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev

Re: [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 +, 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


___
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev

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

[Emilia Käsper via RT]

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

Except, yeah, it relies on undefined behaviour. OTOH as you note we do have a
test for this and we've not seen it fail on any compiler.

I agree the check is pointless if your sizes are correctly represented as
size_t throughout, but perhaps marginally useful for OpenSSL in its current
state. I don't feel too strongly about keeping or removing it - what do others
think?

___
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev

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

[Matt Caswell via RT]

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;
> 
> pkt->curr = buf; pkt->remaining = len; return 1; }
> 
> Link:
> https://github.com/openssl/openssl/blob/310115448188415e270bb0bef958c7c130939838/ssl/packet_locl.h#L113
>
>  The rules of pointer arithmetics are such that the condition (buf +
> len < buf) is always false when it is defined. A modern compiler, or
> even an old compiler, could suppress the entire conditional from the
> generated code without a second thought and without a warning. Please
> read https://lwn.net/Articles/278137/ . Note that in that very
> similar instance, the GCC developers did not acknowledge any bug in
> their compiler, did not change the compiler's behavior, and simply
> regretted that "their project has been singled out in an unfair
> way".
> 
> That LWN story is not a about a compiler bug, or in any case, it is
> about a compiler bug that is here to stay. And according to GCC
> developers, to be common to several C compilers.

The LWN story is about secure coding, and a specific pitfall to be aware
of which may compile away a test for buffer underflow. Specifically they
are talking about a length value received from an untrusted source and
testing that it falls within the bounds of a buffer.

That is *not* the case here. This test is not about adding a critical
security check. The contract that PACKET_buf_init provides to code using
it requires that the buffer provided must be (at least) |len| bytes
long. It is for the calling code to perform this necessary security
checks prior to calling 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. 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.


> 
> As far as I can tell, no current compiler recognizes that the very
> same reasoning as in that old LWN story justifies the suppression of
> the conditional. I tested the compilers currently available on
> https://gcc.godbolt.org . However, any compiler willing to miscompile
> the examples in the LWN article would gladly miscompile the function
> PACKET_buf_init given the chance.
> 
> If the function is intended to return 0 for large values of len, then
> the test should look like:
> 
> if (len > (size_t)(SIZE_MAX / 2)) ...
> 
> Here I have chosen a constant that happens to give a behavior close
> to the one obtained by luck with current compilers. If another
> constant makes sense, then that other constant can be used. The
> current implementation is an invitation for the compiler to generate
> code that, even when len is above the limit, sets pkt->curr to buf,
> sets pkt->remaining to len, and returns 1, which is not what is
> intended, and could have serious security-related consequences latter
> on.

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 0xff plus 5 bytes of message header, i.e. 0x104. 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.


> 
> If, as the comment implies, the function is not intended to be called
> with a len so large that it causes (uintptr_t)buf + len to be lower
> than (uintptr_t)buf, then please, please, please simplify the
> function by removing this nonsensical "sanity check", and make this
> function, which cannot fail, return void-as the history of the rest
> of OpenSSL shows, remembering of testing all error codes returned by
> called functions is difficult enough, even when only functions that
> have reason to fail return such codes.

You are correct that this has historically been a problem. All the dev
team use the "--strict-warnings" parameter to config. One of the effects
of this is to treat warnings as errors and this will therefore fail on
any function declared with "__owur" where the return value is unused. We
should ensure that PACKET_buf_init is declared with this (and I believe
Emilia is making that change). Therefore this should not be an issue for
this code.

> 
> PACKET_buf_init is called with (size_t)-1 for len in
> 

Re: [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
>


___
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev

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

[Salz, Rich]

> 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 things means that it should be an 
assert.
___
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev

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

[Matt Caswell via RT]

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
>> fail.
> 
> I would say that the combination of these two things means that it should be 
> an assert.

assert has its uses, but it depends what you are trying to achieve. If
you are developing new code and trying to track down a difficult to find
problem - very useful. Even potentially if you are trying to track down
a difficult issue in a production scenario, you could create a special
debug build to help you pinpoint what is going wrong. I'm sure there are
other scenarios which would be excellent uses of assert.

IMO this test is about protecting ourselves in the unlikely event that a
programmer error does not present itself until production. To protect
production code assert is useless because it gets compiled away.

If we are concerned about potential programmer errors not appearing
until we get into production then assert is not the right tool. You
could argue that you should use OPENSSL_assert(), but my views on that
are well known. OPENSSL_assert calls abort() even in production which is
wrong IMO and potentially dangerous. I know opinions differ on that
point within the dev team and this ticket is probably not the place to
reopen that particular debate. Suffice it to say I would not support the
use of OPENSSL_assert here.

Matt


___
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev

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

[Salz, Rich via RT]

> 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 things means that it should be an 
assert.


___
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev

Re: [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 0xff plus 5 bytes of message header, i.e. 0x104. 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


___
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev

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

[Kaduk, Ben via RT]

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 for (len == (size_t)-1) is the right thing to do.

-Ben


___
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev

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

[Pascal Cuoq via RT]

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;
return 1;
}

Link: 
https://github.com/openssl/openssl/blob/310115448188415e270bb0bef958c7c130939838/ssl/packet_locl.h#L113

The rules of pointer arithmetics are such that the condition (buf + len < buf) 
is always false when it is defined. A modern compiler, or even an old compiler, 
could suppress the entire conditional from the generated code without a second 
thought and without a warning. Please read https://lwn.net/Articles/278137/ . 
Note that in that very similar instance, the GCC developers did not acknowledge 
any bug in their compiler, did not change the compiler's behavior, and simply 
regretted that "their project has been singled out in an unfair way".

That LWN story is not a about a compiler bug, or in any case, it is about a 
compiler bug that is here to stay. And according to GCC developers, to be 
common to several C compilers.

As far as I can tell, no current compiler recognizes that the very same 
reasoning as in that old LWN story justifies the suppression of the 
conditional. I tested the compilers currently available on 
https://gcc.godbolt.org . However, any compiler willing to miscompile the 
examples in the LWN article would gladly miscompile the function 
PACKET_buf_init given the chance.

If the function is intended to return 0 for large values of len, then the test 
should look like:

if (len > (size_t)(SIZE_MAX / 2)) ...

Here I have chosen a constant that happens to give a behavior close to the one 
obtained by luck with current compilers. If another constant makes sense, then 
that other constant can be used. The current implementation is an invitation 
for the compiler to generate code that, even when len is above the limit, sets 
pkt->curr to buf, sets pkt->remaining to len, and returns 1, which is not what 
is intended, and could have serious security-related consequences latter on.

If, as the comment implies, the function is not intended to be called with a 
len so large that it causes (uintptr_t)buf + len to be lower than 
(uintptr_t)buf, then please, please, please simplify the function by removing 
this nonsensical "sanity check", and make this function, which cannot fail, 
return void-as the history of the rest of OpenSSL shows, remembering of testing 
all error codes returned by called functions is difficult enough, even when 
only functions that have reason to fail return such codes.

PACKET_buf_init is called with (size_t)-1 for len in test/packettest.c:

https://github.com/openssl/openssl/blob/310115448188415e270bb0bef958c7c130939838/test/packettest.c#L341

Since PACKET_buf_init could no longer fail, that test could be simplified, 
which is good.

If a sanity check is indispensable in PACKET_buf_init, but is really a check 
for something not supposed to happen, then in order to keep the type returned 
by the function as void, the check could be expressed as:

if (len > (size_t)(SIZE_MAX / 2)) abort();

or

assert (len <= (size_t)(SIZE_MAX / 2));



___
openssl-bugs-mod mailing list
openssl-bugs-...@openssl.org
https://mta.openssl.org/mailman/listinfo/openssl-bugs-mod___
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev