1577280 - Create a JS Engine callback to validate script filenames

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P1)

Description

[Tom Ritter [:tjr]]

From https://bugzilla.mozilla.org/show_bug.cgi?id=1562221#c4

We'd like to "add a JS engine API to set a process-wide callback to validate all script filenames going through the frontend and throw an exception if the callback rejects it."

In a followup bug I will write the callback and start enforcing acceptable filenames.

Comment 1

[Jan de Mooij [:jandem]]

I should have a patch this week or next week. I also want to make sure we expose this to the JS shell so we can write jit-tests for it.

One thing I wonder about is the PAC (Proxy Auto-Config) runtime. If I remember correctly that runs in the parent process? It would be good to double check that won't cause issues. If it does we could make this per-runtime (thread) but ideally it would be per-process.

Comment 2

[Tom Ritter [:tjr]]

(In reply to Jan de Mooij [:jandem] from comment #1)

One thing I wonder about is the PAC (Proxy Auto-Config) runtime. If I remember correctly that runs in the parent process? It would be good to double check that won't cause issues. If it does we could make this per-runtime (thread) but ideally it would be per-process.

Good point. Yes; it will, because it can load an arbitrary URL or a data: URI.

Instead of making a per-runtime callback; is there anything we can attach to the JSContext, SourceText, CompileOptions, or similar to mark this as a PAC 'thing' and then pass that data back into the callback to positively identify it as something we should relax restrictions for?

CompileOptions seems fruitful since that appears to be where the filename comes from (so most/all locations would have CompileOptions?) If we attached an enum to that we'd be giving the flexibility I kind of expect we'll need to safely permit exceptions to the rule...

Comment 3

[Jan de Mooij [:jandem]]

Attachment: Bug 1577280 - Add a script filename validation callback. r?tcampbell!

Comment 4

[Jan de Mooij [:jandem]]

The WIP patch needs a bit more work (documentation and some more tests) but should be good enough to test in the browser to see if the approach works.

The process-wide callback takes a filename (note: this can be called from background threads!) and must return a bool (true if accepted, false if rejected). If it returns false, the JS engine will throw a JS exception that contains the rejected filename. CompileOptions has a setSkipFilenameValidation(bool) method to opt-out.

The patch adds a testing function to TestingFunctions.cpp that installs a test callback that accepts all filenames starting with "safe". The jit-test has some tests for that.

Comment 5

[Tom Ritter [:tjr]]

Sure seems to be good from my eyes! I wasn't quite sure where to put the call; but I chose XPCJSRuntime::Initialize()

Comment 6

[:Gijs (he/him)]

Does the patch as written also allow us to determine the principal with which the script will be executed?

For context: it seems that although bug 1562221 was originally written to be about the parent process, some of the other deps have addressed system-principal scripts that run from data: URIs in the content process.

In the content process, because of web compat, we will need to allow websites to load data:foo scripts in their own scope. We may still want to prevent that happening in system principal'd execution contexts.

From a quick look, we could determine this in the callback by checking the return value of calling runningWithTrustedPrincipals on the jscontext we get passed - is that correct?

Comment 7

[Jan de Mooij [:jandem]]

(In reply to :Gijs (he/him) from comment #6)

In the content process, because of web compat, we will need to allow websites to load data:foo scripts in their own scope. We may still want to prevent that happening in system principal'd execution contexts.

Hm I thought we would set/install the callback only in the parent process, also for perf reasons (although the overhead is probably negligible if the callback returns immediately if !runningWithTrustedPrincipals). We could add a runningWithTrustedPrincipals bool argument to the callback. I have to look more at the off-thread parse case, though.

It does seem nice to lock things down as much as possible while we're at it.

Comment 8

[Jan de Mooij [:jandem]]

I started working on finishing the patch. Unfortunately handling XDR decoding well gets complicated fast - I filed bug 1582160 as one way forward there (it's something we've wanted to do anyway). We need to handle XDR here to ensure attackers can't trick the parent process into loading malicious code from the bytecode cache.

Comment 9

[Jan de Mooij [:jandem]]

Looking into it more, I can probably do this for now in ScriptSourceObject::initFromOptions instead of in ScriptSource::initFromOptions. Pretty subtle but I think it avoids the issues I was running into.

Comment 10

[:Gijs (he/him)]

(In reply to Jan de Mooij [:jandem] from comment #7)

(In reply to :Gijs (he/him) from comment #6)

In the content process, because of web compat, we will need to allow websites to load data:foo scripts in their own scope. We may still want to prevent that happening in system principal'd execution contexts.

Hm I thought we would set/install the callback only in the parent process, also for perf reasons (although the overhead is probably negligible if the callback returns immediately if !runningWithTrustedPrincipals). We could add a runningWithTrustedPrincipals bool argument to the callback. I have to look more at the off-thread parse case, though.

It does seem nice to lock things down as much as possible while we're at it.

Right - though if it complicates things significantly, I think we could definitely start by using the parent process first and defer child process + distinguishing principal support to a follow-up bug.

Comment 11

[Jan de Mooij [:jandem]]

Updated the patch. This one is nicer and simpler.

I also added an isSystemRealm argument to the callback as requested by Gijs.

Comment 12

[Jan de Mooij [:jandem]]

Should I land this part or (because this was filed as security bug) do you want to hold off until we have the Gecko side ready? I don't mind much either way but we shouldn't wait too long because people are refactoring things in this area in SpiderMonkey.

Comment 13

[Tom Ritter [:tjr]]

No, please land!

Comment 14

[Jan de Mooij [:jandem]]

(In reply to Tom Ritter [:tjr] from comment #13)

No, please land!

Great, will land today.

I'd suggest starting with the isSystemRealm == true case for actually rejecting things, because that's the subset we can probably say most about.

Comment 15

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/integration/autoland/rev/18d269bf8559

Comment 16

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/18d269bf8559

Comment 17

[Ryan VanderMeulen [:RyanVM]]

I assume this is something we aren't intending to backport.