Brandon, seems this should block. Assigning to you for now, but please feel free to reassign as necessary...

I thought Workers should have worked this way already. Do they not inherit the principal of the page that created them? On second though, I could see them having null principals to keep them sandboxed from everything else. Either way, the CSP object sits on the principal so I'll need to figure out a way to find that from the Worker code.

I found this by happening to look at the JSSecurityCallbacks implementations and noting this one flat-out omits the field for eval-checking (struct auto-trailing-member-filling for the loss, I guess): http://mxr.mozilla.org/mozilla-central/source/dom/src/threads/nsDOMThreadService.cpp#1015 It's possible, then, that you might only need to implement that one method, and that workers already get the right principals to make the check.

Spoke to bent and dveditz and while there is a clear way forward to fix this, it is not a Firefox 4 blocker as it will be very difficult to exploit. bent said he will take this. For the record, the plan is to inform the new Worker when it is created whether or not eval is permitted, then the worker security manager can check this value each time eval is called on the worker, and finally the worker security manager can dispatch an event (or similar) back to the main thread to send a report to the site (if reporting is enabled).

I don't expect this to be a 4.0 blocker at this point, but it should block the next one.

Why was blocking2.0 set? Please renominate if you really meant it.

This affects the CSP policy we were going to use in basecamp for privileged/certified apps. We should also make sure that |new Worker("data:...")| doesn't work. Same for |workerglobal.importScripts("data:...")|

Kyle, Andrea, interested?

Andrea, can you work on this one? This is a basecamp (aka b2g) blocker.

> We should also make sure that |new Worker("data:...")| doesn't work. Same > for |workerglobal.importScripts("data:...")| The specs say that this should be allowed: Thus, scripts must either be external files with the same scheme, host, and port as the original page, or data: URLs. For example, you can't load a script from a javascript: URL, and an https: page couldn't start workers using scripts with http: URLs.

I think the key here is in what the CSP 1.0 spec says for pages with workers: https://dvcs.w3.org/hg/content-security-policy/raw-file/5b353a8ac072/csp-1.0-specification.html#processing-model Down near the bottom of that section: "If the user agent is enforcing a CSP policy for the owner document, the user agent must enforce the CSP policy for the worker." This means workers can't use data: URIs if the document cannot due to a CSP. It doesn't have anything to do with the use of eval, but is part of making our workers CSP compliant.

Andrea: CSP is a feature which allows a webpage or webserver to limit which feature set is available to web pages. It allows a website to turn off features of HTML which makes it easier to attack the website. So for example a website can say that it won't ever contain inline scripts, which means that even if a hacker is able to launch an XSS attack against a page which inserts an inline script, that script will be ignored and the website remains safe. You can read more about it here https://developer.mozilla.org/en-US/docs/Security/CSP

Comment on attachment 656951 [details] [diff] [review] patch 1 I'll take this review.

If we can, this patch will also need to report violations of the policy (eval blocked) to an error console and also to any report URIs defined in the CSP header. Example: http://mxr.mozilla.org/mozilla-central/source/caps/src/nsScriptSecurityManager.cpp#522

Somehow, something already reports the violations to the error console. I have not investigate it, but the error console contains a message with filename + line + error: Eval: Timestamp: 08/30/2012 10:08:18 PM Error: call to eval() blocked by CSP Source File: http://mochi.test:8888/tests/dom/workers/test/csp_worker.js Line: 7 javascript: Timestamp: 08/30/2012 10:08:18 PM Error: Failed to load script: javascript:dump(123); (nsresult = 0x805e0006) Source File: http://mochi.test:8888/tests/dom/workers/test/csp_worker.js Line: 14 data: Timestamp: 08/30/2012 10:08:18 PM Error: Failed to load script: data:application/javascript;base64,ZHVtcCgnaGVsbG8gd29ybGQnKQo= (nsresult = 0x805e0006) Source File: http://mochi.test:8888/tests/dom/workers/test/csp_worker.js Line: 12

Looks like the call to eval is already blocked by existing CSP. Are the other two messages due to your patch, Andrea?

There are not 2 messages. And I disable my security check, eval is not blocked. I'll investigate better why we see that message in the error console.

https://mxr.mozilla.org/mozilla-central/source/js/src/builtin/Eval.cpp#164 The exception is thrown by the JS engine because of that security check implemented by this patch. The other message is coming from the worker scriptloader object: https://mxr.mozilla.org/mozilla-central/source/dom/workers/ScriptLoader.cpp#610 No message is due to this patch.

https://tbpl.mozilla.org/?tree=Try&rev=1e4dec96f98d green on try.

Comment on attachment 656951 [details] [diff] [review] patch 1 Review of attachment 656951 [details] [diff] [review]: ----------------------------------------------------------------- This code needs to alert the CSP system (and web developers) when eval is called on their site or in a worker referenced by their site. This should be done with nsIContentSecurityPolicy::LogViolationDetails(). Example: http://mxr.mozilla.org/mozilla-central/source/caps/src/nsScriptSecurityManager.cpp#508

LogViolationDetails() called. Waiting for green on try.

Comment on attachment 658491 [details] [diff] [review] patch 2 Review of attachment 658491 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me. I'm not a peer on content, so will f=me instead of r=me, but the CSP tests and calls into CSP look good.

Comment on attachment 658491 [details] [diff] [review] patch 2 Review of attachment 658491 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/workers/RuntimeService.cpp @@ +17,5 @@ > #include "nsIScriptSecurityManager.h" > #include "nsISupportsPriority.h" > #include "nsITimer.h" > #include "nsPIDOMWindow.h" > +#include "nsIContentSecurityPolicy.h" Nit: Please alphabetize all the headers you add. In general I have the first block reserved for interfaces, the second for miscellaneous stuff, and the third block for worker headers. @@ +251,5 @@ > +class LogViolationDetailsRunnable : public nsRunnable > +{ > + nsCOMPtr<nsIContentSecurityPolicy> mCSP; > + nsString mFileName; > + unsigned mLineNum; This needs an explicitly-sized type (otherwise it might be too big or too small on different platforms). LogViolationDetails in particular takes 'int32_t'. @@ +255,5 @@ > + unsigned mLineNum; > + > +public: > + LogViolationDetailsRunnable(nsIContentSecurityPolicy* aCSP, > + nsString& aFileName, Nit: this should be const. @@ +260,5 @@ > + unsigned aLineNum) > + : mCSP(aCSP), > + mFileName(aFileName), > + mLineNum(aLineNum) > + {} Please assert a non-null csp. @@ +262,5 @@ > + mFileName(aFileName), > + mLineNum(aLineNum) > + {} > + > + NS_IMETHOD Run() Nit: Run on its own line. @@ +266,5 @@ > + NS_IMETHOD Run() > + { > + AssertIsOnMainThread(); > + > + NS_NAMED_LITERAL_STRING(scriptSample, "call to eval() or related function blocked by CSP"); Nit: Unless there's a reason not to you should capitalize "Call" and end the sentence with a period. Also, this is > 80 chars, please split into two lines. @@ +270,5 @@ > + NS_NAMED_LITERAL_STRING(scriptSample, "call to eval() or related function blocked by CSP"); > + mCSP->LogViolationDetails(nsIContentSecurityPolicy::VIOLATION_TYPE_EVAL, > + mFileName, > + scriptSample, > + mLineNum); Nit: You can squeeze more than one arg on a line. @@ +279,5 @@ > + > +JSBool > +ContentSecurityPolicyAllows(JSContext *aCx) > +{ > + WorkerPrivate* worker = GetWorkerPrivateFromContext(aCx); Please use this to AssertIsOnWorkerThread. @@ +280,5 @@ > +JSBool > +ContentSecurityPolicyAllows(JSContext *aCx) > +{ > + WorkerPrivate* worker = GetWorkerPrivateFromContext(aCx); > + if (worker->IsEvalAllowed()) Nit: Please brace all if statements, even if they're a single line. @@ +281,5 @@ > +ContentSecurityPolicyAllows(JSContext *aCx) > +{ > + WorkerPrivate* worker = GetWorkerPrivateFromContext(aCx); > + if (worker->IsEvalAllowed()) > + return JS_TRUE; Nit: 'return true' @@ +283,5 @@ > + WorkerPrivate* worker = GetWorkerPrivateFromContext(aCx); > + if (worker->IsEvalAllowed()) > + return JS_TRUE; > + > + nsAutoString fileName; nsString @@ +289,5 @@ > + > + JSScript* script; > + if (JS_DescribeScriptedCaller(aCx, &script, &lineNum)) { > + if (const char* file = JS_GetScriptFilename(aCx, script)) { > + CopyUTF8toUTF16(nsDependentCString(file), fileName); How about this: JSScript* script; const char* file; if (JS_DescribeScriptedCaller(...) && (file = JS_GetScriptFilename(...))) { fileName.AssignASCII(file); } else { JS_ReportPendingException(cx); } This way we make sure to report any exceptions that happen during this process as well as avoid some unnecessary string objects. @@ +293,5 @@ > + CopyUTF8toUTF16(nsDependentCString(file), fileName); > + } > + } > + > + nsCOMPtr<nsIContentSecurityPolicy> csp = worker->GetCSP(); Hm. At the very least this will addref a JS thing on a non-main thread. It's probably safe but I'd rather not do it (this won't work if we have workers live in another process, for instance). Please instead pass the worker pointer to the runnable and then let the main thread code get the csp. @@ +303,5 @@ > + JS_ReportError(aCx, "Failed to dispatch to main thread!"); > + } > + } > + > + return JS_FALSE; Nit: 'return false;' ::: dom/workers/WorkerPrivate.cpp @@ +23,5 @@ > #include "nsIURI.h" > #include "nsIURL.h" > #include "nsIXPConnect.h" > #include "nsIXPCScriptNotify.h" > +#include "nsIContentSecurityPolicy.h" Nit: alphabetize @@ +1933,5 @@ > mGCZeal = aParent->GetGCZeal(); > #endif > + > + SetEvalAllowed(aParent->IsEvalAllowed()); > + SetCSP(aParent->GetCSP()); This manipulates the CSP thing on a non-main thread thing too. Most other main-thread things come through as null here. @@ +1944,5 @@ > #ifdef JS_GC_ZEAL > mGCZeal = RuntimeService::GetDefaultGCZeal(); > #endif > + > + GetEvalPolicy(); Hm, this can fail, so it shouldn't be in the constructor (since you have no way to bail out if something fails here). I'd move this to the correck spot in WorkerPrivate::Create() so that you can actually handle failures. @@ +1950,5 @@ > } > > template <class Derived> > +void > +WorkerPrivateParent<Derived>::GetEvalPolicy() Let's rework this so that it returns success/failure. ::: dom/workers/WorkerPrivate.h @@ +35,5 @@ > class nsIURI; > class nsPIDOMWindow; > class nsITimer; > class nsIXPCScriptNotify; > +class nsIContentSecurityPolicy; Nit: Please alphabetize. @@ +481,5 @@ > return mIsChromeWorker; > } > > + bool > + IsEvalAllowed() Nit: This can be const @@ +493,5 @@ > + mEvalAllowed = aEvalAllowed; > + } > + > + nsIContentSecurityPolicy* > + GetCSP() Nit: const

Comment on attachment 659670 [details] [diff] [review] patch 3 Review of attachment 659670 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/workers/RuntimeService.cpp @@ +37,5 @@ > #include "WorkerPrivate.h" > > #include "OSFileConstants.h" > > +#include "jsdbgapi.h" Nit: Please insert into the block with xpcpublic.h, and alphabetize. @@ +249,5 @@ > } > > +class LogViolationDetailsRunnable : public nsRunnable > +{ > + WorkerPrivate* mWorkerPrivate; Ordinarily the worker runnables hold weak refs to the WorkerPrivate because they manage the busy count to ensure the worker stays alive. Since we're going to the main thread directly we can't modify the busy count like that, so I think you're going to need to hold a nsRefPtr here. I'm still not sure this is completely safe. It may end up that we need to send a message to the parent, then to its parent, etc., until we get to the main thread (like we do with error events). Not sure. @@ +271,5 @@ > + AssertIsOnMainThread(); > + > + nsIContentSecurityPolicy* csp = mWorkerPrivate->GetCSP(); > + if (csp) { > + NS_NAMED_LITERAL_STRING(scriptSample, "call to eval() or related function blocked by CSP"); Please address previous comment: Nit: Unless there's a reason not to you should capitalize "Call" and end the sentence with a period. Also, this is > 80 chars, please split into two lines. @@ +306,5 @@ > + nsRefPtr<nsRunnable> runnable = new LogViolationDetailsRunnable(worker, > + fileName, > + lineNum); > + if (NS_FAILED(NS_DispatchToMainThread(runnable, NS_DISPATCH_NORMAL))) { > + JS_ReportError(aCx, "Failed to dispatch to main thread!"); Nit: In this case the JS engine is going to stomp over this exception, so let's just NS_WARNING() that. ::: dom/workers/WorkerPrivate.cpp @@ +1893,5 @@ > nsCOMPtr<nsPIDOMWindow>& aWindow, > nsCOMPtr<nsIScriptContext>& aScriptContext, > nsCOMPtr<nsIURI>& aBaseURI, > nsCOMPtr<nsIPrincipal>& aPrincipal, > + nsCOMPtr<nsIDocument>& aDocument, Just so you know this has been removed so I think your tree is slightly out of date. Merge should be simple. @@ +2643,5 @@ > NS_ERROR("Must be chrome or have an domain!"); > return nullptr; > } > + > + nsresult rv = GetContentSecurityPolicy(getter_AddRefs(csp)); GetContentSecurityPolicy returns a bool... @@ +2650,5 @@ > + } > + > + if (csp) { > + nsresult rv = csp->GetAllowsEval(&evalAllowed); > + if (NS_FAILED(rv)) { Nit: |if (csp && NS_FAILED(csp->GetAllowsEval(...)))| @@ +4045,5 @@ > + nsCOMPtr<nsIScriptSecurityManager> ssm = nsContentUtils::GetSecurityManager(); > + > + NS_ASSERTION(ssm, "Failed to get security manager service"); > + if (!ssm) > + return false; Nit: Please brace all if statements @@ +4049,5 @@ > + return false; > + > + nsresult rv; > + nsCOMPtr<nsIPrincipal> subjectPrincipal; > + rv = ssm->GetSubjectPrincipal(getter_AddRefs(subjectPrincipal)); The first thing this function does is try to look up the current JS context. You already know that, so you should modify the function signature to take the current JSContext and then call GetCxSubjectPrincipal instead. @@ +4052,5 @@ > + nsCOMPtr<nsIPrincipal> subjectPrincipal; > + rv = ssm->GetSubjectPrincipal(getter_AddRefs(subjectPrincipal)); > + > + NS_ASSERTION(NS_SUCCEEDED(rv), "CSP: Failed to get nsIPrincipal from js context"); > + if (NS_FAILED(rv)) Nit: if (NS_FAILED(rv)) { NS_ERROR("CSP: Failed to get nsIPrincipal from js context"); return false; } @@ +4056,5 @@ > + if (NS_FAILED(rv)) > + return false; > + > + if (!subjectPrincipal) > + return true; Let's assert subjectPrincipal. The only time it can be null would be a bug I think. @@ +4062,5 @@ > + nsCOMPtr<nsIContentSecurityPolicy> csp; > + rv = subjectPrincipal->GetCsp(getter_AddRefs(csp)); > + NS_ASSERTION(NS_SUCCEEDED(rv), "CSP: Failed to get CSP from principal."); > + if (NS_FAILED(rv)) > + return false; Same nit about using NS_ERROR

Comment on attachment 661153 [details] [diff] [review] patch 4 Review of attachment 661153 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/workers/RuntimeService.cpp @@ +286,5 @@ > + } > +}; > + > +JSBool > +ContentSecurityPolicyAllows(JSContext *aCx) Nit: * on the left. ::: dom/workers/WorkerPrivate.cpp @@ +4039,5 @@ > + nsCOMPtr<nsIScriptSecurityManager> ssm = nsContentUtils::GetSecurityManager(); > + > + NS_ASSERTION(ssm, "Failed to get security manager service"); > + if (!ssm) { > + return false; if (!ssm) { NS_ERROR("Failed to get security manager service"); return false; } @@ +4045,5 @@ > + > + nsCOMPtr<nsIPrincipal> subjectPrincipal = ssm->GetCxSubjectPrincipal(aCx); > + NS_ASSERTION(subjectPrincipal, "Failed to get subjectPrincipal"); > + if (!subjectPrincipal) { > + return true; Oh, this isn't needed. We should just assert and not check/return here.

Thanks for doing this! Looks great!

Green on Try. https://tbpl.mozilla.org/?tree=Try&rev=69b71e275d70 https://hg.mozilla.org/integration/mozilla-inbound/rev/dac711e15d71

This should apply to setTimeout and setInterval with a string argument also, right? Those calls are still misbehaving for me (http://pages.cs.wisc.edu/~mazurak/csp_demo/) on yesterday's nightly, assuming I wrote those tests correctly.