XUL iframe/browser elements w/o type attribute probably have too high privileges
Categories
(Core :: XUL, defect)
Tracking
()
People
(Reporter: glazou, Unassigned)
References
Details
(Keywords: dev-doc-needed, sec-want, Whiteboard: [sg:want] unsafe default)
I have a XUL app that contains an iframe. Whenever a feed of news URLs is updated, the src of that iframe is modified. With a very recent feed update, I had the big surprise to see all my XUL contents go away, replaced by the contents of the target URL... That URL is http://www.lefigaro.fr/actualite-france/2009/02/02/01016-20090202ARTFIG00399-les-hommes-rechignent-toujours-a-faire-le-menage-.php It contains the following script lines : if(window.self != window.top && !document.referrer.match(/https?:\/\/[^?\/]+\.lefigaro\.fr\//)) { top.location.replace(window.location.pathname); } I thought a <xul:iframe> was always of type "content" because devmo does not document a type attribute on iframe like on browser. So my app's problem is really a lack of |type="content"| on my iframe and that's of course no fixed. But the present bug has two sides : 1. devmo's doc on iframe should show the 'type' attribute and clearly explain that w/o that attribute correctly set, the remote JS does have access to the chrome container 2. smaug suggested on IRC that the default type value for the <browser> and <iframe> elements could be changed. For the time being, these elements do grant chrome access to window.top unless the type attribute is correctly set. Smaug suggested to have a default value with higher security than the current one. It probably implies adding a new value "chrome" to the type attribute and making the "content" value the default if the element does not carry the type attribute at all.
Updated•15 years ago
|
Comment 1•15 years ago
|
||
Note, I marked this as security sensitive just in case...
Comment 2•15 years ago
|
||
(1) should definitely be done, and anyone can do it... Want to just write it up? (2) is a problem, because it's severely backwards-incompatible. It would break every single XUL application out there until they were updated to it. It would be really nice if that's how it were designed from the start, but I'm not sure what the costs of switching to it are at this point. Even just finding all the relevant locations to change sounds like a nightmare.
Updated•15 years ago
|
Comment 3•15 years ago
|
||
Why do we grant content access to the chrome container? Admittedly if you're not using type="content" then things like link targeting will be wrong, but at least content wouldn't be able to walk upwards...
Comment 4•15 years ago
|
||
> Why do we grant content access to the chrome container?
In what sense?
We window.parent/window.top operates on same-type docshells. So if you create an iframe without type="content", the code inside will be able to get a handle to the parent window. There is no security issue there.
The location property is one of the very few that are settable cross-site. There is no security check performed for that set. This is needed for web compat.
We could, perhaps, disallow setting location if the principal of the window it's being set on is system and the setting principal is not. But honestly, we have other security checks that also check the docshell type so if you're loading untrusted content in a chrome-type docshell you more or less lose, I think.
Comment 5•15 years ago
|
||
I meant disallow the content window from accessing window.parent/top. Do we know what would break if we disallow non-chrome from loading into chrome-type docshells, and how hard it would be to implement? I suspect that extension authors using xul:iframe often mean to get chrome docshells.
Comment 6•15 years ago
|
||
> Do we know what would break if we disallow non-chrome from loading into
> chrome-type docshells
Chatzilla, possibly. Extensions that load file:// or resource:// or data:// into such docshells. Probably the about: dialog. Heck, about:blank is non-chrome.
At least assuming you meant non-chrome:// when you said "non-chrome".
Comment 7•15 years ago
|
||
I mean that we should allow data: and about:blank as chrome documents when they have chrome privileges, and should disallow file: and resource:. But my question was more *who* we might break, not what.
Comment 8•15 years ago
|
||
That's a really hard question to answer... grepping through AMO might work, if done right...
Comment 9•15 years ago
|
||
unhiding since glazou blogged about this and we want to evangelize it heavily anyway.
Comment 10•15 years ago
|
||
Rey, can you help evangelize this to extension authors? It would be nice to get some list of extensions that use XUL iframes and figure out how bad the problem is now, to decide whether this is something we can easily do for 1.9.1
Comment 11•15 years ago
|
||
Just a note: I blogged on a similar topic a week ago and that blog post was imported into MDC - https://developer.mozilla.org/En/Displaying_web_content_in_an_extension_without_security_issues Unfortunately, none of the extension bugs I talked about in my blog post can be fixed just by adding type="content" to the frame.
Comment 12•15 years ago
|
||
(In reply to comment #10) > It would be nice to get > some list of extensions that use XUL iframes and figure out how bad the problem > is now, to decide whether this is something we can easily do for 1.9.1 I grepped the top 100 most popular extensions for "<iframe" and "<browser", trying to check whether they will load a content URL into a frame without type="content": * Flashgot (probably) * Foxmarks (pretty sure) * Foxy Tunes (definitely) * Glubble (definitely) * Minimap Sidebar (definitely) * All-in-one Sidebar (probably) * Scrapbook (definitely) * StumbleUpon (definitely) * Sxipper (definitely) Some will load a "trusted" URL coming from their own server.
I'm gonna mark this bug security sensitive for now. While some of the data in this bug has been published, the above list, and other things that we're talking about, have not.
Comment 14•15 years ago
|
||
I filed bug 477383 with a suggestion since changing the default will clearly not happen.
Comment 15•15 years ago
|
||
We can't change the default type now for 3.1, but we should have plenty of lead-time for 3.2 right? But then what happens in addons that really do need privileged frames and want to be compatible with trunk and 3.1? would an unknown type attribute (i.e. "chrome" for addons that need it) be "not content" and therefore privileged? Wladimir: as you find extension problems please file placeholder bugs (here) so we can investigate and follow up with the authors, or if you don't have time shoot me an email. There's no good place, addons.mozilla.org::Blocklisting is the closest for now (and hey, maybe the unsubtle implication will encourage speedy revisions). I'll see if I can get a generic "Security" component added to the amo product.
Comment 16•15 years ago
|
||
(In reply to comment #15) > We can't change the default type now for 3.1, but we should have plenty of > lead-time for 3.2 right? Problem is, most <iframe> and <browser> tags out there don't have a type attribute. I only listed the extensions where I saw indication that they need type="content". If I count at all extensions using <iframe> or <browser> without a type attribute, there are 27 (35% of all extensions on my list) - not counting HTML and dynamic element creation. I also counted 5 occurrences in Firefox and 13 in SeaMonkey. > There's no good place, addons.mozilla.org::Blocklisting is > the closest for now I am filing my bugs in AMO::Administration right now.
Comment 17•15 years ago
|
||
In 3.1, all iframes are treated as chrome except those whose type attribute is the string "content" or begins with the string "content-". So type="chrome" would be treated as chrome in 3.1, yes.
Updated•9 years ago
|
Updated•8 years ago
|
Comment 18•4 years ago
|
||
The surface area here is much smaller than before - addons aren't able to create XUL iframes/browsers so we control the consumers and can be careful to set type=content in our own code when needed.
That said, I do notice that we generally use type=content more often than not (https://searchfox.org/mozilla-central/search?q=%3Cbrowser&case=false®exp=false&path=.xhtml and https://searchfox.org/mozilla-central/search?q=createXULElement(%22browser%22&case=false®exp=false&path=).
Perhaps we should change the default behavior of the [type]
checks (like https://searchfox.org/mozilla-central/search?q=TypeAttrName&case=false®exp=false&path=) to treat a nonexistent type attribute as [type=content]
and then use [type=chrome]
to explicitly opt in to the current default behavior. Looks like that would remove some code on net (many JS setAttributes and the attribute in markup) and maybe be a better default. Not sure.. Nika, what do you think?
Comment 19•4 years ago
|
||
I imagine we should probably make XUL browser
be type=content
by default (perhaps even preventing it from being not type=content
?), though I'm less sure about the general XUL iframe
case. It's not super supported to have frames hosting untrusted content which aren't remote <browser>
elements anymore, so it seems a bit odd to have the default behaviour be pseudo-secure with them, despite not providing a proper process boundary. Making more internal code treat XUL browser
and XUL iframe
differently could be unfortunate, though.
The main risk here, I think, is with the number of changes to tests and other unrelated code which would be required to land this change. If someone was willing to do all of the validation work required for that, though, my intuition suggests this could be a reasonable change and may make working with browsers less error-prone.
Comment 20•4 years ago
•
|
||
(In reply to :Nika Layzell (ni? for response) from comment #19)
I imagine we should probably make XUL
browser
betype=content
by default (perhaps even preventing it from being nottype=content
?), though I'm less sure about the general XULiframe
case. It's not super supported to have frames hosting untrusted content which aren't remote<browser>
elements anymore, so it seems a bit odd to have the default behaviour be pseudo-secure with them, despite not providing a proper process boundary. Making more internal code treat XULbrowser
and XULiframe
differently could be unfortunate, though.The main risk here, I think, is with the number of changes to tests and other unrelated code which would be required to land this change. If someone was willing to do all of the validation work required for that, though, my intuition suggests this could be a reasonable change and may make working with browsers less error-prone.
Would it be possible to remove xul:iframe entirely and replace consumers with html:iframe? Or I guess xul:browser[type=chrome]
if that didn't work. From a quick search, in non-tests we seem to only have a few xul iframes created from JS (https://searchfox.org/mozilla-central/search?q=createXULElement(%22iframe&case=false ) and none in markup (https://searchfox.org/mozilla-central/search?q=%3Ciframe&case=false®exp=false&path=.xhtml)
Comment 21•4 years ago
|
||
I'm not sure. FWIW I believe we do use xul:iframe
with type=content
in a few places, at least in tests, but I think also in devtools. I think we'd probably have to try to make the replacements in order to know for certain whether or not doing it is possible.
Updated•2 years ago
|
Description
•