Stuff principal for forms in the contentpolicy calls

RESOLVED FIXED in mozilla16

Status

()

Core
DOM: Core & HTML
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: devd, Assigned: devd)

Tracking

({addon-compat, dev-doc-needed})

unspecified
mozilla16
x86_64
Linux
addon-compat, dev-doc-needed
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

5 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.
(Assignee)

Comment 1

5 years ago
Created attachment 635456 [details] [diff] [review]
simple patch that attaches the principal in specific cases
(Assignee)

Updated

5 years ago
Assignee: nobody → dev.akhawe+mozilla
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.
(Assignee)

Comment 3

5 years ago
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 ?
Attachment #635594 - Flags: review?(bzbarsky)
(Assignee)

Updated

5 years ago
Attachment #635456 - Attachment is obsolete: true
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.
Attachment #635594 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 5

5 years ago
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).
Attachment #635594 - Attachment is obsolete: true
Attachment #635796 - Flags: review?
(Assignee)

Updated

5 years ago
Attachment #635796 - Flags: review? → review?(bzbarsky)
> 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 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
Attachment #635796 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 8

5 years ago
Created attachment 635874 [details] [diff] [review]
Add requestPrincipal to nsIContentPolicy

thanks. Fixed.
Attachment #635796 - Attachment is obsolete: true
Attachment #635874 - Flags: review?(bzbarsky)
Comment on attachment 635874 [details] [diff] [review]
Add requestPrincipal to nsIContentPolicy

r=me
Attachment #635874 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 10

5 years ago
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?
Attachment #635874 - Attachment is obsolete: true
Attachment #637333 - Flags: superreview?(jst)
(Assignee)

Comment 11

5 years ago
Also, here's a link to the Try https://tbpl.mozilla.org/?tree=Try&rev=9a046282d0e6
Comment on attachment 637333 [details] [diff] [review]
Add requestPrincipal to nsIContentPolicy

Carrying over the previous r=bz.
Attachment #637333 - Flags: review+

Updated

5 years ago
Attachment #637333 - Flags: superreview?(jst) → superreview+
(Assignee)

Updated

5 years ago
Attachment #637333 - Flags: checkin?(sstamm)
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.
Target Milestone: --- → mozilla16
Attachment #637333 - Flags: checkin?(sstamm) → checkin+
https://hg.mozilla.org/mozilla-central/rev/2777ec78a2b5
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED

Comment 15

5 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
(Assignee)

Comment 16

5 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.

Comment 17

5 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);
> 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.
(Assignee)

Comment 19

5 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?
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.
(Assignee)

Updated

5 years ago
Depends on: 771736
(Assignee)

Updated

5 years ago
Keywords: addon-compat, dev-doc-needed
(Assignee)

Updated

5 years ago
Blocks: 803765
You need to log in before you can comment on or make changes to this bug.