1599545 - tls-server-no_fuzzer_mode: ASSERT: ss->firstHsDone, at ../../lib/ssl/tls13con.c:801

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Kevin Jacobs [:kjacobs]]

Attachment: clusterfuzz-testcase-minimized-tls-server-no_fuzzer_mode-5673973517385728

oss-fuzz has found an issue in the TLS1.3 state machine:

Assertion failure: ss->firstHsDone, at ../../lib/ssl/tls13con.c:801
	UndefinedBehaviorSanitizer:DEADLYSIGNAL
	==1==ERROR: UndefinedBehaviorSanitizer: ABRT on unknown address 0x000000000001 (pc 0x7f58f91fb428 bp 0x7ffd28df9e70 sp 0x7ffd28df9d08 T1)
	    #0 0x7f58f91fb428 in gsignal /build/glibc-LK5gWL/glibc-2.23/sysdeps/unix/sysv/linux/raise.c:54
	    #1 0x7f58f91fd029 in abort /build/glibc-LK5gWL/glibc-2.23/stdlib/abort.c:89
	    #2 0x7f86ec in PR_Assert nspr/pr/src/io/prlog.c:571:5
	    #3 0x491c41 in tls13_HandleKeyUpdate nss/lib/ssl/tls13con.c:801:5
	    #4 0x4c7bb6 in ssl3_HandleHandshakeMessage nss/lib/ssl/ssl3con.c:12075:22
	    #5 0x4cb7a7 in ssl3_HandleHandshake nss/lib/ssl/ssl3con.c:12247:18
	    #6 0x4ca556 in ssl3_HandleNonApplicationData nss/lib/ssl/ssl3con.c:12766:22
	    #7 0x4cc53a in ssl3_HandleRecord nss/lib/ssl/ssl3con.c:13048:12
	    #8 0x4e5f65 in ssl3_GatherCompleteHandshake nss/lib/ssl/ssl3gthr.c:512:18
	    #9 0x4e917c in ssl_GatherRecord1stHandshake nss/lib/ssl/sslcon.c:73:10
	    #10 0x473d33 in ssl_Do1stHandshake nss/lib/ssl/sslsecur.c:41:14

Test case attached. Can be reproduced with ./build.sh --fuzz --asan and LD_LIBRARY_PATH=../dist/Debug/lib/ ../dist/Debug/bin/nssfuzz-tls-server clusterfuzz-testcase-minimized-tls-server-no_fuzzer_mode-5673973517385728.

Disclosure-2020-02-24
Reported-2019-11-26

Comment 1

[Martin Thomson [:mt:]]

Oh, this is so easy. It's not a security problem, just a misplaced/overzealous assertion.

The story here is that the code mistook post-hello for post-handshake and assumed that the function couldn't be called until the handshake was done.

The fix is to remove the assertion. But before doing that we should add a test case where KeyUpdate is inserted prior to handshake completion. It might be enough to change the type of Finished to KeyUpdate.

Comment 2

[Kevin Jacobs [:kjacobs]]

Yep, just working on getting the test to work. Thanks!

Comment 3

[Kevin Jacobs [:kjacobs]]

Attachment: Bug 1599545 - Fix assertion and add test for early Key Update message

Remove an overzealous assertion when a Key Update message is received too early, and add a test for the expected alert condition.

Also adds TlsEncryptedHandshakeMessageReplacer for replacing TLS 1.3 encrypted handshake messages. This is a simple implementation where only the first byte of the message is changed to the new type (so as to trigger the desired handler).

Comment 4

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

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

Comment 5

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

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

Comment 6

[Ryan VanderMeulen [:RyanVM]]

Doesn't sound like this needs backport.