Closed Bug 1590001 (CVE-2019-17023) Opened 2 years ago Closed 2 years ago

tls-server-no_fuzzer_mode: ASSERT: ss->ssl3.hs.ws == wait_client_hello, at ../../lib/ssl/ssl3con.c:12805

Categories

(NSS :: Libraries, defect, P1)

3.48
defect

Tracking

(firefox-esr68- wontfix, firefox70 wontfix, firefox71 wontfix, firefox72 fixed)

RESOLVED FIXED
Tracking Status
firefox-esr68 - wontfix
firefox70 --- wontfix
firefox71 --- wontfix
firefox72 --- fixed

People

(Reporter: franziskus, Assigned: kjacobs)

References

()

Details

(Keywords: csectype-dos, sec-low, Whiteboard: [disclosure deadline january 2020][adv-main72+][post-critsmash-triage])

Attachments

(4 files)

Attached file 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.

Assignee: nobody → kjacobs.bugzilla
Status: NEW → ASSIGNED

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?

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

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?

Flags: needinfo?(franziskuskiefer)
Whiteboard: [disclosure deadline january 2020]

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

Attachment #9104052 - Attachment description: Bug 1590001 - Constrain TLS server version range after sending HRR. → Bug 1590001 - Prevent negotiation of versions lower than 1.3 after HelloRetryRequest.

(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.

Flags: needinfo?(franziskuskiefer)

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.

Keywords: sec-low

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

Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED

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

Group: crypto-core-security → core-security-release
Flags: needinfo?(kjacobs.bugzilla)

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.

Flags: needinfo?(kjacobs.bugzilla)
Whiteboard: [disclosure deadline january 2020] → [disclosure deadline january 2020][adv-main72+]

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

Flags: needinfo?(jjones)

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.

Flags: needinfo?(jjones)

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?

Flags: needinfo?(jjones)

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

Flags: qe-verify-
Whiteboard: [disclosure deadline january 2020][adv-main72+] → [disclosure deadline january 2020][adv-main72+][post-critsmash-triage]

Does this affect servers that use NSS for their TLS?

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.

Flags: needinfo?(jjones)
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.