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 <m...@openssl.org>
Date: Tue, 3 Nov 2015 14:45:07 +0000
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 so we have it
+ * available in case of an IO retry. We don't know the length of the
+ * next fragment yet so just set that to 0 for now. It will be
+ * updated again later.
+ */
+ dtls1_fix_message_header(s, frag_off, 0);
}
}
return (0);
--
2.1.4
>From 46b946ada0a918123612d5375838b3b1ed3faa0d Mon Sep 17 00:00:00 2001
From: Matt Caswell <m...@openssl.org>
Date: Wed, 4 Nov 2015 11:20:50 +0000
Subject: [PATCH 2/3] Ensure |rwstate| is set correctly on BIO_flush
A BIO_flush call in the DTLS code was not correctly setting the |rwstate|
variable to SSL_WRITING. This means that SSL_get_error() will not return
SSL_ERROR_WANT_WRITE in the event of an IO retry.
---
ssl/d1_both.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/ssl/d1_both.c b/ssl/d1_both.c
index dfae56c..1f0eaac 100644
--- a/ssl/d1_both.c
+++ b/ssl/d1_both.c
@@ -295,6 +295,8 @@ int dtls1_do_write(SSL *s, int type)
blocksize = 0;
frag_off = 0;
+ s->rwstate = SSL_NOTHING;
+
/* 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) {
@@ -342,8 +344,10 @@ int dtls1_do_write(SSL *s, int type)
* grr.. we could get an error if MTU picked was wrong
*/
ret = BIO_flush(SSL_get_wbio(s));
- if (ret <= 0)
+ if (ret <= 0) {
+ s->rwstate = SSL_WRITING;
return ret;
+ }
used_len = DTLS1_RT_HEADER_LENGTH + mac_size + blocksize;
if (s->d1->mtu > used_len + DTLS1_HM_HEADER_LENGTH) {
curr_mtu = s->d1->mtu - used_len;
--
2.1.4
>From d7ed52adf42de98e582f1b3b0c2a621d45bbcb11 Mon Sep 17 00:00:00 2001
From: Matt Caswell <m...@openssl.org>
Date: Tue, 3 Nov 2015 15:49:08 +0000
Subject: [PATCH 3/3] 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
