Update Brotli to latest upstream revision

RESOLVED FIXED in Firefox 45

Status

()

Core
Graphics: Text
RESOLVED FIXED
a year ago
3 months ago

People

(Reporter: fredw, Assigned: fredw)

Tracking

({sec-high})

Trunk
mozilla47
sec-high
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox44 wontfix, firefox45+ fixed, firefox46+ fixed, firefox47+ fixed, firefox-esr38 disabled, firefox-esr45 fixed)

Details

(Whiteboard: [adv-main45+])

Attachments

(2 attachments, 6 obsolete attachments)

208.75 KB, patch
Details | Diff | Splinter Review
1.28 KB, patch
mcmanus
: review+
abillings
: sec-approval+
Details | Diff | Splinter Review
(Assignee)

Description

a year ago
I'm moving this step from bug 1227058 to its own bug.

It seems that the API to decompress brotli streams has changed a few days ago and its no longer necessary to do a final call to BrotliDecompressBufferStreaming with finish=1
(Assignee)

Comment 1

a year ago
Created attachment 8712086 [details] [diff] [review]
Update Brotli to latest upstream revision ; now at 6c98f033e0c17725f4584c17d692eacbbb3d13da
(Assignee)

Comment 2

a year ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=078d9c0e6111
(Assignee)

Comment 3

a year ago
Comment on attachment 8712086 [details] [diff] [review]
Update Brotli to latest upstream revision ; now at 6c98f033e0c17725f4584c17d692eacbbb3d13da

Review of attachment 8712086 [details] [diff] [review]:
-----------------------------------------------------------------

@Jonathan: Please especially look at nsHTTPCompressConv.cpp. The change to handle the end of the stream seems to keep test_content_encoding_gzip.js passing, but I'm not familiar with this code / brotli to be sure it's correct.
Attachment #8712086 - Flags: review?(jfkthame)
Comment on attachment 8712086 [details] [diff] [review]
Update Brotli to latest upstream revision ; now at 6c98f033e0c17725f4584c17d692eacbbb3d13da

Review of attachment 8712086 [details] [diff] [review]:
-----------------------------------------------------------------

honestly, I would have this refuzzed before landing. We've discovered issues before.

::: netwerk/streamconv/converters/nsHTTPCompressConv.cpp
@@ +194,5 @@
>        self->mBrotli->mStatus = NS_ERROR_INVALID_CONTENT_ENCODING;
>        return self->mBrotli->mStatus;
>      }
> +    if (!stream) {
> +      return avail == 0 && res == BROTLI_RESULT_SUCCESS ?

so we generally allow truncated formats.. webcompat tends to require it. so if there are no other errors than the fact that we seem to have an early EOF - I think it would be ok to return NS_OK
(Assignee)

Comment 5

a year ago
(In reply to Patrick McManus [:mcmanus] from comment #4)
> honestly, I would have this refuzzed before landing. We've discovered issues
> before.

Right, that makes sense. I'm cc'ing the fuzzing team.

> > +    if (!stream) {
> > +      return avail == 0 && res == BROTLI_RESULT_SUCCESS ?
> 
> so we generally allow truncated formats.. webcompat tends to require it. so
> if there are no other errors than the fact that we seem to have an early EOF
> - I think it would be ok to return NS_OK

IIUC, you're suggesting to return NS_OK iff BROTLI_RESULT_SUCCESS (i.e. remove avail == 0). In that case, test_content_encoding_gzip.js still passes for me. It does not if I either remove this early return, or always return success or always return failure.
Comment on attachment 8712086 [details] [diff] [review]
Update Brotli to latest upstream revision ; now at 6c98f033e0c17725f4584c17d692eacbbb3d13da

Review of attachment 8712086 [details] [diff] [review]:
-----------------------------------------------------------------

In principle, I'm fine with updating Brotli as far as its use in WOFF2 is concerned. Clearing r? flag here, though, as I wouldn't be the right person to review this in the context of content-encoding support, and Patrick is already looking at it... (thanks!)
Attachment #8712086 - Flags: review?(jfkthame)
(Assignee)

Comment 7

a year ago
Created attachment 8712220 [details] [diff] [review]
Update Brotli to latest upstream revision ; now at 616e11a01ded93edcc3535ce4a79339ef6c74211

Updating to latest revision and removing "avail == 0".
Attachment #8712086 - Attachment is obsolete: true
Attachment #8712220 - Flags: review?(mcmanus)
Comment on attachment 8712220 [details] [diff] [review]
Update Brotli to latest upstream revision ; now at 616e11a01ded93edcc3535ce4a79339ef6c74211

Review of attachment 8712220 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/streamconv/converters/nsHTTPCompressConv.cpp
@@ +181,5 @@
>  
>      // brotli api is documented in brotli/dec/decode.h
>      LOG(("nsHttpCompresssConv %p brotlihandler decompress %d finish %d\n",
>           self, avail, !stream));
> +    res = ::BrotliDecompressStream(

so the !stream case is a synthetic EOF. there is no new data then.. the old api required it be called with the finish flag because it buffered output in brotli internaly. if it no longer does that, then the calling pattern that creates the !stream case should be removed and this can just assert(stream).

does the new library buffer output?

@@ +201,2 @@
>  
>      // in 'the current implementation' brotli consumes all input on success

the comment in the brotli lib that this is based on has disappeared.. (it was in the brotlidecompressbufferstreaming() function. Is it still an invariant? If not, this code is going to need some changes beacuse some input could be lost.
Attachment #8712220 - Flags: review?(mcmanus)
(Assignee)

Updated

a year ago
No longer blocks: 1227058
(Assignee)

Comment 9

a year ago
(In reply to Patrick McManus [:mcmanus] from comment #8)
> ::: netwerk/streamconv/converters/nsHTTPCompressConv.cpp
> @@ +181,5 @@
> >  
> >      // brotli api is documented in brotli/dec/decode.h
> >      LOG(("nsHttpCompresssConv %p brotlihandler decompress %d finish %d\n",
> >           self, avail, !stream));
> > +    res = ::BrotliDecompressStream(
> 
> so the !stream case is a synthetic EOF. there is no new data then.. the old
> api required it be called with the finish flag because it buffered output in
> brotli internaly. if it no longer does that, then the calling pattern that
> creates the !stream case should be removed and this can just assert(stream).

Yes, I agree with that. Yesterday I tried to just remove the BrotliHandler call from nsHTTPCompressConv::OnStopRequest, but that made test_content_encoding_gzip.js fail. As I said in comment 5, there seem to be a need to set a failure status when the internal brotli state is not BROTLI_RESULT_SUCCESS. So we'll have to read that internal state it in some way, although we can probably just check mBrotli->mState in nsHTTPCompressConv::OnStopRequest without calling BrotliHandler at all.

> 
> does the new library buffer output?
> 

I don't know, the API doc does not mention that so I'll have to read the C++ source.

> @@ +201,2 @@
> >  
> >      // in 'the current implementation' brotli consumes all input on success
> 
> the comment in the brotli lib that this is based on has disappeared.. (it
> was in the brotlidecompressbufferstreaming() function. Is it still an
> invariant? If not, this code is going to need some changes beacuse some
> input could be lost.

Same here.
(Assignee)

Comment 10

a year ago
Created attachment 8712685 [details] [diff] [review]
Update Brotli to latest upstream revision ; now at 058d7498a11f19d53d1037df8a4a8e34efc05a4b

> we can probably just check mBrotli->mState in nsHTTPCompressConv::OnStopRequest without calling BrotliHandler at all.

Done. The test I mentioned still passes.

> does the new library buffer output?

As I understand the code, the only thing brotli does with (next_out, available_out) is to copy the output data into next_out, move next_out forward and decrement available_out. This is also the only thing the doc says (in addition to the fact that the buffer must be allocated). So always passing (outBuffer, kOutSize) and forgetting the previous output after calling do_OnDataAvailable as we currently do should be fine.

> Is it still an invariant? If not, this code is going to need some changes
> beacuse some input could be lost.

OK, the invariants are still indicated above BrotliDecompressStream. The input is only guaranteed to be completely consumed when brotli "needs more input". So I've only kept the verification in that case (although I'm not sure it's really necessary) and just updated the calculation of countRead. Reading the doc of nsIInputStream, I believe this should work but again I'm not really familiar with the network code so I'd appreciate more feedback / testing.
Attachment #8712220 - Attachment is obsolete: true
Attachment #8712685 - Flags: feedback?(mcmanus)
:fredw, when this gets ready to land, please make sure to refresh the patch again so that we pick up the upstream fix for the memcpy portability issue, see bug 1224291 comment 13.
Flags: needinfo?(fred.wang)
(Assignee)

Comment 12

a year ago
Thanks, I'll do that. IIUC, the '-DBROTLI_BUILD_PORTABLE' flag won't be necessary once we take https://github.com/google/brotli/pull/308
Flags: needinfo?(fred.wang) → needinfo?(jfkthame)
splinter refuses to show me the diff. So bear with me as I make comments out of band:

> input is only guaranteed to be completely consumed when brotli "needs more
> input".

the old version of decode.h gave that guarantee - but I don't see it in the new code? It seems to me that the input could be partially consumed as long as it was reflected in the available_in response parameter. which means we don't want to return anymore on needs_more_input where avail > 0.

+      *countRead = aAvail - avail;

so that's a red flag when we're returning NS_OK, because this is on the stack from OnDataAvailable and the contract for ODA says you need to deal with all the bytes. so if avail isn't 0 and we aren't reporting an error, something is going to break ;)

+    if (!BrotliStateIsStreamEnd(&mBrotli->mState)) {
+      status = NS_ERROR_NET_PARTIAL_TRANSFER;
+    }

so I would just LOG and NS_WARNING this. We've found in real life that silently truncated compression formats are a real thing and we don't generally break interop over them.

lastly, what do you think about keeping BUILD_PORTABLE? is there some reason to think we don't want it? The one particular error it would have fixed has been fixed upstream, but I think there are likely more and we should keep it.. or has it been totally integrated by default upstream?
Attachment #8712685 - Flags: feedback?(mcmanus)
(Assignee)

Comment 14

a year ago
(In reply to Patrick McManus [:mcmanus] from comment #13)
> splinter refuses to show me the diff. So bear with me as I make comments out
> of band:
> 
> > input is only guaranteed to be completely consumed when brotli "needs more
> > input".
> 
> the old version of decode.h gave that guarantee - but I don't see it in the
> new code? It seems to me that the input could be partially consumed as long
> as it was reflected in the available_in response parameter. which means we
> don't want to return anymore on needs_more_input where avail > 0.

So I was reading https://github.com/google/brotli/blob/master/dec/decode.c#L1843

"when result is "needs more input", [...] all input data MUST be consumed by decoder, so client could swap the input buffer"

But the input can be only partially consumed in case of "success".

> +      *countRead = aAvail - avail;
> 
> so that's a red flag when we're returning NS_OK, because this is on the
> stack from OnDataAvailable and the contract for ODA says you need to deal
> with all the bytes. so if avail isn't 0 and we aren't reporting an error,
> something is going to break ;)

OK, I see. So it seems that we could modify nsHTTPCompressConv::OnDataAvailable to accumulate unconsumed bytes in mInpBuffer and send them back to brotli when more data arrives...

> +    if (!BrotliStateIsStreamEnd(&mBrotli->mState)) {
> +      status = NS_ERROR_NET_PARTIAL_TRANSFER;
> +    }
> 
> so I would just LOG and NS_WARNING this. We've found in real life that
> silently truncated compression formats are a real thing and we don't
> generally break interop over them.

As I said returning success will break test_content_encoding_gzip.js, but I didn't check the details of the test...

> lastly, what do you think about keeping BUILD_PORTABLE? is there some reason
> to think we don't want it? The one particular error it would have fixed has
> been fixed upstream, but I think there are likely more and we should keep
> it.. or has it been totally integrated by default upstream?

As I understood, upstream just fixed some errors without enabling BUILD_PORTABLE, so I was wondering whether it was still necessary. But I guess it does not hurt to keep it in moz.build, so I'll do that.
Flags: needinfo?(jfkthame)
(In reply to Frédéric Wang (:fredw) from comment #14)
>
> https://github.com/google/brotli/blob/master/dec/decode.c#L1843

bingo. thanks. That got moved to the c from the header where it was before.. I thought it had just been removed. If that's the case, the logic looks ok to me. The partial in the case of success can just be ignored (i.e. you can still return aAvail) as trailing garbage. We don't have the compression encodings do their own framing.


> 
> > +    if (!BrotliStateIsStreamEnd(&mBrotli->mState)) {
> > +      status = NS_ERROR_NET_PARTIAL_TRANSFER;
> > +    }
> > 
> > so I would just LOG and NS_WARNING this. We've found in real life that
> > silently truncated compression formats are a real thing and we don't
> > generally break interop over them.
> 
> As I said returning success will break test_content_encoding_gzip.js, but I
> didn't check the details of the test...
>

I just looked.. its a negative test (invalid brotli) that's only 3 bytes long and the decoder no longer notices with that little input.. 
it looks like in WarmupBitReader the first 8 bytes (architecture dependent) aren't enough to trigger the "real" state machine that would reject things..

I think that works out ok for us.. let's use your !BrotliStateIsStreamEnd test only when mBrotli.mTotalOut == 0.. that way we will allow truncated encodings that really were brotli, but other things should generate errors (such as this test and other random inputs). Have it return NS_ERROR_INVALID_CONTENT_ENCODING instead of partial transfer though.

Thanks!
(Assignee)

Comment 16

a year ago
Created attachment 8713833 [details] [diff] [review]
Update Brotli to latest upstream revision ; now at d4f0cb98411491ade114158f363d8693e89ab473

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fb0043d420f2
Attachment #8712685 - Attachment is obsolete: true
(Assignee)

Comment 17

a year ago
Created attachment 8713973 [details] [diff] [review]
Update Brotli to latest upstream revision ; now at d4f0cb98411491ade114158f363d8693e89ab473

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8818919cbe48
Attachment #8713833 - Attachment is obsolete: true
Attachment #8713973 - Flags: review?(mcmanus)
(Assignee)

Comment 18

a year ago
@Jesse: who is in charge of fuzzing the brotli / network code?
Flags: needinfo?(jruderman)
(In reply to Frédéric Wang (:fredw) from comment #18)
> @Jesse: who is in charge of fuzzing the brotli / network code?

fwiw decoder did it in the past.
Attachment #8713973 - Flags: review?(mcmanus) → review+
(Assignee)

Comment 20

a year ago
@Christian: can you please fuzz attachment 8713973 [details] [diff] [review]?
Flags: needinfo?(jruderman) → needinfo?(choller)
In fact, it was Tyson who did the Brotli fuzzing. Forwarding to him.
Flags: needinfo?(twsmith)
(In reply to Christian Holler (:decoder) from comment #21)
> In fact, it was Tyson who did the Brotli fuzzing. Forwarding to him.

my bad!
:fredw, as long as we're rebasing you probably want to udpate from upstream again to grab this:
https://github.com/google/brotli/pull/309/commits
Flags: needinfo?(fred.wang)
(Assignee)

Comment 24

a year ago
Yes, I saw that commit. But are you asking me to do that now or should we wait for Tyson's fuzzing?
Flags: needinfo?(fred.wang) → needinfo?(mcmanus)
I just wanted to make sure you're aware.. I think I would just update the import to tip of upstream and have tyson fuzz the result. I don't think the api will change on you again.
Flags: needinfo?(mcmanus)
(Assignee)

Comment 26

a year ago
Created attachment 8716253 [details] [diff] [review]
Update Brotli to latest upstream revision ; now at 33aa40220b96cf95ad2b9ba61dc8d7fd2f964f2c.

This is a follow-up patch including the latest upstream changes.
(Assignee)

Updated

a year ago
Attachment #8716253 - Attachment description: Bug 1242904 - Update Brotli to latest upstream revision ; now at 33aa40220b96cf95ad2b9ba61dc8d7fd2f964f2c. → Update Brotli to latest upstream revision ; now at 33aa40220b96cf95ad2b9ba61dc8d7fd2f964f2c.
Got this from Jyrki@google

"probably would be good to add this fix into your version of brotli

https://github.com/google/brotli/commit/37a320dd81db8d546cd24a45b4c61d87b45dcade
https://github.com/google/brotli/commit/68f5bbda76f9b36717a4f9aa91a9d5d88ba584d3

It may have security implications by possible wrapped around addresses."

so I think we should just go ahead and land this (the patch from comment 26 has both csets) without blocking on more fuzzing.
Group: network-core-security
(Assignee)

Comment 28

a year ago
Comment on attachment 8716253 [details] [diff] [review]
Update Brotli to latest upstream revision ; now at 33aa40220b96cf95ad2b9ba61dc8d7fd2f964f2c.

OK, can you just rubber stamp this second part?

Are these latest commits fixing a security issue existing in our current brotli version? If so, we will need to follow security bug approval process, I guess.
Attachment #8716253 - Flags: review?(mcmanus)
Attachment #8716253 - Flags: review?(mcmanus) → review+
(In reply to Frédéric Wang (:fredw) from comment #28)
>  
> Are these latest commits fixing a security issue existing in our current
> brotli version? 

I don't have a testcase, but I would have to say yes. Arbitrary input causes underflows. (either from http, or in a woff I would guess.)

>If so, we will need to follow security bug approval process,
> I guess.

yep..
(Assignee)

Comment 30

a year ago
Created attachment 8716439 [details] [diff] [review]
Update Brotli to latest upstream revision ; now at 33aa40220b96cf95ad2b9ba61dc8d7fd2f964f2c.

Merging 8713973 and 8716253 for landing...
Attachment #8713973 - Attachment is obsolete: true
Attachment #8716253 - Attachment is obsolete: true
(Assignee)

Comment 31

a year ago
Patrick: I don't really have time right now to study exactly what this underflow fix is about nor to write the sec-approval request, so if you have a better understanding feel free to do it.
Flags: needinfo?(mcmanus)
Comment on attachment 8716439 [details] [diff] [review]
Update Brotli to latest upstream revision ; now at 33aa40220b96cf95ad2b9ba61dc8d7fd2f964f2c.

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
The general area would be identified (brotli bug fixes upstream) - I'm not aware of any exploits.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
see above

Which older supported branches are affected by this flaw?
all

If not all supported branches, which bug introduced the flaw?

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
the esr38 backport would be tricky. Probably best for esr 45 to be the deployment vector

How likely is this patch to cause regressions; how much testing does it need?
the patch updates the imported library and passes the unit tests. beyond that it is hard to be confident but I don't think it is very risky.
Flags: needinfo?(mcmanus)
Attachment #8716439 - Flags: sec-approval?
(In reply to Patrick McManus [:mcmanus] from comment #32)
> Do you have backports for the affected branches? If not, how different, hard
> to create, and risky will they be?
> the esr38 backport would be tricky. Probably best for esr 45 to be the
> deployment vector

As I understand it, we didn't enable WOFF2 by default until FF39 (bug 1084026), and didn't support it for content-encoding until FF44 (bug 366559). So I don't believe Brotli is exposed to the web on esr38 unless a user explicitly changes the woff2 pref. Given this, I'd agree that backporting there is probably not necessary/justified.
I'm calling this sec-vector and clearing sec-approval?. This is a library update and does not, as far as I can see here, have a security issue in and of itself (though it probably resolves existing security issues). Just get this into trunk and please nominate it for Aurora and Beta as well. I agree about not backporting ESR38.
status-firefox44: --- → wontfix
status-firefox45: --- → affected
status-firefox46: --- → affected
status-firefox-esr38: --- → wontfix
status-firefox-esr45: --- → affected
tracking-firefox45: --- → +
tracking-firefox46: --- → +
tracking-firefox47: --- → +
Keywords: sec-vector
Attachment #8716439 - Flags: sec-approval?
(Assignee)

Comment 35

a year ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/ad43c7344bdb
Duplicate of this bug: 1246742
(In reply to Al Billings [:abillings] from comment #34)
> I'm calling this sec-vector [...]

sec-vector is used for security bugs in some other software (the OS, a driver, or some helper program) that the browser causes to be exposed to the internet. Sometimes we can block the access, sometimes we just have to wait for the vendor to patch it (and the bug would track our attempts at getting that done).

The Brotli code is only on the user's system because it's built into Firefox, so if there's a security bug there it cannot be sec-vector. If you think it's not a security problem but need it hidden then you could use sec-other, but given comment 27 I'd go with at least sec-moderate
status-firefox-esr38: wontfix → disabled
Keywords: sec-vector → sec-moderate
https://hg.mozilla.org/mozilla-central/rev/ad43c7344bdb
status-firefox47: affected → fixed
Target Milestone: --- → mozilla47
Depends on: 1247301
(Assignee)

Comment 39

a year ago
Comment on attachment 8716439 [details] [diff] [review]
Update Brotli to latest upstream revision ; now at 33aa40220b96cf95ad2b9ba61dc8d7fd2f964f2c.

Approval Request Comment
[Feature/regressing bug #]:
Upgrade third-party library "Brotli" to include security fixes mentioned in comment 27.

[User impact if declined]:
Potential security issues with WOFF2 fonts / Brotli content-encoding, as raised by Google engineer in comment 27.

[Describe test coverage new/current, TreeHerder]:
- WOFF2 support tested by layout/reftests/font-face/woff2-1.html
- Brotli content-encoding tested by netwerk/test/unit/test_content_encoding_gzip.js
- We do not have any test cases exhibiting the potential security issue and we have not fuzzed this latest version of Brotli.

[Risks and why]:
This only upgrades a third-party library and so risks are limited to new bugs that might have been introduced upstream.

[String/UUID change made/needed]: None
Attachment #8716439 - Flags: approval-mozilla-beta?
Attachment #8716439 - Flags: approval-mozilla-aurora?
I am sorry but I don't want to take a patch this big at the end of the 45 cycle.
Can we just cherry-pick the two patches mentioned in comment #27 for 45 and take the uplift in 46?
Flags: needinfo?(fred.wang)
Comment on attachment 8716439 [details] [diff] [review]
Update Brotli to latest upstream revision ; now at 33aa40220b96cf95ad2b9ba61dc8d7fd2f964f2c.

Let's try this in aurora.
Attachment #8716439 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(Assignee)

Updated

a year ago
Flags: needinfo?(fred.wang)
Keywords: checkin-needed
Whiteboard: [please land attachment 8716439 to mozilla-aurora branch]
(Assignee)

Comment 42

a year ago
(In reply to Sylvestre Ledru [:sylvestre] from comment #40)
> I am sorry but I don't want to take a patch this big at the end of the 45
> cycle.
> Can we just cherry-pick the two patches mentioned in comment #27 for 45 and
> take the uplift in 46?

I don't know exactly how much the brotli code has changed, but that's possible. I'll try that later.
Group: network-core-security → core-security-release
https://hg.mozilla.org/releases/mozilla-aurora/rev/30828b78dc38
status-firefox46: affected → fixed
Keywords: checkin-needed
Thanks Frédéric. FYI, for now, as it is sec moderate, I am not planning to take it in beta.
Fuzzing is ongoing. I will report any results I find.
Flags: needinfo?(twsmith)
Chrome has shipped this fix already and will eventually open up their bugs (and link to external-reporter bugs in their bounty rewards lists). We need this in 45 rather than having a possible-chemspill hanging over our heads should that happen before we get to Firefox 46
Keywords: sec-moderate → sec-high
See Also: → bug 1246742
Frédéric, have you been able to find the commits to cherry-pick? A 200k patch at the end of the beta cycle is too risky.
Merci
Flags: needinfo?(fred.wang)
(Assignee)

Comment 48

a year ago
I'll try it this afternoon.
Flags: needinfo?(fred.wang)
(Assignee)

Comment 49

a year ago
Created attachment 8721973 [details] [diff] [review]
Small patch for mozilla-beta

Here is a minimal patch for mozilla-beta. I only built it locally on Linux and verified that the two tests mentioned in comment 39 do not break.
Attachment #8721973 - Flags: review?(mcmanus)
Attachment #8721973 - Flags: feedback?(twsmith)
(Assignee)

Updated

a year ago
Whiteboard: [please land attachment 8716439 to mozilla-aurora branch]
Comment on attachment 8721973 [details] [diff] [review]
Small patch for mozilla-beta

Review of attachment 8721973 [details] [diff] [review]:
-----------------------------------------------------------------

I think you have proposed cherry-pick https://github.com/google/brotli/commit/37a320dd81db8d546cd24a45b4c61d87b45dcade

the mail I got from google indicated we wanted that AND https://github.com/google/brotli/commit/68f5bbda76f9b36717a4f9aa91a9d5d88ba584d3
Attachment #8721973 - Flags: review?(mcmanus) → review-
(Assignee)

Comment 51

a year ago
(In reply to Patrick McManus [:mcmanus] from comment #50)
> Comment on attachment 8721973 [details] [diff] [review]
> Small patch for mozilla-beta
> 
> Review of attachment 8721973 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think you have proposed cherry-pick
> https://github.com/google/brotli/commit/
> 37a320dd81db8d546cd24a45b4c61d87b45dcade
> 
> the mail I got from google indicated we wanted that AND
> https://github.com/google/brotli/commit/
> 68f5bbda76f9b36717a4f9aa91a9d5d88ba584d3

Mmh, the first commit adds some code just after the definition of ringbuffer_end_minus_copy_length, while the second commit moves that new code again just before the comment "update the recent distances cache". So I believe this is equivalent to the proposed patch. This is what you are saying in comment 27
Flags: needinfo?(mcmanus)
got it. yes.
Flags: needinfo?(mcmanus)
Attachment #8721973 - Flags: review- → review+
(Assignee)

Comment 53

a year ago
Comment on attachment 8721973 [details] [diff] [review]
Small patch for mozilla-beta

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

This patches a specific place in the code and the upstream comment indicates it is an underflow so the area is easily identified. The patched code is executed inside a bigger function and so without more careful reading it's not necessarily obvious to me which brotli input would produce the desired sequence of states to make this underflow happen or exploit it. I'm not aware of any exploits, but Google/Mozilla engineers may have one.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

No, but it's easy to figure out that this corresponds to upstream change https://github.com/google/brotli/commit/37a320dd81db8d546cd24a45b4c61d87b45dcade which has mentions "pointer underflow" in a code comment and a commit message.

Which older supported branches are affected by this flaw?

Beta and Release branches are affected. A fix landed in Aurora and Nightly. ESR38 is not affected unless a user explicitly changes the preferences. See comment 33

If not all supported branches, which bug introduced the flaw?

Bug 1084026 exposes the flaw via WOFF2 support and bug 366559 exposes the flaw via content-encoding.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

This is patch for the beta branch. decode.c has not changed between release and beta, so the patch is likely to just work for release too:

http://hg.mozilla.org/releases/mozilla-release/log/tip/modules/brotli/dec/decode.c
http://hg.mozilla.org/releases/mozilla-beta/log/tip/modules/brotli/dec/decode.c

How likely is this patch to cause regressions; how much testing does it need?

This is a minimal patch that only contains underflow check so it is unlikely to change the behavior for valid Brotli input.
Attachment #8721973 - Flags: sec-approval?
Comment on attachment 8721973 [details] [diff] [review]
Small patch for mozilla-beta

[Triage Comment]

I am fine with giving this sec-approval+ but we need to get beta approval from Release Management given where we are in the release cycle.
Flags: needinfo?(rkothari)
Flags: needinfo?(lhenry)
Attachment #8721973 - Flags: sec-approval?
Attachment #8721973 - Flags: sec-approval+
Attachment #8721973 - Flags: approval-mozilla-beta+
al, did you mean to set beta+ in comment 54?
Flags: needinfo?(abillings)
Comment on attachment 8721973 [details] [diff] [review]
Small patch for mozilla-beta

My bad, I meant to set beta?.
Flags: needinfo?(abillings)
Attachment #8721973 - Flags: approval-mozilla-beta+ → approval-mozilla-beta?
Hi Sylvestre, it seems like something we should take. But since this is so late in Beta cycle, I hesitate to decide and approve without asking you. :)
Flags: needinfo?(rkothari) → needinfo?(sledru)
Flags: needinfo?(lhenry)
Comment on attachment 8721973 [details] [diff] [review]
Small patch for mozilla-beta

Many thanks, taking it.
Should be in 45 beta 10
Flags: needinfo?(sledru)
Attachment #8721973 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
   https://hg.mozilla.org/releases/mozilla-beta/rev/4a5d8ade4e3e
status-firefox45: affected → fixed
Attachment #8716439 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
status-firefox-esr45: affected → fixed
Blocks: 1246742
Whiteboard: [adv-main45+]
Flags: needinfo?(choller)
Did we forget to mark this bug RESOLVED FIXED (comment 38) or are we holding it open for some reason? If there is a reason for it to be unresolved please comment and point to the comment in the status whiteboard.
Flags: needinfo?(fred.wang)
(Assignee)

Updated

a year ago
Status: NEW → RESOLVED
Last Resolved: a year ago
Flags: needinfo?(fred.wang)
Resolution: --- → FIXED
Blocks: 1254411
Attachment #8721973 - Flags: feedback?(twsmith)
Group: core-security-release
Blocks: 1280047
You need to log in before you can comment on or make changes to this bug.