515460 - (CSP) Enforce policies during redirects

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement, P1)

Description

[Sid Stamm [:geekboy or :sstamm]]

Any URI requested into a document with a CSP should be subject to the CSP at all stages of redirection.

For example, consider a site A with CSP P[A] contains a reference to an image at URI B.  When B is resolved, it is redirected via HTTP redirects to C and then D.
  B->C->D
All B, C and D should be subject to P[A]'s img-src directive before the requests for those URIs go out.  If any B, C or D are disallowed, that specific request (B, C or D) is aborted, breaking the redirect chain, and failing to load the resource at the end of the chain.

This redirect checking should be done for all types of resources protected by CSP.

Comment 1

[Brandon Sterne (:bsterne)]

Attachment: image-redir-v1

Comment 2

[Brandon Sterne (:bsterne)]

Attachment: script-redir-v1

Lets channel redirects for script call into content policy

Comment 3

[Brandon Sterne (:bsterne)]

Attachment: media-redir-v1

Channel redirects for video/audio to call into content policy.

Comment 4

[Brandon Sterne (:bsterne)]

Attachment: object-redir-v1

Channel redirects for objects (plugin content) to call into content policy.

Comment 5

[Brandon Sterne (:bsterne)]

Attachment: frame-redir-v1

puts policy info into channels created to load iframes.

Comment 6

[Brandon Sterne (:bsterne)]

Attachment: csp-image-redir-v2

Comment 7

[Brandon Sterne (:bsterne)]

Attachment: csp-script-redir-v2

Comment 8

[Brandon Sterne (:bsterne)]

Attachment: csp-media-redir-v2

Comment 9

[Brandon Sterne (:bsterne)]

Attachment: csp-object-redir-v2

Comment 10

[Brandon Sterne (:bsterne)]

Attachment: csp-frame-redir-v2

Correct subframe-redirect-handling patch for CSP.

Comment 11

[Brandon Sterne (:bsterne)]

I've cleaned up all the redirect handling patches I have to use the non-renamed necko API (bug 515797).  I'm using the following tests to verify these changes:
http://hackmill.com/csp/tests/test-harness.html

The last thing I need to do for this bug is to check all callers of NS_CheckContentLoadPolicy to see if there are content load types that CSP should be restricting but isn't.

Comment 12

[Brandon Sterne (:bsterne)]

(In reply to comment #11)
> see if there are content load types that CSP should be restricting but isn't.

Right, like CSSLoaderImpl::CheckLoadAllowed.  Well, there's at least one more piece needed to fix this bug...

Comment 13

[Brandon Sterne (:bsterne)]

Attachment: csp-style-redir-v1

Comment 14

[Brandon Sterne (:bsterne)]

There's a NS_CheckContentLoadPolicy check in nsExpatDriver::OpenInputStreamFromExternalDTD before we do a NS_OpenURI, which is basically a wrapper for NS_NewChannel.

Do we think it's necessary for CSP to work with XML documents and specifically requests generated by external DTD files?  If so, I'll need to add code to NS_OpenURI to take a nsIChannelPolicy to pass down to NS_NewChannel.

Comment 15

[Brandon Sterne (:bsterne)]

Attachment: csp-worker-redir-v1

Comment 16

[Brandon Sterne (:bsterne)]

Attachment: csp-font-face-redir-v1

Comment 17

[Brandon Sterne (:bsterne)]

Attachment: csp-external-resource-map-redir-v1

Comment 18

[Brandon Sterne (:bsterne)]

Attachment: csp-xhr-redir-v1

Comment 19

[Brandon Sterne (:bsterne)]

I think I've patched all the necessary places now.  I went through all the callers of NS_CheckContentLoadPolicy and made sure the redirects-to-check-with-ContentPolicy pattern is in place where appropriate.

The only call sites where I didn't add the code were in XML and XSLT documents which I don't believe we intend to work with CSP, though the pattern could easily be added there if we change our mind.

The callers of NS_CheckContentLoadPolicythat didn't get the patch are:
nsExpatDriver::OpenInputStreamFromExternalDTD
CheckPingURI (nsDocShell)
nsDocShell::InternalLoad (Content Policy checks done downstream)
txCompileObserver::loadURI
TX_LoadSheet
txSyncCompileObserver::loadURI
nsXMLContentSink::ProcessStyleLink

Comment 20

[Johnny Stenback (:jst)]

Comment on attachment 421165 [details] [diff] [review]
csp-script-redir-v2

+  rv = mDocument->NodePrincipal()->GetCsp(getter_AddRefs(csp));
+  NS_ENSURE_SUCCESS(rv, rv);

Seems like all our GetCsp() implementations unconditionally return NS_OK, maybe remove this NS_ENSURE_SUCcESS() here? If there's reason to keep it, I'm fine with doing so as well, but it seems like an unnecessary check to me.

Same applies to most (all?) other patches in this bug.

Comment 21

[Johnny Stenback (:jst)]

Comment on attachment 421166 [details] [diff] [review]
csp-media-redir-v2

   nsCOMPtr<nsHTMLMediaElement> mElement;
   PRUint32 mLoadID;
 };
 
+#include "IContentSecurityPolicy.h"
+#include "nsIChannelPolicy.h"
+#include "nsChannelPolicy.h"

Move these to the top of the file where all other #includes are.

Comment 22

[Johnny Stenback (:jst)]

Comment on attachment 421168 [details] [diff] [review]
csp-frame-redir-v2

- In nsDocShell::DoURILoad():

+    // check for Content Security Policy to pass along with the
+    // new channel we are creating
+    nsCOMPtr<nsIChannelPolicy> channelPolicy = 
+        do_CreateInstance("@mozilla.org/nschannelpolicy;1");

The other patches here seem to only create the channel policy object if there's a csp in the principal, so the do_CreateInstance() should move down inside the if (csp) check below.

+    if (IsFrame()) {
+        // check the parent docshell for a CSP
+        nsCOMPtr<IContentSecurityPolicy> csp;
+        nsCOMPtr<nsIDocShellTreeItem> parentItem;
+        GetSameTypeParent(getter_AddRefs(parentItem));
+        if (parentItem) {
+            nsCOMPtr<nsIDOMDocument> domDoc(do_GetInterface(parentItem));

do_GetInterface() is null safe, so remove the if (parentItem) check.

+            if (domDoc) {
+                nsCOMPtr<nsIDocument> doc = do_QueryInterface(domDoc);

... as is do_QueryInterface(), so remove the if (domDoc) check.

Comment 23

[Johnny Stenback (:jst)]

Comment on attachment 422414 [details] [diff] [review]
csp-font-face-redir-v1

- In nsUserFontSet::StartLoad():

+  principal->GetCsp(getter_AddRefs(csp));

The code above here that gets the principal asserts that it's not null, but if it is, it allows the load, and this change would make us crash (in a safe way! :). This code looks like it's all jdaggett's, so requesting review from him to decided one way or another here, but personally I think we should just make this code refuse to load a font if there's no principal.

Comment 24

[Johnny Stenback (:jst)]

Comment on attachment 422566 [details] [diff] [review]
csp-external-resource-map-redir-v1

+  doc->NodePrincipal()->GetCsp(getter_AddRefs(csp));

No check for whether this succeeds or not here, either take my advice to never check (from an earlier patch), or add the same check here.

Comment 25

[John Daggett (:jtd)]

(In reply to comment #16)
> Created an attachment (id=422414) [details]
> csp-font-face-redir-v1

+  principal->GetCsp(getter_AddRefs(csp));
+  NS_ENSURE_SUCCESS(rv, rv);

Hmmm, don't you need to assign the return value of GetCsp to rv?

Comment 26

[Brandon Sterne (:bsterne)]

(In reply to comment #25)
> Hmmm, don't you need to assign the return value of GetCsp to rv?

I certainly do.  Fixed.

Comment 27

[John Daggett (:jtd)]

Comment on attachment 422414 [details] [diff] [review]
csp-font-face-redir-v1

Assuming rv is correctly returned, looks good.  Looks like csp-external-resource-map-redir-v1 and csp-xhr-redir-v1 have the same issue.

Comment 28

[Brandon Sterne (:bsterne)]

Attachment: Tests for CSP redirects

Comment 29

[Brandon Sterne (:bsterne)]

script - http://hg.mozilla.org/mozilla-central/rev/179f87824c88
media - http://hg.mozilla.org/mozilla-central/rev/8a61833bbd33
object - http://hg.mozilla.org/mozilla-central/rev/1a5ca9c3093d
frame - http://hg.mozilla.org/mozilla-central/rev/58a2e617bf99
style - http://hg.mozilla.org/mozilla-central/rev/ec330fa3c73d
worker - http://hg.mozilla.org/mozilla-central/rev/f719a7e559f0
font-face - http://hg.mozilla.org/mozilla-central/rev/b7927f887bcc
xhr - http://hg.mozilla.org/mozilla-central/rev/ae6dc952378c
mochitests - http://hg.mozilla.org/mozilla-central/rev/3d3a5a5ea856

Just need to land images (needs review) and external-resource-map (which I don't have a test for) and we can close this one out.

Comment 30

[Brandon Sterne (:bsterne)]

Comment on attachment 421164 [details] [diff] [review]
csp-image-redir-v2

Stuart, you're probably the best person to review this patch.  I'm happy to provide you with background/context if needed.

Comment 31

[Stuart Parmenter]

Comment on attachment 421164 [details] [diff] [review]
csp-image-redir-v2

joe has looked at this stuff more than I have

Comment 32

[John Daggett (:jtd)]

Looks like you're missing a result value check:

 1.1 --- a/content/base/src/nsXMLHttpRequest.cpp
 1.2 +++ b/content/base/src/nsXMLHttpRequest.cpp
1.13 @@ -1751,8 +1754,22 @@ nsXMLHttpRequest::OpenRequest(const nsAC
1.14} else {
1.15  loadFlags = nsIRequest::LOAD_BACKGROUND;
1.16}
1.17 -  rv = NS_NewChannel(getter_AddRefs(mChannel), uri, nsnull, loadGroup, nsnull,
1.18 - loadFlags);
1.19 +  // get Content Security Policy from principal to pass into channel
1.20 +  nsCOMPtr<nsIChannelPolicy> channelPolicy;
1.21 +  nsCOMPtr<nsIContentSecurityPolicy> csp;
1.22 +  mPrincipal->GetCsp(getter_AddRefs(csp));
1.23 +  if (csp) {
1.24 +channelPolicy = do_CreateInstance("@mozilla.org/nschannelpolicy;1");
1.25 +channelPolicy->SetContentSecurityPolicy(csp);
1.26 +channelPolicy->SetLoadType(nsIContentPolicy::TYPE_XMLHTTPREQUEST);
1.27 +  }

I think you want:

rv = mPrincipal->GetCsp(getter_AddRefs(csp));

Comment 33

[Brandon Sterne (:bsterne)]

Attachment: Fixes comment 32

Missed a return value check.

Comment 34

[Brandon Sterne (:bsterne)]

XHR fix:
http://hg.mozilla.org/mozilla-central/rev/2348aecc2c0f

Comment 35

[Brandon Sterne (:bsterne)]

Joe, any update on the review?  We really need to get this last piece landed?  Thanks.

Comment 36

[Joe Drew (not getting mail)]

Don't know how I missed that comment yesterday! I plan on getting to this review early next week.

Comment 37

[Joe Drew (not getting mail)]

Comment on attachment 421164 [details] [diff] [review]
csp-image-redir-v2

>diff --git a/content/base/src/nsContentUtils.cpp b/content/base/src/nsContentUtils.cpp

>@@ -2452,16 +2455,30 @@ nsContentUtils::LoadImage(nsIURI* aURI, 

>   // We don't use aLoadingPrincipal for anything here yet... but we
>   // will.  See bug 377092.

This comment can probably go now :)

>diff --git a/modules/libpr0n/public/imgILoader.idl b/modules/libpr0n/public/imgILoader.idl

Need to rev imgILoader uuid.

>diff --git a/modules/libpr0n/src/imgLoader.cpp b/modules/libpr0n/src/imgLoader.cpp

> static nsresult NewImageChannel(nsIChannel **aResult,
>                                 nsIURI *aURI,
>                                 nsIURI *aInitialDocumentURI,
>                                 nsIURI *aReferringURI,
>                                 nsILoadGroup *aLoadGroup,
>                                 const nsCString& aAcceptHeader,
>-                                nsLoadFlags aLoadFlags)
>+                                nsLoadFlags aLoadFlags,
>+                                nsIChannelPolicy *channelPolicy)

As much as I dislike it, aPolicy is the standard naming for function parameter names. You'll need to sweep through this file to make sure you get it on all the functions you've added an nsIChannelPolicy parameter to in this file.

>diff --git a/toolkit/system/gnome/nsAlertsIconListener.cpp b/toolkit/system/gnome/nsAlertsIconListener.cpp

>@@ -262,19 +262,21 @@ nsAlertsIconListener::StartRequest(const

>+  // XXXbsterne currently passing in null for channel policy
>+  // I don't have a testcase that triggers this code
>   return il->LoadImage(imageUri, nsnull, nsnull, nsnull, this,
>                        nsnull, nsIRequest::LOAD_NORMAL, nsnull, nsnull,
>-                       getter_AddRefs(mIconRequest));
>+                       nsnull, getter_AddRefs(mIconRequest));

Did you end up writing such a testcase?

>diff --git a/widget/src/cocoa/nsMenuItemIconX.mm b/widget/src/cocoa/nsMenuItemIconX.mm

>@@ -336,19 +355,23 @@ nsMenuItemIconX::LoadIcon(nsIURI* aIconU

>+  // XXXbsterne I'm not sure passing in channelPolicy is actually necessary
>+  // here.  I can trigger this code, for example, by clicking on menu items
>+  // but nsMenuItemIconX::LoadIcon always seems to be called with either
>+  // chrome:// or moz-anno:favicon URIs.
>   rv = loader->LoadImage(aIconURI, nsnull, nsnull, loadGroup, this,
>-                         nsnull, nsIRequest::LOAD_NORMAL, nsnull,
>-                         nsnull, getter_AddRefs(mIconRequest));
>+                         nsnull, nsIRequest::LOAD_NORMAL, nsnull, nsnull,
>+                         channelPolicy, getter_AddRefs(mIconRequest));

You should probably decide one way or the other, and either remove the comment and pass the channelPolicy, or modify the comment slightly (remove the XXXbsterne for one) and pass nsnull.

Comment 38

[Brandon Sterne (:bsterne)]

http://hg.mozilla.org/mozilla-central/rev/30443a0a2394