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
[openssl-dev] [openssl.org #4121] avoid configuring openssl twice
Hi, In a mix of various libraries using openssl it can happen that OPENSSL_config is called multiple times. Usually this works, but e.g. if you have engines configured, the second load of engines will not work. OPENSSL_config checks openssl_configured on begin, but does not set it when done. (only in OPENSSL_no_config). So lets set it at the end of OPENSSL_config. Sent as https://github.com/openssl/openssl/pull/466 Ciao, Marcus -- Marcus Meissner,SUSE LINUX GmbH; Maxfeldstrasse 5; D-90409 Nuernberg; Zi. 3.1-33,+49-911-740 53-432,,serv=loki,mail=wotan,type=real ___ 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 #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