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 <m...@openssl.org>
Date: Tue, 3 Nov 2015 14:45:07 +0000
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 <m...@openssl.org>
Date: Tue, 3 Nov 2015 15:49:08 +0000
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 feeaf6d..26f3c99 100644
--- a/ssl/d1_clnt.c
+++ b/ssl/d1_clnt.c
@@ -315,13 +315,12 @@ int dtls1_connect(SSL *s)
#endif
case SSL3_ST_CW_CLNT_HELLO_A:
- case SSL3_ST_CW_CLNT_HELLO_B:
-
s->shutdown = 0;
/* every DTLS ClientHello resets Finished MAC */
ssl3_init_finished_mac(s);
+ case SSL3_ST_CW_CLNT_HELLO_B:
dtls1_start_timer(s);
ret = ssl3_client_hello(s);
if (ret <= 0)
--
2.1.4
_______________________________________________
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev
