Closed Bug 1184387 Opened 9 years ago Closed 8 years ago

Prevent resource:// URIs from loading local files

Categories

(Core :: Security: CAPS, defect)

Product:
Core
Component:
Security: CAPS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla42
Tracking Flags:
Tracking Status
firefox40 --- wontfix
firefox41 + fixed
firefox42 --- fixed
firefox-esr38 --- wontfix
b2g-v2.0 --- wontfix
b2g-v2.0M --- wontfix
b2g-v2.1 --- wontfix
b2g-v2.1S --- wontfix
b2g-v2.2 --- wontfix
b2g-v2.2r --- wontfix
b2g-master --- fixed

People

(Reporter: bholley, Assigned: bholley)

References

(Regressed 1 open bug)

Details

(Keywords: sec-want, Whiteboard: [post-critsmash-triage][adv-main41-][adv-esr38.3-])

Attachments

(2 files, 1 obsolete file)

Part 1 - Stop using the hidden DOM window in browser_parsable_css. v1
2.56 KB, patch
Gijs
: review+
Details | Diff | Splinter Review
Part 2 - Bail out of file:// loads for all non-chrome:// URIs. v1
2.94 KB, patch
Gijs
: review+
ritu
: approval-mozilla-beta+
Details | Diff | Splinter Review 
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.
Attached patch bug-1184387.patch (obsolete) — — Splinter Review 
Fixed the issues previously pointed out with this patch.  It should be ready to go, anything else you can think of right now Bobby?
Attachment #8634945 - Flags: review?(bobbyholley)
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 on attachment 8634945 [details] [diff] [review]
bug-1184387.patch

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

r=me if it's green on try.
Attachment #8634945 - Flags: review?(bobbyholley) → review+
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.
(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.
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.
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.
(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.
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.
Keywords: sec-want
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.
bobby, do you plan to tryserver this for Cody and whatnot?
Flags: needinfo?(bobbyholley)
(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.
Flags: needinfo?(bobbyholley)
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 =/
Flags: needinfo?(bobbyholley)
Assignee: nobody → bobbyholley
Flags: needinfo?(bobbyholley)
The hidden DOM window has a chrome:// URI on mac and a resource:// URI elsewhere,
which makes it really terrible for stuff like this.
Attachment #8643497 - Flags: review?(gijskruitbosch+bugs)
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.
Attachment #8634945 - Attachment is obsolete: true
Attachment #8643499 - Flags: review?(gijskruitbosch+bugs)
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.
Attachment #8643497 - Flags: review?(gijskruitbosch+bugs) → review+
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...
Attachment #8643499 - Flags: review?(gijskruitbosch+bugs) → review+
(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.
Flags: needinfo?(bobbyholley)
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);
+  });
(In reply to :Gijs Kruitbosch from comment #24)
> On why this broke on try:

sleepy fail, I meant inbound, not try.
https://hg.mozilla.org/mozilla-central/rev/d52716b6cdf0
https://hg.mozilla.org/mozilla-central/rev/6d3e153efe5b
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox42: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Flags: needinfo?(bobbyholley)
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
Attachment #8643499 - Flags: approval-mozilla-beta?
Lawrence - what's going on here?
Flags: needinfo?(lmandel)
It sounds like we want this on ESR, too.
status-firefox40: --- → wontfix
status-firefox41: --- → affected
status-firefox-esr38: --- → affected
[Tracking Requested - why for this release]: defense-in-depth extra protection for a security bug that was found to be exploited in the wild.
tracking-firefox41: --- → ?
tracking-firefox-esr38: --- → ?
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.
Attachment #8643499 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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?
Flags: needinfo?(lmandel) → needinfo?(bobbyholley)
(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.
Flags: needinfo?(bobbyholley)
Ryan, would you mind landing this on your next batch?
Flags: needinfo?(ryanvm)
(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.
Flags: needinfo?(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?
status-b2g-master: --- → fixed
status-firefox41: affected → fixed
Flags: needinfo?(bobbyholley)
Flags: in-testsuite?
(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.
Flags: needinfo?(bobbyholley)
(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.
(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 :(
(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.
Whiteboard: [post-critsmash-triage]
Found an add-on that no longer works because of this fix - filed in bug 1199594. More may be affected, not sure.
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!
Flags: needinfo?(dtownsend)
(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.
Flags: needinfo?(dtownsend)
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.
Group: core-security → core-security-release
(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.
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main41-]
Whiteboard: [post-critsmash-triage][adv-main41-] → [post-critsmash-triage][adv-main41-][adv-esr38.3-]
Depends on: 1224046
Group: core-security-release
Regressions: 1484471
No longer depends on: 1199594
Regressions: 1199594
You need to log in before you can comment on or make changes to this bug.