Closed Bug 1583949 Opened 3 years ago Closed 3 years ago

Perform Eval Checks on Worker Threads


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




Tracking Status
firefox71 --- fixed


(Reporter: tjr, Assigned: tjr)


(Blocks 1 open bug)


(Keywords: sec-audit, Whiteboard: [domsecurity-active][adv-main71-])


(1 file, 1 obsolete file)

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

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

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.

Keywords: sec-audit
Group: core-security → dom-core-security

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.

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.)

Assignee: nobody → tom
Type: task → defect
Summary: Investigate a possible eval usage that is not triggered by these checks → Perform Eval Checks on Worker Threads
Blocks: 1584602
Group: dom-core-security

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.

Priority: -- → P2
Whiteboard: [domsecurity-active]
Attachment #9096753 - Attachment is obsolete: true
Pushed by
Add a check for IsEvalAllowed to the worker callpath for eval() r=ckerschb,baku
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla71
Whiteboard: [domsecurity-active] → [domsecurity-active][adv-main71-]
You need to log in before you can comment on or make changes to this bug.