Last Comment Bug 767134 - Stuff principal for forms in the contentpolicy calls
: Stuff principal for forms in the contentpolicy calls
Status: RESOLVED FIXED
: addon-compat, dev-doc-needed
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: unspecified
: x86_64 Linux
: -- normal (vote)
: mozilla16
Assigned To: Devdatta Akhawe [:devd]
:
: Andrew Overholt [:overholt]
Mentors:
Depends on: 771736
Blocks: 803765
  Show dependency treegraph
 
Reported: 2012-06-21 14:03 PDT by Devdatta Akhawe [:devd]
Modified: 2012-10-19 18:10 PDT (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
simple patch that attaches the principal in specific cases (1.81 KB, patch)
2012-06-21 14:06 PDT, Devdatta Akhawe [:devd]
no flags Details | Diff | Splinter Review
Add a requestPrincipal to nsIcontentpolicy (15.34 KB, patch)
2012-06-21 20:54 PDT, Devdatta Akhawe [:devd]
bzbarsky: review+
Details | Diff | Splinter Review
Add a requestPrincipal to nsIcontentpolicy (14.19 KB, patch)
2012-06-22 10:12 PDT, Devdatta Akhawe [:devd]
bzbarsky: review+
Details | Diff | Splinter Review
Add requestPrincipal to nsIContentPolicy (14.38 KB, patch)
2012-06-22 12:54 PDT, Devdatta Akhawe [:devd]
bzbarsky: review+
Details | Diff | Splinter Review
Add requestPrincipal to nsIContentPolicy (21.79 KB, patch)
2012-06-27 18:23 PDT, Devdatta Akhawe [:devd]
mozbugs: review+
jst: superreview+
mozbugs: checkin+
Details | Diff | Splinter Review

Description Devdatta Akhawe [:devd] 2012-06-21 14:03:14 PDT
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.
Comment 1 Devdatta Akhawe [:devd] 2012-06-21 14:06:08 PDT
Created attachment 635456 [details] [diff] [review]
simple patch that attaches the principal in specific cases
Comment 2 Boris Zbarsky [:bz] (still a bit busy) 2012-06-21 14:28:05 PDT
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.
Comment 3 Devdatta Akhawe [:devd] 2012-06-21 20:54:59 PDT
Created attachment 635594 [details] [diff] [review]
Add a requestPrincipal to nsIcontentpolicy

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 ?
Comment 4 Boris Zbarsky [:bz] (still a bit busy) 2012-06-21 21:10:46 PDT
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.
Comment 5 Devdatta Akhawe [:devd] 2012-06-22 10:12:01 PDT
Created attachment 635796 [details] [diff] [review]
Add a requestPrincipal to nsIcontentpolicy

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).
Comment 6 Boris Zbarsky [:bz] (still a bit busy) 2012-06-22 12:02:08 PDT
> 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?  ;)
Comment 7 Boris Zbarsky [:bz] (still a bit busy) 2012-06-22 12:05:01 PDT
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
Comment 8 Devdatta Akhawe [:devd] 2012-06-22 12:54:10 PDT
Created attachment 635874 [details] [diff] [review]
Add requestPrincipal to nsIContentPolicy

thanks. Fixed.
Comment 9 Boris Zbarsky [:bz] (still a bit busy) 2012-06-22 12:56:51 PDT
Comment on attachment 635874 [details] [diff] [review]
Add requestPrincipal to nsIContentPolicy

r=me
Comment 10 Devdatta Akhawe [:devd] 2012-06-27 18:23:20 PDT
Created attachment 637333 [details] [diff] [review]
Add requestPrincipal to nsIContentPolicy

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?
Comment 11 Devdatta Akhawe [:devd] 2012-06-27 18:25:01 PDT
Also, here's a link to the Try https://tbpl.mozilla.org/?tree=Try&rev=9a046282d0e6
Comment 12 Sid Stamm [:geekboy or :sstamm] 2012-06-28 09:40:22 PDT
Comment on attachment 637333 [details] [diff] [review]
Add requestPrincipal to nsIContentPolicy

Carrying over the previous r=bz.
Comment 13 Sid Stamm [:geekboy or :sstamm] 2012-07-02 16:22:53 PDT
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.
Comment 14 Ed Morley [:emorley] 2012-07-03 02:15:54 PDT
https://hg.mozilla.org/mozilla-central/rev/2777ec78a2b5
Comment 15 Octoploid 2012-07-03 11:28:13 PDT
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
Comment 16 Devdatta Akhawe [:devd] 2012-07-03 11:53:24 PDT
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.
Comment 17 Octoploid 2012-07-03 12:10:12 PDT
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);
Comment 18 Boris Zbarsky [:bz] (still a bit busy) 2012-07-06 13:52:45 PDT
> 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.
Comment 19 Devdatta Akhawe [:devd] 2012-07-06 14:16:32 PDT
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?
Comment 20 Boris Zbarsky [:bz] (still a bit busy) 2012-07-06 18:28:19 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.