1529877 - Do not query the CSP from the principal within LoadFrame, but rather within GetURL, within nsFrameLoader.cpp

Status: RESOLVED FIXED

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

Description

As a follow up to Bug 1518454, we need to figure out how we can query the csp from the right principal, so that within Bug 965637 we can query it from the document/client.

Further info:
test_ext_contentscript_triggeringPrincipal.js
is the test that helps to figure the problem

Probably we within GetURL we don't only do

nsCOMPtr<nsIPrincipal> prin = frame->GetSrcTriggeringPrincipal();
but also have something like:
nsCOMPtr<nsIContentSecurityPolicy> csp = frame->GetSrcCSP();

Comment 1

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1529877_frameloader.patch

Boris,

to recap, the challenge is to query the right CSP within nsFrameLoader::LoadFrame(). In particular in the case of the former GetURL() (within the patch renamed to GetURLPrincipalAndCSP()).
In order to query the right CSP within GetURLPrincipalAndCSP(), we have to expose a GetSrcCSP() on nsGenericHTMLFrameElement - that part is fine. The hard part is to get the right CSP set on nsGenericHTMLFrameElement. In order to do that I had to update AfterMaybeChangeAttr() which also caused me to update AfterSetAttr(), both of which needed to be extended by a CSP argument. At this point I realized I also have to update more Attribute setters, e.g. AfterSetAttr(), SetAttrAndNotify(), Attr::SetValue(), Element::ToggleAttribute(), Element::SetAttribute(), Element::SetAttributeNS(), etc - basically all the versions which allow to set an attribute and take a triggeringPrincipal, now also would need to take a csp argument.

Now the question is, is this the right way to go? Is it worth updating all these attribute setters? And if so, should we then pass some kind of security struct, maybe even the newly created loadURIOptions.webidl, so we can pass more security related info in the future without having to update all these layers and layers of API calls?

What do you think?

Comment 2

[Boris Zbarsky [:bzbarsky]]

the challenge is to query the right CSP within nsFrameLoader::LoadFrame

The first challenge is to decide what the right CSP is. Once we have decided that, we can think about the best way to get at it. So what is the right CSP here? I suggest talking this over with Kris.

Comment 3

[Kris Maglione [:kmag]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #1)

Now the question is, is this the right way to go? Is it worth updating all these attribute setters? And if so, should we then pass some kind of security struct, maybe even the newly created loadURIOptions.webidl, so we can pass more security related info in the future without having to update all these layers and layers of API calls?

I think that would be my preference. I'd actually initially considered updating all of these to store some sort of URLInfo object which held the resolved URL and triggering principal, and could possibly be extended to hold other data, but decided that it seemed like overkill at the time.

But I suppose at this point we'd really want some sort of reusable security info object so that we don't have to add another additional pointer member everywhere we need to store this info. It might still make sense to have a URLInfo object that stores the reference to that reusable object along with the resolved URL.

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

the challenge is to query the right CSP within nsFrameLoader::LoadFrame

The first challenge is to decide what the right CSP is. Once we have decided that, we can think about the best way to get at it. So what is the right CSP here? I suggest talking this over with Kris.

I don't have enough context to answer this question. I guess the CSP we choose here is used to determine whether to allow loading a resource in a frame? In that case, I think we'd default to the document CSP, and override it with a mostly-empty CSP for extension and system callers. I don't know where we'd store that if we remove CSP from the principal, though. Might need to add a field to the compartment private.

Comment 4

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1529877_debug.patch

(In reply to Kris Maglione [:kmag] from comment #3)

But I suppose at this point we'd really want some sort of reusable security info object so that we don't have to add another additional pointer member everywhere we need to store this info. It might still make sense to have a URLInfo object that stores the reference to that reusable object along with the resolved URL.

I also think it would make sense to update all of the attribute setters to pass a URLInfo struct which can hold additional security/privacy related information. E.g. I think it would make sense that this struct also queries the referrer at load start time, rather than quering the referrer within nsFrameLoader.cpp [1]. Anyway, I suspect it wouldn't be that much work to actually update all the attribute setters to pass that new struct and we would provide new infrastructure which allows for potentially more security/privacy related checks when updating attributes in the future.

[1] https://searchfox.org/mozilla-central/source/dom/base/nsFrameLoader.cpp#422

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

The first challenge is to decide what the right CSP is. Once we have decided that, we can think about the best way to get at it. So what is the right CSP here? I suggest talking this over with Kris.

I am trying to provide debug information undeneath for running test_ext_contentscript_triggeringPrincipal.js. If we always query the CSP from the owning doc (see attached patch) then the assertion [2] would fire in the following 10 cases (see debug information underneath). FWIW, please also note that quering the CSP from the owning doc and commenting out the assertion would let the test run to completion succesfully.

Looking at the debug information it seems that the CSP of the triggeringPrincipal (which is an expandedPrincipal) is always null, whereas the explicit CSP is the CSP of the document. I am not entirely sure what the test is trying to accomplish but it seems to me that applying the document's CSP is what should be happening - or am I mistaken?

Kris, do you have any further insights?

[2] https://searchfox.org/mozilla-central/source/docshell/base/nsDocShell.cpp#9968

Ultimately we have 2 options:
a) We extend the attribute setters with a new security struct (which would also hold the CSP) - mapping current behavior in the tree.
b) We use the document's CSP, if that is the right CSP (as currently in the patch) - but then we would need to provide a carveout for the assertion within nsDocshell.

%---snip---

uri: http://localhost:35443/iframe.html?origin=contentScript&source=contentScript
triggeringPrincipal: ExpandedPrincipal
principalCSPCount: 0
argCSPCount: 1
argCSP: default-src 'none' 'report-sample'; script-src 'nonce-deadbeef' 'unsafe-eval' 'report-sample'; report-uri http://localhost:35443/csp-report.sjs

uri: http://localhost:35443/iframe.html?origin=contentScript&source=contentScript-prop
triggeringPrincipal: ExpandedPrincipal
principalCSPCount: 0
argCSPCount: 1
argCSP: default-src 'none' 'report-sample'; script-src 'nonce-deadbeef' 'unsafe-eval' 'report-sample'; report-uri http://localhost:35443/csp-report.sjs

uri: http://localhost:35443/iframe.html?origin=contentScript&source=contentScript-attr-after-inject
triggeringPrincipal: ExpandedPrincipal
principalCSPCount: 0
argCSPCount: 1
argCSP: default-src 'none' 'report-sample'; script-src 'nonce-deadbeef' 'unsafe-eval' 'report-sample'; report-uri http://localhost:35443/csp-report.sjs

uri: http://localhost:35443/iframe.html?origin=contentScript&source=contentScript-prop-after-inject
triggeringPrincipal: ExpandedPrincipal
principalCSPCount: 0
argCSPCount: 1
argCSP: default-src 'none' 'report-sample'; script-src 'nonce-deadbeef' 'unsafe-eval' 'report-sample'; report-uri http://localhost:35443/csp-report.sjs

uri: http://localhost:35443/iframe.html?origin=contentScript&source=contentScript-content-inject-after-attr
triggeringPrincipal: ExpandedPrincipal
principalCSPCount: 0
argCSPCount: 1
argCSP: default-src 'none' 'report-sample'; script-src 'nonce-deadbeef' 'unsafe-eval' 'report-sample'; report-uri http://localhost:35443/csp-report.sjs

uri: http://localhost:43597/iframe.html?origin=contentScript&source=contentScript
triggeringPrincipal: ExpandedPrincipal
principalCSPCount: 0
argCSPCount: 1
argCSP: default-src 'none' 'report-sample'; script-src 'nonce-deadbeef' 'unsafe-eval' 'report-sample'; report-uri http://localhost:43597/csp-report.sjs

uri: http://localhost:43597/iframe.html?origin=contentScript&source=contentScript-prop
triggeringPrincipal: ExpandedPrincipal
principalCSPCount: 0
argCSPCount: 1
argCSP: default-src 'none' 'report-sample'; script-src 'nonce-deadbeef' 'unsafe-eval' 'report-sample'; report-uri http://localhost:43597/csp-report.sjs

uri: http://localhost:43597/iframe.html?origin=contentScript&source=contentScript-attr-after-inject
triggeringPrincipal: ExpandedPrincipal
principalCSPCount: 0
argCSPCount: 1
argCSP: default-src 'none' 'report-sample'; script-src 'nonce-deadbeef' 'unsafe-eval' 'report-sample'; report-uri http://localhost:43597/csp-report.sjs

uri: http://localhost:43597/iframe.html?origin=contentScript&source=contentScript-prop-after-inject
triggeringPrincipal: ExpandedPrincipal
principalCSPCount: 0
argCSPCount: 1
argCSP: default-src 'none' 'report-sample'; script-src 'nonce-deadbeef' 'unsafe-eval' 'report-sample'; report-uri http://localhost:43597/csp-report.sjs

uri: http://localhost:43597/iframe.html?origin=contentScript&source=contentScript-content-inject-after-attr
triggeringPrincipal: ExpandedPrincipal
principalCSPCount: 0
argCSPCount: 1
argCSP: default-src 'none' 'report-sample'; script-src 'nonce-deadbeef' 'unsafe-eval' 'report-sample'; report-uri http://localhost:43597/csp-report.sjs

Comment 5

[Boris Zbarsky [:bzbarsky]]

I am not entirely sure what the test is trying to accomplish

Please see bug 1267027 and its dependencies. Note that this is part of the CSP spec.

Comment 6

[Kris Maglione [:kmag]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #4)

I also think it would make sense to update all of the attribute setters to pass a URLInfo struct which can hold additional security/privacy related information. E.g. I think it would make sense that this struct also queries the referrer at load start time, rather than quering the referrer within nsFrameLoader.cpp 1. Anyway, I suspect it wouldn't be that much work to actually update all the attribute setters to pass that new struct and we would provide new infrastructure which allows for potentially more security/privacy related checks when updating attributes in the future.

I'm honestly not sure how much work it would be. Most of the existing entry points are handled by WebIDL, which passes the subject principal to the relevant DOM APIs. I suppose we could add a new flag to pass a more detailed caller security info struct instead. We'd just need to decide where to create and store that info.

What else we would want to store on that struct is an interesting question. Arguably, storing the referer URI makes sense, but I'm not entirely sure what the referer URI should be in the case of content injected by a content script. Right now, it's the URI of the loading page. One could make an argument for it at least sometimes being the URI of the extension instead (possibly using similar logic to what decides which principal those loads will inherit), but that may have unintended side-effects.

Either way, I'm pretty sure we want the object that we pass around and store to be reused for every call from a given realm (or possibly compartment; probably realm...)

Looking at the debug information it seems that the CSP of the triggeringPrincipal (which is an expandedPrincipal) is always null, whereas the explicit CSP is the CSP of the document. I am not entirely sure what the test is trying to accomplish but it seems to me that applying the document's CSP is what should be happening - or am I mistaken?

Kris, do you have any further insights?

Like Boris said, this is from bug 1267027. There was also a pretty detailed intent to implement thread1. Essentially, though, content injected by extensions is supposed to be immune to page CSP. We currently handle that by storing the principal of the caller who triggered the load, and using that principal's CSP instead of the page's to decide whether to allow a load.

Beyond the CSP side of things, we also use the stored triggerng principal to allow extensions to load their own resources (e.g., bundled images) that pages aren't allowed to load on their own. That isn't super important now, since most extensions that inject resources into web content whitelist those resources to be completely content-accessible, but in the future we'd really like to restrict even whitelisted resource loads to extension triggering principals to prevent websites from probing for the existence extension resources for the sake of fingerprinting.

Ultimately we have 2 options:
a) We extend the attribute setters with a new security struct (which would also hold the CSP) - mapping current behavior in the tree.
b) We use the document's CSP, if that is the right CSP (as currently in the patch) - but then we would need to provide a carveout for the assertion within nsDocshell.

I think we need option a).

Comment 7

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Kris Maglione [:kmag] from comment #6)

Ultimately we have 2 options:
a) We extend the attribute setters with a new security struct (which would also hold the CSP) - mapping current behavior in the tree.
b) We use the document's CSP, if that is the right CSP (as currently in the patch) - but then we would need to provide a carveout for the assertion within nsDocshell.

I think we need option a).

Boris, given Kris' response, how do you feel about option (a)?

Personally I think it makes sense to add a new AttrOptions-struct which could not only hold the principal, but also the CSP and in the future even more security/privacy related information. E.g. I could imagine that in the future we disallow third party scripts to set specific attributes.

I think it wouldn't be too much work to update all the attribute setters and I would do the work but I want to make sure that we are on the same page! I guess a simple C++ class (something like dom/base/AttrOptions.h/cpp) would be sufficient, because there are only C++ callers as far as I can tell.

Comment 8

[Boris Zbarsky [:bzbarsky]]

So I think the setup we have at the moment is actually buggy when same-origin documents have different CSPs and manipulate each other. See bug 1528790 for an example.

Here's what I think we should do:

1. Check whether the triggering principal is an expanded principal. If so, use its CSP (which we are currently planning to keep for expanded principals, right?). Otherwise use the document's CSP. The principal stored in the attribute setter is never system (we ignore those on the binding level), so we don't need to worry about that.

2. Audit all the places where we do loads based on attribute values. This means not just subframes but images, etc, etc. All of those will need the same fix. We may or may not have tests for all of them; if we don't we should add them as needed.

That will address the issue here and bug 1528790 as well.

Comment 9

[Kris Maglione [:kmag]]

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

So I think the setup we have at the moment is actually buggy when same-origin documents have different CSPs and manipulate each other. See bug 1528790 for an example.

Hmm... That shouldn't happen as a consequence of the CSP override stuff we did for add-ons. We should only use the CSP of the triggering principal in specific cases1, namely when a) it's the system principal, b) it's an expanded principal which subsumes the content principal, or c) it's an extension codebase principal. None of those should ever be true for loads triggered by web content.

1. Check whether the triggering principal is an expanded principal. If so, use its CSP (which we are currently planning to keep for expanded principals, right?). Otherwise use the document's CSP. The principal stored in the attribute setter is never system (we ignore those on the binding level), so we don't need to worry about that.

This is more or less what we currently do.

Comment 10

[Boris Zbarsky [:bzbarsky]]

We only use OverridesCSP in two places that I can see: CSPService::ShouldLoad and nsStyleUtil::CSPAllowsInlineStyle. We're not using it for the "CSP to inherit for document loads" bit; we just grab it off the triggering principal. So maybe we basically need to do that to fix bug 1528790. But it's not clear what the right thing to compare to would be there....

Comment 11

[Boris Zbarsky [:bzbarsky]]

I guess for the src attr case we would in fact just compare to the NodePrincipal() of the <iframe> element.

Comment 12

[Christoph Kerschbaumer [:ckerschb]]

Please note that the assertion in nsDocShell only fires for loads of type TYPE_DOCUMENT and loads of type TYPE_SUBDOCUMENT. In the other case where we evaluate a CSP we always check OverridesCSP() first. That means for all other subresource loads I think we do the right thing, but not in this particular case.

My suggestion is to use OverridesCSP() in the frameloader as well, which is also what I incorporated in the patch which I'll upload in a minute. FWIW, I also think that Bug 1528790 is unrelated to the overrideCSP stuff and hopefully goes away when having CSP hang off a document/client rather then the principal.

Comment 13

[Christoph Kerschbaumer [:ckerschb]]

Attachment: Summary: Bug 1529877: Only query the CSP from the Principal in case it's an ExpandedPrincipal within nsFrameLoader. r=bz

Comment 14

[Boris Zbarsky [:bzbarsky]]

I also think that Bug 1528790 is unrelated to the overrideCSP stuff

And yet, the proposed patch fixes that bug, in a surprising coincidence... ;)

Comment 15

[Christoph Kerschbaumer [:ckerschb]]

Can someone check in the phabricator changes please - thanks!

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/90615191631a
Only query the CSP from the Principal in case it's an ExpandedPrincipal within nsFrameLoader. r=bz

Comment 17

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/90615191631a

Comment 18

[Christoph Kerschbaumer [:ckerschb]]

Attachment: Summary: Bug 1529877: Do not query the CSP from the principal within LoadFrame, but rather do not even set the Principal if it does not override the CSP within nsContentUtils::GetAttrTriggeringPrincipal. r=bz,kmag