[openssl-dev] [openssl.org #4121] avoid configuring openssl twice

[Marcus Meissner via RT]

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

[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