Closed Bug 343037 Opened 18 years ago Closed 17 years ago

Prevent themes from executing script with XBL

Categories

(Toolkit Graveyard :: Security, defect)

Product:
Toolkit Graveyard
Component:
Security
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED WONTFIX

People

(Reporter: moco, Assigned: sicking)

References

Details

(Keywords: arch, Whiteboard: [sg:investigate])

Attachments

(3 files)

pseudo-testcase
2.19 KB, patch
Details | Diff | Splinter Review
XBL testing extension
1.75 KB, application/x-xpinstall
Details
Patch to block scripts and plugins
19.32 KB, patch
Details | Diff | Splinter Review 
Do script XBL elements in themes execute?

spun off from bug #342974

assigning to bz, who asked for this bug.
Whiteboard: [sg:investigate]
Did some tests:
 1) scrips in "onfoo" attributes of anonymous nodes are not executed.
 2) scrips in "onfoo" attributes on the <content> element (which is the same as applying the attribute to the XUL element itself) of the binding are executed. :(

pseudo-testcase is coming.
Flags: blocking1.9a1?
Flags: blocking1.8.0.6?
Flags: blocking-firefox2?
Whiteboard: [sg:investigate]
Attached patch pseudo-testcase — — Splinter Review 
The onmouseup event on the <content> element is executed, the <toolbarbutton> one isn't (at least in my 181 tree).

BTW, this binding should go away, I will file a separate bug.
*** Bug 342974 has been marked as a duplicate of this bug. ***
Fwiw, comments 1 and 2 are actually not about this bug.  This bug is about <script> elements....  Event handlers need to be handled quite differently.  I suspect we'll end up with one patch for all, but...

I'm not sure it's worth trying to hunt down every possible place where we execute script and try to patch them all. Currently we have <xul:script>, <html:script>, <svg:script>, <html:object>, <html:embed>, on* on various elements and possibly command handlers. And that's just what I can think of off the top of my head.

I think trying to hunt down each and every place, and keep doing so in the future, to make sure that XBL doesn't execute active content is a loosing battle of whack-a-mole.

Either we should entierly forbid XBL in themes, or we should consider themes as sensitive as extensions and make sure the user understands that.

A possible combination of the two above is that themes can not use XBL, but make it possible to have "theme-extensions" which may contain XBL.
That's another approach, yes.  The other option is to pass more data into the IsScriptEnabled() check -- enough data that we know what DOM node is involved and can then look at binding parents, etc.
Flags: blocking-firefox2? → blocking-firefox2+
Whiteboard: [at risk]
I'm still trying to find time to debug this usefully...  What would really help here is a small extension or something that I could install that would give me the following:

1)  A small XBL file that I can edit as needed to test things.  This needs to be installed as a skin, not content.
2)  A chrome URI that I can pass to -chrome to load a very simple page (a single <hbox> inside a <window> or something) with the XBL from item 1 applied to a node (the <hbox>).

I did try debugging this with the Firefox UI, and that was incredibly painful....  Too much junk around.
Attached file XBL testing extension — 
Something like this?
Oh, and the chrome uri you should use for that is: chrome://xbltest/content/test.xul
Flags: blocking1.8.0.6? → blocking1.8.0.6+
Michael, that's exactly what I needed!

OK, so looks like <script> elements in XBL do not execute for the following reasons:

1)  XUL scripts need an nsXULPrototypeScript to execute, and the XBL content
    sink only creates nsXULPrototypeElements for scripts.  We should probably
    document that this should not change.
2)  HTML and SVG scripts try to execute as soon as they are inserted into the
    XBL document.  This document returns null for GetScriptGlobalObject(), so
    we don't execute the script.  Then we set mEvaluated to true.  When we
    instantiate the binding, we clone, which copies mEvaluated.  So the scripts
    never execute.

These apply to both skin and content bindings.  So things are safe here.

Will look into what's up with the on* attributes next.
OK, here's the story with event listeners.  We pass PR_FALSE for skin packages for the last arg of BindToTree (see nsXBLBinding::InstallAnonymousContent).  This works to prevent attribute event handler compilation for XUL elements (which is what people observed here), but all other element classes completely ignore this argument.  So if I do:

    <content onclick="alert('a')">
      <xul:vbox style="background: red; height: 100px; width: 100px"
                onclick="alert('b')">
        <html:div style="background: yellow; height: 50px; width: 50px"
                  onclick="alert('c');" />
      </xul:vbox>
    </content>

and then click on the yellow area, I get two alerts -- one saying 'c' and one saying 'a'.

Of course the 'a' alert comes about because we just smack those attributes onto mBoundElement...

And sicking is right that there are probably other cases that need considering.

So I think we have three options here (first two are what sicking already mentioned):

1)  Disallow XBL from skin packages (or more precisely from packages that are
    not allowed to run script).
2)  Treat themes as being equivalent to extensions in terms of damage
    potential.
3)  Change all nsIScriptContext methods to take an argument that indicates
    who's asking for the operation to be performed (NOT the principal; the
    actual nsISupports object!).  Change
    nsIScriptSecurityManager::CanExecuteScripts to take this additional
    argument too.  Then hack it to QI to nsIContent, look for a bindingParent,
    get the binding, ask it whether it can execute scripts, etc.  If we take
    this approach, we still need a fix for the <content onclick="whatever">
    issue; we may need to expose IsEventName() on nsIContent or something.  Or
    cast to nsGenericElement..

Doing option 3 on the branch would of course mean adding to those APIs, not changing them.

Thoughts?  Option 3 is probably the most correct one, and the trunk patch won't even be all that bad, I don't think.  But the branch patch will be pretty ugly, and the concept of having more than just the principal decide whether script can run is a little wacky (though not without precedent, since we have this docshell-based disabling thing going on already anyway).
So, killing XBL in skins is not the way we want to go, unless we want to make theming far more limited.  Making themes == extensions means a lot of scary UI when we don't really have to.

3) seems like the best path forward to me, even though it sounds ugly.
Whiteboard: [at risk] → [has analysis, need to decide on right fix]
(In reply to comment #12)
>So, killing XBL in skins is not the way we want to go, unless we want to make
>theming far more limited.
FYI while I was figuring out why my login cookie had expired, I did this query:
http://landfill.mozilla.org/mxr-test/seamonkey/search?string=-binding&find=themes
I think 1 is out, for the reason pointed out by neil, it's simply used too much.

If we can do 3 in a way that we get compile errors if someone forgets to pass in the right context then I think it's the way to go. But I'd rather that we took an nsIContent (or nsINode) if possible?
One problem is that the context would be null in various cases, I suspect.  And yes, we could report errors to console when we deny script evaluation or compilation due to the context.

We could make it an nsIContent, but do we want nsIScriptContext to depend on that?
wouldn't we have to make the implementation depend on it anyway?
The impl depending on it would be nsScriptSecurityManager, which already depends on such things.  The impl of nsIScriptContext itself would just pass through this "context" arg directly to the SSM.
So my problem is that I'm not sure when I'll have time to do something the size of option 3...  In particular, I'm not at all sure I can hit 1.8.0.7 with it...
Jonas - have you had a chance to take a look?
Target Milestone: --- → mozilla1.8.1
Yeah, I'm on this one. We're going to play whack-a-mole for now.
Assignee: bzbarsky → bugmail
Do we want to disable things like <object> <embed> and <applet> in themes as well? Seems like they can do as much damage as scripts.
And what about <iframe>?
I'm trying to figure out how to disable attributes like

   <content onclick="do_evil()">...</content>

from working. The problem is that these attributes are copied to the bound element, and I can't disable scripting on the bound element just because it has theme-xbl attached.

I could simply ignore all attributes whose name start with "on" when the attributes are copied over from the <content> element to the bound element. However i'm worried that this will stop either too much (i.e. non-script related elements whose name happen to start with "on") or too little (attributes that relate to scripting but don't start with "on", such as "src").

Actually, I just realized that it might be possible to create a theme that adds a binding to all <script> elements and set their src attribute to something evil.

So I'm going to just disable this feature of XBL entierly for theme-xbl.
Blocks: 249465
This is a hard problem that's not likely to be solved in the next week for 1.8.0.7, especially given the probable risk of any such change. Moving to next 1.8.0.x release.
Flags: blocking1.8.0.8?
Flags: blocking1.8.0.7-
Flags: blocking1.8.0.7+
Should we just turn off the simple prompt for themes in the meanwhile and force the usual install prompt instead? (easy to do).

Should we inform users so they can be more careful? Or will that alert the bad guys more than it would discourage theme installs?
Whiteboard: [has analysis, need to decide on right fix] → [sg:critical] themes aren't safe. [has analysis, need to decide on right fix]
(In reply to comment #23)

> So I'm going to just disable this feature of XBL entierly for theme-xbl.

How would you usefully override a binding which has attributes on its <content> node. IIRC, once you override the <content> part, the old attributes are gone. Note there are already in-tree use cases. 
Also, tbh, I agree with dveditz that we should simply change the installation UI. I don't think we can fix every possible exploit here without making this feature very limited (and also break backwards-compatibility while we're there). For instance, would you also disable the "allowevents" attribute on anonymous nodes for theme XBL? It (the allowevents attribute) can sure lead to a possible hole in scripts which rely on event.originalTarget, and is still extremely important for the general use case of theme xbl - overriding the toolbarbutton binding.
(In reply to comment #26)
>IIRC, once you override the <content> part, the old attributes are gone.
Worse, the Modern theme wants sidebars to have mini scrollbars so it uses a binding to set the usechromesheets attribute on the appropriate <browser>...
*** Bug 349570 has been marked as a duplicate of this bug. ***
(In reply to comment #29)
> *** Bug 349570 has been marked as a duplicate of this bug. ***

Not so much... my fat fingers did this.
Mano's right; the best way to solve this is to have themes install like other add-ons and use the (ugly) XPI install dialog. Shouldn't require any string changes, so we can do this post-l10n freeze.
--> Mano
Assignee: bugmail → bugs.mano
Status: NEW → ASSIGNED
Priority: -- → P1
Do themes really need to set these attributes, or do they just do it because they can? I have a fairly "done" patch, but I haven't tested it with any themes yet so I don't know what breaks.

I think there are a lot of psychology involved in this, even if we do show the "you're about to install a scary plugin, are you sure you want to" dialog, i think there could be a lot of people out there that expect themes to be safe, and just putting a dialog up there is unlikely to persuade them of otherwise.
(In reply to comment #31)
> Mano's right; the best way to solve this is to have themes install like other
> add-ons and use the (ugly) XPI install dialog. Shouldn't require any string
> changes, so we can do this post-l10n freeze.
> 

Wow, this was already done in bug 304931. I don't see why we should block on this...
Assignee: bugs.mano → bugmail
Status: ASSIGNED → NEW
> Wow, this was already done in bug 304931. I don't see why we should block on
> this...

No, bug 304931 removed the theme exemption from whitelisting. Theme installs are now blocked from non-whitelisted sites, but on whitelisted sites they still get the simple confirm prompt.

To implement my suggestion in comment 25, which sounds like what beltzner wanted (give themes the extension confirm dialog), you'd want to remove the if(SKIN) block here:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/xpinstall/src/nsXPInstallManager.cpp&rev=1.146&mark=286,290#282

Please spin that into a different bug, though. That'd help, but wouldn't counter the assumption of triviality that themes enjoy. I think we do need to neuter themes for good, even if it doesn't make FF2, so I want to keep this bug to cover the original issue.
Thanks, will do.
Filed bug 349603.
unblocking, the Firefox 2 fix will be handled in bug 349603
Flags: blocking-firefox2+ → blocking-firefox2-
"fix"? The Firefox 2 CYA band-aide, you mean. It doesn't "fix" the problem unless we also launch a massive "Themes are extensions!" user education campaign--as unsuccessful as those usually are.

This re-education campaign would have to include, at the very least, mixing themes and extensions in the same management dialog. Not two separate tabs as if they were different, but intermixed in the same list. I imagine that isn't going to be popular. You can judge your own feelings on that idea as to whether even we, knowing about this bug, think of themes as equivalent to full extensions.

It's hard enough convincing people that *extensions* are potentially dangerous installed software, "dangerous themes" is not an idea that's going to catch on until there's one in the wild and all the press are writing about how we got hacked.

One worry I have about bug 349603 is that instead of teaching people to be careful of themes it might instead reinforce the belief that extensions are no big deal. That is, people will learn "extensions are just like themes" rather than the intended "themes are just like extensions".

The solutions Jonas are proposing, and the existing in-tree uses Mano and Neil are countering with, show that this isn't really 1.8 branch material, unfortunately. We're going to break themes but good with this.
Flags: blocking1.9?
I think the real issue we're running into here, again and again, is that XBL simply is not designed with security in mind.  All the security stuff is afterthoughts tacked onto it.  For 1.9 we, imo, need to seriously revisit this setup.
This patch should fix the security issues. However, it may break some themes. I've done some preliminary testing with the default theme and didn't run into any issues, but a lot more testing is needed.


One thing that I don't like about the patch is that i'd like to call DisableAllScriptsInSubtree much earlier, so that I won't have to continiously recheck/recall the method. However it seems like we tear the anonymous content appart when building insertion points, which happens before constructing the nsXBLBinding.

Oh, and I to rev the nsINode iid
I talked with neil, and it seems like themes would indeed break if we disable the ability to put attributes on the <content> element. The example he brought up is that themes set the usechromesheets attribute to change certain aspects of the web page rendering, such as styling of scrollbars.

However there are other mechanisms to style scrollbars, using xulscrollbars.css, so maybe it'd be possible to make the affected themes just use that.

I'm investigating further.
Jonas: please see comment 27, disabling this also makes it impossible to usefully extend a binding which already has attributes on its <content> node.
Well, we have to put some restrictions on theme XBL if we want to keep them safe, that's what this bug is all about.

One thing I thought about was to have a white-list of attributes that bindings would be able to set, however I don't think that that whitelist would include the usechromesheets attribute that was discussed before since I have some real concerns with it. But that might help your issue?
Sorry, in case it wasn't quite clear over IRC, but what the userchromesheets attribute does is to substitute one theme scrollbar CSS file for another.
But couldn't the same thing be accomplished by the theme overriding xulscrollbars.css?
(In reply to comment #46)
>couldn't the same thing be accomplished by the theme overriding xulscrollbars.css?
Sorry, but that question makes no sense; the theme provides xulscrollbars.css
Blocks: 346984
Renominating for Firefox 2. This is a serious problem, and since the fix likely breaks existing themes it's not something that can be rolled into a security update. It needs to be in the original release when themes already have to be updated to take the new features and visual rearrangements into account.
Flags: blocking-firefox2- → blocking-firefox2?
Copying Sheppy, who is working on the "updating your themes to Fx2" docs right now, in fact!
Also, what about onfoo attributes on anonymous nodes? Those doesn't work in theme XBL as far as I can tell; can the <content> element be handled in the same way?
> Those doesn't work in theme XBL as far as I can tell

Only for XUL nodes.  See comment 11.

> can the <content> element be handled in the same way?

Not easily; it would require making changes to all our element classes, to SetAttr, to all future element implementations, etc.  Very fragile; bad security.
So, we have two options here:

* Make themes as safe as we can make them, which is probably whack-a-mole, for the forseeable future for anything 1.8-based, with probable limitations on what themes can do to the UI.  This isn't the first issue with themes being a security risk, and its unlikely to be the last.  I think we want our theme system to be more powerful, rather than less.

* Accept that themes are just another type of add-on, except that "one is always selected" and "options are different and need different UI" and educate users accordingly.

Either we risk ongoing security exposure via whack-a-mole, and probably constrain theme authors in what they can do with the UI, or we make things simple and say that "installing software from the Web should always be treated with caution."  This is only a perceived security hole IF we continue to provide a mixed message about what is safe and what is not on the Web.  In reality, nothing is provably safe, so users should always exercise due caution, whether that addon is a theme, an extension, a plugin, or any other type of add-on we will ever support.
If we decide to play whack-a-mole, let's make sure themes can't modify security dialogs (bug 246375 comment 3 and 4).
(In reply to comment #53)
>If we decide to play whack-a-mole, let's make sure themes can't modify security
>dialogs (bug 246375 comment 3 and 4).
One problem here is that our current widget bindings look to the themes for their appearance, so that a theme could still hide the XPI cancel button.
Given that we don't seem to be confident that we can do both of:
   a) Fix every possible security case
   b) Not savagely break themes 

In the 1.8 time frame and we are taking 349603 that warns users that themes are just as dangerous as add-ons we should not block FF2 release on this.  If we can get a patch in that we are confident of above then we'd take it (we lotsa trunk baking) and we should definitely fix this in 1.9. So I don't want to deter continued efforts to fix this in a holistic way.
Flags: blocking-firefox2? → blocking-firefox2-
Would it be possible to support a class of "themelets" or theme overlays which contain no XBL? In practice most themes are fairly thin modifications (images, colors), they'd be perfectly happy with this restriction if they could be applied "on top" of a standard skin. Currently we make them duplicate all our standard skin XBL and other bits they don't want to change anyway--the bits which could be unsafe if they were changed.

We could make a distinction between this kind of "theme" and our internal "skins", and of course a very small number of people will really want to create a true deep skin to get special effects or more radical changes.

Or maybe it's just easier to launch an education campaign: "Themes are visual extensions", they are as dangerous as extensions (which to many people means not very dangerous).
Could we decide whether we're taking this for 1.8.1 or not?  If not, I need to make sure to get in bug 346984 for 1.8.1....
So brendan and I were just talking about the themelets thing... sicking, what if a theme uses a -moz-binding URI pointing to http:// instead of into the chrome:// of the theme?  Won't that be a binding with script enabled?  So even with this patch themes can execute code, I think; they just need to fetch it from their site...

To do "themelets" safely, we need to basically:

1)  Only allow CSS
2)  Disallow javascript: URIs, -moz-binding.  Anything else?

> 1)  Only allow CSS
> 2)  Disallow javascript: URIs, -moz-binding.  Anything else?

Speaking of which, lots of folk (Six Apart, for one) would be overjoyed if there was a way to turning these off in stylesheets; such a feature would have uses beyond just this one problem.
Ugh, yes, a theme could still point a -moz-binding to a http-url to work around my patch. Bummer.

Could we possibly make it so that themes won't have access to any other urls than other skin urls by adding code to CheckSameOrigin? Or will that not work since we don't do same-origin checks for things like @import.

I'm starting to think that to do this fully right we shouldn't be using chrome: uris for skins at all, but instead something that has lower access than even http
> by adding code to CheckSameOrigin?

You mean to CheckLoadURI?  Yes, we could try.  The problem is that the URI passed to this will often be the document's URI (from the principal).  And of course for XBL we don't know which stylesheet the style data came from, so can only use the document's principal.  :(

> since we don't do same-origin checks for things like @import.

We do CheckLoadURI checks for them.
Err, yes, i mean CheckLoadURI.

I'm not sure themelets is a good idea actually. The problem is that it'll confuse the security picture for users even more. There are some themes that are safe, but not all themes?

Appart from the attribues-on-<content> thing I think we can get this working. With proper CheckLoadURI checks of course.
Neil: I'm still trying to sort out this userchromesheets attribute thing.

Is setting that attribute and pointing it at a theme css file the only way a theme can style the scrollbars? Or can the theme simply provide a stylesheet with a certain name and we'll pick it up that way?
Both; on the Mac, the document viewer loads (presumably from the stylesheet cache) chrome://global/content/nativescrollbars.css while on other platforms it loads chrome://global/content/xulscrollbars.css however the usechromesheets attribute comes in to play should the theme want documents loaded into a certain browser (typically the sidebar) to use a different scrollbar stylesheet.
Ah, now i follow. So one option might simply be to disallow themes from using special scrollbars in selected frames. I'm sure it'd make people sad, but I'm not sure it's a big deal.
Restoring lost blocking flag
Flags: blocking1.8.0.9?
No realistic fix in the 1.8.0.9 timeframe.
Flags: blocking1.8.0.9? → blocking1.8.0.9-
Adding "dev-doc-needed" keyword, since the solution to this will likely involve doc changes, whenever it happens.
Keywords: dev-doc-needed
Flags: blocking1.9a1?
anyone know where progress for this bug is at?   might be a good one for discussion at the security meeting next thursday.
Keywords: arch
jonas, any ideas if progress on the issues here will be made in 1.9?  should the nominiation get plussed?
Yeah, i'll give this one a shot.
Flags: blocking1.9? → blocking1.9+
Since we now give themes the same UI as extensions, we can WONTFIX this bug. I filed bug 387068 on removing the existing attempts at blocking scripts.
Status: NEW → RESOLVED
Closed: 17 years ago
Resolution: --- → WONTFIX
> Since we now give themes the same UI as extensions

Is that true for all Mozilla apps that support themes?  If not, file bugs on the ones that don't do that?  Or at least make a public announcement when we allow themes to run script?
Priority: P1 → --
Summary: Do script XBL elements in themes execute? → Prevent themes from executing script with XBL
Whiteboard: [sg:critical] themes aren't safe. [has analysis, need to decide on right fix]
Target Milestone: mozilla1.8.1 → ---
Removing dev-doc-needed since this is WONTFIX.
Keywords: dev-doc-needed
Whiteboard: [sg:investigate]
Group: core-security
Flags: wanted1.9.0.x-
Flags: wanted1.8.1.x-
Product: Toolkit → Toolkit Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: