611 bytes, text/html
2.13 KB, patch
|Details | Diff | Splinter Review|
1.95 KB, patch
|Details | Diff | Splinter Review|
Interesting. Dragging a chrome: URL onto a tab works in Windows and Mac, but when I try the same in Linux I get: Error: uncaught exception: Drop of chrome://browser/content/aboutDialog.xul denied.
Brandon, what's the reasoning for rating this as moderate rather than critical? The bug allows for arbitrary code execution, 'requiring no user interaction beyond normal browsing'. It also affects the majority of Firefox users (i.e. those with Java installed). I believe about 80% of web users have Java installed, I'm not sure what the percentage is for Firefox users. I also don't think the testcase 'requires the user to perform complicated and/or unlikely steps', unless you count the three clicks as complicated.
I did consider the steps required to exploit this bug as somewhat non-standard. Seeing the attack window appearing and disappearing would certainly alert me (admittedly, not a typical user) that something weird was happening, and I probably wouldn't be compelled to click the second time, dropping the JS URL on the chrome tab. What you are saying is perfectly reasonable, though. If others think the severity should be higher, feel free to make it so.
Escalation to chrome: from content is a critical, moderated to high with insufficient reliability of exploit (for example, it doesn't work for me possibly due to my JS settings). Adjusting to sg:high for now, though I agree that with a really slick exploit it might be even higher.
Seems like we should disallow dropping of URI_INHERITS_SECURITY_CONTEXT URIs.
Created attachment 428128 [details] [diff] [review] patch? This fixes the testcase. I can't think of any use cases for dragging INHERIT_PRINCIPAL URIs that we care to support, really, but maybe I'm missing something. I had to change this code to deal with the !sourceDoc case, since otherwise this testcase hit it (presumably because the drag comes from Java) and dragDropSecurity check returned early. I'm not sure if there's a better principal to use than about:blank's, for the no-document case.
I'm not sure why we need or don't (and want or don't) the DISALLOW_INHERIT_PRINCIPAL change yet. Need to think about it a bit. Can you explain what the net effect of that change is? The other difference seems to be that you're switching from checkLoadURIStr to checkLoadURIStrWithPrincipal. This is obviously a good thing to do, but why does it help in this case? Or is it just the change to not return early if !sourceDoc that helps with that? The fact that chrome:// could be loaded like this seems like the real issue here, since the file being changed was supposed to prevent just that.
The part of that patch that fixes the bug is the change to the !sourceDoc case. With the testcase, the dragSession has no source document, so dragDropSecurityCheck was previously a no-op. The change to the *WithPrincipal version is unrelated cleanup inspired by the deprecation markings in nsIScriptSecurityManager.idl that I figured I'd do while changing the code - it can be omitted, and probably should be for simplicity's sake (we have bug 327244 tracking that anyways). The addition of DISALLLOW_INHERIT_PRINCIPAL was based on an early guess about a solution, from before I'd looked into the code. It seems reasonable to me to block drags of such URIs in general. That's probably best split off into a separate bug about a more general belt-and-suspenders approach to these kinds of bugs.
Created attachment 428142 [details] [diff] [review] minimal patch
I'd prefer a separate bug on that, yeah. Especially since I drag-n-drop data: URIs a good bit. Usually from the URL bar to my editor once it gets too unwieldy in the url bar. ;) The !sourceDoc change is a clear win in my book. The WithPrincipal change I'm fine with happening in a different bug, but let's get it actually happening. The existing code is wrong (in the "won't allow drags that it should" sense) in various cases not dealing with web content, for example.
Blocks 126.96.36.199. This alone wouldn't force a 188.8.131.52, but should definitely be taken if it's branch-safe for that series.
Err, it does block 184.108.40.206, not force a 220.127.116.11, but wanted on that branch, too. Durr.
(In reply to comment #17) > I'd prefer a separate bug on that, yeah. Especially since I drag-n-drop data: > URIs a good bit. Usually from the URL bar to my editor once it gets too > unwieldy in the url bar. ;) This check only applies to drops, so the change wouldn't affect that use case, fwiw. I filed bug 547813.
Hmm. I thought this check was for the drag start. So with the attached patch dragging a file:// URI from outside the browser into the browser won't work, right? That seems suboptimal.
On the other hand, it's not clear to me that we want to allow a drag from java to allow loading file://, and that's precisely the "from outside the browser" case...
(In reply to comment #21) > So with the attached patch dragging a file:// URI from outside the browser into > the browser won't work, right? That seems suboptimal. Hmm, indeed :( I guess there may not currently be a way to differentiate these two cases... So in terms of options for for fixing this testcase, I think that leaves us with pushing for a change in Java (to let us know that the drag is plugin-triggered?), or fixing bug 547813.
(In reply to comment #23) > or fixing bug 547813. Though that approach would require that we explicitly check URIChainHasFlags(URI_INHERITS_SECURITY_CONTEXT) rather than relying on checkLoadURI, or change the "from" to be something with the system principal (?).
Or using file:/// instead of about:blank as the dummy URI, possibly.
(In reply to comment #25) > Or using file:/// instead of about:blank as the dummy URI, possibly. Hmm, that's possibly brilliant. My patch with "file:///" rather than about:blank makes file drops work again. Apart from the link-to-file ability, file:/// is the same as about:blank, right? Trying to think of other things this could break.
(It also makes it possible for Java-triggered drag/drops to load file:// URIs as you pointed out, of course, but that's much less of a problem than this bug.)
> Apart from the link-to-file ability, file:/// is the same as about:blank, right? For CheckLoadURI purposes, I believe this is correct.
Created attachment 428294 [details] [diff] [review] patch using file:///
I've given this some thought, and I can't think of any ways that using file:/// as the default origin would break anything important. It will block drops of URI_IS_UI_RESOURCE/URI_DANGEROUS_TO_LOAD URIs that were previously allowed in the drag-from-external-app-or-plugin case, but I don't think there are any valid use cases for that, and we kind of need to prevent those to avoid this bug. I should probably add a comment about the use of "file:///" as the fallback URI here, though. I'm not sure who else should review this patch...
Comment on attachment 428294 [details] [diff] [review] patch using file:/// Dave, can you review this with comment 30 in mind?
Comment on attachment 428294 [details] [diff] [review] patch using file:/// This makes sense to me
This is reviewed, should it be up for approval?
I landed the patch on the trunk: https://hg.mozilla.org/mozilla-central/rev/66b74d46682e I used a cover bug (bug 549349) to try and reduce the likelihood of this being exposed as a security bug before we have it fixed on all branches. Thanks for the clear report and testcase, Paul!
(In reply to comment #38) > Gavin, I noticed from my server logs that you were trying the testcase on > Linux. It doesn't work particularly well there, since an actual drag gesture is > needed to complete the drop, instead of just a click. Indeed, I was trying on Linux. I was aware of the drag gesture difference, but in my case the applet was failing to load for some reason. I think the java plugin I'm using is just broken, though.
Comment on attachment 428294 [details] [diff] [review] patch using file:/// This applies to 1.9.2 cleanly.
Created attachment 430108 [details] [diff] [review] 1.9.0/1.9.1 patch
Comment on attachment 428294 [details] [diff] [review] patch using file:/// a1922=beltzner
Comment on attachment 430108 [details] [diff] [review] 1.9.0/1.9.1 patch a19019/1919=beltzner
Can we get this landed? Code freeze is Tuesday, March 9th.
1.9.2: https://hg.mozilla.org/releases/mozilla-1.9.2/rev/82fa604cdf23 1.9.1: https://hg.mozilla.org/releases/mozilla-1.9.1/rev/ff4a52b1c2a4 1.9.0: mozilla/toolkit/content/nsDragAndDrop.js 1.11
Paul: just want to be clear in case you can't make sense of our Bugzilla flag gibberish: this will be fixed in Firefox 3.0.19, 3.5.9, and 3.6.2 which are all due out shortly. Thanks again!
Paul, can we get you to test the builds for us and let us know if they're fixed for you? You can get them here: 3.0.19pre: ftp://ftp.mozilla.org/pub/firefox/nightly/latest-mozilla1.9.0/ 3.5.9pre: ftp://ftp.mozilla.org/pub/firefox/nightly/latest-mozilla-1.9.1/ 3.6.2pre: ftp://ftp.mozilla.org/pub/firefox/nightly/latest-mozilla-1.9.2/
Was the 1.9.0 and 1.9.1 version of this patch code reviewed? I don't see a reviewer above or was it close enough to the 1.9.2 patch that it didn't matter (though comment 48 might argue against that)?
Mossop - you reviewed the original patch. Is the 1.9.0/1.9.1 patch a straight port? I didn't notice the lack of review, and the code looks pretty much the same to me. Gavin's out on vacation otherwise I'd be asking him.
(Removing fixed flags for the versions in which Al claims this is not fixed)
The 1.9.0/1.9.1 is functionally the same. The only difference is that a few lines are moved around due to other changes.
I've tested all three builds that Mike linked to and all are indeed fixed. I tested them on Windows XP SP2, as Al did.
Thanks, Paul. Re-marking as FIXED. Al, back to you to verify, maybe use the nightly builds instead of your own local ones?
Al, that's correct. The 'About Firefox' page shouldn't load, and there shouldn't be any alert.
Created attachment 431907 [details] Debug 1.9.0 showing bug occurring This is my debug build from 3/10/2010. This shows that the testcase still repros in debug. In the non-debug nightly on the same machine, the bug is fixed and I never get the chrome window or the alert. That build is Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:18.104.22.168pre) Gecko/2010031005 GranParadiso/3.0.19pre (.NET CLR 3.5.30729).
Created attachment 431908 [details] Debug 1.9.1 showing bug occurring This is a screenshot from my 1.9.1 debug build from 3/10/2010, showing the bug still occurring with the testcase. When I use the non-debug 1.9.1 nightly build, the case no longer reproduces. That build is Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:22.214.171.124pre) Gecko/20100310 Shiretoko/3.5.9pre (.NET CLR 3.5.30729).
So, Paul is correct in that the non-debug builds will no longer reproduce the bug. My debug builds still do reproduce them and I synched and built them around noon on 3/10/2010 so they have the fix's code. I'm not sure if I should mark this as verified fixed for 1.9.0 and 1.9.1 when it still reproduces in debug. I will let Dan Veditz and Mike Beltzner tell me what we want to do here.
File a new bug about how the debug builds still trip the behaviour, mark it as blocking this one.
Al, just to double-check what are the changeset IDs (or even just the hg.mozilla.org URIs) your debug builds list in about:buildconfig ?
Ah crap, my VM blows. I did a rollback and didn't pick up changes so I have pre-fix code. This is my mistake. I'll go fix my builds now.
Paul, If you are interested you are eligible or a security bug bounty on this bug. You can you contact me at email@example.com about the bounty.