Re: [openssl-dev] [openssl.org #4103] Valgrind reported memory leak in X509_PUBKEY_get

[Srinivas Thota via RT]

Please ignore this.

Issue found in user code.

Thank you so much.


On Thu, Oct 22, 2015 at 12:18 AM, Salz, Rich via RT  wrote:

> > I am trying figure out valgrind report leak. in openssl 1.0.1c.
>
> You don't have enough of the backtrace for us to reproduce it.  Please add
> a simple demo program.
>
>

___
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev

[openssl-dev] [openssl.org #4103] Valgrind reported memory leak in X509_PUBKEY_get

[Rich Salz via RT]

closing per request of submittor.
--
Rich Salz, OpenSSL dev team; rs...@openssl.org

___
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