The default bug view has changed. See this FAQ.

MSan: use-of-uninitialized-value in BrotliDecompressedSize

RESOLVED FIXED

Status

()

Core
Networking: HTTP
--
critical
RESOLVED FIXED
2 years ago
9 months ago

People

(Reporter: tsmith, Unassigned)

Tracking

(Blocks: 1 bug, {csectype-uninitialized, sec-moderate})

unspecified
csectype-uninitialized, sec-moderate
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox42 affected, firefox43 affected, firefox44 affected, firefox-esr38 unaffected)

Details

Attachments

(2 attachments)

(Reporter)

Description

2 years ago
Created attachment 8664372 [details]
call_stack.txt

Not sure which component this should go under so I copied bug 366559 for now.

I am fuzzing the latest version of the code from:
https://github.com/google/brotli
(Reporter)

Comment 1

2 years ago
Created attachment 8664373 [details]
test_case.compressed
Note that the brotli github is 7 months newer than what we have and has had many fixes in the meanwhile. Also note that our tree only has the decoder code. Don't know if you were planning to test the whole github repo, but you can skip the encoder.
jyrki.alakuijala what do you think of this?

==12048== WARNING: MemorySanitizer: use-of-uninitialized-value
    #0 0x7fd6d05d9bf6 in BrotliDecompressedSize /home/user/code/brotli/dec/decode.c:801
    #1 0x7fd6d05e1482 in BrotliDecompressStreaming /home/user/code/brotli/dec/decode.c:924
    #2 0x7fd6d05da337 in BrotliDecompress /home/user/code/brotli/dec/decode.c:827
    #3 0x7fd6d0611d40 in main /home/user/code/brotli/tools/bro.cc:177
    #4 0x7fd6cef71ec4 in __libc_start_main /build/buildd/eglibc-2.19/csu/libc-start.c:287
    #5 0x7fd6d03fe31c in _start (/home/user/Desktop/brotli/bro_msan+0x7a31c)

  Uninitialized value was created by an allocation of 's' in the stack frame of function 'BrotliDecompress'
    #0 0x7fd6d05da150 in BrotliDecompress /home/user/code/brotli/dec/decode.c:823
Flags: needinfo?(jyrki.alakuijala)
(In reply to Daniel Veditz [:dveditz] from comment #2)
> Note that the brotli github is 7 months newer than what we have and has

we actually want the current github code - we need the newer streaming interface it provides. Don't need the encode though.
(Reporter)

Comment 5

2 years ago
I would like to note that I have seen a crash while fuzzing with an ASan that I could not reproduce and that is what made me think to try MSan.
looking at this I think the worst thing that can happen is that the decoding ringbuffer gets non optimally sized.. conceivably one more dos available to the server. Jyrki should confirm.

If I've got that right, it means the crash from comment 5 is outstanding.
actually, it might be fatal if it were set too small
Flags: needinfo?(jyrki.alakuijala)
Flags: needinfo?(jyrki.alakuijala)

Comment 8

2 years ago
I'm trying to replicate this now with the current github version
Flags: needinfo?(jyrki.alakuijala)
fwiw I could get the same error using valgrind for that testcase. might easier to repro.
(Reporter)

Comment 10

2 years ago
(In reply to Patrick McManus [:mcmanus] from comment #9)
> fwiw I could get the same error using valgrind for that testcase. might
> easier to repro.

Yeah whatever works for you. I am using MSan for speed while fuzzing :)

Comment 11

2 years ago
I can replicate this with valgrind from the latest github version. It seems simple to fix, but I'm not going to fix it today. We will push a fix in two days.

If you want to make progress with fuzzing in the mean time, you can just completely remove the is_uncompressed test from this function and change it to 'return 0;'.

Comment 12

2 years ago
This should be fixed in the github head now.
(In reply to Patrick McManus [:mcmanus] from comment #4)
> > Note that the brotli github is 7 months newer than what we have and has
> 
> we actually want the current github code - we need the newer streaming
> interface it provides. Don't need the encode though.

I see now that the patches in bug 366559 update our version of brotli.
Group: core-security → network-core-security
I'm setting this to sec-moderate based on comment 6. Please let me know or update it yourself if that is too low.
status-firefox43: --- → unaffected
status-firefox44: --- → affected
Keywords: csectype-uninitialized, sec-moderate
I can verify that cset from github 933bb9bd800c8f5f7f6a02382d33c902a98ef73a makes valgrind pass the testcase.

Unfortunately, I can also say that the cset we are already shipping on >=43 for woff2 fails on the testcase.
status-firefox43: unaffected → affected
status-firefox-esr38: --- → unaffected
I should have said 43 or older (instead of >= which was misleading - my bad).

so 44, 43 and 42..  and esr-38 (woff2 was introduced in 35) are effected - but I think it might be best to wontfix the backports given the relatively low severity and the risk of backporting the whole library.
status-firefox42: --- → affected
status-firefox-esr38: unaffected → affected
AFAICT WOFF2 was not enabled in release until 39, in bug 1084026, which is why I marked 38 as unaffected. Sorry, I should have been clearer in explaining what I was doing.
status-firefox-esr38: affected → unaffected
per https://bugzilla.mozilla.org/show_bug.cgi?id=1207298#c12 we are going to let the brotli library update ride the trains from 44 onwards unless we learn of more severe problems..

bug 1207298 checked in a library update that  will also resolve this issue.
Group: network-core-security → core-security-release
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.