1207667 - heap-buffer-overwrite in BrotliFileInputFunction

Status: VERIFIED FIXED

(Core :: Networking: HTTP, defect)

Description

[Tyson Smith [:tsmith]]

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: valgrid_output.txt

Comment 2

[Tyson Smith [:tsmith]]

Attachment: test_case.compressed

Comment 3

[Jyrki Alakuijala]

We are looking at this

Comment 4

[Zoltan Szabadka]

I could reproduce this on the github version, and confirmed that it is already fixed internally. We will push a new decoder to google/brotli repo soon.

Comment 5

[Zoltan Szabadka]

I updated https://github.com/google/brotli, could you verify that it is fixed?

Comment 6

[Tyson Smith [:tsmith]]

Attachment: valgrid_output.txt

I am still able to reproduce this issue.

I did a git pull and I am not at https://github.com/google/brotli/commit/24dca87f96dcd29a2803ffa697ea1d823d1ea703.

Comment 7

[Tyson Smith [:tsmith]]

Attachment: test_case.compressed

updated test case

Comment 8

[Tyson Smith [:tsmith]]

(In reply to Tyson Smith [:tsmith] from comment #6)
> Created attachment 8665675 [details]
> valgrid_output.txt
> 
> I am still able to reproduce this issue.
> 
> I did a git pull and I am not at
> https://github.com/google/brotli/commit/
> 24dca87f96dcd29a2803ffa697ea1d823d1ea703.

Typo :)
I am now at https://github.com/google/brotli/commit/24dca87f96dcd29a2803ffa697ea1d823d1ea703

Comment 9

[Zoltan Szabadka]

We fixed one more bug in decoder, the new test case is fixed in the latest commit:
https://github.com/google/brotli/commit/6dd53d618377e0efd3b0d2de9f03c284ccf81695

Comment 10

[Tyson Smith [:tsmith]]

That seems to have fixed the bug.

Comment 11

[Andrew McCreight [:mccr8]]

This bug should remain open until the upstream fix has landed on mozilla-central.

Comment 12

[Andrew McCreight [:mccr8]]

[Tracking Requested - why for this release]:

Comment 13

[Patrick McManus [:mcmanus]]

I can verify that cset from github 933bb9bd800c8f5f7f6a02382d33c902a98ef73a makes valgrind pass the testcase. I can also verify that the cset we are shipping on >=43 for woff2 also passes the testcase.

Comment 14

[Patrick McManus [:mcmanus]]

bug 1207298 checked in a library update that  will also resolve this issue.

Comment 15

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Tracked as it's sec-critical.

Comment 16

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

FF44 status is fixed based on comment 14.

Comment 17

[Patrick McManus [:mcmanus]]

I'm clearing the ni flag because I don't see a question to be answered.

Comment 18

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

(In reply to Patrick McManus [:mcmanus] from comment #17)
> I'm clearing the ni flag because I don't see a question to be answered.

Sorry about that. :)

Comment 19

[Matt Wobensmith [:mwobensmith][:matt:]]

Tyson, can you verify fixed? Thanks!