608131 - iframes beneath CSP documents have parent document's policy applied to link clicks

Status: RESOLVED WORKSFORME

(Core :: DOM: Core & HTML, defect)

Description

[Brandon Sterne (:bsterne)]

When a page specifies a CSP such as "allow 'self'" and embeds an iframe from the same origin, that iframe should not have the policy applied to it.  Our implementation is correct except for the case of link clicks.

In such cases when CSPService::ShouldLoad is called it gets passed the nsHTMLIFrameElement as aRequestContext, which is where we get the principal to do our CSP check.  Since the iframe _element_ has the parent document's principal, we wind up using its CSP.  We need to either special case link clicks, or otherwise figure out how to get the correct principal on which to look for a CSP.

Comment 1

[Brandon Sterne (:bsterne)]

Here's a test.  Note the external image loads but you can't follow the link.

Comment 2

[Boris Zbarsky [:bzbarsky]]

> In such cases when CSPService::ShouldLoad is called it gets passed the
> nsHTMLIFrameElement as aRequestContext

That seems like a bug.  We should be passing in the link here, no?

Of course that won't help things like location sets from script...

Perhaps we should pass the window the load is happening in as the request context, unless the load is actually coming from an src attr on a frame?

Comment 3

[Brandon Sterne (:bsterne)]

It does seem like that's the bug, yes.  We'd definitely get the right principal if we passed the link element to ShouldLoad.  I can see why it was implemented this way, though.  With images, we pass the image element because that's where the content is loaded _into_.  The same thinking was probably applied in the iframe case.

Boris, do you envision any side effects of switching aRequestContext to the link element?  I suppose the first step would be to see if any tests break with such a change.

I'm unclear what's happening in the script navigation case that you mentioned (and I updated the test to include that case).  The thing that's passed to ShouldLoad there is a nsISupports and it has the principal of the outer document, so the behavior is wrong there as well.

Comment 4

[Boris Zbarsky [:bzbarsky]]

I doubt we have tests covering this scenario.

The only thing likely to break is things like adblock/flashblock/noscript.  Check with their authors?

> The thing that's passed to ShouldLoad there is a nsISupports and it has the
> principal of the outer document

Well, the same thing is passed for that location set as for the link right now: the iframe element.  The question is what _should_ be passed.  Arguably the Window inside the subframe....

Comment 5

[Giorgio Maone [:ma1]]

So, if I understand correctly, we're talking about shouldLoad() with aContentType == nsIContentPolicy.TYPE_SUBDOCUMENT; we're excluding passing the link element (which does not make sense to me anyway) or the parent window (suggested in early comments), but bz suggest to pass the subframe.contentWindow.

This would not be a problem for me (passing the link or the parent window would be a big one, instead) because we can still grab the frame element (both ABP and NoScript use it to add UI stuff at a later time) through window.frameElement. 

I'm not sure whether this would be an improvement for CSP, though, if it uses aContext to identify the principal for its policies: subframe.contentWindow's principal, at the time shouldLoaad() is called, can be anything (even about:blank) and not necessarily related neither to the loader nor with the content to be loaded... IMHO using aContentLocation would be more sensible, but I don't know what CSP is supposed to do here enough to judge.

Comment 6

[Boris Zbarsky [:bzbarsky]]

I think fundamentally the issue is that CSP wants to know "who originated this load", right?

Maybe we should just go ahead and change the shouldLoad API to take a principal identifying this?  Note that NS_CheckContentLoadPolicy already takes such an argument, so we might not even have to update too many callers....

The other option is that the calls Gecko makes could pass the principal as aExtra for now.  Hacky, but might be good enough.

Either solution would probably need to land in b7 if it lands for 2.0 at all.  :(

Comment 7

[Giorgio Maone [:ma1]]

(In reply to comment #6)
> I think fundamentally the issue is that CSP wants to know "who originated this
> load", right?

If it is so, doesn't shouldLoad() already have this info (more or less, document.domain modifications aside) as the aRequestOrigin URI?

Comment 8

[Boris Zbarsky [:bzbarsky]]

No.  That's the URI of the thing that started the load, which can be quite different from the principal (see about:blank).

Comment 9

[Sid Stamm [:geekboy or :sstamm]]

If we pass in the principal directly to ShouldLoad, it would (by design) represent something a bit different than the requestContext that CSP is currently using to find the CSP object.  Basically there'd be the requestContext that points to the iframe node and a principal that doesn't belong to the iframe.  This might be a bit confusing...

My thought is that it makes more sense to change the requestContext to be the link node so long as it doesn't cause hell for those who currently expect it that way.  If we can't do that, adding the principal as currentPrincipal or requestPrincipal to the API is my second choice since it could potentially be useful for add-ons who want it.

Comment 10

[Boris Zbarsky [:bzbarsky]]

> so long as it doesn't cause hell

Comment 5 says that it causes hell.

Comment 11

[Giorgio Maone [:ma1]]

(In reply to comment #10)
> > so long as it doesn't cause hell
> 
> Comment 5 says that it causes hell.

Yes it does. 
Could you explain me more in depth the difference between "who started the load" (comment #6) and "who originated the load" (comment #8)?

Comment 12

[Boris Zbarsky [:bzbarsky]]

The difference is between uri and principal, not "started" vs "originated"...

Comment 13

[Giorgio Maone [:ma1]]

(In reply to comment #12)
> The difference is between uri and principal, not "started" vs "originated"...

Ah, OK. 
Then having the principal as a further argument is definitely valuable for content policies, and unlikely to break anything. 
I vote for the API change.

Comment 14

[Sid Stamm [:geekboy or :sstamm]]

Attachment: patch to add principal to ShouldLoad/ShouldProcess

Here's a first whack at a patch that adds a new requestPrincipal variable to the ShouldLoad and ShouldProcess bits of the api.

Comment 15

[Sid Stamm [:geekboy or :sstamm]]

That patch to add the principal to the API doesn't completely jive with CSP.  It causes one of the frame-src tests to fail: an iframe source that should be blocked by CSP is in fact not blocked by CSP, which leads me to think that the principal is either null or not the right one. I'm going to try and track it down.

Comment 16

[Wladimir Palant]

Adblock Plus indeed rely on the context being the element where the frame is loaded into to display correct information in the UI. But this would be a rather simple adaptation (unlike compensating for bug 467514 which is somewhat related here and probably affects CSP as well).

As to adding aRequestingPrincipal parameter to ShouldLoad - I would suggest inserting this parameter after aRequestingContext. This would allow all JS-based implementations to work unchanged because IMHO none of them care about aMimeGuess or aExtra parameters. Note also that some extensions are triggering content policies and might cause havoc if not adapted (Flashblock is the one I know about).

Comment 17

[Giorgio Maone [:ma1]]

(In reply to comment #16)

> As to adding aRequestingPrincipal parameter to ShouldLoad - I would suggest
> inserting this parameter after aRequestingContext. This would allow all
> JS-based implementations to work unchanged because IMHO none of them care about
> aMimeGuess or aExtra parameters. 

NoScript does care about both, actually. Don't know about other clients.
However I can adapt quickly, and in fact I plan to use aRequestPrincipal when available, hence I didn't bother suggesting an ugly, albeit easier for me, interface like appending the new argument at the very end of the list.

Comment 18

[Brandon Sterne (:bsterne)]

IMO, the simplest patch will be to special case iframes in CSP's ShouldLoad impl to get the principal from the contentDocument.  Seems like that patch would have the least side effects for other content policies as well.

Comment 19

[Sid Stamm [:geekboy or :sstamm]]

I agree with Brandon.  If this is a bug in the way ShouldLoad is called, we could add to the API or modify the arguments, otherwise there should be a way to get at the right principal from inside the CSP code, and we could just do that.

Comment 20

[Giorgio Maone [:ma1]]

(In reply to comment #18)
>  CSP's ShouldLoad
> impl to get the principal from the contentDocument. 
Is there a way for a JS component to do that?
If not, could it be added? (I was hoping in the requestPrincipal argument).

Comment 21

[Boris Zbarsky [:bzbarsky]]

> special case iframes in CSP's ShouldLoad impl to get the principal from the
> contentDocument.

The point is, that will give the wrong answer, no?  If a script sets the location on an iframe, shouldn't the principal used be that of the script?

Comment 22

[Giorgio Maone [:ma1]]

(In reply to comment #21)
> > special case iframes in CSP's ShouldLoad impl to get the principal from the
> > contentDocument.
> 
> The point is, that will give the wrong answer, no?  If a script sets the
> location on an iframe, shouldn't the principal used be that of the script?

Yes, IMHO.
This is a piece of information we currently don't have in shouldLoad() (the nearest thing we've got is the URI of the document where that script "lives", aRequestOrigin), and having it would be nice for other content policies as well, not just CSP.

Comment 23

[Sid Stamm [:geekboy or :sstamm]]

I think this is related to bug 548984.  Right now CSP blocks all javascript: URIs at both the nsIContentPolicy::ShouldLoad level, but also inside the nsJSProtocolHandler.  In the former, the principal of the script is not available (only the node whose src is being set to a javascript: URI).  In the latter case, there's more context and we can probably make a better decision.

If bug 548984 gets fixed, maybe we could just address it in nsJSProtocolHandler (and the CSP nsIContentPolicy could just ignore javascript: URIs)?

Comment 24

[mhfrltza]

I'm commenting on this bug as I think this is related, even when options eval-script and inline-script are set for the parent, the javascript: url still reports a CSP exception. 

Below is an example php script to demonstrate the issue:

<?php

header("X-Content-Security-Policy: allow 'self'; options eval-script inline-script;");

echo "<html><head></head><body>";

if ($_GET['a']=="Y")
{
echo "Inner frame: <a href=\"javascript:alert('js okay');\">Click to test</a>";
} else {
echo "Outer frame <iframe src=\"?a=Y\">";
}

echo "</body></html>";

Comment 25

[Brandon Sterne (:bsterne)]

(In reply to mhfrltza from comment #24)
> I'm commenting on this bug as I think this is related, even when options
> eval-script and inline-script are set for the parent, the javascript: url
> still reports a CSP exception.

This is a different issue that will be fixed when bug 702439 lands.

Comment 26

[Sid Stamm [:geekboy or :sstamm]]

This has been open a while... is this still an issue?

Comment 27

[Sid Stamm [:geekboy or :sstamm]]

Attachment: example for manual test

Here's a test to try out an inner frame without CSP and an outer frame with CSP (and link clicks/inline scripts).

nsIContentPolicy now has the principal in shouldload/shouldprocess (bug 767134), so the proposed patch is obsolte.

Comment 28

[Sid Stamm [:geekboy or :sstamm]]

I think this works as intended now.  Inner-frame scripts are not subject to outer-frame CSP, and link navigations to sites where the outer frame forbids it's frame-src to be those sites... well, those get blocked appropriately.

I'm going to go ahead and close this bug since I think this got fixed by bug 767134 and bug 702439.  If you don't think this is fixed, please upload a test case that you think breaks with our current implementation of CSP.