515437 - (CSP) Implement connections between loads and policies

Status: RESOLVED FIXED

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

Description

[Sid Stamm [:geekboy or :sstamm]]

Attachment: Connects CSP core to document and subordinate loads

In splitting CSP into manageable pieces, this connects the CSP core to document loading and resource loading.  It includes the Content Policy service and hooks into nsDocument to attach a policy to documents served with the CSP header specified.

Initial patch is attached.

Comment 1

[Sid Stamm [:geekboy or :sstamm]]

Attachment: CSP Connections (v7)

Modules and hooks to enforce CSP content policies on documents.  Updates in this version (v7):
- CSP component instance is stored in the document's principal (instead of in nsDocument) so it can be accessed by caps.
- added some additional error checking

Comment 2

[Sid Stamm [:geekboy or :sstamm]]

Attachment: CSP Connections (v7.1)

This update to v7 adds the following features to the connections patch:
- a new 'csp-on-violate-policy' topic for the nsObserver service: nsIObservers can watch for this topic that is notified when CSP blocks something.  The csp component notifies on this topic when violations occur, and reports are going to be sent (including in report-only mode).
- mochitests for the content restrictions part of CSP (policy and policy directive violation stuff).  Run them with "TEST_PATH=content/base/test/test_CSP.html make -C $(obj-dir) mochitest-plain"

Comment 3

[Sid Stamm [:geekboy or :sstamm]]

Ack, I made some changes to "CSPUtils.jsm" and "contentSecurityPolicy.js" that should probably get pushed down into the core patch, not this one.  Working on fixing the patches for that.

Comment 4

[Sid Stamm [:geekboy or :sstamm]]

Attachment: CSP Connections (v7.1) - fixed

moved modifications of CSPUtils.jsm into the core patch (bug 515433), where they belong.

Comment 5

[Sid Stamm [:geekboy or :sstamm]]

Attachment: CSP Connections (v7.2)

Misc. bug fixes... also no longer register the service in the xpcom-startup category (not needed).  Fixed mochitests so they play nice with other mochitests.

Comment 6

[Sid Stamm [:geekboy or :sstamm]]

We should probably also make sure that plug-ins can't send JS to a protected page unless the plugin is allowed by the script-src directive.

Site designers can then create a site that disallows plug-in content (like swfs) from scripting the page, yet still allows javascripts served by the author.  This would be done by serving object data from a different host than scripts, and setting up a CSP such as: 

  allow 'none'; script-src scripts.self.com; object-src objects.self.com

Current connections in the patch v7.2 don't seem to block scripts compiled due to requests from plug-ins.

Comment 7

[Sid Stamm [:geekboy or :sstamm]]

Attachment: CSP Connections (v7.3)

Fixed some string leaks and deleted some unnecessary code.

Comment 8

[Johnny Stenback (:jst)]

Comment on attachment 407905 [details] [diff] [review]
 CSP Connections (v7.3)

- In nsPrincipal::GetCsp():

+  if (mCSP) {
+    NS_IF_ADDREF(*aCsp = mCSP);
+  }
+  else {
+    *aCsp = nsnull;
+  }

No need for the if check here, just use

+    NS_IF_ADDREF(*aCsp = mCSP);

- In nsPrincipal::SetCsp():

+  mCSP = aCsp;
+  return NS_OK;

Is there ever a valid reason to set csp to null? Would we want to protect against that in this setter and throw?

- In content/base/src/nsCSPService.cpp:

+//#include "nsIServiceManager.h"
+//#include "nsMemory.h"
+//#include "nsICategoryManager.h"

Remove these?

- In CSPService::CSPService():

+  gCSPEnabled = nsContentUtils::GetBoolPref("security.csp.enable", PR_TRUE);
+  nsContentUtils::AddBoolPrefVarCache("security.csp.enable", &gCSPEnabled);

The first of those two lines is not needed, AddBoolPrefVarCache() gets the value foryou, but with a different default value.

- In CSPService::ShouldLoad():

+#ifdef PR_LOGGING 
+    {
+        nsCAutoString location("None");

No need to initialize location to None here, just leave it empty.

+        principal = doc->NodePrincipal();
+        NS_ASSERTION(principal, "Document didn't have a principal.");

No need for that assertion, we'll crash on the next line if we get null back from NodePrincipal(), which never returns null.


+#ifdef PR_LOGGING
+    else {
+        nsCAutoString uriSpec("None");

Same here, no need to give uriSpec a value.

- In CSPService::ShouldProcess():

+        principal = doc->NodePrincipal();
+        rv = principal->GetCsp(getter_AddRefs(csp));
+        //TODO: check principal and rv
+        //IContentSecurityPolicy* csp = doc->GetContentSecurityPolicy();

Remove the TODO, no need to check principal, and rv doesn't even need to be captured here IMO since you null check csp below.

+        // obtain the enforcement decision
+        csp->ShouldProcess(aContentType,
+                            aContentLocation,
+                            aRequestOrigin,
+                            aRequestContext,
+                            aMimeTypeGuess,
+                            aExtra,
+                            aDecision);

Fix the indentation of all but the first argument.

- In content/base/src/nsCSPService.h:

+//#include "nsIGenericFactory.h"

Remove.

- In nsDocument::StartDocumentLoad():

+// --------------  Initialize CSP
+...

This new code block adds a bunch of early returns which makes it hard to notice that code added after this to the end of the function may not be called. I'd recommend you add a new InitializeCSP() function
that returns an nsresult, and only return early here if InitializeCSP() returns an error.

+  cspEnabled = nsContentUtils::GetBoolPref("security.csp.enable", PR_FALSE);

And maybe add an observer for this pref like you do in nsCSPService so we don't need to ask the pref service on every load?

+    nsCOMPtr<nsIAtom> cspHeaderAtom = do_GetAtom("x-content-security-policy");
+    nsCOMPtr<nsIAtom> cspROHeaderAtom = do_GetAtom("x-content-security-policy-report-only");

You should add these atoms to nsGkAtoms so you don't need to do a re-lookup for every load.

+    PRBool system;
+    nsIScriptSecurityManager *ssm = nsContentUtils::GetSecurityManager();

Initialize system to PR_FALSE, just in case.

+        //only makes sense to register new CSP if this document is not priviliged

Add a space after '//' (here, and elsewhere unless existing style conflicts)

+    if (cspHeaderValue.IsEmpty()) {
+        mCSP->SetReportOnlyMode(true);
...

This file uses 2-space indentation, so fix that in all the changes to this file.

- In nsDocument::Destroy():

-
+	

Don't add this whitespace.

This looks really good over all, but I'd like to look it over once more once the above is dealt with, so r- until then.

Comment 9

[Sid Stamm [:geekboy or :sstamm]]

(In reply to comment #8)
> Is there ever a valid reason to set csp to null? Would we want to protect
> against that in this setter and throw?

Hm, originally, we either set the CSP to a valid object or to null (before we put it in the principal) since the object containing the CSP instance might have been reused.  I think in this case, we don't want the CSP value to be explicitly set to null, though if it is null as default it's ok.  A null CSP means that the document is not using CSP.

If there's ever a case where a principal is reused across documents, we need to support setting it to null.  Potentially, there could be a case in the future when we want to kill a CSP during the lifetime of the principal (setting it to null).  What do you think, jst?

> - In nsDocument::StartDocumentLoad():
>
> This new code block adds a bunch of early returns which makes it hard to notice
> that code added after this to the end of the function may not be called. I'd
> recommend you add a new InitializeCSP() function
> that returns an nsresult, and only return early here if InitializeCSP() returns
> an error.

Great idea.

Comment 10

[Sid Stamm [:geekboy or :sstamm]]

Attachment: CSP Connections (v7.4)

This patch is the same as the previous version, with review comments addressed.

Comment 11

[Brandon Sterne (:bsterne)]

:jst and :geekboy,

note that nsDocument::InitCSP needs a |return NS_OK;| at the end (now that bug 529441 is fixed).

Comment 12

[Johnny Stenback (:jst)]

Comment on attachment 418866 [details] [diff] [review]
CSP Connections (v7.4)

- In nsPrincipal::SetCsp(IContentSecurityPolicy* aCsp):

+{
+  mCSP = aCsp;
+  return NS_OK;

Given your comments about reusing principals etc, the answer is no, we don't ever do that AFAIK. And I'd be happy to let code unset the csp, but I think we should throw an error is mCSP is set, and SetCsp() is called with a non-null csp. If down the road we come up with a case where that's too restrictive, we can change this code to permit that once a well understood case for it shows up, but until then, I'd prefer to not permit more than we need to here.

- In CSPService::ShouldProcess():

+        nsCAutoString uriSpec("None");

Loose the "None" argument here as well.

- In nsDocument::InitCSP():

Yes, as Brandon pointed out, you need a return at the end or this method will return an undefined return value, even if the compiler happens to allow this.

r+sr=jst with that.

Comment 13

[Sid Stamm [:geekboy or :sstamm]]

Attachment: CSP Connections (v7.5)

Tweaks as requested by jst.  Carried over r+sr flags, and ready to land.

Comment 14

[Boris Zbarsky [:bzbarsky]]

This looks like it caused a 3.5% pageload time hit on tinderbox.  I'm not sure why, since InitCSP should always be bailing out early, right?  And therefore the CSP service shouldn't actually be calling out to the JS code, if I read it right...

A few other questions:
1)  Why does scanRequestData assume aChannel is non-null?  As it happens, the
    caller can quite easily pass null.
2)  Is IContentSecurityPolicy supposed to be in the mozilla namespace?
3)  In CSPService::ShouldLoad, why not just QI to nsINode instead of separately
    to nsIContent and nsIDocument?  Same for ShouldProcess.

Comment 15

[Sid Stamm [:geekboy or :sstamm]]

(In reply to comment #14)
> This looks like it caused a 3.5% pageload time hit on tinderbox.  I'm not sure
> why, since InitCSP should always be bailing out early, right?  And therefore
> the CSP service shouldn't actually be calling out to the JS code, if I read it
> right...

That is what was intended.  I'm not sure why the pageload hit time... would the extra QIs done to look for a CSP on the principal for each item load slow it down that much?

> 1)  Why does scanRequestData assume aChannel is non-null?  As it happens, the
>     caller can quite easily pass null.

We assumed we're the only user of scanRequestData (it's there so we can store a bit of information about the document being protected).  I suppose it wouldn't hurt at all to add a null check.

> 2)  Is IContentSecurityPolicy supposed to be in the mozilla namespace?

I think so... there was a discussion about naming it a long time ago (mozI? nsI?) and just went with I for some reason.  What are your thoughts?

> 3)  In CSPService::ShouldLoad, why not just QI to nsINode instead of separately
>     to nsIContent and nsIDocument?  Same for ShouldProcess.

Initially we were storing the CSP on the nsDocument so we had to actually find the top-level document.   I suppose now that it is on a principal, we can just check the node's principal, right?  Would the nsIPrincipal instance be the same for the document and all of its subordinate nodes?

Comment 16

[Boris Zbarsky [:bzbarsky]]

> would the extra QIs done to look for a CSP on the principal for each item load
> slow it down that much?

I would think not.

> We assumed we're the only user of scanRequestData

You are, but the caller in this patch happily passes null.  Just load an HTML page from a file:// URI, say.

> What are your thoughts?

It should either be nsIContentSecurityPolicy or mozilla::IContentSecurityPolicy, but not the bare IContentSecurityPolicy it is right now.

> Would the nsIPrincipal instance be the same for the document and all of its
> subordinate nodes?

Yes.

Comment 17

[Reed Loden [:reed]]

If the cause of the regression isn't immediately known, can we back this out for now?

Comment 18

[Henri Sivonen (:hsivonen) (temporarily away from Bugzilla)]

A test case added by this patch failed with html5.enable=true (bug 539686).

Comment 19

[Sid Stamm [:geekboy or :sstamm]]

Attachment: CSP Connections (v7.6)

Addressed bz's comments in comment 14, except for moving IContentSecurityPolicy to nsIContentSecurityPolicy, which was done in bug 515433 (core patch v7.6).

1. Inserted a null check for aChannel in scanRequestData()
2. Rewrote the way the CSPService obtains the nsIPrincipal to only need
one QI instead of 2.
3. Changed all references from IContentSecurityPolicy to nsIContentSecurityPolicy to be compatible with the updates in the core patch.

Carrying over sr+ and adding r?dveditz for review of these really minor changes.

Comment 20

[Daniel Veditz [:dveditz]]

Comment on attachment 422648 [details] [diff] [review]
 CSP Connections (v7.6)

r=dveditz on the incremental changes

Comment 21

[Daniel Veditz [:dveditz]]

Comment on attachment 422648 [details] [diff] [review]
 CSP Connections (v7.6)

r=dveditz on the incremental changes

Comment 22

[Daniel Veditz [:dveditz]]

http://hg.mozilla.org/mozilla-central/rev/7229621a1886

Hopefully it will stick this time.

Comment 23

[Rain [:rain1] (inactive)]

I think this is causing xpcshell tests to crash with debug builds (bug 541653).

Comment 24

[Rain [:rain1] (inactive)]

(In reply to comment #23)
> I think this is causing xpcshell tests to crash with debug builds (bug 541653).

sorry, looks like a build issue, another clobber fixed it. Apologies for the spam.

Comment 25

[Daniel Holbert [:dholbert]]

FWIW, I just pushed a changeset to remove two unused 'nsresult rv' variables in nsCSPService.cpp: http://hg.mozilla.org/mozilla-central/rev/78d11f8b9cae

Comment 26

[Boris Zbarsky [:bzbarsky]]

The pref stuff this added to nsDocument.cpp leaks.  See bug 577953.