1321612 - nsMultiMixedConv::OnDataAvailable may read beyond a buffer when content is received byte-by-byte

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Honza Bambas (:mayhemer)]

Originally reported as part of bug 1320899.  Separated here, since it's a different bug in a different component.

See https://bugzilla.mozilla.org/show_bug.cgi?id=1320899#c4

Comment 1

[Honza Bambas (:mayhemer)]

I think for the start we should clone and enhance netwerk/test/unit/test_multipart_streamconv.js test to deliver the content in one byte pieces and see what happens.

Comment 2

[Honza Bambas (:mayhemer)]

OK, the code in nsMultiMixedConv::OnDataAvailable is completely broken when sending the data byte by byte.  Simple modification to test_multipart_streamconv.js reveals that very quickly.  The best way is to rewrite that code from scratch what I'm right now going to do.

Comment 3

[Honza Bambas (:mayhemer)]

Attachment: v1

I'll ask for r after a deeper testing.  unit tests under netwerk/ pass for it.

Comment 4

[Honza Bambas (:mayhemer)]

Attachment: v1.1 (backup, missing comments and some last polishing)

This patch makes the image from 1320899 load everytime without any problems.

Comment 5

[Honza Bambas (:mayhemer)]

Comment on attachment 8818129 [details] [diff] [review]
v1.1 (backup, missing comments and some last polishing)

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

Comment 6

[Honza Bambas (:mayhemer)]

Valentin, I may ask you for review here (tomorrow), hence CC.

Comment 7

[Honza Bambas (:mayhemer)]

Attachment: v1.2

- this depends on the patch from bug 1322825 which introduces an incremental tokenizer ; to review this patch it's good to get familiar with the IncrementalToeknizer API from that patch first
- the whole parsing code is rewritten using it
- fixed the artificial call of OnStart/OnStop when the root channel fails (e.g. because of cancel during page reload)
- the body raw data are collected as reference to the tokenizer buffer and sent after every feed of the tokenizer from OnDataAvailable (comments should explain the particulars)
- enhancing the tests for few more cases

Valentin, do you feel up to review this?  I really don't know who else would know this class better..  Feel free to bounce.

There are two things that need to be cared of:

- this code allows both \r\nBOUNDARY and BOUNDARY as after-part delimiter.  we have tests that exercise both, unit tests (referred in this patch) expect the final \r\n not be delivered as part of the body data.  it also conforms [1].  but we have other tests, [2] that don't add \r\n before BOUNDARY and used to pass with the old code ; I can't see in the old code anything that would make a decision whether to expect CRLF or not (e.g. based on the content type) but just making it optional doesn't seem right to me either - what if the ending CRLF happen to be part of a binary content, e.g. image?
- and I forgot the other one...

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

[1] https://www.w3.org/Protocols/rfc1341/7_2_Multipart.html
[2] image/test/reftest/gif/webcam.html

Comment 8

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 8818884 [details] [diff] [review]
v1.2

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

Just a few thoughts before I dive into the logic of the state machine.

::: netwerk/streamconv/converters/nsMultiMixedConv.h
@@ +146,3 @@
>      nsCOMPtr<nsIStreamListener> mFinalListener; // this guy gets the converted data via his OnDataAvailable()
>  
> +    nsCOMPtr<nsIChannel> mChannel; // The channel as we get in in OnStartRequest call

Are we sure this isn't creating a cycle? channel->nsMultiMixedConv->channel

@@ +151,5 @@
>      nsCOMPtr<nsISupports> mContext;
>      nsCString           mContentType;
>      nsCString           mContentDisposition;
>      uint64_t            mContentLength;
>      

nit: since we're here, let's delete this whitespace line.

@@ +164,5 @@
> +    // This flag is dropped first time we create a part channel.
> +    // We use it to prevent duplicated OnStopRequest call on the listener
> +    // when we from some reason fail to ever create a part channel that
> +    // ensure correct notifications.
> +    bool                mNeverNotifiedRequestListener;

nit: having 'never' in the variable name makes it harder to figure out what !mNeverNotifiedListener would mean :) mDidNotifyRequestListener is better I think.

::: netwerk/test/unit/test_multipart_streamconv.js
@@ +10,5 @@
>  function make_channel(url) {
>    return NetUtil.newChannel({uri: url, loadUsingSystemPrincipal: true});
>  }
>  
> +var multipartBody = "--boundary\r\n\r\nSome text\r\n--boundary\r\nContent-Type: text/x-test\r\n\r\n<?xml version='1.1'?>\r\n<root/>\r\n--boundary\r\n\r\n<?xml version='1.0'?><root/>\r\n--boundary--";

The way I read the RFC is a missing CRLF should only be accepted before the first boundary. It seems all our tests work like that.
Maybe we should change this to:
var multipartBody = "\r\n--boundary\r\n\r\nSome text\r\n--boundary\r\nContent-Type: text/x-test\r\n\r\n<?xml version='1.1'?>\r\n<root/>\r\n--boundary\r\n\r\n<?xml version='1.0'?><root/>\r\n--boundary--";

::: netwerk/test/unit/test_multipart_streamconv_missing_CRLF_before_boundary.js
@@ +10,5 @@
>  function make_channel(url) {
>    return NetUtil.newChannel({uri: url, loadUsingSystemPrincipal: true});
>  }
>  
> +var multipartBody = "--boundary\r\n\r\nSome text--boundary\r\n\r\n<?xml version='1.0'?><root/>--boundary--";

It also feels odd to me that the CRLF would be an optional part of the encapsulation line. But the current parser is very permissive - it eats up one CRLF preceding the boundary - so we can look at maybe making it more strict in a folloup bug.

Also, I would have thought that we need a test to check that we ignore the preamble and epilogue.  test_multipart_streamconv_missing_lead_boundary.js assumes that the preamble is the first part of the data, but I don't see the reason for going against the RFC in bug 678978.

::: netwerk/test/unit/test_multipart_streamconv_missing_boundary_lead_dashes.js
@@ +10,5 @@
>  function make_channel(url) {
>    return NetUtil.newChannel({uri: url, loadUsingSystemPrincipal: true});
>  }
>  
> +var multipartBody = "\r\nSome text\r\nboundary\r\n\r\n<?xml version='1.0'?><root/>\r\nboundary--";

Do we really want to support this case? The input doesn't look like multipart to me. Is this in bug 1320899 ?
Do any other multipart parsers accept it as valid?

Comment 10

[Honza Bambas (:mayhemer)]

(In reply to Valentin Gosu [:valentin] from comment #8)
> Comment on attachment 8818884 [details] [diff] [review]
> v1.2
> 
> Review of attachment 8818884 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Just a few thoughts before I dive into the logic of the state machine.
> 
> ::: netwerk/streamconv/converters/nsMultiMixedConv.h
> @@ +146,3 @@
> >      nsCOMPtr<nsIStreamListener> mFinalListener; // this guy gets the converted data via his OnDataAvailable()
> >  
> > +    nsCOMPtr<nsIChannel> mChannel; // The channel as we get in in OnStartRequest call
> 
> Are we sure this isn't creating a cycle? channel->nsMultiMixedConv->channel

Does, but it's definitely broken after OnStopRequest (the 'channel' releases mListener).

> nit: having 'never' in the variable name makes it harder to figure out what
> !mNeverNotifiedListener would mean :) mDidNotifyRequestListener is better I
> think.

Yeah, will rename it :)

> 
> ::: netwerk/test/unit/test_multipart_streamconv.js
> @@ +10,5 @@
> >  function make_channel(url) {
> >    return NetUtil.newChannel({uri: url, loadUsingSystemPrincipal: true});
> >  }
> >  
> > +var multipartBody = "--boundary\r\n\r\nSome text\r\n--boundary\r\nContent-Type: text/x-test\r\n\r\n<?xml version='1.1'?>\r\n<root/>\r\n--boundary\r\n\r\n<?xml version='1.0'?><root/>\r\n--boundary--";
> 
> The way I read the RFC is a missing CRLF should only be accepted before the
> first boundary. It seems all our tests work like that.

As I read it, there should always be CRLF, even before the first boundary.  But our tests show something else, so I rather went with what our tests expect.

> Maybe we should change this to:
> var multipartBody = "\r\n--boundary\r\n\r\nSome
> text\r\n--boundary\r\nContent-Type: text/x-test\r\n\r\n<?xml
> version='1.1'?>\r\n<root/>\r\n--boundary\r\n\r\n<?xml
> version='1.0'?><root/>\r\n--boundary--";
> 
> :::
> netwerk/test/unit/test_multipart_streamconv_missing_CRLF_before_boundary.js
> @@ +10,5 @@
> >  function make_channel(url) {
> >    return NetUtil.newChannel({uri: url, loadUsingSystemPrincipal: true});
> >  }
> >  
> > +var multipartBody = "--boundary\r\n\r\nSome text--boundary\r\n\r\n<?xml version='1.0'?><root/>--boundary--";
> 
> It also feels odd to me that the CRLF would be an optional part of the
> encapsulation line. 

You are right in this case.  The CRLF must be present before a boundary when between parts.  I don't remember why I allowed that.  But RFC and test say CRLF is expected here.  It's easy to change in the parser.

> Also, I would have thought that we need a test to check that we ignore the
> preamble and epilogue.  test_multipart_streamconv_missing_lead_boundary.js
> assumes that the preamble is the first part of the data, but I don't see the
> reason for going against the RFC in bug 678978.

-        // this is the first OnData() for this request. some servers
-        // don't bother sending a token in the first "part." This is
-        // illegal, but we'll handle the case anyway by shoving the
-        // boundary token in for the server.

Fixed in 2001, [1]

https://bugzilla.mozilla.org/show_bug.cgi?id=39987#c36

""" - I've added a check for the first OnDataAvailable() callback. There are servers 
out there that send the first block of data *without* a beginning boundary 
token. This was throwing all the parsing off and leading to crashes. Now if we 
catch this case, we generously insert the boundry token ourselves so the parsing 
thinks it was there all along. """

Not sure if the guy that fixed this was aware of the preamble or not.  But I think it's time to simply remove this "faulty server" compat fix.


> 
> :::
> netwerk/test/unit/test_multipart_streamconv_missing_boundary_lead_dashes.js
> @@ +10,5 @@
> >  function make_channel(url) {
> >    return NetUtil.newChannel({uri: url, loadUsingSystemPrincipal: true});
> >  }
> >  
> > +var multipartBody = "\r\nSome text\r\nboundary\r\n\r\n<?xml version='1.0'?><root/>\r\nboundary--";
> 
> Do we really want to support this case? The input doesn't look like
> multipart to me. Is this in bug 1320899 ?

No.

> Do any other multipart parsers accept it as valid?

This is something the original "parser" was accepting, see bug 38864 and [2].  But we didn't have a test for it.  Maybe it's also time to just get rid of that support.


The question here is if we want to rewrite + change the behavior of the parser or just rewrite it functioning the same.  Maybe we can change the behavior and if there are regressions, take them one by one.


[1] https://github.com/englehardt/gecko-dev/commit/343585ffe88acfc1c64fae36f304a06f7d6ce509
[2] https://github.com/englehardt/gecko-dev/commit/c9b5eee480ebb933524415a87d15a19f48f4a11d

Comment 11

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 8818884 [details] [diff] [review]
v1.2

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

This was actually quite a good rewrite.
The state machine does make it a bit harder to visualize, but it's a lot better than we had before.
Just a few minor issues. I'd really like the missing boundary dashes to be preffed off, but it's up to you if you want to leave it for a followup.

::: netwerk/streamconv/converters/nsMultiMixedConv.cpp
@@ +503,5 @@
> +        return NS_ERROR_CORRUPTED_CONTENT;
> +    }
> +    p.SkipWhites();
> +    Unused << p.ReadUntil(Token::EndOfFile(), mBoundary);
> +    mBoundary.Trim(" \""); // ignoring potential qouted string formatting vialoations

typo: quoted, violations
Also, the old code made sure to read until ; (the attrib variable). Shouldn't we maintain that behaviour?

@@ +531,5 @@
> +
> +    SwitchToControlParsing();
> +
> +    mBoundaryToken =
> +      mTokenizer.AddCustomToken(mBoundary, mTokenizer.CASE_SENSITIVE);

I don't think there should be anyone relying on us accepting a boundary with no leading dashes, but to be sure, let's add a pref for this, and make it false by default. The test would need updating.

@@ +601,5 @@
> +      if (token.Equals(mBoundaryToken)) {
> +        mTokenizer.RemoveCustomToken(mBoundaryTokenWithDashes);
> +        mParserState = BOUNDARY_CRLF;
> +        break;
> +      }

Add comment why this block is necessary, and mention if this is preffed off.

@@ +902,5 @@
> +void
> +nsMultiMixedConv::AccumulateData(Token const & aToken)
> +{
> +    if (!mRawData) {
> +        mRawData = aToken.Fragment().BeginReading();

Please comment on why this is OK to do.
Can we ensure that mRawData does not outlive the underlying buffer?

@@ +971,5 @@
>  
> +    switch (mReponseHeader) {
> +    case HEADER_CONTENT_TYPE:
> +      mContentType = mResponseHeaderValue;
> +      mContentType.CompressWhitespace();

The old version called CompressWhitespace for all mResponseHeader types. I think only HEADER_SET_COOKIE would differ here. Does it matter?

@@ +1012,5 @@
> +      break;
> +    }
> +    case HEADER_UNKNOWN:
> +      // We ignore anything else...
> +      NS_WARNING("Header ignored in multipart content");

I assume other headers will commonly be there, even though they don't mean anything. In this case I would remove the warning.

::: netwerk/streamconv/converters/nsMultiMixedConv.h
@@ +195,5 @@
> +      HEADER_SET_COOKIE,
> +      HEADER_CONTENT_RANGE,
> +      HEADER_RANGE,
> +      HEADER_UNKNOWN
> +    } mReponseHeader;

typo: reponse vs reSponse :)

Comment 12

[Carsten Book [:Tomcat]]

sec-high bug and since this affects 53 it needs now sec-approval

Comment 13

[Honza Bambas (:mayhemer)]

(In reply to Valentin Gosu [:valentin] from comment #11)
> Also, the old code made sure to read until ; (the attrib variable).
> Shouldn't we maintain that behaviour?

Yes, good catch.

> I don't think there should be anyone relying on us accepting a boundary with
> no leading dashes, but to be sure, let's add a pref for this, and make it
> false by default. The test would need updating.

I want this but to only fix the security problem we have here but leave the behavior otherwise unchanged.  So, no, let's do this in a followup and different release cycle.  But I would agree this is a bit archaic.

> > +        mRawData = aToken.Fragment().BeginReading();
> 
> Please comment on why this is OK to do.
> Can we ensure that mRawData does not outlive the underlying buffer?

It's ensured, now comment is added.  I'm not super happy with this either.  We could introduce record/claim logic to the incremental tokenizer to encapsulate the lifetime better.  But followup will do here...  Depends also on review of the inc tokenizer patches.

> The old version called CompressWhitespace for all mResponseHeader types. I
> think only HEADER_SET_COOKIE would differ here. Does it matter?

Since I want exactly the same behavior, then I'll compress cookies as well.  Good catch too.

> I assume other headers will commonly be there, even though they don't mean
> anything. In this case I would remove the warning.

Probably yes.

Comment 14

[Honza Bambas (:mayhemer)]

(In reply to Carsten Book [:Tomcat] from comment #12)
> sec-high bug and since this affects 53 it needs now sec-approval

I'm not sure what is the question for me here.

Comment 15

[Carsten Book [:Tomcat]]

oh was thinking if we can land this now (since this has review and so) since this crash came up in bughunter a few times and so i was reminded of this bug :) 

The sec-apporval comment was that this is needed now after the merge day :) (was a general comment)

Comment 16

[Honza Bambas (:mayhemer)]

(In reply to Honza Bambas (:mayhemer) from comment #10)
> But I
> think it's time to simply remove this "faulty server" compat fix.

Same arg as before - no change in behavior.

> The question here is if we want to rewrite + change the behavior of the
> parser or just rewrite it functioning the same.  Maybe we can change the
> behavior and if there are regressions, take them one by one.

And here too.  Any behavioral changes in a followups and different release.

Comment 17

[Honza Bambas (:mayhemer)]

(In reply to Carsten Book [:Tomcat] from comment #15)
> oh was thinking if we can land this now (since this has review and so) since
> this crash came up in bughunter a few times and so i was reminded of this
> bug :) 
> 
> The sec-apporval comment was that this is needed now after the merge day :)
> (was a general comment)

So, to answer: I have an updated patch to attach (carry r+), just needs a try run.  Then we have to wait for bug 1322825 (second review round) to land since this bug is strongly dep on it.

Comment 18

[Honza Bambas (:mayhemer)]

Attachment: v1.3

- merged to the up to date version of the incremental tokenizer patch
- p.ReadUntil(Token::Char(';'), mBoundary);
- bool mInOnDataAvailable diagnostic assert + hard fail preventing reentry of OnDataAvailable in case thread event queue looping from the mixed converter listeners
- comments cleanup
- CRLF is mandatory before boundary ; test_multipart_streamconv_missing_CRLF_before_boundary.js removed
- set-cookie header has CompressWhitespace
- s/mNeverNotifiedRequestListener/mRequestListenerNotified/

rest left, behavior should not change.


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

Comment 19

[Honza Bambas (:mayhemer)]

Attachment: v1.4 (landed on 54 only)

- added back the ability to omit CRLF before boundary ; I have to make better notes for me next time...  would save me time...  some of our tests simply require this:

TEST-UNEXPECTED-FAIL | image/test/mochitest/test_bug733553.html | Test timed out.
TEST-UNEXPECTED-FAIL | image/test/mochitest/test_webcam.html | timing out after 60000ms. Animated image still doesn't look correct, after poll #1055

and one I've already hit before...:

REFTEST TEST-UNEXPECTED-FAIL | file:///home/worker/workspace/build/tests/reftest/tests/image/test/reftest/jpeg/webcam-simulacrum.mjpg == http://localhost:36320/1486146991205/13/blue.jpg | image comparison, max difference: 255, number of differing pixels: 1



since I don't want to change the behavior I leave it in.  This patch has to fix the sec bug, not any other multi-mixed discrepancies.  I'll file followups for all of them.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=11b4a05cf583cd7577f4fe360ceab99eb91c6cdd

Comment 20

[Honza Bambas (:mayhemer)]

Comment on attachment 8833474 [details] [diff] [review]
v1.4 (landed on 54 only)

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

  question is if this is exploitable at all, I personally don't think so.  the patch is pretty complicated (a complete rewrite).  I believe that no one would ever guess that sending content in extremely small chunks leads to out-of-boundary access which doesn't cause a crash in reality and doesn't allow any remote code execution.

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

Which older supported branches are affected by this flaw?  all of them

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?  

  hmm... this code is old, so it will probably apply cleanly, the bug that blocks this one should be simple to port too (with low/zero risk)

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

  we have few tests exercising this code to a good level, then, I did my best to keep the behavior (acceptance of an input) of the code identical to as it was before.  So, I think a chance for a regressions is small.

Comment 21

[Al Billings [:abillings - ex-MoCo]]

sec-approval+ for trunk. We'll want branch patches for this made and nominated.

Comment 22

[Frederik Braun [:freddy]]

Honza, can you create the branch patches rolling?

Comment 23

[Honza Bambas (:mayhemer)]

Comment 19 for try run.

Comment 24

[Honza Bambas (:mayhemer)]

Comment on attachment 8833474 [details] [diff] [review]
v1.4 (landed on 54 only)

Applies cleanly on m-a, m-b.

Approval Request Comment
[Feature/Bug causing the regression]: since ever
[User impact if declined]: potential read out-of-bound (non-exploitable) ; possible broken multi-mixed content shown to the user
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: no.  the original URL [1][2] that detected this problem is not reliable for testing and I don't know about any other way to manually test this.  We have good automated coverage for this.
[List of other uplifts needed for the feature/fix]: bug 1322825
[Is the change risky?]: there is some risk of regressions since this is a relatively complicated patch, but it is well tested via the automated tests
[Why is the change risky/not risky?]: some risk: it's more of a gut feeling since this is a large change
[String changes made/needed]: none

[1] http://tycho.usno.navy.mil/cgi-bin/nph-usnoclock.gif?zone=UTC&ticks=04
[2] http://tycho.usno.navy.mil/cgi-bin/nph-usnoclock.gif?zone=UTC&ticks=04

Comment 25

[Ryan VanderMeulen [:RyanVM]]

Sounds like this needs an esr45 approval request as well?

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f99ea4baeaff2ee4c049eb2aaf273f6c50620cfe

Comment 27

[Honza Bambas (:mayhemer)]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #25)
> Sounds like this needs an esr45 approval request as well?

Hmmm.. This is going to take some time, since on esr45 the multimixed converter still contains support for packaged apps.  It adds some logic to places this patch rewrites from scratch.

One simple option is to remove support for packaged apps from the converter on esr45.  That feature (packaged apps) was never turned on by default (is behind a pref).  Tho, I suspect that removing the support from only one component it resides at is pretty much a risk when someone out there is using it.

I don't know yet how complicated would be to reimplement the packaged app support on top of the patch for this bug.

Comment 28

[Honza Bambas (:mayhemer)]

(In reply to Honza Bambas (:mayhemer) from comment #27)
> Tho, I suspect that removing the support from only one
> component it resides at is pretty much a risk 

I mean: packaged apps are spread over more places in the codebase than just in the converter and rely on the converter.  No idea what may happen when the convert is broken!

Comment 29

[Ryan VanderMeulen [:RyanVM]]

What info are you requesting from me? I don't think I'm in a position to make a decision RE: packaged app support on ESR45.

Comment 30

[Valentin Gosu [:valentin] (he/him)]

For a bit of background, packaged apps were removed in bug 1307456 which landed in Fx52.
The patches are pretty large, not the things you usually uplift. I backported these patches to esr45 - there were a bunch of conflicts, but all trivial, since they only involve code removal.

I would make a case for uplifting the removal since:
* packaged apps were never used. It is a feature meant for FirefoxOS that was never turned on on desktop
* uplifting Honza's patch, and then trying to reimplement the changes to nsMultiMixedConv seems difficult, error-prone, and would be for no reason since we never shipped the feature.

Another option would be to not uplift bug 1307456, but instead just disable all the packaged app tests.

Comment 31

[Honza Bambas (:mayhemer)]

(In reply to Valentin Gosu [:valentin] from comment #30)
> * uplifting Honza's patch, and then trying to reimplement the changes to
> nsMultiMixedConv seems difficult, error-prone, and would be for no reason
> since we never shipped the feature.

This sounds like a good reason, since you wrote the packed app patch for this, you know best how complicated it would be.

> 
> Another option would be to not uplift bug 1307456, but instead just disable
> all the packaged app tests.

If we leave the mixed converter packed apps unaware, but leave the rest, we should hard disable the whole packed apps feature (the pref set to 'true' would just be ignored).  I don't think it's a good idea to leave this in the tree semi-broken.

Comment 32

[Honza Bambas (:mayhemer)]

Gary, we need a driver decision here.

We want to backport this patch for multimixed converter to ESR 45.  But on 45 there is support for "packed apps", that conflicts largely with this patch.  

The packaged apps support has been removed on 52 (bug 1307456).  The pkg-apps feature has never been enabled by default, so it's probably not used at all.


We have these options: 

1. reimplement the support for packed apps on top of this patch solely for esr45 (only in the mixed converter class).  According comment 30 it's risky and complicated.

2. apply patches from bug 1307456+some more (to be determined) also on esr45.  Merging and collecting all necessary patches is in progress.

3. don't take patch for this bug on esr45 (but I would have to make a deep assessment of if this crash is a vulnerability or not).


Thanks.

Comment 33

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/f99ea4baeaff

Comment 34

[Gerry Chang [:gchang]]

(In reply to Honza Bambas (:mayhemer) from comment #32)
> Gary, we need a driver decision here.
> 
> We want to backport this patch for multimixed converter to ESR 45.  But on
> 45 there is support for "packed apps", that conflicts largely with this
> patch.  
> 
> The packaged apps support has been removed on 52 (bug 1307456).  The
> pkg-apps feature has never been enabled by default, so it's probably not
> used at all.
> 
> 
> We have these options: 
> 
> 1. reimplement the support for packed apps on top of this patch solely for
> esr45 (only in the mixed converter class).  According comment 30 it's risky
> and complicated.
> 
> 2. apply patches from bug 1307456+some more (to be determined) also on
> esr45.  Merging and collecting all necessary patches is in progress.
> 
> 3. don't take patch for this bug on esr45 (but I would have to make a deep
> assessment of if this crash is a vulnerability or not).
> 
> 
> Thanks.

Hi :mayhemer, 
For ESR, we only take sec-crit and sec-high fixes on ESR45. This meets the bar. 
However, the first option is risky and second option is a feature uplift. Both options are too risky for ESR45. So, if we don't take this in esr45, how easily will it be an exploit on ES45?

Comment 35

[Honza Bambas (:mayhemer)]

(In reply to Gerry Chang [:gchang] from comment #34)
> Hi :mayhemer, 
> For ESR, we only take sec-crit and sec-high fixes on ESR45. This meets the
> bar. 
> However, the first option is risky 

agree

> and second option is a feature uplift.

[NI here:]
it's actually feature "downlift".  we want to remove a code of a feature that is disabled by default and not adopted on web at all.

would this change your mind?  just want to make sure you fully understand what we want to do.

> Both options are too risky for ESR45. So, if we don't take this in esr45,
> how easily will it be an exploit on ES45?

I will have to assess this and it will probably take some time...

Comment 36

[Honza Bambas (:mayhemer)]

Attachment: v1 (simple fix for m-a, m-b, esr)

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: fixes read after boundary
User impact if declined: expose to very theoretical and probably unexploitable buffer overflow
Fix Landed on Version: m-c, m-a
Risk to taking this patch (and alternatives if risky): nearly zero, according the try run and local testing, this doesn't break anything
String or UUID changes made by this patch: none


This might be a simple fix for the buffer overflow but not for other potential issues this crappy code may contain.  



regular try: 
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8f45024df706600ef9f19725077ddd2c437802e2

complete run with tests added in a separate patch with ASAN (if there is no ASAN complain, this patch should be good to go): 
https://treeherder.mozilla.org/#/jobs?repo=try&revision=aa2e1eeb553e7b2a3b32d70882734d719b18386b

[1] https://bugzilla.mozilla.org/attachment.cgi?id=8815382

Comment 37

[Honza Bambas (:mayhemer)]

(In reply to Honza Bambas (:mayhemer) from comment #36)
> complete run with tests added in a separate patch with ASAN (if there is no
> ASAN complain, this patch should be good to go): 
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=aa2e1eeb553e7b2a3b32d70882734d719b18386b

hopefully this one will go better..:

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

Comment 38

[Julien Cristau [:jcristau]]

(In reply to Honza Bambas (:mayhemer) from comment #24)
> [User impact if declined]: potential read out-of-bound (non-exploitable) ;
> possible broken multi-mixed content shown to the user

I'm trying to reconcile that "non-exploitable" comment with the sec-high rating, could someone clarify how bad we think the issue is?

Comment 39

[Honza Bambas (:mayhemer)]

(In reply to Julien Cristau [:jcristau] from comment #38)
> (In reply to Honza Bambas (:mayhemer) from comment #24)
> > [User impact if declined]: potential read out-of-bound (non-exploitable) ;
> > possible broken multi-mixed content shown to the user
> 
> I'm trying to reconcile that "non-exploitable" comment with the sec-high
> rating, could someone clarify how bad we think the issue is?

When the server sends the data in very small parts (single bytes) we may try to read one byte after an allocated buffer, exactly when the boundary token is read from the input, but there is no more input buffered after it.  We try to check if there is a '-' character right after the boundary token in the data.  If found, we consider this boundary as the marker of end of the last part.

If an attacker has control over the memory behind this buffer and the server is so specific to send the multipart content in such small chunks (perfectly legal), the attacker could persuade us that we are at the end of the stream - that we have just read the last part of the multipart data.  But more data (more parts) could be coming and I'm not sure what could happen then - the crappy coding of the unpatched mixed converted could go crazy, or the consumer (listener) of the multipart convert could get to an unexpected state.  But that's an undiscovered country and none of those suspicions is confirmed.

Anyway, I've created a patch (comment 36) that might workaround (fix) the buffer overflow on esr w/o the rewrite, w/o the conflict w/ packed apps and w/o a need to uplift the tokenizer changes (blocking bug for this one).



(In reply to Honza Bambas (:mayhemer) from comment #37)
> (In reply to Honza Bambas (:mayhemer) from comment #36)
> > complete run with tests added in a separate patch with ASAN (if there is no
> > ASAN complain, this patch should be good to go): 
> > https://treeherder.mozilla.org/#/
> > jobs?repo=try&revision=aa2e1eeb553e7b2a3b32d70882734d719b18386b
> 
> hopefully this one will go better..:
> 
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=fce0b60efa89bec5015a162d947f1e5b80559fea

No, it did not (pushing from ESR45 to try is probably not a good idea...)

So, here is one more https://treeherder.mozilla.org/#/jobs?repo=try&revision=6f09f9ff729c611c0c0523f4e94c71572d5e813b&filter-classifiedState=unclassified&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception

But there is no log from ASAN about any buffer overflow complains when the test, that should reveal it, is ran on the old (unpatched) code.

Comment 40

[Honza Bambas (:mayhemer)]

(In reply to Honza Bambas (:mayhemer) from comment #39)
> Anyway, I've created a patch (comment 36) that might workaround (fix) the
> buffer overflow on esr w/o the rewrite, w/o the conflict w/ packed apps and
> w/o a need to uplift the tokenizer changes (blocking bug for this one).

Note that we'll release ESR52 in about 3 weeks, so I think for that time the small patch is good enough.

Comment 41

[Honza Bambas (:mayhemer)]

Comment on attachment 8835485 [details] [diff] [review]
v1 (simple fix for m-a, m-b, esr)

Please note we can't land the tests I've added for the regular patch with this patch.  The most important one, test_multipart_streamconv-byte-by-byte.js, fails with patch, because it reveals another problem in the multipart decoder logic, but probably not of a security fashion.

Comment 42

[Julien Cristau [:jcristau]]

Comment on attachment 8833474 [details] [diff] [review]
v1.4 (landed on 54 only)

sec-high fix, for aurora53 and beta52

This should be in 52.0b6 next week.

Comment 43

[Honza Bambas (:mayhemer)]

I'm having a test that can reproduce the ASAN warning:
https://treeherder.mozilla.org/#/jobs?repo=try&author=[e-mail]&fromchange=24b3c112223a5a6a977de04b4f0384885c8bd770&selectedJob=76418363

Here is try run to check the ESR patch fixes the problem or not:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2c35641276e32e9e2439eefba5845795a5146772

Comment 44

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/22a9e0f9e9fd
https://hg.mozilla.org/releases/mozilla-beta/rev/5aeb4828195d

Comment 45

[Honza Bambas (:mayhemer)]

(In reply to Honza Bambas (:mayhemer) from comment #43)
> I'm having a test that can reproduce the ASAN warning:
> https://treeherder.mozilla.org/#/jobs?repo=try&author=honzab.moz@firemni.
> cz&fromchange=24b3c112223a5a6a977de04b4f0384885c8bd770&selectedJob=76418363
> 
> Here is try run to check the ESR patch fixes the problem or not:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=2c35641276e32e9e2439eefba5845795a5146772

Yeah, the esr45 patch fixes the problem.

Comment 46

[Honza Bambas (:mayhemer)]

We have a simpler patch just for esr now.

Comment 47

[Gerry Chang [:gchang]]

Comment on attachment 8835485 [details] [diff] [review]
v1 (simple fix for m-a, m-b, esr)

Fix a sec-high. ESR45+.

Comment 48

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr45/rev/41c80ecafa99

Comment 49

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/5aeb4828195d

Comment 50

[Honza Bambas (:mayhemer)]

Reopening because of bug 1341932.  I think we should for now backout aurora and beta patches for this bug as have been landed and accept the small esr45 patch on those branches.  I'm afraid the fix could be something not easy to uplift.

Comment 51

[Honza Bambas (:mayhemer)]

Attachment: reverse v1.4 from m-a, m-b patch

Reverses the complex patch from m-c, m-a, m-b.

Comment 52

[Honza Bambas (:mayhemer)]

Comment on attachment 8835485 [details] [diff] [review]
v1 (simple fix for m-a, m-b, esr)

This should also go to aurora and beta instead of the complex patch.  Checked already on esr branch that this patch is sufficient to fix the buffer overflow, leaving other (likely non-critical) issues unaddressed.

Comment 53

[Honza Bambas (:mayhemer)]

Comment on attachment 8840456 [details] [diff] [review]
reverse v1.4 from m-a, m-b patch

esr52 is affected as well.

Comment 54

[Ryan VanderMeulen [:RyanVM]]

Assuming we're not backing this out from trunk, let's leave this bug closed and just reset the tracking flags for the affected branches.

Comment 55

[Julien Cristau [:jcristau]]

Comment on attachment 8840456 [details] [diff] [review]
reverse v1.4 from m-a, m-b patch

backout previous patch that caused some regression, aurora53+/beta52+

clearing esr52 flag as that branch will get synced from beta.

Comment 56

[Julien Cristau [:jcristau]]

Comment on attachment 8835485 [details] [diff] [review]
v1 (simple fix for m-a, m-b, esr)

minimal fix for this out of bounds read, aurora53+/beta52+

Comment 57

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/0c629c468574
https://hg.mozilla.org/releases/mozilla-aurora/rev/c2ddbee95da8

https://hg.mozilla.org/releases/mozilla-beta/rev/26c625adfe63
https://hg.mozilla.org/releases/mozilla-beta/rev/6e16e25f11e9

Comment 58

[Honza Bambas (:mayhemer)]

Thank you, Ryan!

Comment 59

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/26c625adfe63
https://hg.mozilla.org/releases/mozilla-esr52/rev/6e16e25f11e9