2.39 KB, patch
|Details | Diff | Splinter Review|
5.17 KB, application/zip
2.12 KB, patch
|Details | Diff | Splinter Review|
5.25 KB, application/zip
Confirming, not sure this rises to blocking the FF3 freight train at this late point, but should be fixed as soon as possible after that.
Assignee: nobody → dveditz
Status: UNCONFIRMED → NEW
Component: Security → Security
Ever confirmed: true
Product: Firefox → Core
Dan, do you know what's going on here? Isn't this exactly the situation we tried to deal with by sharing principals for subframe loads?
So... the issue is that aOwner is null for link clicks and form-submit loads. we _used_ to use the right owner, I think. The change to not use an owner here was made in revision 1.639 of nsWebShell.cpp. Checkin comment: 1.639 <firstname.lastname@example.org> 2004-06-23 10:42 Fixing regression bug 246923. Bring back to life the fix for bug 13871, and improve on the fix for bug 246448. email@example.com, firstname.lastname@example.org The current setup looks pretty dangerous to me, for what it's worth. It's relying on the fact that nsDocShell::InternalLoad handles the INHERIT_OWNER flag before doing load targeting and the fact that we don't do the link click event processing if the document is no longer the docshell's current document. But it seems to me that we should just be passing the NodePrincipal() of the link node (if there is a node) as aOwner here... jst, do you recall what the deal here is?
And I'd assumed we'd tested this when we tested the file:// changes... :( I definitely think we need to fix this in the first point release if we don't fix it for final. This is a pretty serious regression.
Alternately, we need to inherit the current principal for file:// URIs when INHERIT_OWNER is set...
OS: Windows Vista → All
Hardware: PC → All
Version: unspecified → Trunk
This must block 3.0.1, but we will want to carefully verify a lot of stuff that doesn't have automated coverage, so not going to rush it for 3.0.
I really think we should consider this for RC2 if we have one. We worked hard to avoid this exact brokenness since we strongly suspected that it would break lots of locally stored HTML, such as documentation and help systems.
We did consider it for the likely RC2, and decided that we should neither rush a fix for file security (no patch here, only musings on how we might go about fixing it) nor delay a possible RC2 indefinitely while we get confident that we didn't regress other things. Renom with a reviewed patch and test cases in the next much-less-than-24 hours, I guess. 3.0.1 isn't that far away either, and if we hadn't discovered it until after RC2 we certainly wouldn't roll an RC3 just for it.
Whiteboard: [RC2] → [RC2-]
This makes sure to set the owner to our inherited principal (which means either whatever is in our docshell right now or whatever our parent's principal is if we have nothing in our docshell) whenever the INHERIT_PRINCIPAL flag is set and we're loading a local file URI. Since we don't set the principal on the channel unless either the URI being loaded inherits or the principal is allowed to load the file in question, just setting the owner up here is safe. It's not pretty (I still think we should propagate the principal of the node through to make this pretty), but it's easier to reason about than the fixes that remove the general fragility here. Bug 230606 seems to have no tests of any sort attached, not even manual ones. :( I guess I'll see what I can do to scrounge up some tests.
Thanks! Re-adding to the nom queue and otherwise exercising my flag-fingers. (I am so glad it's late May.)
Whiteboard: [RC2-] → [RC2?]
Attachment #322592 - Flags: superreview?(jst) → superreview+
This exercises link clicks and subframe loads. A number of these fail without the patch in this bug. This does NOT yet exercise window.location sets, but those always set the owner, so this patch should not affect them, since the new code only runs if !owner. To test: Download ZIP file to disk. Unzip it. Clear your error console. Load the two index.html files in the zip (the under under test/ and the one under test/subdir) in separate windows or tabs. Click all the links as needed to run all 12 tests. There should be no alerts and no errors in the error console.
Assignee: dveditz → bzbarsky
Status: NEW → ASSIGNED
Attachment #322595 - Flags: review?(dveditz)
Jonas will take the lead on testing and building confidence around this patch, with ETA to land at end of business. He should call me on my phone if he needs approval. :)
Flags: blocking1.9? → blocking1.9+
Comment on attachment 322592 [details] [diff] [review] I think this is quite safe When I open a file: browser window from a chrome-privileged script the file: window seems to pick up a null principal. Much better than getting the chrome principal (for sure!) but might interfere with remembered permissions and the like (not that those work right now). Otherwise this looks good and works, but I'd be a smidgen happier if this were restricted to the case when the opener was definitely a file: URI.
Attachment #322592 - Flags: review?(dveditz) → review+
Attachment #322592 - Flags: approval1.9?
This patch didn't change that, though, right? That's the behavior bug 230606 gives, as far as I can tell....
Comment on attachment 322595 [details] Some tests Looks good, r=dveditz. nit1: might be better to always reference frames by name rather than mix in the frames[n] shortcuts, just in case someone edits the test in the future and messes it up by inserting an <iframe>. Also having "frame1" be frames and so on is slightly disorienting and might lead to future mistakes. nit2: your top-level index.html opens a frame on a directory listing ("frame5") but doesn't test to make sure it cannot be read from. Would be worth another test checking after loading a second directory by a link click.
Attachment #322595 - Flags: review?(dveditz) → review+
Comment on attachment 322592 [details] [diff] [review] I think this is quite safe a=beltzner, please land before noon PDT und danke!
Attachment #322592 - Flags: approval1.9? → approval1.9+
Do we really want the URIIsLocalFile(aURI) check to override the |aLoadType != LOAD_NORMAL_EXTERNAL| and |!owner| checks? With this patch we'd grab an owner if someone does an external file load.
Other than that my testing hasn't uncovered anything bad yet...
sicking's right: the patch was missing a pair of parens. The desired check is: "not external and not owner and inherit flag and (inherits or file)"
Attachment #322807 - Flags: superreview?(jonas) → superreview+
Comment on attachment 322807 [details] [diff] [review] Fix the parens r=dveditz Sorry I missed the parens.
Attachment #322807 - Flags: review?(dveditz) → review+
Comment on attachment 322807 [details] [diff] [review] Fix the parens a=shaver, thanks for everyone's diligence. You are cleared for landing on CVS trunk with all due haste.
Attachment #322807 - Flags: approval1.9+
I forgot I could use window.frames[name]. Did that. The test already checked that it couldn't read frame5. I've added another test which loads a directory in a subframe via a link and then tries reading it. The other thing the tests could use is that all the places we expect an exception we should probably assert that it's in fact a security exception and not a "you made a typo" exception...
Checked in on the CVS trunk.
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Verified with RC2 (Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9) Gecko/2008052903 Firefox/3.0). No alerts or errors in the error console from any of the links in the test case.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.