1416045 - (CVE-2018-5164) CSP is not applied to documents sent through multipart/x-mixed-replace

Status: RESOLVED FIXED

(Core :: DOM: Security, defect, P2)

Description

[Khalil Zhani]

From bug 1223743.

PoC: http://mallory.csrf.jp/x-mixed-replace/csp/bad.php

Comment 1

[:Gijs (he/him)]

Christoph, thoughts?

Comment 2

[Christoph Kerschbaumer [:ckerschb]]

(In reply to :Gijs (slow, PTO recovery mode) from comment #1)
> Christoph, thoughts?

Beats me, because we wrote a test for that within Bug 1223743. Apparently there is still something missing, we need to investigate why this is still happening. The alert is blocked on Safari and Chrome, so something must be wrong with our implementation.

Vino, can you investigate why this is happening?

Comment 3

[Vinothkumar Nagasayanan [:vinoth]]

Logs show that CSP from the multipart/x-mixed-replace is not being applied.

Got the following log,

"[Main Thread]: D/CSP no CSP for document, http://mallory.csrf.jp/x-mixed-replace/csp/bad.php"

Investigating further on this.

Comment 4

[Vinothkumar Nagasayanan [:vinoth]]

Attachment: fix-for-bug-1416045.patch

Comment 5

[Vinothkumar Nagasayanan [:vinoth]]

nsMultiMixedConv.cpp is not adding the csp headers to the basechannel, hence the issue. Fixed that.

Test file in Bug 1223743 seems to be set only on the parent Headers and not added in the x-mixed-replace content itself. Will work on changing this test.

Comment 6

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8929498 [details] [diff] [review]
fix-for-bug-1416045.patch

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

That looks reasonable to me, but I am not a Necko peer. Honza, what do you think?
We also need an automated test to make sure we never regress that.

Comment 7

[Honza Bambas (:mayhemer)]

Comment on attachment 8929498 [details] [diff] [review]
fix-for-bug-1416045.patch

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

Thanks for this patch, but there are few things to update and make sure of first.

First of all, I'm not sure this is a 100% correct fix also because I'm no expert to the CSP handling code.  I can only speak for the code changes in mixedconverter which are (modulo formatting) technically correct.  


Few notes on formatting:
- use only two spaces (no tabs) for indention
- submit the patch with 8 lines context and function names (-U 8 -p)
- please don't include any commit message except the bug number and reviewers list as this is a sec bug
- spaces after argument separating commas


Asking Chris to confirm this is the right approach (I think we will need some tweaks here, I'm not certain a pure merging of every CSP response header from each part to the root channel is right)

Comment 8

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8929498 [details] [diff] [review]
fix-for-bug-1416045.patch

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

(In reply to Honza Bambas (:mayhemer) from comment #7)
> Asking Chris to confirm this is the right approach (I think we will need
> some tweaks here, I'm not certain a pure merging of every CSP response
> header from each part to the root channel is right)

I am not sure myself. Personally I thought we fixed the issue of CSP on multipart channels when we landed [1] over in Bug 1223743, but apparently not. Interesting though is that within [1] the base channel already has the right CSP headers attached. I don't fully understand what the difference between Bug 1223743 and this one is that we are not getting the right headers.

Thoughts?

[1] https://hg.mozilla.org/mozilla-central/rev/de7773dd42e1#l1.12

Comment 9

[Christoph Kerschbaumer [:ckerschb]]

Honza, please have a look at comment 8.

Comment 10

[Honza Bambas (:mayhemer)]

The fix in bug 1223743 is about something else: nsDocument::InitCSP is trying to reach the nsHttpChannel and get the response CSP headers out from it.  In case of multipart content it was unreachable, tho.  The http channel in the multipart case is the one processing the Content-type: multipart and seeing the whole response (not split to parts).  But it was invisible/unreachable to nsDocument::InitCSP since we layer things like this: nsHttpChannel -> nsMultimixedConv ->* PartChannel -> nsDocument.  Hence, the aChannel arg in InitCSP is PartChannel and not nsHttpChannel.

bug 1223743 allowed getting the root nsHttpChannel from the PartChannel, nothing more.  So, when a CSP header is present on the Content-type: multipart root response, nsDocument::InitCSP can see it now.


So what is the difference?  Each part can provide response headers of its own!  This bugs is about the fact that we completely ignore CSP response headers in parts (I had to make it more clear probably).  We only process few headers listed at [1] being present on parts, others are ignored.

What the patch for this bug does is that it merged CSP response headers from each part to the root response.  Then nsDocument::InitCSP can see it (thanks bug 1223743 fix).

There seems to be no restriction in rfc1341 on what headers can or can't be contained in parts or which of them to ignore or accept.  I don't think we should trivially merge all found CSP response headers to the root response CSP header.  It seems to me bad to let every part just add new directives so that e.g. part3 will be subject to CSP provided by part1 and 2.

Few options, based only on common sense, not a sec analyzes (!):

1. update the root channel CSP header respectively for each part as: CSP-root + CSP-partN, where N is the part number (most secure IMO)
2. provide CSP-part, if present, or CSP-root, if present (less secure, since if there is an attack possible on part headers, CSP could be loosen - not sure if merging as suggested in 1. already can do the same, tho)
3. disallow CSP response headers being present on parts completely and throw a security error from the part channel

I would prefer the option 1 probably, but again, just a common sense conclusion.  There could be a trick found to fool us when CSP is accepted from parts.


What other browsers do is also a good question to ask.


[1] https://dxr.mozilla.org/mozilla-central/rev/da90245d47b17c750560dedb5cbe1973181166e3/netwerk/streamconv/converters/nsMultiMixedConv.h#195

Comment 11

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Honza Bambas (:mayhemer) from comment #10)
> So what is the difference?  Each part can provide response headers of its
> own!

Thanks for shedding some light on that; makes sense now.

> What other browsers do is also a good question to ask.

Agreed. We shouldn't just simply merge all found CSP response headers.
Vino, can you take a look how other browsers handle that?
Based on that info we can then define a path forward.

Comment 12

[Anne (:annevk)]

Other browsers have much worse support; see bug 1276918 about either removing or drastically simplifying (as Chrome has done) multipart/x-mixed-replace. I still think that's something we should investigate as there are likely more security vulnerabilities hiding there.

Comment 13

[Frederik Braun [:freddy]]

(In reply to Honza Bambas (:mayhemer) from comment #10)
> So what is the difference?  Each part can provide response headers of its
> own!  This bugs is about the fact that we completely ignore CSP response
> headers in parts (I had to make it more clear probably).  We only process
> few headers listed at [1] being present on parts, others are ignored.
> 
> [1]
> https://dxr.mozilla.org/mozilla-central/rev/
> da90245d47b17c750560dedb5cbe1973181166e3/netwerk/streamconv/converters/
> nsMultiMixedConv.h#195

And what does that mean for other security checks that come in headers?
Seems like we should consider further investigation.

Comment 14

[Khalil Zhani]

Any update here?

Comment 15

[Vinothkumar Nagasayanan [:vinoth]]

Attachment: Bug-1416045.patch

Comment 16

[Vinothkumar Nagasayanan [:vinoth]]

> Few options, based only on common sense, not a sec analyzes (!):
> 
> 1. update the root channel CSP header respectively for each part as:
> CSP-root + CSP-partN, where N is the part number (most secure IMO)


Patch is created based on the suggested 1st solution.
Root channel CSP is updated for each part.

Comment 17

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8946200 [details] [diff] [review]
Bug-1416045.patch

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

::: dom/security/test/csp/file_multipart_testserver.sjs
@@ -1,2 @@
>  // SJS file specifically for the needs of bug
> -// Bug 1223743 - CSP: Check baseChannel for CSP when loading multipart channel

Please leave that test as is. Feel free to add a second test into that file, but we don't want to loose test coverage here.

::: netwerk/streamconv/converters/nsMultiMixedConv.cpp
@@ +488,4 @@
>          if (NS_FAILED(rv)) {
>              return rv;
>          }
> +        rv = httpChannel->GetResponseHeader(NS_LITERAL_CSTRING("content-security-policy"), mRootContentSecurityPolicy);

nit: 80 chars per line

@@ +1058,5 @@
>        break;
>      }
> +    case HEADER_CONTENT_SECURITY_POLICY: {
> +      mContentSecurityPolicy = mResponseHeaderValue;
> +      mContentSecurityPolicy.CompressWhitespace();

nit: those two assignments only need to happen if there is an httpchannel, right? Please move assignments into the if-clause.

@@ +1065,5 @@
> +        if (!mContentSecurityPolicy.IsEmpty()) {
> +            // Reset and Append Part CSP to root CSP for each Part
> +            mContentSecurityPolicy.Append(";");
> +            mContentSecurityPolicy.Append(mRootContentSecurityPolicy);
> +            DebugOnly<nsresult> rv = httpChannel->SetResponseHeader(NS_LITERAL_CSTRING("Content-Security-Policy"),mContentSecurityPolicy,true);

nit: 80 chars per line

@@ +1067,5 @@
> +            mContentSecurityPolicy.Append(";");
> +            mContentSecurityPolicy.Append(mRootContentSecurityPolicy);
> +            DebugOnly<nsresult> rv = httpChannel->SetResponseHeader(NS_LITERAL_CSTRING("Content-Security-Policy"),mContentSecurityPolicy,true);
> +            MOZ_ASSERT(NS_SUCCEEDED(rv));
> +        } else {

I don't fully understand what that 'else' is doing and what it's used for. Can you pleas elaborate the comment?

@@ +1070,5 @@
> +            MOZ_ASSERT(NS_SUCCEEDED(rv));
> +        } else {
> +            // Set back to root CSP if Part CSP is not available
> +            DebugOnly<nsresult> rv = httpChannel->SetResponseHeader(NS_LITERAL_CSTRING("Content-Security-Policy"),mRootContentSecurityPolicy,true);
> +            MOZ_ASSERT(NS_SUCCEEDED(rv));

Other return value checks within this function actually return the error and not just MOZ_ASSERT() them. Please do the same as used elsewhere in that function.

Ultimately Honza has to sign off on those changes anyway.

Comment 18

[Vinothkumar Nagasayanan [:vinoth]]

Attachment: 1416045.patch

Comment 19

[Vinothkumar Nagasayanan [:vinoth]]

Attachment: 1416045.patch

Comment 20

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8951649 [details] [diff] [review]
1416045.patch

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

Test looks good. For the actual nsMultiMixedConv.cpp changes I rely on Honza, but I think that looks good as well.

::: netwerk/streamconv/converters/nsMultiMixedConv.cpp
@@ +1063,5 @@
> +      if (httpChannel) {
> +        mContentSecurityPolicy = mResponseHeaderValue;
> +        mContentSecurityPolicy.CompressWhitespace();
> +        if (!mContentSecurityPolicy.IsEmpty()) {
> +            // Previous parts may have appended its CSP with root CSP. So Append current Part's CSP to root CSP and reset root CSP.

Nit: 80 char lines please

@@ +1074,5 @@
> +            }
> +        } else {
> +            // Previous parts may have appended its CSP with root CSP. So set back to root CSP if this Part doesn't contain a CSP.
> +            nsresult rv = httpChannel->SetResponseHeader(
> +                                NS_LITERAL_CSTRING("Content-Security-Policy"),mRootContentSecurityPolicy,true);

Isn't the SetResponseHeader part of the patch the same in the if and else clause? If so, then why not:

if (httpChannel) {
        mContentSecurityPolicy = mResponseHeaderValue;
        mContentSecurityPolicy.CompressWhitespace();
        if (!mContentSecurityPolicy.IsEmpty()) {
            mContentSecurityPolicy.Append(";");
            mContentSecurityPolicy.Append(mRootContentSecurityPolicy);
        }
        nsresult rv = httpChannel->SetResponseHeader(
          NS_LITERAL_CSTRING("Content-Security-Policy"),mContentSecurityPolicy,true);
	NS_ENSURE_SUCCES(rv, NS_ERROR_CORRUPTED_CONTENT);

Comment 21

[Honza Bambas (:mayhemer)]

Attachment: (resubmitted with 8 line context)

sorry, but it's nearly impossible to review this w/o it...

Comment 22

[Honza Bambas (:mayhemer)]

Comment on attachment 8952164 [details] [diff] [review]
(resubmitted with 8 line context)

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

thanks for the patch, it only needs some cleanup (formatting, some simplifications, better comments)

r- for all the comments that all need to be addressed.  note that I have to repeat some of them from the last time, please make sure you address them all before you submit a new patch.

please also produce a patch with an 8 line context.

::: netwerk/streamconv/converters/nsMultiMixedConv.cpp
@@ +488,5 @@
>          if (NS_FAILED(rv)) {
>              return rv;
>          }
> +        rv = httpChannel->GetResponseHeader(NS_LITERAL_CSTRING("content-security-policy"),
> +                                            mRootContentSecurityPolicy);

do:

nsCString csp;
rv = httpChannel->GetResponseHeader(NS_LITERAL_CSTRING("content-security-policy"), csp);
if (NS_SUCCEEDED(rv)) {
  mRootContentSecurityPolicy = csp;
}

@@ +1063,5 @@
> +      if (httpChannel) {
> +        mContentSecurityPolicy = mResponseHeaderValue;
> +        mContentSecurityPolicy.CompressWhitespace();
> +        if (!mContentSecurityPolicy.IsEmpty()) {
> +            // Previous parts may have appended its CSP with root CSP. So Append current Part's CSP to root CSP and reset root CSP.

80 cols max (on all places you exceed)

I don't under the comment much.  like: what does the "reset root CSP" mean?

@@ +1067,5 @@
> +            // Previous parts may have appended its CSP with root CSP. So Append current Part's CSP to root CSP and reset root CSP.
> +            mContentSecurityPolicy.Append(";");
> +            mContentSecurityPolicy.Append(mRootContentSecurityPolicy);
> +            nsresult rv = httpChannel->SetResponseHeader(
> +                                NS_LITERAL_CSTRING("Content-Security-Policy"),mContentSecurityPolicy,true);

spaces after ,

the last arg has to be false, we don't want to merge!!!  see https://searchfox.org/mozilla-central/rev/5536f71c3833018c4f4e2c73f37eae635aab63ff/netwerk/protocol/http/nsIHttpChannel.idl#319

@@ +1074,5 @@
> +            }
> +        } else {
> +            // Previous parts may have appended its CSP with root CSP. So set back to root CSP if this Part doesn't contain a CSP.
> +            nsresult rv = httpChannel->SetResponseHeader(
> +                                NS_LITERAL_CSTRING("Content-Security-Policy"),mRootContentSecurityPolicy,true);

spaces after ,

@@ +1079,5 @@
> +            if (NS_FAILED(rv)) {
> +                return NS_ERROR_CORRUPTED_CONTENT;
> +            }
> +        }
> +      }

please keep the correct indention (2 spaces)

and what about simplifying as:

if (!mContentSecurityPolicy.IsEmpty()) {
  mContentSecurityPolicy.Append(';');
}
mContentSecurityPolicy += mRootContentSecurityPolicy;

set the header...
check error...


also, why exactly are you appending the root CSP _after_ the part CSP?  I don't say it's wrong, but I'd like to know why and also add a comment why

this is really very very important how we do it, if the multi-part code is going to stick for few more years on

Comment 23

[Honza Bambas (:mayhemer)]

Comment on attachment 8952164 [details] [diff] [review]
(resubmitted with 8 line context)

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

::: dom/security/test/csp/test_multipartchannel.html
@@ +15,5 @@
>  
> +var testsToRunMultipartCSP = {
> +    rootCSP_test: false,
> +    partCSP_test: false,
> +  };

no leading spaces

@@ +23,5 @@
> +function checkTestsCompleted() {
> +    for (var prop in testsToRunMultipartCSP) {
> +      // some test hasn't run yet so we're not done
> +      if (!testsToRunMultipartCSP[prop])
> +        return;

if () {
}

@@ +36,5 @@
>  window.addEventListener("message", receiveMessage);
>  function receiveMessage(event) {
> +
> +	switch(event.data.test) {
> +

indention, overall formatting

@@ +45,5 @@
> +
> +      case "partCSP_test":
> +        is(event.data.msg, "part2-img-blocked", "image should be blocked");
> +        testsToRunMultipartCSP["partCSP_test"] = true;
> +        break;

isn't this message sent twice from the iframe?  there should IMO be 3 tests, not only two...

@@ +48,5 @@
> +        testsToRunMultipartCSP["partCSP_test"] = true;
> +        break;
> +
> +    }
> +  	checkTestsCompleted();

again, formatting please... use spaces, not tabs

Comment 24

[Phabricator Automation]

Attachment: Bug 1416045

Comment 25

[Vinothkumar Nagasayanan [:vinoth]]

I will fix the issues in test file and add for review separately.

Comment 26

[Phabricator Automation]

Comment on attachment 8956030 [details]
Bug 1416045

Christoph Kerschbaumer [:ckerschb] has approved the revision.

https://phabricator.services.mozilla.com/D675

Comment 27

[Phabricator Automation]

Comment on attachment 8956030 [details]
Bug 1416045

Christoph Kerschbaumer [:ckerschb] has been removed from the revision.

https://phabricator.services.mozilla.com/D675

Comment 28

[Vinothkumar Nagasayanan [:vinoth]]

Test files also added to patch.

Comment 29

[Phabricator Automation]

Comment on attachment 8956030 [details]
Bug 1416045

Honza Bambas (:mayhemer) has approved the revision.

https://phabricator.services.mozilla.com/D675

Comment 30

[Honza Bambas (:mayhemer)]

Comment on attachment 8956030 [details]
Bug 1416045

stupid fabrykator

Comment 31

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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

Comment 32

[Julien Cristau [:jcristau]]

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

Comment 33

[Ryan VanderMeulen [:RyanVM]]

I'm guessing we'll want to uplift this to Beta too?

Comment 34

[Honza Bambas (:mayhemer)]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #33)
> I'm guessing we'll want to uplift this to Beta too?

Probably yes if it should open soon.  I think this will apply cleanly.

Comment 35

[Honza Bambas (:mayhemer)]

Comment on attachment 8956030 [details]
Bug 1416045

Approval Request Comment
[Feature/Bug causing the regression]: since ever (not a regr.)
[User impact if declined]: inaccuracy in content security policy application of a content delivered via multimixed part (possibly fail to load something we should allow and vise versa - load something we should not)
[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]: I think not
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: mildly
[Why is the change risky/not risky?]: this is a change to how we treat CSP response headers in multimixed parts, so hard to say what regressions will emerge, but there is a possibility ; however, the use case here is not anything widely used on the web, so we may not start getting regression reports sooner than even up from Release
[String changes made/needed]: none

Comment 36

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8956030 [details]
Bug 1416045

Approved for 60.0b7.

Comment 37

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/1d6d0bc1cb27eb3e90fa33367869297b451ddb9c

Comment 38

[Khalil Zhani]

Any bounty for this report? Thanks!

Comment 39

[Julien Cristau [:jcristau]]

(In reply to Khalil Zhani from comment #38)
> Any bounty for this report? Thanks!

https://www.mozilla.org/en-US/security/client-bug-bounty/ describes the process if you want to claim a bounty.