1207298 - MSan: use-of-uninitialized-value in ReadHuffmanCode

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[Tyson Smith [:tsmith]]

Attachment: 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

Comment 1

[Tyson Smith [:tsmith]]

Attachment: test_case.compressed

Comment 2

[Jyrki Alakuijala]

sorry, missed this -- we will look at this tomorrow

Comment 3

[Jyrki Alakuijala]

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

[Eugene Kliuchnikov]

We found the problem. Fix will be committed on Monday.

Comment 5

[Patrick McManus [:mcmanus]]

(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

Comment 6

[Eugene Kliuchnikov]

It is already landed to github (PR #183, commit 933bb9bd800c8f5f7f6a02382d33c902a98ef73a)

Comment 7

[Jyrki Alakuijala]

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).

Comment 8

[Patrick McManus [:mcmanus]]

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.

Comment 9

[Patrick McManus [:mcmanus]]

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.

Comment 10

[Patrick McManus [:mcmanus]]

Attachment: update brotli library

Comment 11

[Jyrki Alakuijala]

If you need our help with testing the old brotli versions, we would be happy to help.

Comment 12

[Daniel Veditz [:dveditz]]

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.

Comment 13

[Patrick McManus [:mcmanus]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/532703f2629fac53bd762bfd0b1704bc06ed5301
bug 1207298 - update brotli library r=jfkthame

Comment 14

[Patrick McManus [:mcmanus]]

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

Comment 15

[Patrick McManus [:mcmanus]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3c6d5e64a11d

Comment 16

[Patrick McManus [:mcmanus]]

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

[Eugene Kliuchnikov]

We're working on it now.

Comment 18

[Eugene Kliuchnikov]

We have a fix for it. It will be pushed to github on Monday.

Comment 19

[Patrick McManus [:mcmanus]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8881708f5c15249df6e9fefaa3fb2af5b1e98711
bug 1207298 - update brotli library r=jfkthame

Comment 20

[Patrick McManus [:mcmanus]]

(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.

Comment 21

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/8881708f5c15