1566482 - Crash [@ CharAt] through [@ mozilla::net::nsHttpTransaction::SetHttpTrailers] with invalid read

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect, P1)

Description

[Christian Holler (:decoder)]

The attached testcase crashes on mozilla-central revision 38aa0201f645+ (build with --enable-tests --enable-address-sanitizer --disable-jemalloc --enable-optimize=-O2 --enable-fuzzing --disable-debug).

For detailed crash information, see attachment.

To reproduce the issue, perform the following steps:

1. Download the attached testcase, save as "test.bin".
2. Apply the patch from bug 1566342 to get the required fuzzing target.
3. Build with --enable-fuzzing (requires Clang and ASan, also build gtests using ./mach gtest dontruntests).
4. Run MOZ_RUN_GTEST=1 LIBFUZZER=1 FUZZER=NetworkHttp2 objdir/dist/bin/firefox test.bin

This issue looks like an actional invalid/out-of-bounds read. I have not investigated yet what is going on, but I will attach a full log as a start. Marking sec-high based on the assumption that this is an invalid read of some sort.

Comment 1

[Christian Holler (:decoder)]

Attachment: Detailed Crash Information

Comment 2

[Christian Holler (:decoder)]

Attachment: Testcase

Comment 3

[Christian Holler (:decoder)]

Attachment: Full log with nsFuzzing:5,nsFuzzingNecko:5,nsHttp:5,nsSocketTransport:5,sync

Comment 4

[Christian Holler (:decoder)]

Attachment: Bug 1566482 - Properly calculate end in SetHttpTrailers. r?dragana

Comment 5

[Christian Holler (:decoder)]

So the problem here is the out-of-bounds access aTrailers[newline - 1] while newline is 0. This looks like an easy fix, confirmed that the patch attached fixes the problem.

I also think this might not be exploitable because end only flows into the creation of an nsDependentCSubstring and nsTDependentSubstring<T>::Rebind checks that start and end are within bounds of the string's length (this would otherwise be a problem if an attacker could get the \r condition right and set end to -1).

Comment 6

[BugBot [:suhaib / :marco/ :calixte]]

:decoder, since this bug is a regression, could you fill (if possible) the regressed_by field?
For more information, please visit auto_nag documentation.

Comment 7

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/integration/autoland/rev/6c522d98565a3b9829be1e79c0a9fc5cdd777cd9
https://hg.mozilla.org/mozilla-central/rev/6c522d98565a

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Given the age of the bug and the lack of reports from in the wild, I'm assuming we can let this fix ride the trains.