881509 - Content Security Policy ShouldLoad and ShouldProcess do not use request principal

Status: RESOLVED FIXED

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

Description

[Deian Stefan]

Attachment: Try to use aRequestPrincipal if available

The CSP |ShouldLoad| and |ShouldProcess| don't use |aRequestPrincipal|, if available, but try getting the requesting from the DOM node using |aRequestingContext|. This means that we can't enforce CSP (namely apply the connect-src directive) in |Cu.Sandbox|es if the sandbox principal has an associated CSP policy.

Comment 1

[Ian Melven :imelven]

Adding a few more people who might have feedback as well. I'll do my best to take a look at this by sometime early next week - I have a couple of larger patches I'd like to get through first :)

Comment 2

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

This patch seems reasonable. But, if this patch is correct then it seems like other nsIContentPolicy implementations are also wrong, if I am understanding it correctly. Are we supposed to always use aRequestPrincipal if it is available, and then fall back if aRequestPrincipal is not available? Why wouldn't we just require that aRequestPrincipal must always be passed in, to simplify this? Under which conditions will we not have aRequestPrincipal but we will have aRequestingContext and/or aRequestingLocation?

Comment 3

[Boris Zbarsky [:bzbarsky]]

For nsIContentPolicy calls coming from inside Gecko code, I believe we will always have aRequestPrincipal.

For calls from addons, all bets are off.

Comment 4

[Devdatta Akhawe [:devd]]

It is not clear to me what is the correct solution for this either. We do want addons to load resources unconstrained by the page's CSP. Note that this is different from mixed-content, where we don't want even addons to load plain-text stuff in secure pages. 

Maybe for CSP, the right solution is to only use aRequestPrincipal? Gecko code will always pass it in and addons that want to be constrained by aRequestPrincipal can set the principal to the document's principal. I am not sure.

Comment 5

[Deian Stefan]

Attachment: Fallback to aRequestPrincipal

So I think the better approach here is to actually use the node
principal and then fall back on the aRequestinPrincipal. 

In some cases the aRequestPrincipal |Equals| the (node) principal from
aRequestContext, but may be missing the CSP. (This is somewhat
worrisome, but maybe someone can explain why it's the case.) For
example, when creating a Worker (WorkerPrivate::Create) the worker
principal (and mCSP) is set to the subject principal (and its CSP,
resp.). However, the worker script loader sets the principal
(SetPrincipal) to the channel principal (in
Worker::ScriptLoader::OnStreamCompleteInternal), which does not have
the CSP -- when using XHR the CSP policy attached to this principal is
used. (I don't think there is a mochitest for this yet, but I'll upload one.)

Comment 6

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Boris Zbarsky (:bz) from comment #3)
> For nsIContentPolicy calls coming from inside Gecko code, I believe we will
> always have aRequestPrincipal.

We should be MOZ_ASSERT()ing this, with fallback logic for release builds where the assertion doesn't hold. And/or, we should change whatever APIs that addons would call so that the aRequestPrincipal becomes mandatory (the API should throw an exception when not provided). 

(In reply to Devdatta Akhawe [:devd] from comment #4)
> It is not clear to me what is the correct solution for this either. We do
> want addons to load resources unconstrained by the page's CSP.

Why do we want that?

> Maybe for CSP, the right solution is to only use aRequestPrincipal? Gecko
> code will always pass it in and addons that want to be constrained by
> aRequestPrincipal can set the principal to the document's principal. I am
> not sure.

Addons that are running in a chrome context should pass in the system principal. Then we can enforce certain properties more comprehensively (e.g. mixed content blocker can block http:// scripts loaded from addons).

Comment 7

[Devdatta Akhawe [:devd]]

I think they are equal because the worker principal and the channel principal is same-origin (line 280 in nsPrincipal.cpp). 

The cleaner fix, it seems to me, is that the Worker code set the aRequestPrincipal to the correct, CSP-carrying principal. nsIContentPolicy is a security interface, the aRequestPrincipal argument should be the principal whose security policy should be enforced on this element. NodePrincipal does not have this semantic---heck, it is not even clear to me /why/ the Worker code is setting the NodePrincipal; there is no "Node" to speak of.

Note that the original motivation for this bug is exactly this issue: the current code is using this ill-defined "NodePrincipal" (which doesn't work for sandboxes), instead of using aRequestPrincipal which is intended to be "the principal whose security policy should be used"

That said, I (obviously) defer to others on the CC list. I imagine bz knows why Worker code is doing what it is doing.

Comment 8

[Boris Zbarsky [:bzbarsky]]

I don't even know which worker code you're talking about.  ;)  Let's try Ben for worker stuff.

Comment 9

[Devdatta Akhawe [:devd]]

> Why do we want that?

I thought that was intended behavior. The spec and discussions on the WG certainly indicate that to be the case. But, given the problem of malicious addons, I can see the value of allowing websites to override addons.

> Addons that are running in a chrome context should pass in the system
> principal. Then we can enforce certain properties more comprehensively (e.g.
> mixed content blocker can block http:// scripts loaded from addons).

I think the problem is getting addons to switch and the compat hit.

Comment 10

[Tanvi Vyas[:tanvi]]

(In reply to Devdatta Akhawe [:devd] from comment #7)
> The cleaner fix, it seems to me, is that the Worker code set the
> aRequestPrincipal to the correct, CSP-carrying principal. nsIContentPolicy
> is a security interface, the aRequestPrincipal argument should be the
> principal whose security policy should be enforced on this element.
> NodePrincipal does not have this semantic---heck, it is not even clear to me
> /why/ the Worker code is setting the NodePrincipal; there is no "Node" to
> speak of.
> 
> Note that the original motivation for this bug is exactly this issue: the
> current code is using this ill-defined "NodePrincipal" (which doesn't work
> for sandboxes), instead of using aRequestPrincipal which is intended to be
> "the principal whose security policy should be used"
> 
> That said, I (obviously) defer to others on the CC list. I imagine bz knows
> why Worker code is doing what it is doing.

We came across a bug in the Mixed Content Blocker implementation - see bug 909920.  We have an HTTP page that loads an HTTPS stylesheet.  The HTTPS stylesheet loads an HTTP image.  The HTTP image triggers a warning by the Mixed Content Blocker because MCB uses aRequestingLocation and aRequestPrincipal before it uses aRequestingContext.  aRequestingLocation and aRequestPrincipal are the location and principal of the HTTPS css stylesheet and NOT that of the HTTP page.

From this bug, it sounds like the same thing would be true for a Worker, who's principal is set to worker principal instead of the CSP-carrying top-level page or frame principal.  This may also be the case for objects and SVG, though we have not confirmed this.

Hence, it is arguably a good thing that CSP is relying on aRequestingContext first.  In Bug 909920, we are changing the MCB code to do the same.  And then fall back to aRequestPrincipal or aRequestingLocation if we can't get a principal and uri from aRequestingContext.  If you think this is the wrong way to go about this for MCB (as this bug implies for CSP), please let us know.

Comment 11

[Devdatta Akhawe [:devd]]

I think the cleaner fix is to change aRequestPrincipal to be the principal of the Window, not the sheet. atleast, that's how I understand the semantics of aRequestPrincipal. The good thing about aRequestPrincipal is that right now it is (afaik) only used inside Gecko so we can make sure it is the "right" security principal. Since in this case, the downloaded image will be shown in the owning window, that is what the principal should be.

bz: any reason why the aRequestPrincipal should be the principal of the sheet?

Comment 12

[Deian Stefan]

Semantically, I agree with Dev.

I'll try to looking into the worker stuff more this weekend, but as
said in comment #5 I think that falling back on the
aRequestingPrincipal for now is actually the way to go here (vs. not
even looking at it).

Comment 13

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to Devdatta Akhawe [:devd] from comment #7)
> The cleaner fix, it seems to me, is that the Worker code set the
> aRequestPrincipal to the correct, CSP-carrying principal. nsIContentPolicy
> is a security interface, the aRequestPrincipal argument should be the
> principal whose security policy should be enforced on this element.
> NodePrincipal does not have this semantic---heck, it is not even clear to me
> /why/ the Worker code is setting the NodePrincipal; there is no "Node" to
> speak of.

I'm not a CSP expert by any means but maybe I can help by explaining the current behavior.

When a page loads a worker we consult the CSP and the node principal of the page to make sure that the load is allowed. Once the script loads we set the worker principal to the channel principal of the script and then use any CSP associated with that principal for all additional loads.

For dedicated workers we could certainly keep the original CSP. For shared workers we cannot since shared workers don't belong to a single document. Our existing code ensures the same behavior for both. I like our current behavior because the idea of making dedicated and shared workers behave differently seems wrong. However this isn't really a decision that Gecko should make. The web security working group should be clear about what should happen here.

Comment 14

[Boris Zbarsky [:bzbarsky]]

> I think the cleaner fix is to change aRequestPrincipal to be the principal of the
> Window, 

aRequestPrincipal is the principal of the thing doing the load.  When that's the sheet, it needs to be the principal of the sheet, not of the window.  Those are the intended semantics of aRequestPrincipal, I believe, as of when we landed it.  Those are _certainly_ the intended semantics of the nsIPrincipal argument to NS_CheckContentLoadPolicy.

Changing what we pass to NS_CheckContentLoadPolicy would for, e.g., privileged sheets added to content pages would break loads from those sheets, no?

> The good thing about aRequestPrincipal is that right now it is (afaik) only used inside
> Gecko

It's used to determine aRequestOrigin, too.  Addons most certainly use that. 

Again, unless the suggestion is to have aRequestPrincipal not be the principal that was passed to NS_CheckContentLoadPolicy.

> is that right now it is (afaik) only used inside Gecko

You've checked addons?

Here's a thought experiment: Consider a stylesheet being shared across documents that has an @import rule.  Which principal should be passed to the content policy check for that @import load?

Comment 15

[Deian Stefan]

Attachment: 0001-Bug-881509-Use-aRequestPrincipal-in-CSP-Should-Load-.patch

The worker CSP is not used for anything other than reporting. The node
principal is used for everything else (so setting the CSP according to
headers does nothing without this patch).

Also: IMHO, we should not be keeping the CSP separate from the policy;
is there a reason this was done this way?

Comment 16

[Tanvi Vyas[:tanvi]]

Comment on attachment 8402263 [details] [diff] [review]
0001-Bug-881509-Use-aRequestPrincipal-in-CSP-Should-Load-.patch

This patch uses aRequestPrincipal first (loadInfo->triggeringPrincipal) as the principal to use when determining the CSP of the page.  And then fallbacks to the aRequestContext's node principal (loadInfo->loadingPrincipal).  I think we should do the opposite.

Should the CSP of a document be overriden by the CSP of an imported stylesheet that tries to import more subresources?  I don't think so.  This patch would allow that.

However, I don't understand all the nuances around workers.  Shared workers don't have nodes attached to them, so the current code wouldn't apply any CSP policy to them even if one is set by the worker (is that correct?).  In that case I believe a code change that falls back to aRequestPrincipal in the absence of aRequestContext would work.

Comment 17

[Christoph Kerschbaumer [:ckerschb]]

Hey Deian, Ethan is about to look into Bug 959388. Since this bug is blocking Bug 959388 I assigned this one to Ethan as well. Since you mentioned you are pretty busy with other stuff at the moment, I hope you don't mind.

Comment 18

[Tanvi Vyas[:tanvi]]

From my understanding, the change that needs to be made here is:
if aRequestingContext doesn't yield a principal in CSP, fall back to aRequestPrincipal.

Why is this blocking bug 959388?

Comment 19

[Ethan Tseng [:ethan]]

(In reply to Tanvi Vyas [:tanvi] from comment #18)
> Why is this blocking bug 959388?

Deian set this dependency in bug 959388.
Deian, could you explain why?

Comment 20

[Deian Stefan]

(In reply to Ethan Tseng [:ethan] from comment #19)
> (In reply to Tanvi Vyas [:tanvi] from comment #18)
> > Why is this blocking bug 959388?
> 
> Deian set this dependency in bug 959388.
> Deian, could you explain why?

Thanks for taking over these, Ethan.

IIRC for the worker the path was falling back to the requesting principal since the requesting context didn't yield a principal, but I honestly don't remember well enough to say this with any confidence.

Comment 21

[Christoph Kerschbaumer [:ckerschb]]

Within Bug 1208559 we are changing the behavior within CSP::ShouldLoad():
1) We try to query the principal from the requesting context
2) if that is null, then we fall back to using the requestingPrincipal, see
http://hg.mozilla.org/mozilla-central/annotate/412e4d7ce98c/dom/security/nsCSPService.cpp#l202

Marking this bug as fixed.