Devdatta Akhawe [:devd] Assignee
	Description • 12 years ago

Imagine a.com has a form that posts to b.com that we want CSP to block (CSP 1.1 has this feature currently). Alternatively, a.com has a link to b.com that we want to block (CSP might consider in the future).

Currently, the NS_CheckContentLoadPolicy calls from nsdocshell do not send the principal that caused the request (in this case a.com). Since the CSP is attached to the principal, it is not possible for CSPService to know that there is a CSP for this request.

I suggest using the "extra" parameter in the call to attach the principal causing the request, so that CSP can use it.

	Boris Zbarsky [:bzbarsky]
	Comment 2 • 12 years ago

Or we could just actually start passing the request principal along as part of the normal content policy API... Pretty sure we have bugs on that.

The point of aExtra is that it's an extension mechanism that we promise to never use ourselves, so we can't use it here.

	Devdatta Akhawe [:devd] Assignee
	Comment 3 • 12 years ago

Modified to just add a new argument to ContentPolicy, as discussed with bz. 

This patch is only for making changes to the IDL, haven't modified CSP to make use of this, nor have I modified the individual NS_CheckContentPolicy calls to pass a relevant argument. Should those be on this bug or a new bug ?

	Boris Zbarsky [:bzbarsky]
	Comment 4 • 12 years ago

Comment on attachment 635594 [details] [diff] [review]
Add a requestPrincipal to nsIcontentpolicy

NS_CheckContentLoadPolicy already had a originPrincipal argument.  No need to add a new argument to that method.  Same for NS_CheckContentProcessPolicy.

Please follow local argument-naming style (aRequestPrincipal in various places where that's the style).

Missing space before "decision" in nsContentPolicy::CheckPolicy.

For nsContentPolicy::CheckPolicy, please pass the requestPrincipal as the arg _before_ decision, since decision is the out param.

Missing space after ',' in the NS_STDCALL_FUNCPROTO gunk in nsContentPolicy.h

I think you should just change loadingPrincipal in the docshell code to be the QI from aOwner if that's not null.  If it's null, then fall back to what it does right now.

Please don't name locals with names starting with "a": those indicate arguments in that code.

Please fix the indent in nsContentBlocker::ShouldLoad.

r=me with those nits (and the one substantive change to loadingPrincipal) picked.

	Devdatta Akhawe [:devd] Assignee
	Comment 5 • 12 years ago

Thanks for the quick review.

I incorporated all your changes. The spacing in nsContentBlocker::ShouldLoad was already broken, and I tried fixing it, but it still looks weird in the diff (the actual file looks all right in vim).

	Boris Zbarsky [:bzbarsky]
	Comment 6 • 12 years ago

> but it still looks weird in the diff

That's because there were some stray tabs in the file.  Just replace tab with 8 spaces globally on it?  ;)

	Boris Zbarsky [:bzbarsky]
	Comment 7 • 12 years ago

Comment on attachment 635796 [details] [diff] [review]
Add a requestPrincipal to nsIcontentpolicy

>+++ b/docshell/base/nsDocShell.cpp
>+    if(ownerPrincipal){

Space before '(', please.

Also, probably better to do this earlier like so:

  nsCOMPtr<nsIPrincipal> loadingPrincipal = do_QueryInterface(aOwner);

and do the various loadingPrincipal stuff we do right now only if the result is null.

r=me

	Devdatta Akhawe [:devd] Assignee
	Comment 8 • 12 years ago

thanks. Fixed.

	Boris Zbarsky [:bzbarsky]
	Comment 9 • 12 years ago

Comment on attachment 635874 [details] [diff] [review]
Add requestPrincipal to nsIContentPolicy

r=me

	Devdatta Akhawe [:devd] Assignee
	Comment 10 • 12 years ago

Made the argument optional since it seems calling this code is actually pretty common. Anyone know how I can carry over the r+ from bz?

	Devdatta Akhawe [:devd] Assignee
	Comment 11 • 12 years ago

Also, here's a link to the Try https://tbpl.mozilla.org/?tree=Try&rev=9a046282d0e6

	Sid Stamm [:geekboy or :sstamm]
	Comment 12 • 12 years ago

Comment on attachment 637333 [details] [diff] [review]
Add requestPrincipal to nsIContentPolicy

Carrying over the previous r=bz.

	Sid Stamm [:geekboy or :sstamm]
	Comment 13 • 12 years ago

Pushed to inbound:
http://hg.mozilla.org/integration/mozilla-inbound/rev/2777ec78a2b5

For future patches, dev, please make sure your user is set in HG and the bug number is in the summary for the patch file.

	Ed Morley [:emorley]
	Comment 14 • 12 years ago

https://hg.mozilla.org/mozilla-central/rev/2777ec78a2b5

	Octoploid
	Comment 15 • 12 years ago

FYI this commit "breaks" the RequestPolicy add-on.
When you (manually) enter a new URL on a existing page RequestPolicy always
prompts "This webpage has asked to redirect to (new URL) Allow, Deny, More...". 

See: https://github.com/RequestPolicy/requestpolicy/issues/318

	Devdatta Akhawe [:devd] Assignee
	Comment 16 • 12 years ago

Weird, the requestPolicy code uses the JS interface to nsIContentPolicy, which hasn't changed at all. I am going to wait for Justin to comment on the bug over there.

	Octoploid
	Comment 17 • 12 years ago

This patch "fixes" the issue:

diff --git a/docshell/base/nsDocShell.cpp b/docshell/base/nsDocShell.cpp
index c497410..9760c22 100644
--- a/docshell/base/nsDocShell.cpp
+++ b/docshell/base/nsDocShell.cpp
@@ -8162,8 +8162,8 @@ nsDocShell::InternalLoad(nsIURI * aURI,
     }
 
     // XXXbz would be nice to know the loading principal here... but we don't
-    nsCOMPtr<nsIPrincipal> loadingPrincipal = do_QueryInterface(aOwner);
-    if (!loadingPrincipal && aReferrer) {
+    nsCOMPtr<nsIPrincipal> loadingPrincipal;
+    if (aReferrer) {
         nsCOMPtr<nsIScriptSecurityManager> secMan =
             do_GetService(NS_SCRIPTSECURITYMANAGER_CONTRACTID, &rv);
         NS_ENSURE_SUCCESS(rv, rv);

	Boris Zbarsky [:bzbarsky]
	Comment 18 • 12 years ago

> which hasn't changed at all.

Well, it has.  We now send a different principal for pageloads, no?  That will affect the aRequestOrigin in the JS API, to the extent that we have an owner and its URI does not match the aReferrer value...

It sounds like we used to send an empty referrer for loads from the URL bar but the principal for those loads is the currently-loaded page?  Have you tried to look at the callstack to InternalLoad with the steps to reproduce and see what aOwner is and where it comes from?

It sounds like we need to get a bug filed on this, blocking this one, by the way.

	Devdatta Akhawe [:devd] Assignee
	Comment 19 • 12 years ago

There was a previous version of the patch that modified NS_CheckContentPolicy (adding 1 more argument) but Comment 4 suggested that we just reuse the existing argument. How about we go back to adding 1 more argument to the NS_CheckContentPolicy ? That should fix this, right?

	Boris Zbarsky [:bzbarsky]
	Comment 20 • 12 years ago

Well, yes, at the cost of making most consumers of NS_Check* more complicated....

We could make the one more argument optional (defaulting to null) and when it's null use the existing principal argument.  Then have only docshell pass the optional argument.

But this case:

> Alternatively, a.com has a link to b.com that we want to block (CSP might consider in
> the future).

would then break for loads from the browser UI, no?  Because they would look like they're coming from a.com.  So it might be better to really figure out what's going on here.

And again, it's best to do it in a separate bug.