Closed Bug 515442 Opened 10 years ago Closed 10 years ago

(CSP) Implement "no inline scripts" base restriction

Categories

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

enhancement
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: geekboy, Assigned: geekboy)

References

(Blocks 1 open bug, )

Details

Attachments

(2 files, 7 obsolete files)

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

Initial patch is attached.
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.
Attachment #399534 - Attachment is obsolete: true
Added a few mochitests for the CSP inline script base restrictions.
Attachment #401113 - Attachment is obsolete: true
Fixed mochitests to play nice with other mochitests.  Misc minor fixes (mostly cosmetic).
Attachment #402603 - Attachment is obsolete: true
Attachment #403667 - Attachment description: CSP Core Modules (v7.2) → CSP Inline Script Restrictions (v7.2)
Depends on: 522457
Depends on: 530647
Includes some bug fixes and modifications based on informal, brief reviews.  (Also had pretty nasty bit rot.)
Attachment #403667 - Attachment is obsolete: true
Attachment #420201 - Flags: review?(jonas)
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 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
(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 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.
Attachment #420201 - Flags: review?(jonas) → review-
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.
Attachment #420201 - Attachment is obsolete: true
Attachment #423596 - Flags: review?
Attachment #423596 - Flags: review? → review?(jonas)
Erg, forgot to hg add the ...^headers^ file to the patch.
Attachment #423596 - Attachment is obsolete: true
Attachment #423689 - Flags: review?(jonas)
Attachment #423596 - Flags: review?(jonas)
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
Added the "click()" test, carrying over r+jonas.
Attachment #423689 - Attachment is obsolete: true
Attachment #423915 - Flags: review+
http://hg.mozilla.org/mozilla-central/rev/4cf64dc759fe

In the check-in comment I forgot to note  "patch by sstamm, r=sicking"
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
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;
>+      }
>+    }
Fixes the build bustage for inline scripts test that wasn't completing (the mouse click in the test was not being dispatched).
Depends on: 548993
(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
You need to log in before you can comment on or make changes to this bug.