1590001 - (CVE-2019-17023) tls-server-no_fuzzer_mode: ASSERT: ss->ssl3.hs.ws == wait_client_hello, at ../../lib/ssl/ssl3con.c:12805

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Franziskus Kiefer [:franziskus]]

Attachment: testcase

The fuzzer found an issue.
Marking this as critical as it appears to be a state machine issue in TLS 1.3 (ssl3con.c:12805), i.e. the state is ssl_ct_application_data and ssl_0rtt_ignore_hrr while not waiting for a client hello (wait_client_hello).

I recommend removing the offending code change https://hg.mozilla.org/projects/nss/rev/bc77cf318f388f55790b99d5f23a9c1f2bd9f900?revcount=10000, i.e. disable TLS 1.3 again until the issue is fixed.

To reproduce build nss with ./build.sh --fuzz --asan and run LD_LIBRARY_PATH=../dist/Debug/lib/ nssfuzz-tls-server testcase.

Comment 1

[Martin Thomson [:mt:]]

Yeah, I analyzed this today. I found that we aren't recording anything about having sent HelloRetryRequest. The fix is trivial: we check ss->ssl3.hs.helloRetry and reject versions less than TLS 1.3 when selecting a version AND/OR constrain the version range to include just TLS 1.3 when we send HelloRetryRequest.

The trick is in replicating the scenario. I've a test that is partially there, but not yet crashing. Kevin, do you want to take the patch?

Comment 2

[Kevin Jacobs [:kjacobs]]

Sure, I'll make the prescribed change and work on a test. Thanks.

Comment 3

[Daniel Veditz [:dveditz]]

Guessing about the disclosure deadline: I can't see the oss-fuzz issue so I don't know when it was filed, but typically they make those public after 90 days.

What kind of crash is this? Some kind of memory corruption that would be useful for an exploit? Something that could be manipulated to cause a bogus insecure TLS state instead of crashing? Or does it just crash and save us from doing something insecure?

Comment 4

[Kevin Jacobs [:kjacobs]]

Attachment: Bug 1590001 - Prevent negotiation of versions lower than 1.3 after HelloRetryRequest.

This patch prevents negotiation of TLS versions lower than 1.3 after an HRR has been sent.

Comment 5

[Franziskus Kiefer [:franziskus]]

(In reply to Daniel Veditz [:dveditz] from comment #3)

Guessing about the disclosure deadline: I can't see the oss-fuzz issue so I don't know when it was filed, but typically they make those public after 90 days.

Reported-2019-10-19
Disclosure-2020-01-17

What kind of crash is this? Some kind of memory corruption that would be useful for an exploit? Something that could be manipulated to cause a bogus insecure TLS state instead of crashing? Or does it just crash and save us from doing something insecure?

No memory issue here. NSS allows to negotiate TLS < 1.3 after an HRR has been sent and thus runs into an assert that checks whether NSS expects a client hello or not. In debug builds we crash and in release build this simply stops handling the incoming record. I don't see any easy way to exploit this.

Comment 6

[Martin Thomson [:mt:]]

Yes, in effect, we are permitting clients to start with TLS 1.3 and then continue the handshake with TLS 1.2. So we definitely have a screwy TLS state, albeit one the client asked for. I checked fairly thoroughly and couldn't find any landmines other than the version downgrade, but I can't be entirely sure. The only things in our favour are that the work we do prior to sending HelloRetryRequest is pretty minimal AND our ClientHello processing assumes nothing, not even a clean slate, thanks to our renegotiation support.

So there is some uncertainty here, but most of the signs indicate that this isn't that bad. A client that gets the HelloRetryRequest won't do this if it is at all sane, and otherwise a client wouldn't do this unless it wanted to cause us to crash (which we won't in release builds as far as I can see, though I'm not completely sure).

The actual asserting code that we hit here is a dead-end that just causes the packet to be dropped. So if someone gets us into this state, in a non-debug build, we will just drop every Application Data record we get. I couldn't find a crasher for a regular build. And dropping data probably isn't that useful to an attacker.

As the tests paint a target on the problem, (even though the first is a red herring), I think that we should probably split the patch into two and land the fix, but I would only wait until the fix is in a build before doing so.

Comment 7

[Kevin Jacobs [:kjacobs]]

Attachment: Bug 1590001 - Additional HRR Tests. r?mt

This patch adds new tests for version limitations after a HRR.

Comment 8

[J.C. Jones [:jcj] (he/him)]

https://hg.mozilla.org/projects/nss/rev/d64102b76a437f24d98a20480dcc9f1655143e7c

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Is this something we eventually need on ESR68 as well or is riding with Fx72 sufficient?

Comment 10

[J.C. Jones [:jcj] (he/him)]

I don't think this warrants ESR uplift. If an attacker prompts this scenario, it's a DoS, which we don't usually uplift unless egregious.

Comment 11

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

If the client needs to ask to negotiate 1.2 after a HRR, is this triggerable through Firefox?

Comment 12

[J.C. Jones [:jcj] (he/him)]

I believe so, yes, however it doesn't lead to a crash unless you're in a debug build. The bad transition just makes the connection stall.

Comment 13

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Attachment: advisory.txt

Comment 14

[Martin Thomson [:mt:]]

Time to land the tests? This doesn't affect others and the fix is now out there. I would rather not hold the tests back too long. JC, WDYT?

Comment 15

[J.C. Jones [:jcj] (he/him)]

Yep, I agree. I'll land the tests tomorrow unless there's dissent.

Comment 16

[J.C. Jones [:jcj] (he/him)]

https://hg.mozilla.org/projects/nss/rev/8a2bd40e7f89a796cf24a0ff7cfb67c6e69c5c78

Comment 17

[Mike Hommey [:glandium]]

Does this affect servers that use NSS for their TLS?

Comment 18

[Martin Thomson [:mt:]]

Yes. The but the effect is only that clients can force you to negotiate a nonsensical session that is still as secure as a regular handshake with TLS 1.2. We still send the downgrade indicators properly, so there is no risk to a valid client.