515443 - (CSP) Implement "no eval" base restriction

Status: RESOLVED FIXED

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

Description

[Sid Stamm [:geekboy or :sstamm]]

Attachment: Disables 'eval' in its various flavors when CSP is set for a site

Disables "eval" in its various flavors when CSP is applied to a page (unless the policy permits eval).  "eval" disabling includes setTimeout(String,int) and setInterval(String,int).

Incomplete patch is attached (still needs to disable the 'eval' function, but setTimeout(String,int) and setInterval(String,int) are disabled).

Comment 1

[Sid Stamm [:geekboy or :sstamm]]

Bug 399588 addresses the need for a nsCxPusher and a null context.  Once that gets rolled in, we can remove the parts of this patch that use it.

Comment 2

[Sid Stamm [:geekboy or :sstamm]]

(In reply to comment #1)
> Bug 399588 addresses the need for a nsCxPusher and a null context.  Once that
> gets rolled in, we can remove the parts of this patch that use it.

Er, that should be Bug 515475.

Comment 3

[Sid Stamm [:geekboy or :sstamm]]

Attachment: CSP Eval Script Restrictions (v7)

This version (v7) disables the following eval() family of features unless the 'eval-script' option is specified in the policy:
- eval()
- new Function(String)
- setTimeout(String)
- setInterval(String)

There's a callback from JS into caps set up that is called when new Function() or eval() are called -- this callback finds the document's principal given the JS context and queries the appropriate CSP instance for 'allowsEval'.

Comment 4

[Sid Stamm [:geekboy or :sstamm]]

Attachment: CSP Eval Script Restrictions (v7.1)

Added mochitests for eval restrictions (listed in previous comment).

Comment 5

[Sid Stamm [:geekboy or :sstamm]]

Attachment: CSP Eval Script Restrictions (v7.2)

Fixed mochitests to play nice with other tests.  Misc fixes, including breakage of eval() even when CSP wasn't used (or available, as in the cases for some unit tests).

Comment 6

[Sid Stamm [:geekboy or :sstamm]]

Attachment: CSP Eval Script Restrictions (v7.4)

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

Comment 7

[Sid Stamm [:geekboy or :sstamm]]

The change to jsobj.cpp:obj_eval() returns from the function without cleaning up properly.  An exit path through the label "out" should be done instead:

--- a/js/src/jsobj.cpp	Tue Jan 05 14:10:29 2010 -0800
+++ b/js/src/jsobj.cpp	Wed Jan 06 11:23:09 2010 -0800
@@ -1452,5 +1452,6 @@ obj_eval(JSContext *cx, JSObject *obj, u
     if (!js_CheckCSPPermitsJSAction(cx)) {
         JS_ReportError(cx, "call to eval() blocked by CSP");
-        return JS_FALSE;
+        ok = JS_FALSE;
+        goto out;
     }

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

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

diff -r 746c844cd056 caps/src/nsScriptSecurityManager.cpp
@@ -510,16 +511,72 @@ NS_IMPL_ISUPPORTS5(nsScriptSecurityManag
                    nsIObserver)
 
 ///////////////////////////////////////////////////
 // Methods implementing nsIScriptSecurityManager //
 ///////////////////////////////////////////////////
 
 ///////////////// Security Checks /////////////////
 JSBool
+nsScriptSecurityManager::ContentSecurityPolicyPermitsJSAction(JSContext *cx)
+{
+    // Get the security manager
+    nsScriptSecurityManager *ssm =
+        nsScriptSecurityManager::GetScriptSecurityManager();

I think we need to make sure that we're on the main thread here. Otherwise a call to eval from a worker thread will cause problems. You should be able to use NS_IsMainThread (and if so, allow access).

+        return JS_FALSE; //no principal? NOT GOOD

Please flesh out this comment a bit or remove it.

+    ////XXX: This stack push and associated pop went away once the security check
+    //// was removed from JS (Bug 515475, rev 4448e1b581b7)
+    //nsCxPusher pusher;
+    //pusher.PushNull();

No need for this bit of history, just remove these lines.

+    //pusher.Pop(); //-removed as of rev 4448e1b581b7

This too.

diff -r 746c844cd056 dom/base/nsJSTimeoutHandler.cpp
@@ -238,16 +239,50 @@ nsJSScriptTimeoutHandler::Init(nsGlobalW
     ::JS_ReportError(cx, "useless %s call (missing quotes around argument?)",
                      *aIsInterval ? kSetIntervalStr : kSetTimeoutStr);
 
     // Return an error that nsGlobalWindow can recognize and turn into NS_OK.
     return NS_ERROR_DOM_TYPE_ERR;
   }
 
   if (expr) {
+    // if CSP is enabled, and setTimeout/setInterval was called with a string
+    // or object, disable the registration and log an error
+    nsCOMPtr<nsIDOMDocument> domdoc;
+    aWindow->GetDocument(getter_AddRefs(domdoc));
+    nsCOMPtr<nsIDocument> doc = do_QueryInterface(domdoc);
+    

Uber-nit: trailing whitespace here.

Less of a nit: GetDocument will create a document if one doesn't exist. Do you want to use nsPIDOMWindow::GetExtantDocument? Also, can we get here without a document? I guess it's worth it to be safe.

+      if(csp) {
+

Uber-nit: remove this blank line.

+        ////XXX: This stack push and associated pop went away once the security check
+        //// was removed from JS (Bug 515475, rev 4448e1b581b7)
+        //nsCxPusher pusher;
+        //pusher.PushNull();

Again, just get rid of these.

diff -r 746c844cd056 js/src/jsobj.cpp
+    if (callbacks) 
+        return callbacks->contentSecurityPolicyAllows &&
+               callbacks->contentSecurityPolicyAllows(cx);

Nit: braces around multiline consequent.

Comment 9

[Sid Stamm [:geekboy or :sstamm]]

Attachment: CSP Eval Script Restrictions (v7.6)

Thanks, mrbkap.  Attached is an updated patch with comments addressed, so I'm carrying over the r+.  In addition, it also assumes that IContentSecurityPolicy has been renamed to nsIContentSecurityPolicy (bug 515437).

> I think we need to make sure that we're on the main thread here. 
> Otherwise a call to eval from a worker thread will cause problems. 
> You should be able to use NS_IsMainThread (and if so, allow access).

We talked about this and because it's in a security callback, it should be ok (plus, NS_IsMainThread was not available from within that callback's scope for some reason or another).

Comment 10

[Sid Stamm [:geekboy or :sstamm]]

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

Brendan, could you give us a sanity check -- take a look at this and let us know if we're missing some obvious "eval()"-like feature or if we're going about this in a dumb way?

Comment 11

[Brendan Eich [:brendan]]

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

>+    if (!subjectPrincipal)
>+    {
>+        NS_WARNING("Checking for CSP on eval w/o principals; should this happen?");

Can setTimeout(eval, 0, str) provoke this for str a string to evaluate?

If it's testable don't warn (esp. with a question ;-), just fail. If it's not testable assert, because a warning won't be noticed.

>+    if (NS_FAILED(rv))
>+    {
>+      NS_WARNING("CSP: failed to get allowsEval");
>+      return JS_TRUE; // fail open to not break sites.
>+    }

Four-space c-basic-offset indentation was violated here.

>@@ -3391,17 +3441,18 @@ nsresult nsScriptSecurityManager::Init()
>     NS_ENSURE_SUCCESS(rv, rv);
> 
>     rv = runtimeService->GetRuntime(&sRuntime);
>     NS_ENSURE_SUCCESS(rv, rv);
> 
>     static JSSecurityCallbacks securityCallbacks = {
>       CheckObjectAccess,
>       NULL,
>-      NULL
>+      NULL,
>+      ContentSecurityPolicyPermitsJSAction

Here too, pre-existing code but feel free to fix if it's local to this initializer.

>diff -r 96c17502dfb2 js/src/jsapi.h
>--- a/js/src/jsapi.h	Tue Jan 05 14:10:10 2010 -0800
>+++ b/js/src/jsapi.h	Wed Jan 20 10:47:49 2010 -0800
>@@ -2034,16 +2034,17 @@ JS_DropPrincipals(JSContext *cx, JSPrinc
>      : (principals)->refcount)
> #endif
> 
> 
> struct JSSecurityCallbacks {
>   JSCheckAccessOp            checkObjectAccess;
>   JSPrincipalsTranscoder     principalsTranscoder;
>   JSObjectPrincipalsFinder   findObjectPrincipals;
>+  JSCSPEvalChecker           contentSecurityPolicyAllows;

I must have missed earlier patches, but please use c-basic-offset: 4 per the modeline in js*.{h,cpp}.

>     /* Belt-and-braces: check that the caller has access to parent. */
>     if (!js_CheckPrincipalsAccess(cx, parent, principals,
>                                   CLASS_ATOM(cx, Function))) {
>         return JS_FALSE;
>     }
> 
>+    /*
>+     * CSP check: is new Function() allowed at all?
>+     * Report errors via CSP is done in the script security manager.
>+     * js_CheckCSPPermitsJSAction is defined in jsobj.cpp
>+     */
>+    if (!js_CheckCSPPermitsJSAction(cx)) {

Since we're in JS land, the names are longer and can be even clearer: js_CheckContentSecurityPolicy would be best IMHO, but you might tweak the above to js_CheckThatCSPPermitsAction (no need for "JS" before "Action"). The run-on PP in CSPPermits makes this lose to my favored js_CheckContentSecurityPolicy, though :-P.

>+      JS_ReportError(cx, "call to new Function() blocked by CSP");
>+      return JS_FALSE;

Ditto on 4-space indentation.

Please add the message to js.msg and use the JS_ReportErrorNumber API, here and in obj_eval. I would not write "new Function()" either, since you don't know that new was used -- just "call to Function blocked by CSP". I'm assuming you guys will make the CSP TLA a household name so this error message is sensible to users, and localizable.

r=me with these tweaks, no need for review but if you feel the need, bug mrbkap ;-).

/be

Comment 12

[Sid Stamm [:geekboy or :sstamm]]

Attachment: CSP Eval Script Restrictions (v7.7)

Updated patch with minor repairs for Brendan's review.

> Can setTimeout(eval, 0, str) provoke this for str a string to evaluate?

It's blocked "eval by another name", but yes, we can test it.  I took out the warning and just fail if that happens.  I also added a unit test for this case.

> Four-space c-basic-offset indentation was violated here.

Ah, yeah, sorry, messy code.

> Since we're in JS land, the names are longer and can be even clearer:
> js_CheckContentSecurityPolicy would be best IMHO, 

Done.  Good call, this is a bit clearer.

> Please add the message to js.msg and use the JS_ReportErrorNumber API, here and
> in obj_eval. I would not write "new Function()" either, since you don't know
> that new was used -- just "call to Function blocked by CSP". I'm assuming you
> guys will make the CSP TLA a household name so this error message is sensible
> to users, and localizable.

I was unaware of js.msg... great idea.

Thanks for the review, Brendan!

Comment 13

[Sid Stamm [:geekboy or :sstamm]]

Attachment: CSP Eval Script Restrictions (v7.7)

There was an assertion condition I had reversed, should be NS_SUCCEEDED: 

--- a/caps/src/nsScriptSecurityManager.cpp	Tue Jan 26 16:09:17 2010 -0800
+++ b/caps/src/nsScriptSecurityManager.cpp	Wed Jan 27 09:15:48 2010 -0800
@@ -530,5 +530,5 @@ nsScriptSecurityManager::ContentSecurity
     nsIPrincipal* subjectPrincipal = ssm->GetSubjectPrincipal(cx, &rv);
 
-    NS_ASSERTION(NS_FAILED(rv), "CSP: Failed to get nsIPrincipal from js context");
+    NS_ASSERTION(NS_SUCCEEDED(rv), "CSP: Failed to get nsIPrincipal from js context");
     if (NS_FAILED(rv))
         return JS_FALSE; // Not just absence of principal, but failure.


Also rebased this patch, so it should have less bit rot.  When things get added to /js/src/js.msg, a slightly convoluted error comes out of make:

/js/src/jscntxt.cpp:1803: error: too many initializers for ‘JSErrorFormatString [247]’

This just means a number in js/src/js.msg is duplicated and is something to watch out for when merging for check-in.

This patch is a trivial change (NS_FAILED->NS_SUCCEEDED), so I'm carrying over the r+mrbkap,brendan.

Comment 14

[Jeff Walden [:Waldo]]

Drive-bying, perhaps for a followup patch, but these days, unless you're not completely confident that an assertion always holds, you should use NS_ABORT_IF_FALSE to perform assertions (except in SpiderMonkey, where JS_ASSERT continues to rule).  An abort is rather harder to ignore than a printed message, which makes it generally preferable until we've reduced the assertion count enough to be able to regularly dogfood debug builds with all assertions made to abort.  (Making assertions fatal has been in the works for about forever, but with the last several years of automated testing work and tinderbox improvements, it's now a very imaginable goal.)

Comment 15

[Daniel Veditz [:dveditz]]

http://hg.mozilla.org/mozilla-central/rev/9b3422abc2f8

Comment 16

[Daniel Veditz [:dveditz]]

also http://hg.mozilla.org/mozilla-central/rev/0e4edea6abc3

Comment 17

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 423805 [details] [diff] [review]
CSP Eval Script Restrictions (v7.7)

Review of attachment 423805 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/nsJSTimeoutHandler.cpp
@@ +203,5 @@
>  
>    JSAutoRequest ar(cx);
>  
>    if (argc < 1) {
> +    ::JS_ReportError(cx, "Function %s requires at least 2 parameter",

I'm three years late, but: faulty grammar!