Closed Bug 1577122 Opened 6 years ago Closed 4 years ago

Restrict the schemes allowed for ChromeUtils.import to resource:// and chrome://

Categories

(Core :: XPConnect, task)

Product:
Core
Component:
XPConnect
Type:
task
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
101 Branch
Tracking Flags:
Tracking Status
firefox-esr91 --- wontfix
firefox99 --- wontfix
firefox100 --- wontfix
firefox101 --- fixed

People

(Reporter: freddy, Assigned: freddy)

References

Details

(Keywords: sec-want, Whiteboard: [post-critsmash-triage][adv-main101-])

Attachments

(1 file, 4 obsolete files)

Bug 1577122 - mozJSComponentLoader::Import() should only work for internal URLs, and update some tests r=mccr8
48 bytes, text/x-phabricator-request
Details | Review

We're thinking about restricting script loading in the parent process to things that are part of the build. This bug will find out if it's feasible to require ChromeUtils.import() to always take a string literal.

I'm going to mark this sec-want under the assumption that this is more of a hardening measure than something that you expect will find a problem.

Keywords: sec-want

Yes, sec-want is correct. This is not a security bug and I don't expect to find security bugs with this. It's hardening.

Assignee: nobody → fbraun
Assignee: fbraun → nobody

Unassigning this hardening bug. I don't think I will be able to continue working on this.

Tom, Do you think we should add this to our todo list?
thanks

Flags: needinfo?(tom)

I'm not sure. The work I'm doing in Bug 1562221 will prevent this at runtime in the browser. So you can use a variable name but it will need to pass that runtime inspection. On the flip side; a "data:" URI can be a string literal and pass the linter but not pass runtime inspection.

And a question is from a workflow perspective: do developers lint their code before they run it and see if it crashes? (I suspect they do not.)

So I'm inclined not to put in the work to clean up the existing violations just from a workload perspective...

Flags: needinfo?(tom)

(In reply to Tom Ritter [:tjr] (needinfo for responses to sec-[approval/ratings/advisories]) from comment #7)

And a question is from a workflow perspective: do developers lint their code before they run it and see if it crashes? (I suspect they do not.)

They usually don't but as it would show at review phase, not a big deal

@Standard8 (and others): What do you think? Is this still useful as a companion and early-warning to help folks catch during development than rather at runtime?
If so, the patch still seems to work but has lots of known failures on try, where we'd need to make sure that automation only complains about new failures (Unfortunately, I forgot how that exactly works for known linter violations in eslint).

I'm also happy to spend some time on this but just equally happy mark this WONTFIX if you think it's not.

Flags: needinfo?(standard8)

(In reply to Frederik Braun [:freddy] from comment #9)

@Standard8 (and others): What do you think? Is this still useful as a companion and early-warning to help folks catch during development than rather at runtime?
If so, the patch still seems to work but has lots of known failures on try, where we'd need to make sure that automation only complains about new failures (Unfortunately, I forgot how that exactly works for known linter violations in eslint).

You can use a default level of "warning". Or we could add annotations for the existing violations (there's only about 2 dozen, per the try job, that's not that bad...)

I'm also happy to spend some time on this but just equally happy mark this WONTFIX if you think it's not.

I think we should clamp down on the dynamic checks and make them crash. I don't think the work in bug 1562221 is quite the same - I think for ChromeUtils.import we should feasibly be able to restrict to chrome and resource URLs. But of course I haven't tried that...

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

(In reply to Frederik Braun [:freddy] from comment #9)

@Standard8 (and others): What do you think? Is this still useful as a companion and early-warning to help folks catch during development than rather at runtime?
If so, the patch still seems to work but has lots of known failures on try, where we'd need to make sure that automation only complains about new failures (Unfortunately, I forgot how that exactly works for known linter violations in eslint).

You can use a default level of "warning". Or we could add annotations for the existing violations (there's only about 2 dozen, per the try job, that's not that bad...)

I'm not sure "warn" is ideal for this case - I think this should be either it is fixed or it is explicitly allowed.

If we add annotations for the existing violations, then we should ensure that those existing violations are acceptable when we do that, and if not we should fix them. I don't think we should remain in a state where we're not sure about the existing violations in the tree - there aren't lots, so this doesn't seem like too much work.

I'm also happy to spend some time on this but just equally happy mark this WONTFIX if you think it's not.

I think we should clamp down on the dynamic checks and make them crash. I don't think the work in bug 1562221 is quite the same - I think for ChromeUtils.import we should feasibly be able to restrict to chrome and resource URLs. But of course I haven't tried that...

This sounds like it may be the better option overall as it would ensure we catch all cases (e.g. there is some code that isn't linted).

Flags: needinfo?(standard8)

Yeah, I agree that it would be more desirable to do something at runtime rather than linting.

(In reply to Mark Banner (:standard8) from comment #11)

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

(In reply to Frederik Braun [:freddy] from comment #9)
I think we should clamp down on the dynamic checks and make them crash. I don't think the work in bug 1562221 is quite the same - I think for ChromeUtils.import we should feasibly be able to restrict to chrome and resource URLs. But of course I haven't tried that...

This sounds like it may be the better option overall as it would ensure we catch all cases (e.g. there is some code that isn't linted).

I gave this a try and my first test run about file: URIs which we seem to need for XPIDatabase.jsm::getResourceURI. I could imagine just giving in and allowing file:. It does weaken the "part of the build" precondition that chrome/resource URIs had, but still confines the target resource to local data, which is much better than "anything goes".
I'll try again with file allowed and will repurpose this bug if it works out. The latest try push is https://treeherder.mozilla.org/#/jobs?repo=try&revision=c98917b439539e23a2e8f5fee3d6a3ee7e73abd1.

As an aside, I have also noticed that ChromeUtils.import() is (obviously) just one of many ways to load and execute privileged JavaScript. With this caveat, I might also abandon this bug completely and find a lower-level control instead.

(In reply to Frederik Braun [:freddy] from comment #12)

Yeah, I agree that it would be more desirable to do something at runtime rather than linting.

(In reply to Mark Banner (:standard8) from comment #11)

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

(In reply to Frederik Braun [:freddy] from comment #9)
I think we should clamp down on the dynamic checks and make them crash. I don't think the work in bug 1562221 is quite the same - I think for ChromeUtils.import we should feasibly be able to restrict to chrome and resource URLs. But of course I haven't tried that...

This sounds like it may be the better option overall as it would ensure we catch all cases (e.g. there is some code that isn't linted).

I gave this a try and my first test run about file: URIs which we seem to need for XPIDatabase.jsm::getResourceURI.

Uhh... I am confused - the callers of that method seem to care about images. Why are we ending up in ChromeUtils.import with those URIs?

Flags: needinfo?(fbraun)

Ouch. I tracked this to the wrong function! The test in docshell/test/unit/test_allowJavascript.js has its own getResourceuRI that just returns Services.io.newFileURI(do_get_file(file)).spec;.

Thank you, Gijs.

Tracing this again, all I can find are test-only occurrences:

  • file in docshell/test/unit/test_allowJavascript.js
  • file indom/indexedDB/test/unit/test_globalObjects_other.js
  • file injs/xpconnect/tests/unit/test_bug408412.js
  • jar: in js/xpconnect/tests/unit/test_import.js

I suppose one could continue restricting to "chrome, resource, jar, file" first and then iterate to either remove the usage of file & jar URLs or allow them only when under test.

I'll do another test :)

Flags: needinfo?(fbraun)

It's green on try.
TLDR: We could change ChromeUtils.import() to only allow URLs of schemes file, jar, chrome, resource.
File and jar are required for tests and could be deprecated iteratively, would require some more work.

Tom, what's your take? Proceed or invest time elsewhere?

Flags: needinfo?(tom)

(In reply to Frederik Braun [:freddy] from comment #14)

Ouch. I tracked this to the wrong function! The test in docshell/test/unit/test_allowJavascript.js has its own getResourceuRI that just returns Services.io.newFileURI(do_get_file(file)).spec;.

Thank you, Gijs.

Tracing this again, all I can find are test-only occurrences:

  • file in docshell/test/unit/test_allowJavascript.js
  • file indom/indexedDB/test/unit/test_globalObjects_other.js
  • file injs/xpconnect/tests/unit/test_bug408412.js
  • jar: in js/xpconnect/tests/unit/test_import.js

I suppose one could continue restricting to "chrome, resource, jar, file" first and then iterate to either remove the usage of file & jar URLs or allow them only when under test.

I'll do another test :)

I think for all of these the solution is likely to end up being putting the requested modules in a TESTING_JS_MODULES in the moz.build file and then using a resource or chrome URI to import it. It doesn't look super-onerous to do it here but if you want to split it up that also wfm.

The only weird case is that jar: one (test_import.js) - the comments are a bit confusing; it looks like it was changed in part 5 of bug 1383215 to expect things to throw instead of work. If you made the changes here throw an exception instead of crash then we would probably be OK even without changing that test (though it'd be nice if we could update the code comment to make it clearer what is being tested)? ;-)

With this change, the function will throw an exception when the
supplied URL does not have the scheme resource, chrome or file.

Assignee: nobody → fbraun
Status: NEW → ASSIGNED

It looks like you've settled on a dynamic rather than linting solution, so I'm going to move this to XPConnect.

Group: firefox-core-security → core-security
Component: Lint and Formatting → XPConnect
Product: Firefox Build System → Core
Group: core-security → dom-core-security
Attachment #9265353 - Attachment description: Bug 1577122 - throw for non-internal URLs in ChromeUtils.import() r=gijs → Bug 1577122 - mozJSComponentLoader::Import() should only work for internal URLs in r=gijs
Attachment #9088706 - Attachment is obsolete: true

Seems it's about done, which SGTM. It seems like we should add this to our exploit mitigation list.

Flags: needinfo?(tom)

Please file a followup bug about eliminating the jar and file schemes, and include or refer to your analysis in comment 14 in that bug.

Please also file a bug in Thunderbird::Upstream Synchronization. Thunderbird does some weird stuff, so who knows what it might break. Hopefully nothing, but it would be better to give them a heads up, as the failures might be mysterious without digging in.

Also, please update the summary of this bug to reflect what it is about now. Thanks.

Flags: needinfo?(fbraun)

While looking at your test, I noticed that I think we may already reject everything except file, jar and resource URIs.

In my local testing, it looks like the test passes even without your patch, while printing out this message: exception (expected) for unknown schemes: [Exception... "Component does not have requested interface" nsresult: "0x80004002 (NS_NOINTERFACE)" location: "JS frame :: [...]test_import.js :: run_test :: line 96" data: no]

Also, you can rewrite the test to use Assert.throws. There's an example of it earlier in the file.

I think this does more like what you want:

  var scope4 = {};
  var dataPath = "data:text/javascript,var a = {a:1}";
  Assert.throws(() => ChromeUtils.import(dataPath, scope4),
                /SecurityError/);

This fails without your patch and passes with it. However, I looked into what that NS_NOINTERFACE error actually means, and it looks like it comes from this line in Import: baseFileURL = do_QueryInterface(info.ResolvedURI(), &rv);

What this means is that the import code tries to resolve the URI, then attempts to QI it to either nsIJARURI or nsIFileURL. A javascript URI isn't either, so we reject it. ResolveURI only changes resource and chrome URIs, so I think in practice we already only allow jar, file, resource and chrome URIs. Well, maybe not jar because you said we reject them already somehow?

Anyways, we should probably get a better handle on whether this patch actually does anything before we land it, though I guess maybe it is good to have the restrictions more explicitly checked.

Never mind what I said in comment 20, as we should figure out what the status is before filing a Thunderbird or followup bug.

Flags: needinfo?(fbraun)

You're right. Rereading the code just confirms what you say in comment 21. The implementation only works for the interfaces of jar and file URLs. In essence, the only< remaining work is then to merely add a test for the types of URLs we hope never to support.

(In reply to Frederik Braun [:freddy] from comment #23)

You're right. Rereading the code just confirms what you say in comment 21. The implementation only works for the interfaces of jar and file URLs. In essence, the only< remaining work is then to merely add a test for the types of URLs we hope never to support.

So, fwiw, I disagree a bit with this. In particular, I think there is value in restricting this so that we can only import chrome and resource URIs, not other jar URLs or file URLs, because it restricts ChromeUtils.import to the stuff that shipped with the browser. This would make it harder to run local escalation attacks where you'd need/want to load external files from disk. You can dynamically add mappings for chrome/resource URLs if you know how to, and if whatever exploit path it is allows you to run the relevant code -- so how big a hurdle that is probably depends on the rest of the exploit chain. But it still seems reasonable to encode our assumptions (ie that ChromeUtils.import only gets used for in-browser code) into code here.

Actually, come to think of it, I would expect that you can't add chrome/resource mappings from the child process, but you may be able to load arbitrary file URLs from, say, the temp or downloads directory of the user's machine...

I agree that we would ideally only allow chrome and resource URIs, but the current patch does not do that, as it continues to allow file URIs. The question is, is it worth landing the current patch as-is, or should we wait until the extra work is done to also restrict file URIs?

FWIW, I modified my test case from comment 21 so that dataPath is a file URI referring to a file on my computer, and it did attempt to load it. Of course, outside of an xpcshell test, you'd be limited to where you could load a file from, as you said.

I've updated the summary to reflect what the patch is currently doing.

Summary: Allow ChromeUtils.import for string literals only. → Restrict the schemes allowed for ChromeUtils.import to resource, chrome and file

I can write up a patch to fix the 4 tests (comment 14) that need fixing to not import file.

Flags: needinfo?(gijskruitbosch+bugs)

Thanks, Gijs.

Summary: Restrict the schemes allowed for ChromeUtils.import to resource, chrome and file → Restrict the schemes allowed for ChromeUtils.import to resource:// and chrome://
Attached file Bug 1577122 - fix tests, r?mccr8 (obsolete) —
Flags: needinfo?(gijskruitbosch+bugs)
Attachment #9265810 - Attachment is obsolete: true
Attachment #9265353 - Attachment is obsolete: true
Attachment #9266062 - Attachment description: WIP: Bug 1577122 - mozJSComponentLoader::Import() should only work for internal URLs in r=mccr8 → Bug 1577122 - mozJSComponentLoader::Import() should only work for internal URLs in r=mccr8

I'm surprised we missed those in the latest try push from yesterday. Maybe "mach try auto" was not enough? Anyway, I'll fold them into my patch and try again, when I get back. Thanks!

Gijs, could you update your patch to fix test_import_fail.js, too? I think the breakage is fallout from that. Then we could land this, even before Freddy is back. Thanks.

Flags: needinfo?(gijskruitbosch+bugs)

I had a diff that should work, but doesn't and I'm indeed unlikely to finish to this before I get back Apr 4th.

diff --git a/toolkit/components/extensions/moz.build b/toolkit/components/extensions/moz.build
index e2f408da316a9..8cc1fc1bec37e 100755
--- a/toolkit/components/extensions/moz.build
+++ b/toolkit/components/extensions/moz.build
@@ -53,6 +53,8 @@ TESTING_JS_MODULES += [
     "ExtensionTestCommon.jsm",
     "ExtensionXPCShellUtils.jsm",
     "MessageChannel.jsm",
+    "test/xpcshell/data/TestWorkerWatcherChild.jsm",
+    "test/xpcshell/data/TestWorkerWatcherParent.jsm",
 ]
 
 DIRS += [
diff --git a/toolkit/components/extensions/test/xpcshell/head_service_worker.js b/toolkit/components/extensions/test/xpcshell/head_service_worker.js
index a2c8cf326039a..cefd26f6afda2 100644
--- a/toolkit/components/extensions/test/xpcshell/head_service_worker.js
+++ b/toolkit/components/extensions/test/xpcshell/head_service_worker.js
@@ -111,15 +111,12 @@ class TestWorkerWatcher extends ExtensionCommon.EventEmitter {
 
   registerProcessActor() {
     const { JS_ACTOR_NAME } = this;
-    const getModuleURI = fileName =>
-      Services.io.newFileURI(do_get_file(`${this.dataRelPath}/${fileName}`))
-        .spec;
     ChromeUtils.registerProcessActor(JS_ACTOR_NAME, {
       parent: {
-        moduleURI: getModuleURI(`${JS_ACTOR_NAME}Parent.jsm`),
+        moduleURI: `resource://testing-common/${JS_ACTOR_NAME}Parent.jsm`,
       },
       child: {
-        moduleURI: getModuleURI(`${JS_ACTOR_NAME}Child.jsm`),
+        moduleURI: `resource://testing-common/${JS_ACTOR_NAME}Child.jsm`,
       },
     });
   }

I tried debugging with this diff, but didn't get very far:

diff --git a/js/xpconnect/loader/mozJSComponentLoader.cpp b/js/xpconnect/loader/mozJSComponentLoader.cpp
index 104f260f1779b..19639074ee853 100644
--- a/js/xpconnect/loader/mozJSComponentLoader.cpp
+++ b/js/xpconnect/loader/mozJSComponentLoader.cpp
@@ -1256,6 +1256,9 @@ nsresult mozJSComponentLoader::Import(JSContext* aCx,
     // Reject imports from untrusted sources.
     if ((!info.URI()->SchemeIs("resource")) &&
         (!info.URI()->SchemeIs("chrome"))) {
+      nsAutoCString scheme;
+      info.URI()->GetAsciiSpec(scheme);
+      fprintf(stderr, "\n\nTEST-UNEXPECTED-FAIL import w/ scheme %s\n\n", scheme.get());
       return NS_ERROR_DOM_SECURITY_ERR;
     }
Flags: needinfo?(fbraun)

(In reply to Andrew McCreight [:mccr8] from comment #35)

Gijs, could you update your patch to fix test_import_fail.js, too? I think the breakage is fallout from that. Then we could land this, even before Freddy is back. Thanks.

Sorry, I've been kind of swamped. I think there are at least 3 other tests that also failed per comment #33, but I'll try taking a look.

test_import_fail.js failure is android-only - it passed on Linux on the trypush and it passes for me on macOS locally. It's complaining that the exception object it gets doesn't have a fileName property. I'll push to try to figure out what's wrong there, I guess, but I'll look at the other ones first.

Oh, OK, it's because my other patch removes that file from the list of support-files, and the code in test_import_fail.js loads the same file using resource://test/ which fails when we don't copy the file using the support-files mechanism. I'm not reproducing locally because I've already done a build and without a clobber, the objdir still has those support files in it so the test is happy.

This actually shows that we should just be using resource://test/ to access these files. I'll change all the test updates to use that (much smaller changeset), fold it into the single patch from Freddy and push to try.

Flags: needinfo?(gijskruitbosch+bugs)
Attachment #9266062 - Attachment description: Bug 1577122 - mozJSComponentLoader::Import() should only work for internal URLs in r=mccr8 → Bug 1577122 - mozJSComponentLoader::Import() should only work for internal URLs, and update some tests r=mccr8
Attachment #9265754 - Attachment is obsolete: true
Regressions: 1762611

Looks like this can land once jdescottes has fixed bug 1762611.

Depends on: 1762611

mozJSComponentLoader::Import() should only work for internal URLs, and update some tests r=mccr8
https://hg.mozilla.org/integration/autoland/rev/fafb7b2268f385506c5c650c819ac89f8a5b1648
https://hg.mozilla.org/mozilla-central/rev/fafb7b2268f3

Group: dom-core-security → core-security-release
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
status-firefox101: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 101 Branch

Is this something we were thinking we wanted in 100 ahead of pwn2own?

status-firefox100: --- → affected
status-firefox99: --- → wontfix
status-firefox-esr91: --- → wontfix
Flags: needinfo?(fbraun)

I don't believe this will be make a meaningful difference given the preconditions required to carry out an attack against ChromeUtils.import().
Tom, objections?

Flags: needinfo?(fbraun) → needinfo?(tom)

No objections

Flags: needinfo?(tom)
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main101-]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: