Re: [openssl-dev] [openssl.org #4119] DTLS resets handshake hash too frequently for ClientHello
On 04/11/15 15:30, David Benjamin via RT wrote: > On Wed, Nov 4, 2015 at 7:04 AM Matt Caswell via RT wrote: > >> >> >> On 03/11/15 17:43, David Benjamin via RT wrote: >> >>> I'm not sure that fix quite works though. If BIO_flush completes >>> asynchronously >> >> Ahhh, yes good point. Updated patch attached. >> >>> (hrm, it's missing an rwstate update), >> >> Yes, just discovered that myself and then came back and reread your >> email to find out you already pointed it out! Also addressed in updated >> patch. >> > > The new patch seems to almost work. I merged it into our codebase since we > hadn't diverged too much on that function yet and ran it against our tests > (fixed to actually stress low MTUs). The s->init_off <= > DTLS1_HM_HEADER_LENGTH assertion is only true in the frag_off > 0 case. > After moving it there, everything passes. > > For reference, here's the merged version: > https://boringssl-review.googlesource.com/#/c/6424/ Great! Thanks David. Matt ___ openssl-dev mailing list To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev
Re: [openssl-dev] [openssl.org #4119] DTLS resets handshake hash too frequently for ClientHello
On Wed, Nov 4, 2015 at 7:04 AM Matt Caswell via RT wrote: > > > On 03/11/15 17:43, David Benjamin via RT wrote: > > > I'm not sure that fix quite works though. If BIO_flush completes > > asynchronously > > Ahhh, yes good point. Updated patch attached. > > > (hrm, it's missing an rwstate update), > > Yes, just discovered that myself and then came back and reread your > email to find out you already pointed it out! Also addressed in updated > patch. > The new patch seems to almost work. I merged it into our codebase since we hadn't diverged too much on that function yet and ran it against our tests (fixed to actually stress low MTUs). The s->init_off <= DTLS1_HM_HEADER_LENGTH assertion is only true in the frag_off > 0 case. After moving it there, everything passes. For reference, here's the merged version: https://boringssl-review.googlesource.com/#/c/6424/ David PS: By the way, you might want to consider doing something similar to test all the resume points, since they're apparently pretty subtle and hard to get right sometimes. :-) I added an async mode to our tests which installs a filter BIO that only lets one byte/datagram through at a time. https://boringssl.googlesource.com/boringssl/+/a97b737fb09509dcedf0d15b51b60d2349821605/ssl/test/async_bio.cc It also makes every callback take two steps where possible. https://boringssl.googlesource.com/boringssl/+/a97b737fb09509dcedf0d15b51b60d2349821605/ssl/test/bssl_shim.cc#510 That's driven like this: https://boringssl.googlesource.com/boringssl/+/a97b737fb09509dcedf0d15b51b60d2349821605/ssl/test/bssl_shim.cc#802 https://boringssl.googlesource.com/boringssl/+/a97b737fb09509dcedf0d15b51b60d2349821605/ssl/test/bssl_shim.cc#873 Then that gets run through a series of tests intended to cover all the various possible handshake shapes. https://boringssl.googlesource.com/boringssl/+/a97b737fb09509dcedf0d15b51b60d2349821605/ssl/test/runner/runner.go#2539 It's worked pretty well, assuming I remember to cover all the interesting cases. (Apparently I missed one this time around, so it's not perfect. :-) Still, it's caught a lot of mistakes early.) ___ openssl-dev mailing list To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev
Re: [openssl-dev] [openssl.org #4119] DTLS resets handshake hash too frequently for ClientHello
On 03/11/15 17:43, David Benjamin via RT wrote: > I'm not sure that fix quite works though. If BIO_flush completes > asynchronously Ahhh, yes good point. Updated patch attached. > (hrm, it's missing an rwstate update), Yes, just discovered that myself and then came back and reread your email to find out you already pointed it out! Also addressed in updated patch. > then I believe you'll > be in a state where you *do* want to repeat the init_off / init_num adjust. > You might be able to get away with using init_off/init_num with some minor > tweaks? Another problem: because the fragment headers clobber whatever was > already written, msg_callback sees garbage. Yuck. Not addressed that issue yet. I'll deal with that separately. > Yeah, this part of DTLS (like much of it) is woefully underspecified. We > keep stuffing things into ClientHellos, so if one does need to support > fragmented ones, I think the right way to do stateless HelloVerifyRequest > is to silently drop all non-initial ClientHello fragments and require the > initial one be large enough to include the client_random and whatever else > you figure into the cookie. I really like that idea. I'll take a look at doing that in the new DTLSv1_listen code. Matt >From d973a67f17917869ab2238c041c447996ff94976 Mon Sep 17 00:00:00 2001 From: Matt Caswell Date: Tue, 3 Nov 2015 14:45:07 + Subject: [PATCH 1/3] Fix DTLS handshake fragment retries If using DTLS and NBIO then if a second or subsequent handshake message fragment hits a retry, then the retry attempt uses the wrong fragment offset value. This commit restores the fragment offset from the last attempt. --- ssl/d1_both.c | 63 --- 1 file changed, 43 insertions(+), 20 deletions(-) diff --git a/ssl/d1_both.c b/ssl/d1_both.c index c2c8d57..dfae56c 100644 --- a/ssl/d1_both.c +++ b/ssl/d1_both.c @@ -297,6 +297,39 @@ int dtls1_do_write(SSL *s, int type) frag_off = 0; /* s->init_num shouldn't ever be < 0...but just in case */ while (s->init_num > 0) { +if (type == SSL3_RT_HANDSHAKE && s->init_off != 0) { +/* We must be writing a fragment other than the first one */ + +if (s->init_off <= DTLS1_HM_HEADER_LENGTH) { +/* + * Each fragment that was already sent must at least have + * contained the message header plus one other byte. Therefore + * if |init_off| is non zero then it must have progressed by at + * least |DTLS1_HM_HEADER_LENGTH + 1| bytes. If not something + * went wrong. + */ +return -1; +} + +if (frag_off > 0) { +/* + * This is the first attempt at writing out this fragment. + * Adjust |init_off| and |init_num| to add a new message + * header. + */ +s->init_off -= DTLS1_HM_HEADER_LENGTH; +s->init_num += DTLS1_HM_HEADER_LENGTH; +} else { +/* + * We must have been called again after a retry so use the + * fragment offset from our last attempt. We do not need + * to adjust |init_off| and |init_num| as above, because + * that should already have been done before the retry. + */ +frag_off = s->d1->w_msg_hdr.frag_off; +} +} + used_len = BIO_wpending(SSL_get_wbio(s)) + DTLS1_RT_HEADER_LENGTH + mac_size + blocksize; if (s->d1->mtu > used_len) @@ -336,25 +369,6 @@ int dtls1_do_write(SSL *s, int type) * XDTLS: this function is too long. split out the CCS part */ if (type == SSL3_RT_HANDSHAKE) { -if (s->init_off != 0) { -OPENSSL_assert(s->init_off > DTLS1_HM_HEADER_LENGTH); -s->init_off -= DTLS1_HM_HEADER_LENGTH; -s->init_num += DTLS1_HM_HEADER_LENGTH; - -/* - * We just checked that s->init_num > 0 so this cast should - * be safe - */ -if (((unsigned int)s->init_num) > curr_mtu) -len = curr_mtu; -else -len = s->init_num; -} - -/* Shouldn't ever happen */ -if (len > INT_MAX) -len = INT_MAX; - if (len < DTLS1_HM_HEADER_LENGTH) { /* * len is so small that we really can't do anything sensible @@ -442,7 +456,16 @@ int dtls1_do_write(SSL *s, int type) } s->init_off += ret; s->init_num -= ret; -frag_off += (ret -= DTLS1_HM_HEADER_LENGTH); +ret -= DTLS1_HM_HEADER_LENGTH; +frag_off += ret; + +/* + * We save the fragment offset for the next fragment s
Re: [openssl-dev] [openssl.org #4119] DTLS resets handshake hash too frequently for ClientHello
On Tue, Nov 3, 2015 at 12:42 PM David Benjamin wrote: > I'm not sure that fix quite works though. If BIO_flush completes > asynchronously (hrm, it's missing an rwstate update), then I believe you'll > be in a state where you *do* want to repeat the init_off / init_num adjust. > You might be able to get away with using init_off/init_num with some minor > tweaks? Another problem: because the fragment headers clobber whatever was > already written, msg_callback sees garbage. Also this function is used for > the unfragmented ChangeCipherSpec, so it's even messier. > > I dunno, this code is too stateful by several orders of magnitude. I think > I'm going to toy with rewriting it now rather than think too hard about the > existing mess. > This still needs to be reviewed, but here's a go at a cleaner version on our end. It passes our test suite, even after modifying it to stress the async write + low MTU case. (And the old code indeed does not.) https://boringssl-review.googlesource.com/#/c/6420/ https://boringssl-review.googlesource.com/#/c/6421/ David ___ openssl-dev mailing list To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev
Re: [openssl-dev] [openssl.org #4119] DTLS resets handshake hash too frequently for ClientHello
On 03/11/15 18:28, Viktor Dukhovni wrote: > On Tue, Nov 03, 2015 at 04:16:37PM +, Matt Caswell via RT wrote: > >> One other related point is that fragmenting ClientHellos is probably a >> bad idea. The whole ClientHello/HelloVerifyRequest mechanism is meant to >> be implemented without storing state on the server. That isn't possible >> if you have to deal with fragment reassembly. In the new DTLSv1_listen >> implementation in master we drop fragmented ClientHellos. > > I assume you mean fragmentation across multiple TLS record layer > packets, not UDP fragmentation into multiple IP layer fragments... Yes - multiple DTLS record layer packets. > > Presumably the kernel delivers reassembled UDP datagrams to user-land, > so OpenSSL's DTLS never sees UDP fragmentation. Yes. > > I expect that DTLS is allowed to use UDP datagrams that are larger > than the IP MTU, but if these MUST be fragmented at TLS record > layer instead, then client HELLO packets can't carry very large > extensions, and in particular session tickets could run into trouble... OpenSSL tries to keep DTLS packets within the MTU if possible. I like David's idea of dropping non-initial ClientHello fragments and only requiring that the cookie needed for ClientHello/HelloVerifyRequest is kept within the initial fragment, rather than requiring that the whole ClientHello fits into a single fragment. I'll take a look at that. Matt ___ openssl-dev mailing list To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev
Re: [openssl-dev] [openssl.org #4119] DTLS resets handshake hash too frequently for ClientHello
On Tue, Nov 03, 2015 at 04:16:37PM +, Matt Caswell via RT wrote: > One other related point is that fragmenting ClientHellos is probably a > bad idea. The whole ClientHello/HelloVerifyRequest mechanism is meant to > be implemented without storing state on the server. That isn't possible > if you have to deal with fragment reassembly. In the new DTLSv1_listen > implementation in master we drop fragmented ClientHellos. I assume you mean fragmentation across multiple TLS record layer packets, not UDP fragmentation into multiple IP layer fragments... Presumably the kernel delivers reassembled UDP datagrams to user-land, so OpenSSL's DTLS never sees UDP fragmentation. I expect that DTLS is allowed to use UDP datagrams that are larger than the IP MTU, but if these MUST be fragmented at TLS record layer instead, then client HELLO packets can't carry very large extensions, and in particular session tickets could run into trouble... I don't know whether the code that inserts the TLS padding extension is common to the TLS and DTLS code paths, ideally DTLS should at least avoid bloat from the padding extension. -- Viktor. ___ openssl-dev mailing list To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev
Re: [openssl-dev] [openssl.org #4119] DTLS resets handshake hash too frequently for ClientHello
On Tue, Nov 3, 2015 at 11:16 AM Matt Caswell via RT wrote: > Whilst investigating this I noticed another bug which is actually > probably more significant. My eyeball only look at the BoringSSL source > suggests that it is there too, so I'm not sure why you haven't seen it > in the test that you mentioned. > > If a retry occurs on a second or subsequent fragment of a handshake > message then when we do the retry the wrong fragment offset and length > is used. The impact isn't that great because the messages got dropped by > the peer, and then when they get retransmitted they have the correct > values inserted...so the handshake succeeds - but it gets delayed. > Perhaps that is why you don't see it in your test. > Our tests are pretty strict and deterministic (clock is mocked out, Go DTLS half is intentionally much stricter than a real impl), so it actually fails if packets are dropped that aren't supposed to be. I think it's actually because my async tests, uh, didn't stress the low MTU case at all. :-) Thanks! (The handshake hash bug requires a fragmented ClientHello to trigger in OpenSSL because you only update the handshake hash as it gets written, whereas BoringSSL updates it up-front to simplify a few things. I actually didn't think much about fragmentation when working on it our end and only realized it was a precondition for you later.) In fact, apparently I'd noticed frag_off was messed up before, added TODOs, and forgot about it. :-) I guess this must have been the other reason I'd previously assumed retrying writes in the DTLS code didn't work at all. It mostly doesn't. https://boringssl.googlesource.com/boringssl/+/master/ssl/d1_both.c#285 I'm not sure that fix quite works though. If BIO_flush completes asynchronously (hrm, it's missing an rwstate update), then I believe you'll be in a state where you *do* want to repeat the init_off / init_num adjust. You might be able to get away with using init_off/init_num with some minor tweaks? Another problem: because the fragment headers clobber whatever was already written, msg_callback sees garbage. Also this function is used for the unfragmented ChangeCipherSpec, so it's even messier. I dunno, this code is too stateful by several orders of magnitude. I think I'm going to toy with rewriting it now rather than think too hard about the existing mess. > This second issue occurs in all branches. > > One other related point is that fragmenting ClientHellos is probably a > bad idea. The whole ClientHello/HelloVerifyRequest mechanism is meant to > be implemented without storing state on the server. That isn't possible > if you have to deal with fragment reassembly. In the new DTLSv1_listen > implementation in master we drop fragmented ClientHellos. > Yeah, this part of DTLS (like much of it) is woefully underspecified. We keep stuffing things into ClientHellos, so if one does need to support fragmented ones, I think the right way to do stateless HelloVerifyRequest is to silently drop all non-initial ClientHello fragments and require the initial one be large enough to include the client_random and whatever else you figure into the cookie. David ___ openssl-dev mailing list To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev
Re: [openssl-dev] [openssl.org #4119] DTLS resets handshake hash too frequently for ClientHello
Hi David On 03/11/15 01:58, David Benjamin via RT wrote: > Hey folks, > > We found a small DTLS bug while writing some tests. I think it affects > 1.0.1 and 1.0.2 too, so I thought I'd send you a note. (Note sure about > master. I'm unfamiliar with the new state machine mechanism.) > > In DTLS, each ClientHello is supposed to reset the handshake hash (in case > of HelloVerifyRequest). The old state machine calls ssl3_init_finished_mac > in the SSL3_ST_CW_CLNT_HELLO_* states. > > https://git.openssl.org/gitweb/?p=openssl.git;a=blob;f=ssl/d1_clnt.c;h=feeaf6d0656f5d0868121852d42b5037b8823111;hb=refs/heads/OpenSSL_1_0_2-stable#l317 > > If the ClientHello is fragmented and takes multiple iterations to write, > the state machine is re-entered in SSL3_ST_CW_CLNT_HELLO_B, which > calls ssl3_init_finished_mac again, and clobbers what was sampled to the > handshake hash/buffer previously. > > This requires the transport return a retriable error on write, which > probably isn't common for DTLS. It came up because WebRTC uses a custom BIO > with a fixed-size buffer, so it can actually do this. Even then, a > ClientHello is unlikely to fill up the buffer. Our tests repro'd it in > BoringSSL by forcing every write to take two iterations. I can confirm this issue on 1.0.2 (and almost certainly 1.0.1). It does not affect master. Whilst investigating this I noticed another bug which is actually probably more significant. My eyeball only look at the BoringSSL source suggests that it is there too, so I'm not sure why you haven't seen it in the test that you mentioned. If a retry occurs on a second or subsequent fragment of a handshake message then when we do the retry the wrong fragment offset and length is used. The impact isn't that great because the messages got dropped by the peer, and then when they get retransmitted they have the correct values inserted...so the handshake succeeds - but it gets delayed. Perhaps that is why you don't see it in your test. This second issue occurs in all branches. One other related point is that fragmenting ClientHellos is probably a bad idea. The whole ClientHello/HelloVerifyRequest mechanism is meant to be implemented without storing state on the server. That isn't possible if you have to deal with fragment reassembly. In the new DTLSv1_listen implementation in master we drop fragmented ClientHellos. Patch attached for these two issues (patch against 1.0.2). Matt >From 35b6c161b032bd2e04e54e80120f72b5d586c031 Mon Sep 17 00:00:00 2001 From: Matt Caswell Date: Tue, 3 Nov 2015 14:45:07 + Subject: [PATCH 1/2] Fix DTLS handshake fragment retries If using DTLS and NBIO then if a second or subsequent handshake message fragment hits a retry, then the retry attempt uses the wrong fragment offset value. This commit restores the fragment offset from the last attempt. --- ssl/d1_both.c | 21 +++-- 1 file changed, 19 insertions(+), 2 deletions(-) diff --git a/ssl/d1_both.c b/ssl/d1_both.c index c2c8d57..59a79a7 100644 --- a/ssl/d1_both.c +++ b/ssl/d1_both.c @@ -337,9 +337,26 @@ int dtls1_do_write(SSL *s, int type) */ if (type == SSL3_RT_HANDSHAKE) { if (s->init_off != 0) { +/* We must be writing a fragment other than the first one */ OPENSSL_assert(s->init_off > DTLS1_HM_HEADER_LENGTH); -s->init_off -= DTLS1_HM_HEADER_LENGTH; -s->init_num += DTLS1_HM_HEADER_LENGTH; + +if (frag_off > 0) { +/* + * This is the first attempt at writing out this fragment. + * Adjust |init_off| and |init_num| to add a new message + * header. + */ +s->init_off -= DTLS1_HM_HEADER_LENGTH; +s->init_num += DTLS1_HM_HEADER_LENGTH; +} else { +/* + * We must have been called again after a retry so use the + * fragment offset from our last attempt. We do not need + * to adjust |init_off| and |init_num| as above, because + * that should already have been done before the retry. + */ +frag_off = s->d1->w_msg_hdr.frag_off; +} /* * We just checked that s->init_num > 0 so this cast should -- 2.1.4 >From b0ff7bdae8e7ae6b46a1d95d73ef887673684d0a Mon Sep 17 00:00:00 2001 From: Matt Caswell Date: Tue, 3 Nov 2015 15:49:08 + Subject: [PATCH 2/2] Only call ssl3_init_finished_mac once for DTLS In DTLS if an IO retry occurs during writing of a fragmented ClientHello then we can end up reseting the finish mac variables on the retry, which causes a handshake failure. We should only reset on the first attempt not on retries. --- ssl/d1_clnt.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/ssl/d1_clnt.c b/ssl/d1_clnt.c index
Re: [openssl-dev] [openssl.org #4119] DTLS resets handshake hash too frequently for ClientHello
Hi David, On 03/11/15 01:58, David Benjamin via RT wrote: > Hey folks, > > We found a small DTLS bug while writing some tests. I think it affects > 1.0.1 and 1.0.2 too, so I thought I'd send you a note. (Note sure about > master. I'm unfamiliar with the new state machine mechanism.) Just from looking at the code I think master should be ok. In the new state machine, writes go through a "pre-work" phase where ssl3_init_finished_mac is called for DTLS. This pre-work is skipped if the actual write needs a retry. Matt ___ openssl-dev mailing list To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev
[openssl-dev] [openssl.org #4119] DTLS resets handshake hash too frequently for ClientHello
Hey folks, We found a small DTLS bug while writing some tests. I think it affects 1.0.1 and 1.0.2 too, so I thought I'd send you a note. (Note sure about master. I'm unfamiliar with the new state machine mechanism.) In DTLS, each ClientHello is supposed to reset the handshake hash (in case of HelloVerifyRequest). The old state machine calls ssl3_init_finished_mac in the SSL3_ST_CW_CLNT_HELLO_* states. https://git.openssl.org/gitweb/?p=openssl.git;a=blob;f=ssl/d1_clnt.c;h=feeaf6d0656f5d0868121852d42b5037b8823111;hb=refs/heads/OpenSSL_1_0_2-stable#l317 If the ClientHello is fragmented and takes multiple iterations to write, the state machine is re-entered in SSL3_ST_CW_CLNT_HELLO_B, which calls ssl3_init_finished_mac again, and clobbers what was sampled to the handshake hash/buffer previously. This requires the transport return a retriable error on write, which probably isn't common for DTLS. It came up because WebRTC uses a custom BIO with a fixed-size buffer, so it can actually do this. Even then, a ClientHello is unlikely to fill up the buffer. Our tests repro'd it in BoringSSL by forcing every write to take two iterations. David ___ 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