1184387 - Prevent resource:// URIs from loading local files

Status: RESOLVED FIXED

(Core :: Security: CAPS, defect)

Description

[Bobby Holley (:bholley)]

There's no reason this should actually be allowed - it just happens to be the case right now. Cody has an experimental patch to drop support for this - let's try it out.

Comment 1

[Cody Crews]

Attachment: bug-1184387.patch

Fixed the issues previously pointed out with this patch.  It should be ready to go, anything else you can think of right now Bobby?

Comment 2

[Cody Crews]

I have to say that I didn't update my local repository so the parent change set once again is probably wrong.

I've requested level 1 commit access(try server) and that is over in bug 1184727.  Bobby, bz if either of you vouch for me I'll try my absolute best not to make that a mistake.  If it makes things quicker one of you guys could just push it to try for me.

My net is officially being transferred as of now so until this is all completely finished(moving my stuff this weekend) I'll have to rely on accessible networks where I can find them.  That's also why I didn't update my local repository and generate this patch properly because I made the terrible mistake of hitting up McDonalds and oh my was that an awful idea.  A simple pull of changes wouldn't even complete.

I guess just assign this bug to me unless it seems like this already has it.  My access and time to work is terrible but right now but I'll find a way to get it done.

Comment 3

[Bobby Holley (:bholley)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ab93c40b01cc

Comment 4

[Bobby Holley (:bholley)]

Comment on attachment 8634945 [details] [diff] [review]
bug-1184387.patch

Review of attachment 8634945 [details] [diff] [review]:
-----------------------------------------------------------------

r=me if it's green on try.

Comment 5

[Cody Crews]

I'm still working on processing all the info provided from this, but it's looking like this might break quite a few internal things.  I'll be looking over the source for places where such a minimal change could cause all the failures I'm seeing.

Comment 6

[Cody Crews]

(In reply to Cody Crews (if gone back by 07/21) from comment #5)
> I'm still working on processing all the info provided from this, but it's
> looking like this might break quite a few internal things.  I'll be looking
> over the source for places where such a minimal change could cause all the
> failures I'm seeing.

I thought I should clarify I know this changes things majorly, but since this should be prevented anyway why would people be relying on the ability for resource:// URIs to load file:// URIs?  I'm thinking it might be some of the tests themselves, but that wouldn't be good either hmm.

Comment 7

[Bobby Holley (:bholley)]

That try push looks really good actually - only one failure to look into as far as I can see:

browser/base/content/test/general/browser_parsable_css.js

Looks like the problem is just in the test.

Comment 8

[Cody Crews]

That's funny, I was seeing 14 failures having to do with timeouts.  Of course I'm not well versed in how the try server works as of yet but that's good.

Comment 9

[Bobby Holley (:bholley)]

(In reply to Cody Crews (if gone back by 07/21) from comment #8)
> That's funny, I was seeing 14 failures having to do with timeouts.  Of
> course I'm not well versed in how the try server works as of yet but that's
> good.

Yes, a lot of those are intermittent and unrelated to the patch. The key is to look whether the same test fails in both debug/opt, and whether it re-appears if you retrigger the job.

Comment 10

[Daniel Veditz [:dveditz]]

Calling this "sec-want" as a preventative measure, but it's been used as part of a sec-high/sec-critical exploit chain in the past.

Comment 11

[Cody Crews]

I'm still waiting on the net right now, but I'm eager to get back to this.  If it's looking good on those re-queues can someone handle pushing this forward like I want until I can get back stable and online.

Comment 12

[Boris Zbarsky [:bzbarsky]]

bobby, do you plan to tryserver this for Cody and whatnot?

Comment 13

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky [:bz] from comment #12)
> bobby, do you plan to tryserver this for Cody and whatnot?

I did already in comment 3 - there's just one failure that needs to be fixed.

Comment 14

[Cody Crews]

I hope you guys can pick up the slack on this one, I'm still not in any situation to work.  I'll be back to this ASAP but I'm not sure when that will be.  Sorry to whoever steps in and I would make an estimation on the time again but that doesn't seem to be working =/

Comment 15

[Bobby Holley (:bholley)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=86170ffb6015

Comment 16

[Bobby Holley (:bholley)]

Attachment: Part 1 - Stop using the hidden DOM window in browser_parsable_css. v1

The hidden DOM window has a chrome:// URI on mac and a resource:// URI elsewhere,
which makes it really terrible for stuff like this.

Comment 17

[Bobby Holley (:bholley)]

Attachment: Part 2 - Bail out of file:// loads for all non-chrome:// URIs. v1

bz is out, so I think it's fine for Gijs to rubber-stamp this. This is just
a slight cleanup of the version that cody wrote and I reviewed.

Comment 18

[Bobby Holley (:bholley)]

(In reply to Bobby Holley (:bholley) from comment #15)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=86170ffb6015

With a fix for the bc/dt failures: https://treeherder.mozilla.org/#/jobs?repo=try&revision=aa2ef1f3e636

Comment 19

[:Gijs (he/him)]

Comment on attachment 8643497 [details] [diff] [review]
Part 1 - Stop using the hidden DOM window in browser_parsable_css. v1

Review of attachment 8643497 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/test/general/browser_parsable_css.js
@@ +55,5 @@
>    return false;
>  }
>  
> +function once(target, name) {
> +  return new Promise(function f(resolve, reject) {

Nit: we have all the new JS goodness. Please (resolve, reject) => { ... }

@@ +71,5 @@
>    // our zipreader APIs are all sync)
>    let uris = yield generateURIsFromDirTree(appDir, ".css");
>  
> +  // Create a clean iframe to load all the files into. This needs to live at a
> +  // file:// URI so that it's allowed to load other file:// URIs.

Does this work to load jar:file URIs?

... I guess the trypush says it does? Although actually, I expect that "filePath", while a nice name, is just going to be a jar:file: URI in that case anyway. Certainly in the binaries we ship, e.g. resource://gre/modules/Task.jsm resolves to omni.ja!modules/Task.jsm, so the name of that variable and the comment here are misleading me.

Nit: Can you update those, please? :-)

@@ +83,1 @@
>    let doc = iframe.contentWindow.document;

Now doc is going to be the about:blank document, is it not? Seems like this line should be after the yield iframeLoaded;


... oh, I see, you noticed this in the followup trypush.

Comment 20

[:Gijs (he/him)]

Comment on attachment 8643499 [details] [diff] [review]
Part 2 - Bail out of file:// loads for all non-chrome:// URIs. v1

Review of attachment 8643499 [details] [diff] [review]:
-----------------------------------------------------------------

rs=me assuming this is all reviewed and whatnot already.

::: caps/nsScriptSecurityManager.cpp
@@ -847,5 @@
> -        // That's bogus!!  Fix this.  But watch out for
> -        // the view-source stylesheet?
> -        bool sourceIsChrome;
> -        rv = NS_URIChainHasFlags(sourceURI,
> -                                 nsIProtocolHandler::URI_IS_UI_RESOURCE,

This is potentially going to impact add-on consumers that try to use this. Cf. https://mxr.mozilla.org/addons/search?string=URI_IS_UI_RESOURCE .

That doesn't need to show-stop this, but maybe we should be more cautious than this patch?

OTOH, that list is not super-long, and it looks like a number of these are Kris Maglione's addons. He works for us, we could ping him and see what this protocol.js(m) thing is and why he needs it and if it's busted with this change or not.

I foresee that maybe we also don't want to specialcase addons here because the security vulnerabilities will then just target addons with these kinds of protocol handlers...

Comment 21

[Bobby Holley (:bholley)]

(In reply to :Gijs Kruitbosch from comment #20)
> This is potentially going to impact add-on consumers that try to use this.
> Cf. https://mxr.mozilla.org/addons/search?string=URI_IS_UI_RESOURCE .
> 
> That doesn't need to show-stop this, but maybe we should be more cautious
> than this patch?

That's the least of my addon compat worries, really. The fact that UI_RESOURCE URIs are able to load local files is pretty accidental, and I highly doubt that those custom protocol handlers are depending on it. My bigger concern is that there are addons that depend on being able to load file:// from resource:// (which is what we're trying to eliminate in this bug), but I'm optimistic given the lack of heavy breakage in our tree.

Time will tell.

Comment 22

[Bobby Holley (:bholley)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/78e2ba8842d4
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/f4fa8c49ebc6

Comment 23

[Ryan VanderMeulen [:RyanVM]]

Backed out for browser_parsable_css.js failures.
https://treeherder.mozilla.org/logviewer.html#?job_id=12529821&repo=mozilla-inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/a7860794b00e

Comment 24

[:Gijs (he/him)]

On why this broke on try:

The original patch had:

+  return new Promise(function f(resolve, reject) {
+    target.addEventListener(name, function() {
+      target.removeEventListener(name, f);

I should have caught that this is a no-op because "f" isn't the thing that got addEventListener'd. (if you copy-pasted this from somewhere, we should fix it there!)

Then I made you change f to not be a named function but a nameless arrow function, and then it properly went pear-shaped. Sorry.

Anyway, obvious fix is something like:

+  return new Promise((resolve, reject) => {
+    let cb = () => {
+      target.removeEventListener(name, cb);
+      resolve();
+    };
+    target.addEventListener(name, cb);
+  });

Comment 25

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #24)
> On why this broke on try:

sleepy fail, I meant inbound, not try.

Comment 26

[Bobby Holley (:bholley)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=cd5e05ed9bc6

Comment 27

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d52716b6cdf0
https://hg.mozilla.org/mozilla-central/rev/6d3e153efe5b

Comment 28

[Bobby Holley (:bholley)]

Comment on attachment 8643499 [details] [diff] [review]
Part 2 - Bail out of file:// loads for all non-chrome:// URIs. v1

This is very important defense-in-depth, and fixes one of the issues exploited in bug 1191284. There's a small risk that it might break weird add-ons, but we absolutely need to figure that out now (by uplifting to Beta) rather than letting it sit on Aurora for six weeks.

Approval Request Comment
[Feature/regressing bug #]: None
[User impact if declined]: potential vulnerability vector
[Describe test coverage new/current, TreeHerder]: None - just disallows a case.
[Risks and why]: May break addons that expect to load file:// from resource:// URIs, but hopefully not.
[String/UUID change made/needed]: None

Comment 29

[Bobby Holley (:bholley)]

Lawrence - what's going on here?

Comment 30

[Andrew McCreight [:mccr8]]

It sounds like we want this on ESR, too.

Comment 31

[Andrew McCreight [:mccr8]]

[Tracking Requested - why for this release]: defense-in-depth extra protection for a security bug that was found to be exploited in the wild.

Comment 32

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Tracked.

Comment 33

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8643499 [details] [diff] [review]
Part 2 - Bail out of file:// loads for all non-chrome:// URIs. v1

Approved for uplift to Beta as it's a potential vulnerability.

Comment 34

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Bobby, I was not part of the security group, which is why this beta uplift never showed up on my radar. :(

Do we need also need to uplift this to ESR38?

Comment 35

[Bobby Holley (:bholley)]

(In reply to Ritu Kothari (:ritu) from comment #34)
> Bobby, I was not part of the security group, which is why this beta uplift
> never showed up on my radar. :(

Ah I see - can we get that problem fixed somehow? This seems very likely to cause serious problems.

> Do we need also need to uplift this to ESR38?

Only once we successfully ship it on release, I think.

Comment 36

[Bobby Holley (:bholley)]

Ryan, would you mind landing this on your next batch?

Comment 37

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

(In reply to Bobby Holley (:bholley) from comment #35)
> (In reply to Ritu Kothari (:ritu) from comment #34)
> > Bobby, I was not part of the security group, which is why this beta uplift
> > never showed up on my radar. :(
> 
> Ah I see - can we get that problem fixed somehow? This seems very likely to
> cause serious problems.
> 
> > Do we need also need to uplift this to ESR38?
> 
> Only once we successfully ship it on release, I think.

I am a part of the security-group now, so I should be seeing all the nominations going forward.

Comment 38

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/5877058bc14d
https://hg.mozilla.org/releases/mozilla-beta/rev/19903924c38f

Is this something we should consider backporting to supported B2G branches too?

Comment 39

[Bobby Holley (:bholley)]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #38)
> https://hg.mozilla.org/releases/mozilla-beta/rev/5877058bc14d
> https://hg.mozilla.org/releases/mozilla-beta/rev/19903924c38f
> 
> Is this something we should consider backporting to supported B2G branches
> too?

We should take this on esr and b2g once it ships on release.

Comment 40

[Ryan VanderMeulen [:RyanVM]]

Backed out for browser_parsable_css.js failures.
https://treeherder.mozilla.org/logviewer.html#?job_id=480254&repo=mozilla-beta

https://hg.mozilla.org/releases/mozilla-beta/rev/822a3a2056cd

Comment 41

[:Gijs (he/him)]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #40)
> Backed out for browser_parsable_css.js failures.
> https://treeherder.mozilla.org/logviewer.html#?job_id=480254&repo=mozilla-
> beta
> 
> https://hg.mozilla.org/releases/mozilla-beta/rev/822a3a2056cd

This is because beta doesn't have bug 1161831. Don't know if we're going to need that for other reasons (seems like we might?) and/or how safe for uplift it is. If we don't uplift that, s/nsISubstitutingProtocolHandler/nsIResProtocolHandler/ should do the trick.

Comment 42

[Bobby Holley (:bholley)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ead429d4c20c

Comment 43

[Ryan VanderMeulen [:RyanVM]]

(In reply to Bobby Holley (PTO through Sept 8) from comment #42)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=ead429d4c20c

browser_parsable_css.js is timing out :(

Comment 44

[Bobby Holley (:bholley)]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #43)
> (In reply to Bobby Holley (PTO through Sept 8) from comment #42)
> > https://treeherder.mozilla.org/#/jobs?repo=try&revision=ead429d4c20c
> 
> browser_parsable_css.js is timing out :(

Looks like I also need to backport the addition of resource_test_file.html. With that it passes locally.

Comment 45

[Bobby Holley (:bholley)]

remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/d80a3d669b40
remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/5abf8bfc1c17

Comment 46

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

Found an add-on that no longer works because of this fix - filed in bug 1199594. More may be affected, not sure.

Comment 47

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Dave, in the 41.0b5 sign offs, QE team has run into an issue with add-ons no longer working owing to this fix. So far, this is limited to one add-on and they are testing a bit more. 

Could you please help investigate the extent of this regression? We need to determine whether the issue is significant enough to stop the Beta5 push. Thanks!

Comment 48

[Dave Townsend [:mossop]]

(In reply to Ritu Kothari (:ritu) from comment #47)
> Dave, in the 41.0b5 sign offs, QE team has run into an issue with add-ons no
> longer working owing to this fix. So far, this is limited to one add-on and
> they are testing a bit more. 

Comment 28 pointed out that this kind of breakage is expected. I'd expect it to affect quite a number of add-ons, particularly older ones that probably won't see updates to fix them. I'm not sure why we needed to rush this to beta, I'd argue for backing it out of beta and letting it ride the trains normally so we can have more time to understand how many add-ons this breaks.

Comment 49

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Thanks Dave. We will wait to see what kind of impact this has on the existing add-on ecosystem and then determine what the best next steps are - backout or another option. Backout does not seem like a good idea because we will leave ourselves vulnerable to this sec attack.

Comment 50

[Bobby Holley (:bholley)]

(In reply to Dave Townsend [:mossop] from comment #48)
> Comment 28 pointed out that this kind of breakage is expected. I'd expect it
> to affect quite a number of add-ons, particularly older ones that probably
> won't see updates to fix them. I'm not sure why we needed to rush this to
> beta, I'd argue for backing it out of beta and letting it ride the trains
> normally so we can have more time to understand how many add-ons this breaks.

The point of uplifting this to beta was that beta is the only population where we get any data about addon compatibility (so the time on aurora is basically wasted).

Backing this patch out would be ok, but we should only do that if we think this is likely to affect a number of addons (rather than just one).

Given that there's been no activity here and no other broken addons reported in the last three weeks, it looks like we're going to ship this, which I think is probably the right call given what we know.