Note: There are a few cases of duplicates in user autocompletion which are being worked on.

MSan: use-of-uninitialized-value in ReadHuffmanCode

RESOLVED FIXED in Firefox 44

Status

()

Core
Networking: HTTP
--
critical
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: tsmith, Assigned: mcmanus)

Tracking

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

unspecified
mozilla44
csectype-uninitialized, sec-low, testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox43 wontfix, firefox44 fixed, firefox-esr38 unaffected)

Details

(Whiteboard: [adv-main44+])

Attachments

(3 attachments)

(Reporter)

Description

2 years ago
Created attachment 8664417 [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 8664419 [details]
test_case.compressed
(Assignee)

Updated

2 years ago
Flags: needinfo?(jyrki.alakuijala)
Group: core-security → network-core-security

Updated

2 years ago
Flags: needinfo?(jyrki.alakuijala)

Comment 2

2 years ago
sorry, missed this -- we will look at this tomorrow

Comment 3

2 years ago
I am able to reproduce this with the latest github head. 

$ valgrind ./bro --decompress --input corrupt.xyzzy --output /dev/null --force

shows the violators. We will get this fixed on Monday or latest on Tuesday.

Comment 4

2 years ago
We found the problem. Fix will be committed on Monday.
(Assignee)

Comment 5

2 years ago
(In reply to Eugene Kliuchnikov from comment #4)
> We found the problem. Fix will be committed on Monday.

Awesome, please let me know when it lands (cset would be great) and then I'll take a snapshot to gecko to fix 1207298 1207256 and 1207667

Dan - what do you think about backports and landings as it seems plausible (but unknown) that these could impact deployed woff2
Flags: needinfo?(eustas.ru)
Flags: needinfo?(dveditz)

Comment 6

2 years ago
It is already landed to github (PR #183, commit 933bb9bd800c8f5f7f6a02382d33c902a98ef73a)
Flags: needinfo?(eustas.ru)

Comment 7

2 years ago
It is possible that the older version of woff2.0/brotli you have snapshotted has a different collection of security problems. We practically rewrote the decoder during the summer.

Backports would not only fix possible security problems, but also give a significant performance boost. (about 2x speedup). 

If you want to backport brotli issues within woff 2.0, it is likely that you should just backport the whole woff 2.0 for simplicity. We reworked the API between woff 2.0 and brotli somewhere around April, so if your snapshot is older than that April 2015, you need to possibly get the whole woff 2.0 (not sure about this, just thinking aloud).
(Assignee)

Comment 8

2 years ago
The new github repairs this testcase (and the ones in 1207256). Thanks!

I also tested against our old snapshot (for woff2) and it did not have a problem with this testcase under valgrind.
(Assignee)

Comment 9

2 years ago
44 (44 introduced the http encoding) will use the same code for both http encodings and woff2. Not sure whether that would impact the ease of a backport.

I'm hoping that because the only known issue with the older code is 1207256 and that is sec-moderate we can just restrict the landing to nightly. Changing a whole library on a channel later in the pipeline is fraught with risk.
(Assignee)

Comment 10

2 years ago
Created attachment 8666811 [details] [diff] [review]
update brotli library
Attachment #8666811 - Flags: review?(jfkthame)
(Assignee)

Updated

2 years ago
Assignee: nobody → mcmanus
Status: NEW → ASSIGNED

Comment 11

2 years ago
If you need our help with testing the old brotli versions, we would be happy to help.
Attachment #8666811 - Flags: review?(jfkthame) → review+
It appears that WOFF2 wasn't enabled until Firefox 39 and if so we don't need to worry about it or brotli on the ESR-38 branch. Unless further testing of WOFF2 on Beta shows a pressing need to fix security bugs then your plan to let the library upgrade ride the trains is good.
Flags: needinfo?(dveditz)
(Assignee)

Comment 13

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/532703f2629fac53bd762bfd0b1704bc06ed5301
bug 1207298 - update brotli library r=jfkthame
(Assignee)

Comment 14

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/636b763d4a927f956674db242f7a7f5b822f79b6
bug 1207298 Backed out changeset 532703f2629f for build failures r=backout CLOSED TREE
(Assignee)

Comment 15

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3c6d5e64a11d
(Assignee)

Comment 16

2 years ago
one of the existing warnings in c code got promoted to an inline macro and thus popped up in directories that enforce fail on warning at compile time.

Comment 17

2 years ago
We're working on it now.
status-firefox44: --- → affected
status-firefox-esr38: --- → unaffected

Comment 18

2 years ago
We have a fix for it. It will be pushed to github on Monday.
(Assignee)

Comment 19

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/8881708f5c15249df6e9fefaa3fb2af5b1e98711
bug 1207298 - update brotli library r=jfkthame
(Assignee)

Comment 20

2 years ago
(In reply to Eugene Kliuchnikov from comment #18)
> We have a fix for it. It will be pushed to github on Monday.

Thanks! The only real problematic part was the one warning from bit_reader.h (signed/unsigned comparison) because that gets included outside of the brotli objects (the brotli objects are whitelisted into having that warning). I was able to fix it with a one line cast and landed that knowing that the next time we do an import there will be a fix upstream already.
https://hg.mozilla.org/mozilla-central/rev/8881708f5c15
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox44: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Group: network-core-security → core-security-release
Depends on: 1213136
status-firefox43: --- → wontfix
Whiteboard: [adv-main44-]
Keywords: sec-low
Whiteboard: [adv-main44-] → [adv-main44+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.