476464 - XUL iframe/browser elements w/o type attribute probably have too high privileges

Status: NEW

(Core :: XUL, defect)

Description

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

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.

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

Note, I marked this as security sensitive just in case...

Comment 2

[Boris Zbarsky [:bzbarsky]]

(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.

Comment 3

[Benjamin Smedberg]

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

[Boris Zbarsky [:bzbarsky]]

> 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

[Benjamin Smedberg]

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

[Boris Zbarsky [:bzbarsky]]

> 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

[Benjamin Smedberg]

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

[Boris Zbarsky [:bzbarsky]]

That's a really hard question to answer... grepping through AMO might work, if done right...

Comment 9

[Benjamin Smedberg]

unhiding since glazou blogged about this and we want to evangelize it heavily anyway.

Comment 10

[Benjamin Smedberg]

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

[Wladimir Palant]

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

[Wladimir Palant]

(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.

Comment 13

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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

[Wladimir Palant]

I filed bug 477383 with a suggestion since changing the default will clearly not happen.

Comment 15

[Daniel Veditz [:dveditz]]

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

[Wladimir Palant]

(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

[Boris Zbarsky [:bzbarsky]]

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.

Comment 18

[Brian Grinstead [:bgrins]]

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&regexp=false&path=.xhtml and https://searchfox.org/mozilla-central/search?q=createXULElement(%22browser%22&case=false&regexp=false&path=).

Perhaps we should change the default behavior of the [type] checks (like https://searchfox.org/mozilla-central/search?q=TypeAttrName&case=false&regexp=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

[Nika Layzell [:nika] (ni? for response)]

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

[Brian Grinstead [:bgrins]]

(In reply to :Nika Layzell (ni? for response) from comment #19)

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.

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&regexp=false&path=.xhtml)

Comment 21

[Nika Layzell [:nika] (ni? for response)]

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.