515442 - (CSP) Implement "no inline scripts" base restriction

Status: RESOLVED FIXED

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

Description

[Sid Stamm [:geekboy or :sstamm]]

Attachment: Disables inline scripts when CSP is set for a site

Disables inline scripts when CSP is applied to a page (unless the policy permits inline scripts). 

Initial patch is attached.

Comment 1

[Sid Stamm [:geekboy or :sstamm]]

Attachment: CSP Inline Script Restrictions (v7)

Update to the inline script restrictions for CSP (v7).  Updates in this patch:
- CSP is found in the document's principal now (not in nsDocument)
- 'inline' prohibition is not cached, and is checked for every inline script so that the appropriate number of violation reports can be generated.

Comment 2

[Sid Stamm [:geekboy or :sstamm]]

Attachment: CSP Inline Script Restrictions (v7.1)

Added a few mochitests for the CSP inline script base restrictions.

Comment 3

[Sid Stamm [:geekboy or :sstamm]]

Attachment: CSP Inline Script Restrictions (v7.2)

Fixed mochitests to play nice with other mochitests.  Misc minor fixes (mostly cosmetic).

Comment 4

[Sid Stamm [:geekboy or :sstamm]]

Attachment: CSP Inline Script Restrictions (v7.4)

Includes some bug fixes and modifications based on informal, brief reviews.  (Also had pretty nasty bit rot.)

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 420201 [details] [diff] [review]
CSP Inline Script Restrictions (v7.4)

>diff -r 4533858b6fff content/base/src/nsScriptLoader.cpp
>@@ -540,16 +552,42 @@ nsScriptLoader::ProcessScriptElement(nsI
>         nsContentUtils::AddScriptRunner(new nsRunnableMethod<nsScriptLoader>(this,
>           &nsScriptLoader::ProcessPendingRequests));
>       }
> 
>       return request->mDefer || aElement->GetScriptAsync() ?
>         NS_OK : NS_ERROR_HTMLPARSER_BLOCK;
>     }
>   }
>+  else {
>+    // in-line script
>+    nsCOMPtr<IContentSecurityPolicy> csp;
>+    nsresult rv = mDocument->NodePrincipal()->GetCsp(getter_AddRefs(csp));
>+    NS_ENSURE_SUCCESS(rv, rv);
>+
>+    if (csp) {
>+#ifdef PR_LOGGING
>+      PR_LOG(gCspPRLog, PR_LOG_DEBUG, ("ScriptLoader's document has CSP"));
>+#endif
>+      PRBool inlineOK = PR_TRUE;
>+      // this call will send violation reports when necessary
>+      rv = csp->GetAllowsInlineScript(&inlineOK);
>+      NS_ENSURE_SUCCESS(rv, rv);
>+
>+      if (!inlineOK) {
>+#ifdef PR_LOGGING
>+        PR_LOG(gCspPRLog, PR_LOG_DEBUG, (" CSP blocked inline scripts (1)"));
>+#endif
>+        return NS_ERROR_FAILURE;
>+      }
>+    }
>+#ifdef PR_LOGGING
>+    else PR_LOG(gCspPRLog, PR_LOG_DEBUG, ("ScriptLoader's document NO CSP"));
>+#endif
>+  }

This block of code isn't needed. You'll catch inline scripts with your changes below.

>@@ -558,16 +596,44 @@ nsScriptLoader::ProcessScriptElement(nsI
>     request->mIsInline = PR_FALSE;
>     request->mLoading = PR_TRUE;
> 
>     rv = StartLoad(request, type);
>     if (NS_FAILED(rv)) {
>       return rv;
>     }
>   } else {
>+    // in-line script
>+    nsCOMPtr<IContentSecurityPolicy> csp;
>+    nsresult rv = mDocument->NodePrincipal()->GetCsp(getter_AddRefs(csp));
>+
>+    // if NodePrincipal is a null principal, GetCsp will complain that CSP is
>+    // not available (it doesn't make sense for a null principal).  So we have
>+    // to continue, and csp will be empty when it's checked in a couple lines.
>+    if (rv == NS_ERROR_NOT_AVAILABLE)
>+      rv = NS_OK; 
>+    NS_ENSURE_SUCCESS(rv, rv);

Do you mean "system principal" instead of "null principal" above?

In any case, it seems cleaner to make the relevant principals return NS_OK, but set |csp| to null. Using generic error codes like the above for errors that you want to handle is always fragile.

>+    if (csp) {
>+#ifdef PR_LOGGING
>+      PR_LOG(gCspPRLog, PR_LOG_DEBUG, ("New ScriptLoader i ****with CSP****"));
>+#endif

No need to use the #ifdef/#endif. PR_LOG will expand to nothing if logging is disabled. Applies in a lot of other places.


Still reviewing...

Comment 6

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 420201 [details] [diff] [review]
CSP Inline Script Restrictions (v7.4)

>diff -r 4533858b6fff content/base/test/file_CSP_inlinescript.sjs
>--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
>+++ b/content/base/test/file_CSP_inlinescript.sjs	Tue Jan 05 14:10:25 2010 -0800
>@@ -0,0 +1,30 @@
>+// SJS file for CSP inline script mochitests
>+
>+function handleRequest(request, response)
>+{
>+  var query = {};
>+  request.queryString.split('&').forEach(function (val) {
>+    var [name, value] = val.split('=');
>+    query[name] = unescape(value);
>+  });
>+
>+  //avoid confusing cache behaviors
>+  response.setHeader("Cache-Control", "no-cache", false);
>+
>+  if ("main" in query) {
>+    var xhr = Components.classes["@mozilla.org/xmlextras/xmlhttprequest;1"]  
>+                   .createInstance(Components.interfaces.nsIXMLHttpRequest);
>+    //serve the main page with a CSP header!
>+    // -- anything served from 'self' (localhost:8888) will be allowed,
>+    // -- anything served from other hosts (example.com:80) will be blocked.
>+    // -- XHR tests are set up in the file_CSP_main.js file which is sourced.
>+    response.setHeader("X-Content-Security-Policy", 
>+                       "allow 'self'",
>+                       false);
>+    xhr.open("GET", "http://localhost:8888/tests/content/base/test/file_CSP_inlinescript_main.html", false);
>+    xhr.send(null);
>+    if(xhr.status == 200) {
>+      response.write(xhr.responseText);
>+    }
>+  }
>+}

Just use a ^headers^ file.

https://developer.mozilla.org/En/Mochitest#How_do_I_change_the_HTTP_headers_or_status_sent_with_a_file_used_in_a_Mochitest.3f

Comment 7

[Jeff Walden [:Waldo]]

(In reply to comment #6)
> >+    xhr.open("GET", "http://localhost:8888/tests/content/base/test/file_CSP_inlinescript_main.html", false);
> >+    xhr.send(null);
> >+    if(xhr.status == 200) {
> >+      response.write(xhr.responseText);
> >+    }

I'm slightly surprised this even works.  The server is not designed to be reentrant except when you process the response asynchronously (see response.processAsync()), yet you expect the XHR to occur synchronously and to reenter the server; you're playing with fire doing this.  I've never attempted to make any sort of interaction like this have reliable semantics, and I'd probably WONTFIX bugs asking for it, given the presence of a fairly simple, supported workaround (below).

More broadly, synchronous XHR is always a bad idea -- it freezes the browser UI for some length of time (in this case maybe you can not care, but in general someone *will* get bitten *sometime*), and it pounds on the rest of the browser in bad ways, requiring hacks in numerous places to (only semi-?) preserve JS's run-to-completion semantics.  Use async XHR and continue with callbacks; it may be more complicated, but it's much more reliable.

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 420201 [details] [diff] [review]
CSP Inline Script Restrictions (v7.4)

>diff -r 4533858b6fff docshell/base/nsDocShell.cpp
>--- a/docshell/base/nsDocShell.cpp	Tue Jan 05 13:53:50 2010 -0800
>+++ b/docshell/base/nsDocShell.cpp	Tue Jan 05 14:10:25 2010 -0800
>@@ -204,16 +204,18 @@
> 
> #if NS_PRINT_PREVIEW
> #include "nsIDocumentViewerPrint.h"
> #include "nsIWebBrowserPrint.h"
> #endif
> 
> #include "nsPluginError.h"
> 
>+#include "IContentSecurityPolicy.h"
>+
> static NS_DEFINE_CID(kDOMScriptObjectFactoryCID,
>                      NS_DOM_SCRIPT_OBJECT_FACTORY_CID);
> static NS_DEFINE_CID(kAppShellCID, NS_APPSHELL_CID);
> 
> #if defined(DEBUG_bryner) || defined(DEBUG_chb)
> //#define DEBUG_DOCSHELL_FOCUS
> #define DEBUG_PAGE_CACHE
> #endif

Why this change?

looks good otherwise, but would still like to see a new patch. So r- for now.

Comment 9

[Sid Stamm [:geekboy or :sstamm]]

Attachment: CSP Inline Script Restrictions (v7.6)

Thanks jonas, waldo for the review comments.  I don't know why I went so crazy to do the XHR for that test... I've stopped that nonsense.  I should probably check to see if any other CSP tests use it too.

Jonas: I addressed your comments and the new patch is attached.  Let me know what you think, we're hoping to land this soon.

Comment 10

[Sid Stamm [:geekboy or :sstamm]]

Attachment: CSP Inline Script Restrictions (v7.6)

Erg, forgot to hg add the ...^headers^ file to the patch.

Comment 11

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 423689 [details] [diff] [review]
CSP Inline Script Restrictions (v7.6)

Looks good. Though it would be good to add a test that checks that

<a href="javascript:...">.click()

doesn't execute the script-uri

Comment 12

[Sid Stamm [:geekboy or :sstamm]]

Attachment: CSP Inline Script Restrictions (v7.6)

Added the "click()" test, carrying over r+jonas.

Comment 13

[Daniel Veditz [:dveditz]]

http://hg.mozilla.org/mozilla-central/rev/4cf64dc759fe

In the check-in comment I forgot to note  "patch by sstamm, r=sicking"

Comment 14

[Daniel Veditz [:dveditz]]

forgot to hg add the tests:
http://hg.mozilla.org/mozilla-central/rev/dd14908d0b40

Comment 15

[Nochum Sossonko [:Natch]]

Comment on attachment 423915 [details] [diff] [review]
 CSP Inline Script Restrictions (v7.6)

>diff -r 5bbb7413f4ac content/base/src/nsScriptLoader.cpp
>--- a/content/base/src/nsScriptLoader.cpp	Tue Jan 26 15:22:22 2010 -0800
>+++ b/content/base/src/nsScriptLoader.cpp	Wed Jan 27 18:16:32 2010 -0800
>@@ -65,16 +65,23 @@
> #include "nsUnicharUtils.h"
> #include "nsAutoPtr.h"
> #include "nsIXPConnect.h"
> #include "nsContentErrors.h"
> #include "nsIParser.h"
> #include "nsThreadUtils.h"
> #include "nsIChannelClassifier.h"
> #include "nsDocShellCID.h"
>+#include "nsIContentSecurityPolicy.h"
>+#include "prlog.h"
>+
>+#ifdef PR_LOGGING
>+static PRLogModuleInfo* gCspPRLog;
>+#endif
>+

[...snip...]

>+      rv = NS_SUCCEEDED(csp->GetAllowsInlineScript(&inlineOK));

That seems pretty strange, and I don't think it works (as in the correct error won't be propagated)...

>+      NS_ENSURE_SUCCESS(rv, rv);
>+
>+      if (!inlineOK) {
>+        PR_LOG(gCspPRLog, PR_LOG_DEBUG, ("CSP blocked inline scripts (2)"));
>+        return NS_ERROR_FAILURE;
>+      }
>+    }

Comment 16

[Sid Stamm [:geekboy or :sstamm]]

Attachment: build-bustage test hang fix

Fixes the build bustage for inline scripts test that wasn't completing (the mouse click in the test was not being dispatched).

Comment 17

[Daniel Veditz [:dveditz]]

Comment on attachment 428565 [details] [diff] [review]
build-bustage test hang fix

r=dveditz

http://hg.mozilla.org/mozilla-central/rev/58c21f1a4074

Comment 18

[Daniel Veditz [:dveditz]]

(In reply to comment #15)
> >+      rv = NS_SUCCEEDED(csp->GetAllowsInlineScript(&inlineOK));
> 
> That seems pretty strange, and I don't think it works (as in the correct error
> won't be propagated)...

Filed bug 548993