tls13_ClientHandleKeyShareXtn() called from non-1.3 code

RESOLVED FIXED in 3.29

Status

NSS
Libraries
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: ttaubert, Assigned: ttaubert)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Assignee)

Description

2 years ago
Created attachment 8822951 [details]
ssltrace.log

At the top of there is this:

> if (ss->version < SSL_LIBRARY_VERSION_TLS_1_3) {
>     /* This can't happen because the extension processing
>      * code filters out TLS 1.3 extensions when not in
>      * TLS 1.3 mode. */
>     PORT_SetError(SEC_ERROR_LIBRARY_FAILURE);
>     return SECFailure;
> }

I added an assertion here because I saw the fuzzer taking this code path. See the attached SSLTRACE log.

Assertion failure: 0, at ../../lib/ssl/tls13exthandle.c:263
==6620== ERROR: libFuzzer: deadly signal
    #0 0x4cc3f0 in __sanitizer_print_stack_trace (/home/worker/dist/Debug/bin/nssfuzz-client+0x4cc3f0)
    #1 0x51251a in fuzzer::Fuzzer::CrashCallback() /home/worker/nss/out/Debug/../../fuzz/libFuzzer/FuzzerLoop.cpp:272:5
    #2 0x5124a2 in fuzzer::Fuzzer::StaticCrashSignalCallback() /home/worker/nss/out/Debug/../../fuzz/libFuzzer/FuzzerLoop.cpp:261:6
    #3 0x54f498 in fuzzer::CrashHandler(int, siginfo_t*, void*) /home/worker/nss/out/Debug/../../fuzz/libFuzzer/FuzzerUtilPosix.cpp:37:3
    #4 0x7f6a9a7ad38f  (/lib/x86_64-linux-gnu/libpthread.so.0+0x1138f)
    #5 0x7f6a9a204427 in gsignal /build/glibc-t3gR2i/glibc-2.23/signal/../sysdeps/unix/sysv/linux/raise.c:54
    #6 0x7f6a9a206029 in abort /build/glibc-t3gR2i/glibc-2.23/stdlib/abort.c:89
    #7 0x7f6a99a80cf9 in PR_Assert /home/worker/nspr/Debug/pr/src/io/../../../../pr/src/io/prlog.c:553:5
    #8 0x7f6a9aac54e9 in tls13_ClientHandleKeyShareXtn /home/worker/nss/out/Debug/../../lib/ssl/tls13exthandle.c:263:9
    #9 0x7f6a9aa574a9 in ssl3_HandleParsedExtensions /home/worker/nss/out/Debug/../../lib/ssl/ssl3ext.c:331:22
    #10 0x7f6a9aa578d7 in ssl3_HandleExtensions /home/worker/nss/out/Debug/../../lib/ssl/ssl3ext.c:360:10
    #11 0x7f6a9aa2ff16 in ssl3_HandleServerHello /home/worker/nss/out/Debug/../../lib/ssl/ssl3con.c:6808:18
    #12 0x7f6a9aa2c468 in ssl3_HandleHandshakeMessage /home/worker/nss/out/Debug/../../lib/ssl/ssl3con.c:11813:18
    #13 0x7f6a9aa34cff in ssl3_HandleHandshake /home/worker/nss/out/Debug/../../lib/ssl/ssl3con.c:12005:18
    #14 0x7f6a9aa31b9c in ssl3_HandleRecord /home/worker/nss/out/Debug/../../lib/ssl/ssl3con.c:12773:22
    #15 0x7f6a9aa676fb in ssl3_GatherCompleteHandshake /home/worker/nss/out/Debug/../../lib/ssl/ssl3gthr.c
    #16 0x7f6a9aa71098 in ssl_GatherRecord1stHandshake /home/worker/nss/out/Debug/../../lib/ssl/sslcon.c:78:10
    #17 0x7f6a9aa7b45a in ssl_Do1stHandshake /home/worker/nss/out/Debug/../../lib/ssl/sslsecur.c:65:14
    #18 0x7f6a9aa7eb45 in SSL_ForceHandshake /home/worker/nss/out/Debug/../../lib/ssl/sslsecur.c:414:14
    #19 0x4f5c07 in LLVMFuzzerTestOneInput /home/worker/nss/out/Debug/../../fuzz/client_target.cc:91:10
    #20 0x5154d0 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) /home/worker/nss/out/Debug/../../fuzz/libFuzzer/FuzzerLoop.cpp:544:13
    #21 0x515cc8 in fuzzer::Fuzzer::RunOne(unsigned char const*, unsigned long) /home/worker/nss/out/Debug/../../fuzz/libFuzzer/FuzzerLoop.cpp:495:3
    #22 0x4f8349 in fuzzer::RunOneTest(fuzzer::Fuzzer*, char const*, unsigned long) /home/worker/nss/out/Debug/../../fuzz/libFuzzer/FuzzerDriver.cpp:267:6
    #23 0x4fdcd8 in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) /home/worker/nss/out/Debug/../../fuzz/libFuzzer/FuzzerDriver.cpp:485:9
    #24 0x4f76b3 in main /home/worker/nss/out/Debug/../../fuzz/libFuzzer/FuzzerMain.cpp:20:10
    #25 0x7f6a9a1ef82f in __libc_start_main /build/glibc-t3gR2i/glibc-2.23/csu/../csu/libc-start.c:291
    #26 0x41dc48 in _start (/home/worker/dist/Debug/bin/nssfuzz-client+0x41dc48)
(Assignee)

Comment 1

2 years ago
Created attachment 8822952 [details]
crash-5a6b36f836b8881d252a383fad2a5bf1545ea45d (fuzz corpus)
(Assignee)

Updated

2 years ago
Blocks: 1177759
(Assignee)

Comment 2

2 years ago
This is contrary to what tls13_ClientHandlePreSharedKeyXtn() does:

> /* If we are doing < TLS 1.3, then ignore this. */
> if (ss->version < SSL_LIBRARY_VERSION_TLS_1_3) {
>     return SECSuccess;
> }

We need to either ignore 1.3-only extensions when the server picks 1.2 or lower, or filter out 1.3-only extensions as tls13_ClientHandleKeyShareXtn() believes we do.
(Assignee)

Comment 3

2 years ago
tls13_ClientHandleEarlyDataXtn() does something else again:

> /* If we are doing < TLS 1.3, then ignore this. */
> if (ss->version < SSL_LIBRARY_VERSION_TLS_1_3) {
>     PORT_SetError(SSL_ERROR_EXTENSION_DISALLOWED_FOR_VERSION);
>     return SECFailure;
> }
(Assignee)

Comment 4

2 years ago
Ekr, what do you think should we do here? Ignore & succeed, Fail, Filter & Assert? I think that the first option, return SECSuccess and carry on, is the easiest to implement and seems correct too if we encounter an extension that TLS 1.2 doesn't know about.
Flags: needinfo?(ekr)

Comment 5

2 years ago
Ouch. Yeah, that first comment is just wrong. We filter out v2 extensions in v3 but not the other way around.

All of these should behave the same way, the way in tls13_ClientHandleEarlyDataXtn() does. There's no legitimate reason for the server to be sending us a 1.3-only extension, so we should be setting SSL_ERROR_EXTENSION_DISALLOWED_FOR_VERSION and returning SECFailure.

ttaubert: do you see any actual sign this is a security problem or are you just being cautious?
Flags: needinfo?(ekr) → needinfo?(ttaubert)
(Assignee)

Comment 6

2 years ago
(In reply to Eric Rescorla (:ekr) from comment #5)
> All of these should behave the same way, the way in
> tls13_ClientHandleEarlyDataXtn() does. There's no legitimate reason for the
> server to be sending us a 1.3-only extension, so we should be setting
> SSL_ERROR_EXTENSION_DISALLOWED_FOR_VERSION and returning SECFailure.

SGTM, will fix.

> ttaubert: do you see any actual sign this is a security problem or are you
> just being cautious?

No, was just cautious until I had time to investigate further. This is not security issue, we handle/ignore 1.3-only extensions mostly right.
Group: crypto-core-security
Flags: needinfo?(ttaubert)
(Assignee)

Updated

2 years ago
Assignee: nobody → ttaubert
Status: NEW → ASSIGNED
(Assignee)

Comment 8

2 years ago
https://hg.mozilla.org/projects/nss/rev/6a0943df7cd7
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → 3.29
You need to log in before you can comment on or make changes to this bug.