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
[openssl-dev] [openssl.org #4096] apps/req.c: support start_days_before option for x509
Hi,
I'm using the openssl command to generate test certificates and I'm
running into an annoying "not valid yet" certificate issue. I cannot
set the notbefore field using the openssl command.
I've made a patch (see attachment) that add the support of a
"-start-days-before' option which is symmetric to the "-days" option.
Patch commit message:
"Sometimes the generated X509 certificate notbefore date must be in the
past. For instance, if this certificate is going to be included in a
device that might get its clock reset to its default time value, the
included certificate notbefore field must match (or be prior) its
default date value which might be in the past.
This patch adds the support of a -start-days-before option which is
symmetric to the -days option. "
I'm not sure this is the best approach but having a -notbefore option
taking a date string would require date parsing support which is not
easy across platforms. I think that this totally symmetric to "-days"
option is simple enough and should cover most of the use case.
What do you think ?
Cheers,
Jérémy
--
One Emacs to rule them all
>From 9a39603bc25a63e5e716b0d57116cef8793fd7e2 Mon Sep 17 00:00:00 2001
From: Jeremy Compostella
Date: Thu, 15 Oct 2015 17:12:49 +0200
Subject: [PATCH] apps/req.c: support start_days_before option for x509
Sometimes the generated X509 certificate notbefore date must in the
past. For instance, if this certificate is going to be included in a
device that might get its clock reset to its default time value of its
clock back to the default value. In that situation, the included
certificate notbefore field must match the default date value which
might be in the past.
Change-Id: I2c01bb4f8ea52ab8dd93666ff5eb091fbe236ae0
Tracked-On: NOT YET
Signed-off-by: Jeremy Compostella
---
apps/req.c | 12 +---
1 file changed, 9 insertions(+), 3 deletions(-)
diff --git a/apps/req.c b/apps/req.c
index 57781c9..398ee11 100644
--- a/apps/req.c
+++ b/apps/req.c
@@ -163,7 +163,7 @@ int MAIN(int argc, char **argv)
{
ENGINE *e = NULL, *gen_eng = NULL;
unsigned long nmflag = 0, reqflag = 0;
-int ex = 1, x509 = 0, days = 30;
+int ex = 1, x509 = 0, days = 30, start_days_before = 0;
X509 *x509ss = NULL;
X509_REQ *req = NULL;
EVP_PKEY_CTX *genctx = NULL;
@@ -350,6 +350,10 @@ int MAIN(int argc, char **argv)
days = atoi(*(++argv));
if (days == 0)
days = 30;
+ } else if (strcmp(*argv, "-start-days-before") == 0) {
+if (--argc < 1)
+goto bad;
+start_days_before = atoi(*(++argv));
} else if (strcmp(*argv, "-set_serial") == 0) {
if (--argc < 1)
goto bad;
@@ -426,7 +430,9 @@ int MAIN(int argc, char **argv)
BIO_printf(bio_err,
" -x509 output a x509 structure instead of a cert. req.\n");
BIO_printf(bio_err,
- " -days number of days a certificate generated by -x509 is valid for.\n");
+ " -days number of days a certificate generated by -x509 is valid for starting now.\n");
+BIO_printf(bio_err,
+ " -start-days-before number of days before now a certificate generated by -x509 is valid.\n");
BIO_printf(bio_err,
" -set_serialserial number to use for a certificate generated by -x509.\n");
BIO_printf(bio_err,
@@ -799,7 +805,7 @@ int MAIN(int argc, char **argv)
if (!X509_set_issuer_name(x509ss, X509_REQ_get_subject_name(req)))
goto end;
-if (!X509_gmtime_adj(X509_get_notBefore(x509ss), 0))
+if (!X509_time_adj_ex(X509_get_notBefore(x509ss), -start_days_before, 0, NULL))
goto end;
if (!X509_time_adj_ex(X509_get_notAfter(x509ss), days, 0, NULL))
goto end;
--
1.9.1
___
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
Re: [openssl-dev] [openssl.org #3712] TLS Renegotiation with Java is broken
On Friday 16 October 2015 13:52:14 Matt Caswell via RT wrote: > On 16/10/15 10:56, Hubert Kario via RT wrote: > > On Friday 16 October 2015 08:53:06 Matt Caswell via RT wrote: > >> So now I really don't know what the "right" way forward is. Should > >> we > >> be applying the patch or not? > > > > I can't think of a way to exploit it if two assumptions hold: > > 1). we have secure renegotiation > > 2). API calls return metadata (certificates especially) from > > *active* > > context, not one currently negotiated > > So these API calls will return the *new* certificate and verification > result *before* a CertificateVerify has been received. > > Fixing this sort of problem is going to be *hard* and probably require > quite a lot of non-trivial changes - definitely not the sort of the > thing I want to be doing in a stable branch. Fixing this is an > example of what I meant by "onerous mitigations", but I now realise > it is absolutely necessary if we wanted to pursue this. > > I think we should be marking this as a "won't fix" for all released > versions. The question is whether we should even attempt to fix it for > 1.1.0 or not. we may actually be able to patch this up partially in 1.0.x the original problem description mentions server being unable to process application data before Certificate/Client Key Exchange, not in any place what so ever (Albe, please double check if you didn't saw Java sending app data at any different point) unless the server is completely asynchronous, it's unlikely it will send application data messages between handshake messages from a single flight, it will send app data only between different flights in other words, we should still be able to accept this data before the client responses had any chance to modify the certificates in the server. of course, that doesn't allow us to fix it for the other side of connection - where the application data is sent by server after Server Hello Done and before server Change Cipher Spec -- Regards, Hubert Kario Quality Engineer, QE BaseOS Security team Web: www.cz.redhat.com Red Hat Czech s.r.o., Purkyňova 99/71, 612 45, Brno, Czech Republic signature.asc Description: PGP signature ___ openssl-dev mailing list To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev
Re: [openssl-dev] [openssl.org #3712] TLS Renegotiation with Java is broken
On 16/10/15 10:56, Hubert Kario via RT wrote:
> On Friday 16 October 2015 08:53:06 Matt Caswell via RT wrote:
>> So now I really don't know what the "right" way forward is. Should we
>> be applying the patch or not?
>
> I can't think of a way to exploit it if two assumptions hold:
> 1). we have secure renegotiation
> 2). API calls return metadata (certificates especially) from *active*
> context, not one currently negotiated
But here is where we come unstuck I think. I've given this some thought
and I think there is an exploit if we were to apply the patch as is.
Consider a reneg where the server has requested a client certificate.
The client responds with a valid certificate and chainbut where they
do not possess the private key, i.e. they are attempting to impersonate
someone else. The client sends the Certificate message and the server
extracts it and verifies it successfully.
On the server side the Certificate message is processed via the function
"ssl3_get_client_certificate()". At the end of this function there are
these two lines of code:
s->session->peer = sk_X509_shift(sk);
s->session->verify_result = s->verify_result;
|s->session->peer| stores the newly received Certificate away for future
use. Note that at this point the result of the verification (which is
successful) is stored in both |s->session->verify_result| and
|s->verify_result|.
Now, before sending the CertificateVerify message, the client sends
application data. Maybe that application data causes the application to
try to do something privileged that only authorised users are allowed to
do, so the application calls SSL_get_peer_certificate():
X509 *SSL_get_peer_certificate(const SSL *s)
{
X509 *r;
if ((s == NULL) || (s->session == NULL))
r = NULL;
else
r = s->session->peer;
if (r == NULL)
return (r);
X509_up_ref(r);
return (r);
}
And SSL_get_verify_result():
long SSL_get_verify_result(const SSL *ssl)
{
return (ssl->verify_result);
}
So these API calls will return the *new* certificate and verification
result *before* a CertificateVerify has been received.
Fixing this sort of problem is going to be *hard* and probably require
quite a lot of non-trivial changes - definitely not the sort of the
thing I want to be doing in a stable branch. Fixing this is an example
of what I meant by "onerous mitigations", but I now realise it is
absolutely necessary if we wanted to pursue this.
I think we should be marking this as a "won't fix" for all released
versions. The question is whether we should even attempt to fix it for
1.1.0 or not.
Matt
___
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev
[openssl-dev] [openssl.org #3645] openssl-1.0.1h-cmp - Linking issue
Thanks Martin. (Re-closing the ticket.) ___ openssl-dev mailing list To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev
Re: [openssl-dev] [openssl.org #3712] TLS Renegotiation with Java is broken
Hubert Kario wrote: > On Friday 16 October 2015 08:53:06 Matt Caswell via RT wrote: >> I raised the ambiguity in the spec about when in the handshake >> interleaved app data is allowed with the TLS WG. You can see the >> thread here: >> https://www.ietf.org/mail-archive/web/tls/current/threads.html#18017 >> >> I got a few responses, not all of which were consistent, and giving >> different views. To summarise what I interpret as the main points: >> >> 1) Where a view was given it seemed to concur with the views expressed >> here that the most sensible interpretation of the spec wording is >> that interleaved app data is allowed up until the CCS, but not >> between CCS and Finished. >> 2) There was also a view expressed that, regardless of what the spec >> says, allowing interleaved app data is *dangerous*! >> 3) There seemed to be differing views on just how dangerous ranging >> from it being "a highly dangerous idea" to "...there is a need for >> caution, but in reality, it's not like you can use renegotiation to >> hand-off to someone else entirely. The person you are talking to >> hasn't changed. What is dangerous is making assertions about *new* >> things that the renegotiation introduces", although the same person >> who made that last observation also provided a list of very onerous >> mitigations that we should put in place if were to do it (none of >> which are likely to be adopted IMO without some form of official >> advice from the TLS WG). >> >> So now I really don't know what the "right" way forward is. Should we >> be applying the patch or not? > > I can't think of a way to exploit it if two assumptions hold: > 1). we have secure renegotiation > 2). API calls return metadata (certificates especially) from *active* > context, not one currently negotiated I tend to agree. But isn't point 2) exactly what Matt calls a "very onerous mitigation"? Reading the replies Matt got from the TLS WG I got the impression that they go "yuck" when they hear the word "renegotiation", but that's hardly an option if you want to implement a RFC that says otherwise. Being the one who reported the problem, I am not impartial as to whether the patch should go in or not. Would it be an option to have the patch as it is, and if it is too hard to have all API calls return data pertaining to the old context, have them throw an error in that case? That would keep things on the safe side and improve interoperability. Yours, Laurenz Albe ___ openssl-dev mailing list To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev
Re: [openssl-dev] [openssl.org #3712] TLS Renegotiation with Java is broken
Hubert Kario wrote: > On Friday 16 October 2015 08:53:06 Matt Caswell via RT wrote: >> I raised the ambiguity in the spec about when in the handshake >> interleaved app data is allowed with the TLS WG. You can see the >> thread here: >> https://www.ietf.org/mail-archive/web/tls/current/threads.html#18017 >> >> I got a few responses, not all of which were consistent, and giving >> different views. To summarise what I interpret as the main points: >> >> 1) Where a view was given it seemed to concur with the views expressed >> here that the most sensible interpretation of the spec wording is >> that interleaved app data is allowed up until the CCS, but not >> between CCS and Finished. >> 2) There was also a view expressed that, regardless of what the spec >> says, allowing interleaved app data is *dangerous*! >> 3) There seemed to be differing views on just how dangerous ranging >> from it being "a highly dangerous idea" to "...there is a need for >> caution, but in reality, it's not like you can use renegotiation to >> hand-off to someone else entirely. The person you are talking to >> hasn't changed. What is dangerous is making assertions about *new* >> things that the renegotiation introduces", although the same person >> who made that last observation also provided a list of very onerous >> mitigations that we should put in place if were to do it (none of >> which are likely to be adopted IMO without some form of official >> advice from the TLS WG). >> >> So now I really don't know what the "right" way forward is. Should we >> be applying the patch or not? > > I can't think of a way to exploit it if two assumptions hold: > 1). we have secure renegotiation > 2). API calls return metadata (certificates especially) from *active* > context, not one currently negotiated I tend to agree. But isn't point 2) exactly what Matt calls a "very onerous mitigation"? Reading the replies Matt got from the TLS WG I got the impression that they go "yuck" when they hear the word "renegotiation", but that's hardly an option if you want to implement a RFC that says otherwise. Being the one who reported the problem, I am not impartial as to whether the patch should go in or not. Would it be an option to have the patch as it is, and if it is too hard to have all API calls return data pertaining to the old context, have them throw an error in that case? That would keep things on the safe side and improve interoperability. Yours, Laurenz Albe ___ openssl-dev mailing list To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev
Re: [openssl-dev] [openssl.org #3712] TLS Renegotiation with Java is broken
On 16/10/15 11:57, Kurt Roeckx via RT wrote: > On Fri, Oct 16, 2015 at 08:53:06AM +, Matt Caswell via RT wrote: >> >> So now I really don't know what the "right" way forward is. Should we be >> applying the patch or not? > > Has anybody contact Oracle about this issue? It seems useful that > they fix it on their end, regardless of what we do. No, but according to the spec they are doing it right - nothing to fix. That's part of the problem: the spec says one thing but the advice we are getting from TLS WG says something slightly different. Matt ___ openssl-dev mailing list To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev
Re: [openssl-dev] [openssl.org #3712] TLS Renegotiation with Java is broken
On Fri, Oct 16, 2015 at 08:53:06AM +, Matt Caswell via RT wrote: > > So now I really don't know what the "right" way forward is. Should we be > applying the patch or not? Has anybody contact Oracle about this issue? It seems useful that they fix it on their end, regardless of what we do. Kurt ___ openssl-dev mailing list To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev
Re: [openssl-dev] [openssl.org #4095] X509_STORE_get_by_subject crash
My application is written for vxWorks OS and openssl and vxWorks are part of the binary that I need to verify On Fri, Oct 16, 2015 at 3:13 PM, tosif tamboli wrote: > Hi, > > below is my application code > sshX509CACertStore = X509_STORE_new(); > > X509_STORE_set_verify_cb_func(sshX509CACertStore, > sshX509CertVerifyCallback); > pLookup = X509_STORE_add_lookup(sshX509CACertStore, > X509_LOOKUP_file()); > X509_LOOKUP_load_file(pLookup,caFile,X509_FILETYPE_PEM) > > X509_STORE_CTX_init (pStoreCtx, sshX509CACertStore, pX509, NULL); > > ret = X509_verify_cert (pStoreCtx); > > in the callback function I just checked for > retVal = X509_STORE_get_by_subject (&storeCtx, X509_LU_CRL, > pSubject, &x509_obj); > > retVal = X509_STORE_get_by_subject (&storeCtx, X509_LU_CRL, > pIssuer, &x509_obj); > > older openssl used md5 hash and newer doesn't seem to use it > As you mentioned about c_rehash. How should I create new symlink in code. > My application is to verify the certificate and signature in image > > It will be helpful if you can provide your inputs for crash of above > application at > X509_STORE_get_by_subject (&storeCtx, X509_LU_CRL, > pIssuer, &x509_obj); > > Thanks & regards, > Tosif > > > On Thu, Oct 15, 2015 at 8:16 PM, Emilia Käsper wrote: > >> This sounds like an application problem. >> 1) Did you recompile your source? 0.9.7 and 1.0.1 are not >> binary-compatible. >> 2) The certificate hash format has changed between 1.0.1 and 0.9.7, which >> could >> explain why the lookup no longer works: >> https://www.openssl.org/docs/manmaster/apps/rehash.html >> >> If the above isn't helpful, try asking for help on openssl-users@. >> Rejecting >> the ticket (though please reopen if you find new evidence that this is a >> bug >> within OpenSSL). >> >> Cheers, >> Emilia >> >> > ___ openssl-dev mailing list To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev
Re: [openssl-dev] [openssl.org #3712] TLS Renegotiation with Java is broken
On Friday 16 October 2015 08:53:06 Matt Caswell via RT wrote: > On 13/10/15 12:31, Hubert Kario via RT wrote: > > On Tuesday 13 October 2015 09:22:53 Matt Caswell via RT wrote: > >> On 12/10/15 17:19, Matt Caswell via RT wrote: > >>> On 12/10/15 16:39, Matt Caswell via RT wrote: > The value of "in_read_app_data" not being true when it is > supposed > to > appears to be running into a slightly different bug. It's also > present in 1.0.2 but you have to switch off version negotiation. > So running s_server like this in 1.0.2 and rerunning Hubert's > test > will hit it: > > openssl s_server -www -tls1_2 > > The 1.0.2 version negotiation is hiding the issue. In master > version neg has been completely rewritten and simplified - but in > doing so no longer hides the problem. :-( > >>> > >>> Having done some more digging it seems the problem only occurs if > >>> you > >>> get the initial handshake, following by a second reneg handshake > >>> *and* interleaved app data all within the scope of a *single* > >>> SSL_read call. AFAICT if SSL_read returns between the first > >>> handshake and the second, you don't get the problem. > >> > >> Ok, updated version of the patch attached. This is for 1.0.2 but > >> should pass Hubert's tests even when you run s_server with > >> "-tls1_2". > > > > yup, looks good with -tls1_2 now too > > > > for some reason my side can't negotiate TLS 1.1 or TLS 1.0 correctly > > so can't test -tls1_1 or -tls1 (I'm likely generating malformed CKE > > there, but need to check to be sure) > > I raised the ambiguity in the spec about when in the handshake > interleaved app data is allowed with the TLS WG. You can see the > thread here: > https://www.ietf.org/mail-archive/web/tls/current/threads.html#18017 > > I got a few responses, not all of which were consistent, and giving > different views. To summarise what I interpret as the main points: > > 1) Where a view was given it seemed to concur with the views expressed > here that the most sensible interpretation of the spec wording is > that interleaved app data is allowed up until the CCS, but not > between CCS and Finished. > 2) There was also a view expressed that, regardless of what the spec > says, allowing interleaved app data is *dangerous*! > 3) There seemed to be differing views on just how dangerous ranging > from it being "a highly dangerous idea" to "...there is a need for > caution, but in reality, it's not like you can use renegotiation to > hand-off to someone else entirely. The person you are talking to > hasn't changed. What is dangerous is making assertions about *new* > things that the renegotiation introduces", although the same person > who made that last observation also provided a list of very onerous > mitigations that we should put in place if were to do it (none of > which are likely to be adopted IMO without some form of official > advice from the TLS WG). > > So now I really don't know what the "right" way forward is. Should we > be applying the patch or not? I can't think of a way to exploit it if two assumptions hold: 1). we have secure renegotiation 2). API calls return metadata (certificates especially) from *active* context, not one currently negotiated -- Regards, Hubert Kario Quality Engineer, QE BaseOS Security team Web: www.cz.redhat.com Red Hat Czech s.r.o., Purkyňova 99/71, 612 45, Brno, Czech Republic signature.asc Description: PGP signature ___ openssl-dev mailing list To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev
Re: [openssl-dev] [openssl.org #3712] TLS Renegotiation with Java is broken
On Friday 16 October 2015 09:55:41 Matt Caswell wrote: > On 16/10/15 09:53, Matt Caswell via RT wrote: > > On 13/10/15 12:31, Hubert Kario via RT wrote: > >> On Tuesday 13 October 2015 09:22:53 Matt Caswell via RT wrote: > >>> On 12/10/15 17:19, Matt Caswell via RT wrote: > On 12/10/15 16:39, Matt Caswell via RT wrote: > > The value of "in_read_app_data" not being true when it is > > supposed > > to > > appears to be running into a slightly different bug. It's also > > present in 1.0.2 but you have to switch off version negotiation. > > So running s_server like this in 1.0.2 and rerunning Hubert's > > test > > will hit it: > > > > openssl s_server -www -tls1_2 > > > > The 1.0.2 version negotiation is hiding the issue. In master > > version neg has been completely rewritten and simplified - but > > in > > doing so no longer hides the problem. :-( > > Having done some more digging it seems the problem only occurs if > you > get the initial handshake, following by a second reneg handshake > *and* interleaved app data all within the scope of a *single* > SSL_read call. AFAICT if SSL_read returns between the first > handshake and the second, you don't get the problem. > >>> > >>> Ok, updated version of the patch attached. This is for 1.0.2 but > >>> should pass Hubert's tests even when you run s_server with > >>> "-tls1_2". > >> > >> yup, looks good with -tls1_2 now too > >> > >> for some reason my side can't negotiate TLS 1.1 or TLS 1.0 > >> correctly so can't test -tls1_1 or -tls1 (I'm likely generating > >> malformed CKE there, but need to check to be sure) > > > > I raised the ambiguity in the spec about when in the handshake > > interleaved app data is allowed with the TLS WG. You can see the > > thread here: > > https://www.ietf.org/mail-archive/web/tls/current/threads.html#18017 > > > > I got a few responses, not all of which were consistent, and giving > > different views. To summarise what I interpret as the main points: > > > > 1) Where a view was given it seemed to concur with the views > > expressed here that the most sensible interpretation of the spec > > wording is that interleaved app data is allowed up until the CCS, > > but not between CCS and Finished. > > 2) There was also a view expressed that, regardless of what the spec > > says, allowing interleaved app data is *dangerous*! > > 3) There seemed to be differing views on just how dangerous ranging > > from it being "a highly dangerous idea" to "...there is a need for > > caution, but in reality, it's not like you can use renegotiation to > > hand-off to someone else entirely. The person you are talking to > > hasn't changed. What is dangerous is making assertions about *new* > > things that the renegotiation introduces", although the same person > > who made that last observation also provided a list of very onerous > > mitigations that we should put in place if were to do it (none of > > which are likely to be adopted IMO without some form of official > > advice from the TLS WG). > > > > So now I really don't know what the "right" way forward is. Should > > we be applying the patch or not? > > I should add that another interesting point was that BoringSSL > prohibits interleaved app data. just like OpenSSL now :) -- Regards, Hubert Kario Quality Engineer, QE BaseOS Security team Web: www.cz.redhat.com Red Hat Czech s.r.o., Purkyňova 99/71, 612 45, Brno, Czech Republic signature.asc Description: This is a digitally signed message part. ___ openssl-dev mailing list To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev
[openssl-dev] [openssl.org #4093] Problem loading engine from config
On Wed Oct 14 19:29:42 2015, beld...@gmail.com wrote: > Hello! > > The attached patch fixes it. Patch applied. Thanks! Matt ___ openssl-dev mailing list To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev
Re: [openssl-dev] [openssl.org #4095] X509_STORE_get_by_subject crash
Hi, below is my application code sshX509CACertStore = X509_STORE_new(); X509_STORE_set_verify_cb_func(sshX509CACertStore, sshX509CertVerifyCallback); pLookup = X509_STORE_add_lookup(sshX509CACertStore, X509_LOOKUP_file()); X509_LOOKUP_load_file(pLookup,caFile,X509_FILETYPE_PEM) X509_STORE_CTX_init (pStoreCtx, sshX509CACertStore, pX509, NULL); ret = X509_verify_cert (pStoreCtx); in the callback function I just checked for retVal = X509_STORE_get_by_subject (&storeCtx, X509_LU_CRL, pSubject, &x509_obj); retVal = X509_STORE_get_by_subject (&storeCtx, X509_LU_CRL, pIssuer, &x509_obj); older openssl used md5 hash and newer doesn't seem to use it As you mentioned about c_rehash. How should I create new symlink in code. My application is to verify the certificate and signature in image It will be helpful if you can provide your inputs for crash of above application at X509_STORE_get_by_subject (&storeCtx, X509_LU_CRL, pIssuer, &x509_obj); Thanks & regards, Tosif On Thu, Oct 15, 2015 at 8:16 PM, Emilia Käsper wrote: > This sounds like an application problem. > 1) Did you recompile your source? 0.9.7 and 1.0.1 are not > binary-compatible. > 2) The certificate hash format has changed between 1.0.1 and 0.9.7, which > could > explain why the lookup no longer works: > https://www.openssl.org/docs/manmaster/apps/rehash.html > > If the above isn't helpful, try asking for help on openssl-users@. > Rejecting > the ticket (though please reopen if you find new evidence that this is a > bug > within OpenSSL). > > Cheers, > Emilia > > ___ openssl-dev mailing list To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev
Re: [openssl-dev] who wants to fix travis builds?
I've opened the following PR to add support for GCC v5 and address sanitizer (not sure if we want valgrind as well...): https://github.com/openssl/openssl/pull/429 > > I've commented there on other -fsanitize options as well as about > option to execute them without --debug and/or --strict-warnings. But I > failed to recall all the details from last time the question was risen > in context of RT#3422 through 3324. -fsanitize=undefined is expected > to work only with --strict-warnings (or more specifically with > -DPEDANTIC) and I wouldn't be surprised if -fsanitize=memory works > only with -DPURIFY. The main weakness of the endeavour is that testing is limited to x86 hardware. I don't mean specifically -fsanitize, but in general. But let's concentrate on -fsanitize for a moment. Or more specifically why are there limitations for it, ones mentioned above. In the nutshell, remaining -fsanitize warnings were effectively about the fact that we sometimes rely on platform- or implementation-specific behaviour. But it is *conscious* choice and there always is platform-neutral fallback. So that above mentioned limitations are actually about nothing else but driving compiler toward these platform-neutral fallbacks. One can even take next step and say that in order to maximize -fsanitize utility, one should expose as much C code to -fsanitize as possible, or in other words adhere even to no-asm with -fsanitize. Incidentally this would also at least partially mitigate the main weakness, as -fsanitize should expose spots where platform-neutral fallback is not as platform-neutral as it should be, wouldn't it? ___ openssl-dev mailing list To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev
Re: [openssl-dev] [openssl.org #3712] TLS Renegotiation with Java is broken
On 16/10/15 09:53, Matt Caswell via RT wrote: > > > On 13/10/15 12:31, Hubert Kario via RT wrote: >> On Tuesday 13 October 2015 09:22:53 Matt Caswell via RT wrote: >>> On 12/10/15 17:19, Matt Caswell via RT wrote: On 12/10/15 16:39, Matt Caswell via RT wrote: > The value of "in_read_app_data" not being true when it is supposed > to > appears to be running into a slightly different bug. It's also > present in 1.0.2 but you have to switch off version negotiation. > So running s_server like this in 1.0.2 and rerunning Hubert's test > will hit it: > > openssl s_server -www -tls1_2 > > The 1.0.2 version negotiation is hiding the issue. In master > version neg has been completely rewritten and simplified - but in > doing so no longer hides the problem. :-( Having done some more digging it seems the problem only occurs if you get the initial handshake, following by a second reneg handshake *and* interleaved app data all within the scope of a *single* SSL_read call. AFAICT if SSL_read returns between the first handshake and the second, you don't get the problem. >>> >>> Ok, updated version of the patch attached. This is for 1.0.2 but >>> should pass Hubert's tests even when you run s_server with "-tls1_2". >> >> yup, looks good with -tls1_2 now too >> >> for some reason my side can't negotiate TLS 1.1 or TLS 1.0 correctly so >> can't test -tls1_1 or -tls1 (I'm likely generating malformed CKE there, >> but need to check to be sure) > > I raised the ambiguity in the spec about when in the handshake > interleaved app data is allowed with the TLS WG. You can see the thread > here: > https://www.ietf.org/mail-archive/web/tls/current/threads.html#18017 > > I got a few responses, not all of which were consistent, and giving > different views. To summarise what I interpret as the main points: > > 1) Where a view was given it seemed to concur with the views expressed > here that the most sensible interpretation of the spec wording is that > interleaved app data is allowed up until the CCS, but not between CCS > and Finished. > 2) There was also a view expressed that, regardless of what the spec > says, allowing interleaved app data is *dangerous*! > 3) There seemed to be differing views on just how dangerous ranging from > it being "a highly dangerous idea" to "...there is a need for caution, > but in reality, it's not like you can use renegotiation to hand-off to > someone else entirely. The person you are talking to hasn't changed. > What is dangerous is making assertions about *new* things that the > renegotiation introduces", although the same person who made that last > observation also provided a list of very onerous mitigations that we > should put in place if were to do it (none of which are likely to be > adopted IMO without some form of official advice from the TLS WG). > > So now I really don't know what the "right" way forward is. Should we be > applying the patch or not? I should add that another interesting point was that BoringSSL prohibits interleaved app data. Matt ___ openssl-dev mailing list To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev
Re: [openssl-dev] [openssl.org #3712] TLS Renegotiation with Java is broken
On 13/10/15 12:31, Hubert Kario via RT wrote: > On Tuesday 13 October 2015 09:22:53 Matt Caswell via RT wrote: >> On 12/10/15 17:19, Matt Caswell via RT wrote: >>> On 12/10/15 16:39, Matt Caswell via RT wrote: The value of "in_read_app_data" not being true when it is supposed to appears to be running into a slightly different bug. It's also present in 1.0.2 but you have to switch off version negotiation. So running s_server like this in 1.0.2 and rerunning Hubert's test will hit it: openssl s_server -www -tls1_2 The 1.0.2 version negotiation is hiding the issue. In master version neg has been completely rewritten and simplified - but in doing so no longer hides the problem. :-( >>> >>> Having done some more digging it seems the problem only occurs if >>> you >>> get the initial handshake, following by a second reneg handshake >>> *and* interleaved app data all within the scope of a *single* >>> SSL_read call. AFAICT if SSL_read returns between the first >>> handshake and the second, you don't get the problem. >> >> Ok, updated version of the patch attached. This is for 1.0.2 but >> should pass Hubert's tests even when you run s_server with "-tls1_2". > > yup, looks good with -tls1_2 now too > > for some reason my side can't negotiate TLS 1.1 or TLS 1.0 correctly so > can't test -tls1_1 or -tls1 (I'm likely generating malformed CKE there, > but need to check to be sure) I raised the ambiguity in the spec about when in the handshake interleaved app data is allowed with the TLS WG. You can see the thread here: https://www.ietf.org/mail-archive/web/tls/current/threads.html#18017 I got a few responses, not all of which were consistent, and giving different views. To summarise what I interpret as the main points: 1) Where a view was given it seemed to concur with the views expressed here that the most sensible interpretation of the spec wording is that interleaved app data is allowed up until the CCS, but not between CCS and Finished. 2) There was also a view expressed that, regardless of what the spec says, allowing interleaved app data is *dangerous*! 3) There seemed to be differing views on just how dangerous ranging from it being "a highly dangerous idea" to "...there is a need for caution, but in reality, it's not like you can use renegotiation to hand-off to someone else entirely. The person you are talking to hasn't changed. What is dangerous is making assertions about *new* things that the renegotiation introduces", although the same person who made that last observation also provided a list of very onerous mitigations that we should put in place if were to do it (none of which are likely to be adopted IMO without some form of official advice from the TLS WG). So now I really don't know what the "right" way forward is. Should we be applying the patch or not? 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 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
