Closed
Bug 1207256
Opened 9 years ago
Closed 9 years ago
MSan: use-of-uninitialized-value in BrotliDecompressedSize
Categories
(Core :: Networking: HTTP, defect)
Core
Networking: HTTP
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)
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•9 years ago
|
||
Comment 2•9 years ago
|
||
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.
Comment 3•9 years ago
|
||
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
Updated•9 years ago
|
Flags: needinfo?(jyrki.alakuijala)
Comment 4•9 years ago
|
||
(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•9 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.
Comment 6•9 years ago
|
||
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.
Comment 7•9 years ago
|
||
actually, it might be fatal if it were set too small
Updated•9 years ago
|
Flags: needinfo?(jyrki.alakuijala)
Updated•9 years ago
|
Flags: needinfo?(jyrki.alakuijala)
Comment 8•9 years ago
|
||
I'm trying to replicate this now with the current github version
Flags: needinfo?(jyrki.alakuijala)
Comment 9•9 years ago
|
||
fwiw I could get the same error using valgrind for that testcase. might easier to repro.
Reporter | ||
Comment 10•9 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•9 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•9 years ago
|
||
This should be fixed in the github head now.
Comment 13•9 years ago
|
||
(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.
Updated•9 years ago
|
Group: core-security → network-core-security
Comment 14•9 years ago
|
||
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
Comment 15•9 years ago
|
||
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.
Updated•9 years ago
|
Comment 16•9 years ago
|
||
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.
Comment 17•9 years ago
|
||
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.
Updated•9 years ago
|
Comment 18•9 years ago
|
||
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
Updated•8 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•