This is one of the pieces of bug 863933. Basically, <marquee> exposes _setEventListener, which, when given a string, does |new Function(str)|. When XBL was just content code, this was fine. But now this allows the XBL scope to be trivially hijacked. The fix is simple. I'll post it soon, but we should also audit our other in-content XBL bindings.

I'm assuming this is a regression from bug 821850 (well the bug that flipped it on, but whatever).

tracking as the is a sec-high Fx 21 regression. Our next beta will go-to-build on TUesday and a low risk uplift by then to avoid introducing a known sec-crit regression.

This pattern is actually used extensively in XBL bindings. I'm going to fix it for <marquee>, because I think that's the only one of the affected bindings that runs in content (and therefore in an XBL scope). But I would like somebody who knows more about this (a browser frontend engineer or someone on the security team) to verify this. http://mxr.mozilla.org/mozilla-central/search?string=new+Function&find=xml

I don't see any string-y event handlers from the binding itself, so it seems same to assume that any strings that appear must come from from content. Please double -check that in review though.

Comment on attachment 742716 [details] [diff] [review] Force all string event handlers to be evaluated in the content scope. v1 Review of attachment 742716 [details] [diff] [review]: ----------------------------------------------------------------- I checked for other content-accessible XBL bindings that use this technique and didn't see any others. I also didn't find any places where these strings were coming from a non-content origin. It would be good though to get a review from a layout peer/owner since I'm not one.

(In reply to Bobby Holley (:bholley) from comment #3) > This pattern is actually used extensively in XBL bindings. I'm going to fix > it for <marquee>, because I think that's the only one of the affected > bindings that runs in content (and therefore in an XBL scope). But I would > like somebody who knows more about this (a browser frontend engineer or > someone on the security team) to verify this. What does "runs in content" mean? Or I guess more specifically, what is the definition of "in-content XBL binding"? The other bindings that do this aren't intended to be used from content (and we don't explicitly use them that way), but they are in a contentaccessible="yes" package for the most part. Do we have some other protection against them being bound to content elements? (Testing with chrome://global/content/bindings/dialog.xml, I wasn't able to bind it to a content element using content CSS, and I'm not sure why).

(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #7) > (In reply to Bobby Holley (:bholley) from comment #3) > > This pattern is actually used extensively in XBL bindings. I'm going to fix > > it for <marquee>, because I think that's the only one of the affected > > bindings that runs in content (and therefore in an XBL scope). But I would > > like somebody who knows more about this (a browser frontend engineer or > > someone on the security team) to verify this. > > What does "runs in content" mean? Or I guess more specifically, what is the > definition of "in-content XBL binding"? An XBL binding that can be applied to elements in a non-SystemPrincipaled scope (this triggers the use of an XBL scope with an nsExpandedPrincipal). > The other bindings that do this > aren't intended to be used from content (and we don't explicitly use them > that way), but they are in a contentaccessible="yes" package for the most > part. Do we have some other protection against them being bound to content > elements? (Testing with chrome://global/content/bindings/dialog.xml, I > wasn't able to bind it to a content element using content CSS, and I'm not > sure why). I would have thought you would know the answer to this. I don't, so we should definitely figure out who to ask here. Maybe boris?

Comment on attachment 742716 [details] [diff] [review] Force all string event handlers to be evaluated in the content scope. v1 Hmph. r=me but can we get a spec bug filed to consider adding event handler attributes for onstart/onfinish/onbounce on marquee stuff? Then all this would just happen the normal way in C++ and we could rip out a bunch of this code.

> I wasn't able to bind it to a content element using content CSS, and I'm not sure why When loading XBL bindings we do several security checks: 1) A CheckLoadURI check (more precisely CheckSecurityBeforeLoad, which is a bit stronger in some ways). 2) If the principal of the CSS rule that has the -moz-binding is not system or a chrome:// URL, and the binding url is not data: or chrome://, a check that the bound document is same-origin with the binding URL. 3) If the principal of the CSS rule that has the -moz-binding is not system or a chrome:// URL, then a check that the bound document is allowed to use XUL and XBL. I would think that #3 is what you run into here. So in practice, unless a web site is whitelisted to use XUL/XBL the only bindings that can be applied to its nodes are ones linked from system-privileged stylesheets. But if a site is whitelisted to allow XUL/XBL it can link to any chrome:// bindings in a contentaccessible="yes" package.

Comment on attachment 742716 [details] [diff] [review] Force all string event handlers to be evaluated in the content scope. v1 [Security approval request comment] How easily could an exploit be constructed based on the patch? Somewhat. This patch makes it pretty obvious that it's easy to run arbitrary code in an XBL scope. But you'd then still need to use your control of the XBL scope to do something interesting (which is what bug 863933 does). Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? No more than the patch itself. Which older supported branches are affected by this flaw? XBL scopes exist in m-c, m-a, and m-b. If not all supported branches, which bug introduced the flaw? bug 834697. Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? Should be identical. How likely is this patch to cause regressions; how much testing does it need? Pretty safe, I think. We should get this in ASAP so that it makes Tuesday's beta.

(In reply to Boris Zbarsky (:bz) from comment #9) > Hmph. r=me but can we get a spec bug filed to consider adding event handler > attributes for onstart/onfinish/onbounce on marquee stuff? Then all this > would just happen the normal way in C++ and we could rip out a bunch of this > code. Filed https://www.w3.org/Bugs/Public/show_bug.cgi?id=21871

Comment on attachment 742716 [details] [diff] [review] Force all string event handlers to be evaluated in the content scope. v1 [Approval Request Comment] Bug caused by (feature/regressing bug #): XBL scopes (bug 834697) User impact if declined: potential vector for security issues like bug 863933. Testing completed (on m-c, etc.): Just pushed to m-i. Try is green. Risk to taking this patch (and alternatives if risky): Patch is low-risk. String or IDL/UUID changes made by this patch: None.

Given this is sec-high and we want to take this in FX21 beta which is going to build tomorrow, I am already approving this on branches while this gets merged on m-c in parallel. Please add qawanted,verifyme if this needs any additional QA testing.

https://hg.mozilla.org/releases/mozilla-aurora/rev/ee0b46c56ebe https://hg.mozilla.org/releases/mozilla-beta/rev/dd7008be6bb3

What scope was the XBL evaluated in if not content? This is likely sec-critical rather than sec-high.

(In reply to Daniel Veditz [:dveditz] from comment #19) > What scope was the XBL evaluated in if not content? This is likely > sec-critical rather than sec-high. It runs with in the XBL scope, which is an nsExpandedPrincipal(contentPrincipal). It's somewhere between content and chrome, and there's one of them per DOM scope. Per IRL discussion, I think this is sec-high.

Please see bug 871887.