Closed Bug 1207256 Opened 9 years ago Closed 9 years ago

MSan: use-of-uninitialized-value in BrotliDecompressedSize

Categories

(Core :: Networking: HTTP, defect)

defect
Not set
critical

Tracking

()

RESOLVED FIXED
Tracking Status
firefox42 --- affected
firefox43 --- affected
firefox44 --- affected
firefox-esr38 --- unaffected

People

(Reporter: tsmith, Unassigned)

References

Details

(Keywords: csectype-uninitialized, sec-moderate)

Attachments

(2 files)

Attached file 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
Attached file 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.
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)
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.
(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 :)
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;'.
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.
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.
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.
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.
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
Closed: 9 years ago
Resolution: --- → FIXED
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: