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 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
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 list To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev
[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: https://mta.openssl.org/mailman/listinfo/openssl-dev
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, 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
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
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
[Sorry, sent unfinished variant.] 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 the problem is that it's not entirely clear what this check is for at all. If it's there only to catch negative values it's easy to fix -- replace it with "len > SIZE_MAX /2", "len >> (sizeof len * 8 - 1)" or something. If the check is not needed at all it's easy to fix too:-) But if the intention was to specifically check for pointer overflow everything is a bit more complicated. 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 but it's kinda ugly. -- 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
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
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
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
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. -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
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
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
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
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
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
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 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
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
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
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
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 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
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
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
> 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
> 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
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 > te
[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 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
[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; 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