1555050 - CSP-valued members of loadinfo are really confusing; various uses of GetCsp() are likely wrong

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

[Tracking Requested - why for this release]: Possible security issues due to using wrong CSP.

LoadInfo now has the following CSP-valued members, quoted along with their documentation, from nsILoadInfo.idl and LoadInfo.h:

  /**
   * Query the CSP (or Preload CSP for preloads) which should be
   * enforced for the channel this loadinfo belongs to.
   */
  [notxpcom,nostdcall] CSPRef GetCsp();
  [notxpcom,nostdcall] CSPRef GetPreloadCsp();

  // Certain schemes need to inherit the CSP. If needed, we temporarily
  // store the CSP from the embedding/opening document here which then
  // gets propagated to the new doc within Document::InitCSP(). This
  // member is only ever non-null if the new doc actually needs to
  // inherit the CSP from the embedding/opening document.
  nsIContentSecurityPolicy* GetCSPToInherit() { return mCspToInherit; }

My understanding (which is not really made completely clear from the documentation) is that GetCsp/GetPreloadCsp is the CSP that affects the load itself (so things like HSTS upgrades, redirects, etc). It's not clear to me whether it's affected in any way by CSP headers received on responses for the channel the loadinfo corresponds to, but I assume it's not? It's also not clear to me whether

GetCSPToInherit, on the other hand, is the CSP of the document that "started the load" but is only set on some types of channels that might need to inherit the CSP. In some cases GetCSPToInherit and GetCsp might return the same thing; in other cases they are different, right?

What confuses me is that GetCsp for navigations in particular seems to be pretty inconsistent. For example, in this testcase:

<meta http-equiv="Content-Security-Policy" content="script-src 'self' 'unsafe-inline'">
<iframe name="aaa"></iframe>
<a href="https://example.org" target="aaa">Load subframe via link</a>
<a href="https://example.org" target="_blank">Load new window via link</a>
<button onclick="frames[0].location.href = 'https://example.org'">
  Load subframe via location.href
</button>
<button onclick="window.open('https://example.org')">
  Load new window via window.open
</button>
<button onclick="window.open().location.href = 'https://example.org'">
  Load new window via location.href
</button>

I see the following behavior:

• GetCsp() is non-null for the two loads in the subframe.
• GetCsp() is null for all the other loads.

I would have expected the behavior to be the same for all these loads, honestly...

Given that behavior, though, various uses of GetCsp() seem to be wrong. For example, nsDocShell::EndPageLoad seems to be using GetCsp() to create an inherited CSP, which doesn't seem right at all. Similarly, I suspect the use in nsContentUtils::AttemptLargeAllocationLoad is wrong, though I can't tell for sure because the documention for nsIWebBrowserChrome3.reloadInFreshProcess does not make it clear which of the various CSPs is meant to be passed in. The fact that null will get passed in for window.open loads really doesn't seem right to me, though.

In general, it seems like we should audit all consumers of GetCsp() to figure out what CSP they're really looking for here, for the case of navigations.

Comment 1

[Christoph Kerschbaumer [:ckerschb]]

Thanks Boris for the detailed description. In fact, I am really sorry for not adding more sophisticated documentation around GetCSP() and GetCSPToInherit() in the first place. I just wrote a patch which will make up for it by adding clear comments around the difference between CSP in relation to subresource loads and top-level loads.

Further, I evaluated all the callsites of GetCSP() and it seems we are using loadinfo->GetCSP() correctly in all of the cases except the two pointed out by you:

• nsDocShell::EndPageLoad
• nsContentUtils::AttemptLargeAllocation
  In both cases we used to query call triggeringPrincipal->GetCSP() which basically translates to loadinfo->GetCspToInherit() in the new world. I updated those two cases and further re-evaluated all of those cases [1] where we used to call triggeringPrincipal->GetCSP().

Please let me know if I am still missing something.

[1] https://hg.mozilla.org/mozilla-central/rev/ddf4012a7652

Comment 2

[Christoph Kerschbaumer [:ckerschb]]

Attachment: Bug 1555050: Always (if non null) set any CSP as cspToInherit on the loadinfo of new document load. Update documentation for GetCSP, GetPreloadCSP() and GetCSPToInherit and update two callsites which called GetCSP instead of GetCSPToInherit. r=bz

Comment 3

[Boris Zbarsky [:bzbarsky]]

it seems we are using loadinfo->GetCSP() correctly in all of the cases except the two pointed out by you

OK, I went through the other GetCSP callers. The one I don't understand is WorkerLoadInfo::SetPrincipalsAndCSPFromChannel. It used to ask for the CSP from the channel result principal. Now it's asking for the loadinfo's GetCsp. But why is it conditioned on CSP_ShouldResponseInheritCSP? If the CSP is what was delivered with the channel, shouldn't it apply no matter what CSP_ShouldResponseInheritCSP says? And if we're really conditioning this on the inheritance thing, shouldn't we then be using the "csp we should inherit", which would be the loadinfo's GetCspToInherit?

Are there tests for this function's CSP handling?

Comment 4

[Boris Zbarsky [:bzbarsky]]

Also, the various comments about CSP and principal interaction in ServiceWorkerPrivate::SpawnWorkerIfNeeded seem to be wrong now that CSP does not live in the principal, and it's not clear to me whether the code there makes sense.

Comment 5

[Christoph Kerschbaumer [:ckerschb]]

Boris,

here is a summary of changes I have incorporated based on our zoom discussion yesterday:

• Let's expose GetCspToInherit() on nsILoadInfo. Please note that I am not exposing the setter on nsILoadinfo because SetCSPToInherit() should only ever be called within nsDocshell.
• Updating documentation for GetCSP(), GetPreloadCSP() as well GetCSPToInherit() within nsIloadInfo.idl.
• We do not implement the 'navigate-to' directive as of now.
• For upgrading insecure requests we are using the CSP that is handed to the docshell, which is the CSP of the document that triggered the load, see:
  https://searchfox.org/mozilla-central/source/docshell/base/nsDocShell.cpp#10036
• Updating comments within worker code, especially around the inheritance bits. FWIW, there is dom/workers/test/test_csp.html which exercises that codepath.
• Finally, I am providing a test for "Large-Allocation" based on upgrade-insecure-requests.

Thanks again for your time and the insightful discussion yesterday!

Comment 6

[Christoph Kerschbaumer [:ckerschb]]

Attachment: Bug 1555050: Write test for large-allocation. r=bz

Comment 7

[Boris Zbarsky [:bzbarsky]]

Christoph, are you still looking into my question about https://w3c.github.io/webappsec-csp/#should-block-request?

Comment 8

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #7)

Christoph, are you still looking into my question about https://w3c.github.io/webappsec-csp/#should-block-request?

Boris, I am pasting your question here for the sake of completeness:

Going further along, the basic check in https://w3c.github.io/webappsec-csp/#should-block-request uses the request's client. I believe we currently use GetCsp() for this check, right? But the request's client's CSP is more like our GetCSPToInherit(), as far as I can tell. Again, are there tests for this? Does our behavior match other browsers? Is the spec just wrong here?

I was looking into that actually. Let me paste the first sentence: of 'integration with fetch' part:

Given a request (request), this algorithm returns Blocked or Allowed and reports violations based on request’s client’s Content Security Policy.

To me that means any given request (e.g an image request) get's blocked based on the request's client CSP, which is queried using GetCSP() in our implementation. Maybe I am just not understanding your question, but isn't any test within dom/security/test/csp just testing exactly that?

Or are you solely talking about navigations? Remember, as of now we do not support navigate-to. If we would, then we would basically use cspToInherit(). Which is the CSP that is passed to docshell here:
https://searchfox.org/mozilla-central/source/docshell/base/nsDocShell.cpp#10044

Does that make sense or am I misinterpreting your question?

Comment 9

[Boris Zbarsky [:bzbarsky]]

Sorry, let me try to be clearer.

I'm solely talking about navigations, and in particular subframe navigations. Do we apply frame-src checks to those, even if the navigation was not due to changing the "src" attribute?

If so, the issue is that for subframe navigations in our impl GetCsp() always returns the CSP of the <iframe> element's owner document, not the CSP of the navigation's fetch's client, right?

Where do we do frame-src enforcement, and which CSP do we use for it? Which CSP does the spec say to use for it? Is frame-src enforcement done by https://w3c.github.io/webappsec-csp/#should-block-request or something else in the spec?

Comment 10

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #9)

Sorry, let me try to be clearer.

Thanks for the clarification. For iframe loads due to changing the "src" we perform CSP checks within CSPService::ConsultCSP() {basically ShouldLoad()} and we use the CSP of the <iframe> element's owner document (loadinfo->GetCSP()). In other words, frame-src only applies to the parent changing the iframe src=.

I just discussed those things with Dan as well and it seems there is no mochi test and also no web-platform test that covers frame-src in combination with changing the frame's location. Whatever the expected behavior is - we should add a test for that.

Either way, those problems were pre-existing and not introduced by Bug 965637. Hence I am suggesting the following:

• We keep this bug to basically fix what we discussed in the zoom meeting (using cspToInherit in ::EndPageLoad() and for large-allocation) including the test for that behavior.
• I filed Bug 1557114 to investigate your questions within comment 9. I have to meet with Anne and Dan to make sure we are not missing anything around frame-navigation.

Does that sound like an acceptable path forward to you?
If so, please take a look at phabricator review requests attached to this bug - thanks again for raising those questions and concerns - much appreciated!

Comment 11

[Boris Zbarsky [:bzbarsky]]

In other words, frame-src only applies to the parent changing the iframe src=.

It doesn't apply to location.href sets inside the iframe? I'm trying to figure out where in our code we end up treating those different from src= changes for CSP purposes... I guess we can follow up on this in bug 1557114.

Either way, those problems were pre-existing and not introduced by Bug 965637.

Hmm. Because we used to not use the triggeringPrincipal for the CSP in web-exposed cases? OK, I buy that.

Does that sound like an acceptable path forward to you?

This seems ok, but makes it a bit hard to evaluate how we should document GetCsp(). I'll think about it a bit and follow up in the phabricator review.

Thank you for your patience!

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b5b99e78b753
Always (if non null) set any CSP as cspToInherit on the loadinfo of new document load. Update documentation for GetCSP, GetPreloadCSP() and GetCSPToInherit and update two callsites which called GetCSP instead of GetCSPToInherit. r=bzbarsky
https://hg.mozilla.org/integration/autoland/rev/f5e954d593f8
Write test for large-allocation. r=bzbarsky

Comment 13

[Bogdan Tara[:bogdan_tara | bogdant]]

Backed out 2 changesets (Bug 1555050) for test_reloadInFreshProcess.html failures

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=f5e954d593f83990d3091b3178fec2b0d64a4be2&selectedJob=250441905

Backout link: https://hg.mozilla.org/integration/autoland/rev/7ce067624706c0623ebf97696ac30c3e3622495c

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=250441905&repo=autoland&lineNumber=1510

[task 2019-06-06T19:18:53.195Z] 19:18:53 INFO - 59 INFO TEST-START | dom/security/test/csp/test_punycode_host_src.html
[task 2019-06-06T19:18:53.196Z] 19:18:53 INFO - 60 INFO TEST-OK | dom/security/test/csp/test_punycode_host_src.html | took 6028ms
[task 2019-06-06T19:18:53.196Z] 19:18:53 INFO - 61 INFO TEST-START | dom/security/test/csp/test_reloadInFreshProcess.html
[task 2019-06-06T19:24:20.295Z] 19:24:20 INFO - 62 INFO TEST-UNEXPECTED-FAIL | dom/security/test/csp/test_reloadInFreshProcess.html | Test timed out.
[task 2019-06-06T19:24:20.295Z] 19:24:20 INFO - SimpleTest.ok@SimpleTest/SimpleTest.js:275:18
[task 2019-06-06T19:24:20.295Z] 19:24:20 INFO - reportError@SimpleTest/TestRunner.js:121:22
[task 2019-06-06T19:24:20.296Z] 19:24:20 INFO - TestRunner._checkForHangs@SimpleTest/TestRunner.js:142:7

Comment 14

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Bogdan Tara[:bogdan_tara] from comment #13)

Backed out 2 changesets (Bug 1555050) for test_reloadInFreshProcess.html failures

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=250441905&repo=autoland&lineNumber=1510

Ah, I forgot, there is no ssl support on android, we had to disable other upgrade-insecure-reuqest tests too, e.g:
https://searchfox.org/mozilla-central/source/dom/security/test/csp/mochitest.ini#289

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fd95af25f3c5
Always (if non null) set any CSP as cspToInherit on the loadinfo of new document load. Update documentation for GetCSP, GetPreloadCSP() and GetCSPToInherit and update two callsites which called GetCSP instead of GetCSPToInherit. r=bzbarsky
https://hg.mozilla.org/integration/autoland/rev/0c2b0dd884cc
Write test for large-allocation. r=bzbarsky

Comment 16

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/fd95af25f3c5
https://hg.mozilla.org/mozilla-central/rev/0c2b0dd884cc

Comment 17

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/fd95af25f3c5
https://hg.mozilla.org/mozilla-central/rev/0c2b0dd884cc