Closed Bug 435362 Opened 16 years ago Closed 16 years ago

[FIX]cross-domain javascript security restrictions incorrectly applied to local HTML files loaded into an iframe

Categories

(Core :: Security, defect)

Product:
Core
Component:
Security
Type:
defect
Priority:
Not set
Severity:
major

Tracking

()

Status:
VERIFIED FIXED

People

(Reporter: tog, Assigned: bzbarsky)

References

(Depends on 1 open bug)

Details

(Keywords: regression, Whiteboard: [RC2+])

Attachments

(4 files)

I think this is quite safe
2.39 KB, patch
dveditz
: review+
jst
: superreview+
beltzner
: approval1.9+
Details | Diff | Splinter Review
Some tests
5.17 KB, application/zip
dveditz
: review+
Details
Fix the parens
2.12 KB, patch
dveditz
: review+
sicking
: superreview+
shaver
: approval1.9+
Details | Diff | Splinter Review
Tests updated to comment 15
5.25 KB, application/zip
Details 
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9) Gecko/2008051206 Firefox/3.0
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9) Gecko/2008051206 Firefox/3.0

A local html file (file:/// protocol) that has been loaded into an iframe as an anchor tag target cannot access javascript properties of its containing parent frames or topmost frame.

Attempts to access javascript properties belonging to parent frames will cause "Permission denied to get property xxx" errors. The correct behavior is the allow this access because the new stricter local file security model in Firefox 3 should allow it because the HTML files are all in the same local directory.

This problem did not occur in Firefox 2. The workaround is to set security.fileuri.strict_origin_policy to false.

This bug will cause problems for those making offline HTML help documentation that is distributed on CD-ROMs.


Reproducible: Always

Steps to Reproduce:
Including contents of three local test HTML files that demonstrate this bug.

File: index.html
----------------
<script language="Javascript" type="text/javascript">
  var msg = "The message";
</script>
<body>
This is index.html.
<a href="content2.html" target="content">Load Content2 Frame</a>
<iframe src="content.html" name="content"></iframe>
</body>


File: content.html
------------------
<body>
This is content frame. It will be able to access top.msg
<a href="javascript:alert(top.msg)">alert(top.msg)</a>
</body>


File: content2.html
-------------------
<body>
This is content2 frame. 
Attempts to access top.msg after it has been loaded into the iframe causes Permission denied to get property error.
<a href="javascript:alert(top.msg)">alert(top.msg)</a>
</body>

Actual Results:  
When index.html is first loaded, it will load content.html into the iframe. content.html is able to access parent's msg javascript property as expected.

But when content2.html is loaded into the iframe, content2.html cannot access parent's msg javascript property.

Expected Results:  
After content2.html is loaded into the iframe, it should be able to display top.msg javascript property.
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
Blocks: 230606
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: blocking1.9.0.1?
Keywords: regression
Product: Firefox → Core
Flags: blocking1.9?
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 <jst@mozilla.jstenback.com> 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. r=dveditz@cruzio.com, sr=darin@meer.net

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.
Flags: in-testsuite?
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.
Flags: blocking1.9?
Flags: blocking1.9.0.1?
Flags: blocking1.9.0.1+
Flags: blocking1.9-
Keywords: relnote
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.
Whiteboard: [RC2]
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.
Attachment #322592 - Flags: superreview?(jst)
Attachment #322592 - Flags: review?(dveditz)
Thanks!  Re-adding to the nom queue and otherwise exercising my flag-fingers.

(I am so glad it's late May.)
Flags: wanted1.9+
Flags: blocking1.9?
Flags: blocking1.9-
Whiteboard: [RC2-] → [RC2?]
Attachment #322592 - Flags: superreview?(jst) → superreview+
Attached file Some tests — 
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)
Summary: cross-domain javascript security restrictions incorrectly applied to local HTML files loaded into an iframe → [FIX]cross-domain javascript security restrictions incorrectly applied to local HTML files loaded into an iframe
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[0] 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+
Whiteboard: [RC2?] → [RC2+]
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...
Attached patch Fix the parens — — Splinter Review 
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)
Attachment #322807 - Flags: review?(dveditz)
Depends on: 424484
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
Closed: 16 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
Flags: blocking1.9.0.1+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: