Re: [openssl-dev] [openssl.org #4119] DTLS resets handshake hash too frequently for ClientHello

[Matt Caswell via RT]

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 

Re: [openssl-dev] [openssl.org #4119] DTLS resets handshake hash too frequently for ClientHello

[David Benjamin via RT]

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

[Matt Caswell via RT]

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

[Matt Caswell via RT]

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

Re: [openssl-dev] [openssl.org #4119] DTLS resets handshake hash too frequently for ClientHello

[Viktor Dukhovni]

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

[Matt Caswell]

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

[David Benjamin via RT]

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

[David Benjamin via RT]

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

[Matt Caswell via RT]

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 

[openssl-dev] [openssl.org #4119] DTLS resets handshake hash too frequently for ClientHello

[David Benjamin via RT]

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