Closed Bug 1449160 Opened 3 years ago Closed 3 years ago

tls13_GetHash crash


(NSS :: Libraries, defect)

Not set


(Not tracked)



(Reporter: franziskus, Assigned: mt)


(Keywords: sec-low)


(2 files)

Attached file tls13_GetHash
Our fuzzer found a crash in tls13_GetHash.

#0 0x7f5cf1771427 in gsignal /build/glibc-Cl5G7W/glibc-2.23/sysdeps/unix/sysv/linux/raise.c:54
#1 0x7f5cf1773029 in abort /build/glibc-Cl5G7W/glibc-2.23/stdlib/abort.c:89
#2 0x6fe4fc in PR_Assert /src/nspr/pr/src/io/prlog.c:553:5
#3 0x4590d0 in tls13_GetHash /src/nss/lib/ssl/tls13con.c:281:5
#4 0x45928a in tls13_ComputeHash /src/nss/lib/ssl/tls13con.c:345:42
#5 0x46167f in tls13_ReinjectHandshakeTranscript /src/nss/lib/ssl/tls13con.c:2150:10
#6 0x46139c in tls13_HandleHelloRetryRequest /src/nss/lib/ssl/tls13con.c:2234:10
#7 0x485b26 in ssl3_HandleServerHello /src/nss/lib/ssl/ssl3con.c:6391:14
#8 0x483f95 in ssl3_HandleHandshakeMessage /src/nss/lib/ssl/ssl3con.c:11351:18
#9 0x4874e6 in ssl3_HandleHandshake /src/nss/lib/ssl/ssl3con.c:11536:18
#10 0x4863d4 in ssl3_HandleNonApplicationData /src/nss/lib/ssl/ssl3con.c:12059:22
#11 0x487fbf in ssl3_HandleRecord /src/nss/lib/ssl/ssl3con.c:12316:12

I attached the test case.
To reproduce:
* ./ --fuzz
* nssfuzz-tls-client tls13_GetHash
Looks like we don't always set the suite_def correctly on retry.
Flags: needinfo?(martin.thomson)
Oh, that's nasty.  Another win for the fuzzer.  We compare ServerHello.random with the HelloRetryRequest special value and assume HelloRetryRequest even if the selected version isn't TLS 1.3.

Two choices:

1. pretend it isn't HRR if version < 1.3
2. explode if it is HRR and version < 1.3

The odds of a collision are low enough that I could care less which one it is.  ekr, wdyt?
Flags: needinfo?(martin.thomson) → needinfo?(ekr)
I think #2 is better.
Flags: needinfo?(ekr)
OK, patch incoming, but first some analysis.

The bug here is caused by our code assuming a HelloRetryRequest based solely on the value of ServerHello.random.  We don't cross-check with the negotiated version.  That leads to us hitting the TLS 1.3 HelloRetryRequest processing while things like the negotiated version and cipher suite not being correct for TLS 1.3.  That leads to some of our assumptions being wrong.  In this case, it's the properties of the cipher suite, but there are probably a few more assertions scattered around that we might hit.

It's bad in that there is a logic error, but the odds of actually getting this random are infinitesimal (2^-256), so the only potential advantage here is to an attacker.

I checked whether this would crash, first by disabling assertions and then with an ASAN build.  It doesn't.  All we get is another ClientHello.  That causes the handshake to fail.  A MitM attacker can do much worse.

Based on that, I'm going to say that we mark this sec-low and let the fix land as normal.  No need to uplift.

We'll hold onto the fix until we're sure there are no latent issues, just to be conservative.
Assignee: nobody → martin.thomson
Keywords: sec-low
OS: Unspecified → All
Hardware: Unspecified → All
Target Milestone: --- → 3.37
Comment on attachment 8962994 [details]
Bug 1449160 - Test for HelloRetryRequest random values, r?franziskus

Franziskus Kiefer [:fkiefer or :franziskus] has approved the revision.
Attachment #8962994 - Flags: review+
Franziskus, do you know why we found this recently? This defect has been there a while.
Flags: needinfo?(franziskuskiefer)
We found this recently because the fuzzer, running 24/7, found an input that triggered it. As mt said, finding an input that triggers this is not very likely. I think finding this with the fuzzer shows that it is working pretty well.
The fault could've been found earlier in review or with an appropriate test (like the one mt wrote now). But this type of logic error is pretty hard to catch.
Flags: needinfo?(franziskuskiefer)
Closed: 3 years ago
Resolution: --- → FIXED
Group: crypto-core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.