1786018 - UndefinedBehaviorSanitizer: security/nss/lib/ssl/ssl3con.c:13150:47: runtime error: applying zero offset to null pointer with HTTP/2 Proxy

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[Christian Holler (:decoder)]

The attached testcase crashes on mozilla-central revision 20220819-58a08b3c7632 (build with (metadatabuildFlags not available)).

For detailed crash information, see attachment.

To reproduce the issue, perform the following steps:

1. Download the attached testcase, save as "test.bin".
   2a. Build with --enable-fuzzing (requires Clang and ASan, also build gtests using ./mach gtest dontruntests).
   2b. Alternatively you can download builds from TC using python -mfuzzfetch -a --fuzzing --target firefox gtest (see https://github.com/MozillaSecurity/fuzzfetch).
2. Run FUZZER=NetworkHttp2ProxyHttp2 objdir/dist/bin/firefox test.bin

I'm marking this one s-s because I don't know what exactly is happening here and the failure is deep inside critical TLS code. I can't easily test what would happen without UBSan but having null pointers with null offsets there feels wrong in any case. Note that if this is UB, the compiler can reorder/optimize things and potentially worse things can happen.

I also minimized the attached testcase on the byte level to make it easier to figure out what's going on.

Comment 1

[Christian Holler (:decoder)]

Attachment: Detailed Crash Information

Comment 2

[Christian Holler (:decoder)]

Attachment: Testcase

Comment 3

[Kershaw Chang [:kershaw]]

According to this comment, it seems this undefined behavior is fine.

Comment 4

[Kershaw Chang [:kershaw]]

More details:

• The crash is at this line where cText->buf->buf is NULL and ivLen is 0.
• cText->buf->buf is NULL because we got an empty record. This line shows gs->inbuf.len is 0.

I think this is more like an NSS issue, since what necko did is only calling SSL_ForceHandshake.

Dennis, could you take a look?
Thanks.

Comment 5

[Daniel Veditz [:dveditz]]

Doesn't look exploitable, but also we probably didn't want a null pointer there and will crash soon anyway? Or if there's a null check saving us maybe that could be moved earlier.

Comment 6

[Leander Schwarz]

Attachment: Bug 1786018 - Skip zero-length unprotected records in ssl3_HandleRecord() to avoid undefined behavior. Enabled ubsan 'pointer-overflow' check for test coverage. r?djackson

Comment 7

[BugBot [:suhaib / :marco]]

The bug assignee is inactive on Bugzilla, and this bug has severity 'S2'.
:beurdouche, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 8

[Dennis Jackson]

Attachment: Bug 1786018 - Refactor zero length record tests. r=mt

This ensures we properly test the different DTLS / TLS versions and makes the
expected behaviour explicit.

Comment 9

[Dennis Jackson]

Attachment: Bug 1786018 - Add explicit handling of zero length records. r=mt

This is based on the patch developed by Leander in D157183, but is a
little more explicit.

Co-Authored-By: Leander Schwarz

Depends on D176156

Comment 10

[Benjamin Beurdouche [:beurdouche]]

@Dennis, when you have some time, can you land this please ? : ) Thanks !

Comment 11

[Dennis Jackson]

This was landed over a year ago in https://hg.mozilla.org/projects/nss/rev/d7004499cff88b5fd04c41f5c17bfbbbb8679f76