1583949 - Perform Eval Checks on Worker Threads

Status: RESOLVED FIXED

(Core :: DOM: Security, defect, P2)

Description

[Tom Ritter [:tjr]]

While working on Bug 1582512 I found a case where we may not be triggering our eval check correctly. (Unless I'm missing something simple.) To test this you'll need the patches in Bug 1582512.

./mach test --headless devtools/client/framework/test/browser_browser_toolbox.js --debugger=gdb
(gdb) break nsContentSecurityUtils::IsEvalAllowed
(gdb) break nsContentSecurityUtils::ValidateScriptFilename if $_streq((char*)$rdi, "resource://gre/modules/workers/require.js line 137 > Function line 3 > eval")
(gdb) run

Eventually you'll see

Thread 69 "DOM Worker" hit Breakpoint 1, nsContentSecurityUtils::ValidateScriptFilename (
aFilename=0x7fffdd7f88d0 "resource://gre/modules/workers/require.js line 137 > Function line 3 > eval", aIsSystemRealm=<optimized out>)
at /home/tom/Documents/moz/mozilla-unified-2/dom/security/nsContentSecurityUtils.cpp:506

This maps to https://searchfox.org/mozilla-central/rev/f43ae7e1c43a4a940b658381157a6ea6c5a185c1/toolkit/components/workerloader/require.js#137

However, no calls to IsEvalAllowed happened (or will happen) despite this being an eval usage. (Two eval usages actually.)

Comment 1

[Tom Ritter [:tjr]]

Tracing this out, it appears that one of the calls to JS_SetSecurityCallbacks here passes a function (this one) which does perform CSP checks but does not include calls to our IsEvalAllowed Function.

Comment 2

[Tom Ritter [:tjr]]

Attached is a patch which shows what we need to do; but it doesn't work.

Inside IsEvalAllowed we call Preferences::GetString, which calls InitStaticMembers which requires MainThread. We're on a worker thread.

Comment 3

[Tom Ritter [:tjr]]

Nika suggested putting an if (NS_IsMainThread()) { } around the general.config.filename check. If a userchromejs user has a script that creates a worker that uses eval - we will break that eval usage (when we want to allow it). On one hand that's unlikely to happen and on the same hand how much do we really want to support userchromejs?

However I went down that route and started to hit lots more things that assume or require main thread. Recording a Telemetry Event assumed main thread with pref accesses - I cut those over to StaticPrefs for OMT access but I'm not certain Recording a Telemetry Event is threadsafe in general.

After that I hit problems with the console error reporting. Specifically mozilla::services::GetStringBundleService() requires Main Thread - from AddRef(). I think (assume?) I could move the Telemetry and the console error over to a Main Thread runnable though. (However I'll probably skip that in Debug builds since we're about to crash and there's no chance it would get run.)

Comment 4

[Tom Ritter [:tjr]]

Attachment: Bug 1583949 - Add a check for IsEvalAllowed to the worker callpath for eval()

Comment 5

[Tom Ritter [:tjr]]

Attachment: Bug 1583949 - Add a check for IsEvalAllowed to the worker callpath for eval() r?ckerschb

This patch does several things. Because Workers aren't on the main thread,
many of the things done are in the name of off main thread access.

1. Changes a parameter in IsEvalAllowed from a nsIPrincipal to a bool.
   We only used the principal to determined if it was the System Principal.
   Principals aren't thread safe and can only be accessed on Main Thread, so
   if we passed a Principal in, we would be in error. Instead only pass in
   the bool which - for workers - comes from a thread-safe location.

2. Separates out the Telemetry Event Recording and sending a message to the
   console into a new function nsContentSecurityUtils::NotifyEvalUsage. (And
   creates a runnable that calls it.)

   We do this because we will need to only call this method on the main thread.

   Telemetry Event Recording has only ever been called on the Main Thread.
   While I possibly-successfully cut it over to happen Off Main Thread (OMT)
   by porting preferences to StaticPrefs, I don't know if there were other
   threading assumptions in the Telemetry Code. So it would be much safer to
   just continue recording Event Telemetry on the main thread.

   Sending a message to the console requires calling GetStringBundleService()
   which requires main thread. I didn't investigate if this could be made
   thread-safe, I just threw it onto the main thread too.

   If, in IsEvalAllowed, we are on the main thread - we call NotifyEvalUsage
   directly. If we are not, we create a runnable which will then call
   NotifyEvalUsage for us on the main thread.

3. Ports allow_eval_with_system_principal and allow_eval_in_parent_process
   from bools to RelaxedAtomicBool - because we now check these prefs OMT.

4. In RuntimeService.cpp, adds the call to IsEvalAllowed.

5. Add resource://gre/modules/workers/require.js to the allowlist of eval
   usage. This was the script that identified this gap in the first place.
   It uses eval (twice) for structural reasons (scope and line number
   massaging.) The contents of the eval are the result of a request to a
   uri (which may be internal, like resource://). The whole point of this
   is to implement a CommonJS require() api.

   This usage of eval is safe because the only way an attacker can inject
   into it is by either controlling the response of the uri request or
   controlling (or appending to) the argument. If they can do that, they
   are able to inject script into Firefox even if we cut this usage of eval
   over to some other type of safe(r) script loader.

   Bug 1584564 tracks making sure calls to require.js are safe.

6. Adds cld-worker.js to the allowlist. Bug 1584605 is for refactoring that
   eval usage, which is decidedly non-trivial.

7. Does not enforce the eval restrictions for workers. While I've gotten
   try to be green and not throw up any instances of eval-usage by workers,
   it is much safer to deploy this is Telemetry-only mode for Workers for
   a little bit to see if anything pops up from the Nightly population.

   Bug 1584602 is for enforcing the checks.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3e896e8ca6b7
Add a check for IsEvalAllowed to the worker callpath for eval() r=ckerschb,baku

Comment 7

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/3e896e8ca6b7