Closed Bug 911864 (CVE-2014-1479) Opened 11 years ago Closed 11 years ago

Using XBL scopes its possible to steal(clone) native anonymous content.

Categories

(Core :: XBL, defect)

23 Branch
x86_64
Windows 7
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla28
Tracking Status
firefox26 --- wontfix
firefox27 --- fixed
firefox28 --- fixed
firefox-esr24 27+ fixed
b2g18 --- wontfix
b2g-v1.1hd --- wontfix
b2g-v1.2 27+ fixed
b2g-v1.3 --- fixed
b2g-v1.3T --- fixed

People

(Reporter: codycrews00, Assigned: bholley)

References

Details

(Keywords: sec-high, Whiteboard: [uplift needs bug 822425][embargo until b2g18 EOL][qa-][adv-main27+][adv-esr24.3+])

Attachments

(13 files, 1 obsolete file)

1.80 KB, text/html
Details
423.51 KB, text/html
Details
7.61 KB, patch
smaug
: review+
Details | Diff | Splinter Review
4.73 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
11.80 KB, patch
smaug
: review+
Details | Diff | Splinter Review
5.68 KB, patch
smaug
: review+
Details | Diff | Splinter Review
5.90 KB, patch
smaug
: review+
Details | Diff | Splinter Review
4.92 KB, patch
smaug
: review+
Details | Diff | Splinter Review
4.88 KB, patch
smaug
: review+
Details | Diff | Splinter Review
2.19 KB, patch
smaug
: review-
Details | Diff | Splinter Review
16.43 KB, patch
smaug
: review+
Details | Diff | Splinter Review
5.23 KB, patch
smaug
: review+
Details | Diff | Splinter Review
47.11 KB, patch
abillings
: sec-approval+
Details | Diff | Splinter Review
Attached file cloneAnon.html —
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:23.0) Gecko/20100101 Firefox/23.0 (Beta/Release)
Build ID: 20130814063812

Steps to reproduce:

I used an iframe with a data URL to load an XML file to get access to the observe function that xml pretty print bindings provide, I then used this function and some finagling to get fully anonymous content appended as a child of content that was accessible from the XML iframe.  This works because the observe function shares the scope with other anonymous content that might be provided with other bindings.  I believe its actually a cross-compartment wrapper bypass.  The interesting part is that once the anonymous content that is inaccessible is the child of content I can access, that cloneNode(true) clones that content and makes a fully accessible clone.  I cant help but reference my bug here:

https://bugzilla.mozilla.org/show_bug.cgi?id=817922

which points out way back then that this would probably come to pass.  Ill be CC'ing bholley on this one because i mentioned it to him some months back now, I just haven't had time at all lately to focus on security research and I'm even using my day off for labor day weekend to get this public ASAP.  I've also noticed that in the newest nightly these XUL elements I'm cloning have become HTMLDivElements.  I assume that change is to mitigate future problems like this, and maybe in those instances someone wouldn't even realize that they had gained access to native anonymous content.  As I've said I have very little time lately, so please fill in the blanks and correct me on what I'm wrong about here.  Thanks.


Actual results:

Using this technique I was able to clone an XUL element being provided as anonymous content from the pluginProblem.xml bindings.


Expected results:

The reference to the XUL element that can be obtained by using document.getAnonymousNodes/document.getAnonymousElementByAttribute should not allow itself to be appended to the content accessible from the XML iframe.
Attachment #798654 - Attachment mime type: text/plain → text/html
Thought I should mention also that once you see the content from pluginProblem.xml appended to the actual xmliframe data, it loses all styling provided by the chrome style sheets, and the text box for putting in comments that can be expanded in size becomes visible, if you use your mouse and attempt to enlarge it it causes a crash which I believe will turn out to be a null pointer dereference.  I just did a clean install from a full format and I dont have all my sources and debug enviroment setup just yet, so I cant tell you exactly where it takes place.  Ill work on seeing if that can be triggered without the mouse(meaning code generated expansion of the text box without clicks which might need to be trusted.)

If I can trigger it Ill get my dev/debug environment setup and come back to give more specific details. Btw crash occurs even in newest nightly.
Flags: needinfo?(bobbyholley+bmo)
Sorry if this is stupid to ask, but do you need info from me?  Info on the crash?  I've realized that its not actually resizing the text input box that causes the crash but just clicking in it to type does.  I can fire up a dump now that I have my environment setup if you need.
(In reply to codyc from comment #2)
> Sorry if this is stupid to ask, but do you need info from me?  Info on the
> crash?  I've realized that its not actually resizing the text input box that
> causes the crash but just clicking in it to type does.  I can fire up a dump
> now that I have my environment setup if you need.

No, I was flagging it on myself, to make sure that I get back to this today. Looking at it now.
Flags: needinfo?(bobbyholley+bmo)
(In reply to codyc from comment #0)
> I used an iframe with a data URL to load an XML file to get access to the
> observe function that xml pretty print bindings provide, I then used this
> function and some finagling to get fully anonymous content appended as a
> child of content that was accessible from the XML iframe.

The content in the testcase is an <embed> element, with the pluginProblem.xml binding applied. This content is anonymous, but not naturally native-anonymous. We make it act like NAC by setting chromeOnlyContent=true in the pluginProblem.xml binding (something smaug added about a year ago). This is mostly to prevent people from poking around at the plugin warning and performing clickjacking attacks. According to MXR, pluginProblem.xml is our only usage of chromeOnlyContent.

For the most part, SOWs are just designed to prevent pages from rearranging content in ways that the browser doesn't expect. The one place I know of that we care about read-access is with <input type="file">.

An important difference between bonafide NAC and chromeOnlyContent is that the latter is returned by document.getAnonymousNodes (with a SOW around it), whereas NAC is not. This means that the testcase in this bug, which relies on document.getAnonymousNodes, won't work for NAC like <file> and <video>. Cody, can you think of a way to make this testcase work if we can't call getAnonymousNodes on the bound element? If I switch the <embed> to <input type="file">, We currently throw in the first observe() call in finishClone(). 

As long as this attack only works against chromeOnlyContent, I don't think it's a security problem. If it works against NAC, then we'd consider that a security bug, though I don't know how severe. Sec-moderate maybe?

> This works
> because the observe function shares the scope with other anonymous content
> that might be provided with other bindings.  I believe its actually a
> cross-compartment wrapper bypass.

Explain this more?

> The interesting part is that once the
> anonymous content that is inaccessible is the child of content I can access,
> that cloneNode(true) clones that content and makes a fully accessible clone.

If this were true for <file>, that would indeed be a problem.

> I've also noticed that in the newest nightly these XUL elements I'm cloning
> have become HTMLDivElements.  I assume that change is to mitigate future
> problems like this

This is because pluginProblem itself switched from XUL elements to HTML elements in bug 888510. I'm pretty sure the reasons were orthogonal.

> and maybe in those instances someone wouldn't even
> realize that they had gained access to native anonymous content.

I think anyone poking this deeply wouldn't be so easily fooled. ;-)

Blake, can you confirm that my analysis here makes sense?
Flags: needinfo?(mrbkap)
Flagging NEEDINFO from cody to see if we can make this testcase work for <file>.
Flags: needinfo?(codycrews00)
(In reply to Bobby Holley (:bholley) from comment #4)
> > This works
> > because the observe function shares the scope with other anonymous content
> > that might be provided with other bindings.  I believe its actually a
> > cross-compartment wrapper bypass.
> 
> Explain this more?

Correct me if I'm wrong, but during the major XBL rework that you powered through some time back all XBL code runs in a separate compartment right?  When gaining access to any content provided by XBL bindings you are essentially provided with a SOW in place of that content.  The main point here is that all functions accessible by content through the binding names available as windows properties can bypass SOWs to an extent and see the original content, but they are limited in the fact that the function will only do certain actions with arguments passed in, or a new |this| through binding that function elsewhere.  It is the fact that these functions see the original content that allows the XUL element to be appended to the anonymous content of the iframe document in the first place, and this is what I was referring to as a cross-compartment wrapper bypass.

> > The interesting part is that once the
> > anonymous content that is inaccessible is the child of content I can access,
> > that cloneNode(true) clones that content and makes a fully accessible clone.
> 
> If this were true for <file>, that would indeed be a problem.

I noticed this problem some months back and didn't fully follow up on what all can be done because it always depends on the predefined functions provided by XBL, Ill take some time and dig deep to see if I can make it work with file, or even gain access to the video controls again(my OCD makes me want to do this anyway for some reason.)  Ill start off tonight, and see what I can come up with by tomorrow evening.  Is there a list of elements, and the bindings they trigger when added to a document somewhere so I could get a head start on this?  If not its fine, I have a fairly good idea already I'm just probably missing a couple at least.

A quick question, I've noticed while using the DOM inspector that content appended to anonymous content appears as anonymous content afterwards, would that content then be obtainable by getAnonymousNodes/getAnonymousElementByAttribute ?
(In reply to codyc from comment #6)
> Correct me if I'm wrong, but during the major XBL rework that you powered
> through some time back all XBL code runs in a separate compartment right?

All in-content XBL, correct. Chrome XBL (and remote XUL XBL) runs same-compartment.

> When gaining access to any content provided by XBL bindings you are
> essentially provided with a SOW in place of that content.

Not entirely. You can access anonymous content. This restriction applies only to _native_ anonymous content and chromeAccessOnly.

>  The main point
> here is that all functions accessible by content through the binding names
> available as windows properties can bypass SOWs to an extent and see the
> original content, but they are limited in the fact that the function will
> only do certain actions with arguments passed in, or a new |this| through
> binding that function elsewhere.  It is the fact that these functions see
> the original content that allows the XUL element to be appended to the
> anonymous content of the iframe document in the first place, and this is
> what I was referring to as a cross-compartment wrapper bypass.

I see. I think "SOW bypass" might be a more appropriate way to refer to it, but it's all semantics.

> > > The interesting part is that once the
> > > anonymous content that is inaccessible is the child of content I can access,
> > > that cloneNode(true) clones that content and makes a fully accessible clone.
> > 
> > If this were true for <file>, that would indeed be a problem.
> 
> I noticed this problem some months back and didn't fully follow up on what
> all can be done because it always depends on the predefined functions
> provided by XBL, Ill take some time and dig deep to see if I can make it
> work with file, or even gain access to the video controls again(my OCD makes
> me want to do this anyway for some reason.)  Ill start off tonight, and see
> what I can come up with by tomorrow evening.  Is there a list of elements,
> and the bindings they trigger when added to a document somewhere so I could
> get a head start on this?  If not its fine, I have a fairly good idea
> already I'm just probably missing a couple at least.

The element-bound ones are in layout/style/html.css. Basically just video, audio, and marquee. There's also pluginProblem, and things like scale.xml (used by other stuff like video). XML pretty printers is another case that I hadn't thought of until you found it (very clever!).

As for NAC, have a look at things that implement nsIAnonymousContentCreator: http://mxr.mozilla.org/mozilla-central/search?string=CreateAnonymousContent 

> A quick question, I've noticed while using the DOM inspector that content
> appended to anonymous content appears as anonymous content afterwards

Anything descending from an anonymous root is itself anonymous.

> , would
> that content then be obtainable by
> getAnonymousNodes/getAnonymousElementByAttribute ?

I think so, but I'm not sure. Blake would know.
(In reply to Bobby Holley (:bholley) from comment #4)
> Blake, can you confirm that my analysis here makes sense?

I think your analysis does make sense.

One thing to keep in mind here is that native anonymous content actually has two uses: for "true" *native* anonymous content, we've always tried to prevent content from messing with it for fear that the C++ code that created it would get confused and end up doing something exploitable.

When we implemented <audio> and <video>, we wanted to a) implement the controls in XBL and JavaScript and b) hide the implementation from the web page in order to not accidentally create a second API that web authors would use instead of the standard one. In order to accomplish that, we used regular XBL bindings and then set the "native" bit on them (this is roughly equivalent to "chrome only" bindings, I think).

In the first case, being able to change the shadow DOM would be considered an sg:crit, whereas in the second, we're less worried about exploits and more worried about polluting the web platform. It seems that, for the moment, this bug is only about the second case.

(In reply to codyc from comment #6)
> A quick question, I've noticed while using the DOM inspector that content
> appended to anonymous content appears as anonymous content afterwards, would
> that content then be obtainable by
> getAnonymousNodes/getAnonymousElementByAttribute ?

Yes. The state of being "anonymous content" is, in part, inherited from an element's parentNode. The one exception is the root of an anonymous subtree, and we don't allow those to be reparented or moved in the DOM.
Flags: needinfo?(mrbkap)
Bobby, what sort of sec rating should this get?  If you can tell yet.
Component: Untriaged → XBL
Product: Firefox → Core
(In reply to Blake Kaplan (:mrbkap) [PTO Sep 5-Sep 19] from comment #8)
> When we implemented <audio> and <video>, we wanted to a) implement the
> controls in XBL and JavaScript and b) hide the implementation from the web
> page in order to not accidentally create a second API that web authors would
> use instead of the standard one. In order to accomplish that, we used
> regular XBL bindings and then set the "native" bit on them (this is roughly
> equivalent to "chrome only" bindings, I think).

So, this is what I thought, but smaug was adamant in a thread in December that this wasn't strictly accurate.

> Nothing artificial there. videocontrols lives under NAC, and NAC propagates to descendants.
> That also ensures sane event handling.
> And scrollbars use XBL and have lived in NAC forever.
> ...
> I don't understand where you get this "artificial".

Indeed, I don't see anywhere that we actually set the bit for video controls. Is there one that I'm missing? If not, this seems to be the primary distinction here. <video> AC gets SOWs because it lives underneath a piece of bonafide NAC (which incidentally also prevents it from being returned by document.getAnonymousNodes). pluginProblem gets its SOWs from the explicit "chromeOnlyContent" attribute, which doesn't preclude accessing it from getAnonymousNodes. We could fix that, but it's not a major security issue AFAICT. Probably better to just land bug 912322 and embargo this bug.

> In the first case, being able to change the shadow DOM would be considered
> an sg:crit, whereas in the second, we're less worried about exploits and
> more worried about polluting the web platform. It seems that, for the
> moment, this bug is only about the second case.

No, this bug is more about pluginProblem, which we can't allow content to change due to the threat of clickjacking. <video> is neither involved nor affected, since it uses NAC-inheritance instead of chromeOnlyContent. Modifying pluginProblem AC would indeed be a problem, but AFAICT the exploit here only allows it to be read/cloned.
Flags: needinfo?(mrbkap)
(In reply to Andrew McCreight [:mccr8] from comment #9)
> Bobby, what sort of sec rating should this get?  If you can tell yet.

sec-other for now.
Keywords: sec-other
Just a quick update, nothing worth putting here as of now, but there's some very promising things that I'm running into while digging into this.  The focus has currently shifted away from file input elements for the moment but I plan to come full circle back around to that before I'm done.  Some very tricky things can be achieved with the pieces I have already, now If only I could get more hours in the day.
Thought I would come back and touch on this while it was on my mind.  I've been digging deep like I said, and I'm certain that I can accomplish click-jacking on content provided by pluginProblem.xml already at this momemnt.  I'm still working on ways to confuse and maybe trick getAnonymousNodes/getAnonymousElementsByAttribute to allow bypassing the true NAC that prevents the subsequent anonymous children from being returned.  Also I'm currently able to obtain a reference to (or even modify the original) iframe that is provided by embeds say in the case of <embed src='data:application/pdf,'/> where the embed allows to preview the plugin content.  Also using functions provided by the marquee bindings its possible to call addEventListener on content protected by SOWs and then even dispatch a corresponding event to those elements to trigger the code added.  That part has limitations I'm working on understanding, but I'm trying to keep this short.  This all has led me down the path of being able to inject certain elements into rss feeds, and I know that since firefox prevents the inheriting of principals its probably going to be unimpressive/unproductive overall but its still interesting.  My main concern and the reason I'm putting all this out there is I have the problem of time again, even when I have time to do this research I want to spend it more on getting my C++ up to a level at least close to a lot of you, so I can start actually contributing to development some too.  This is all part of bigger things I'm trying to build up to putting together in the future, and I have decent C skills mainly when using command line driven apps and have never spent much time indulging in the win API to give nice GUI experiences.  I sidetracked myself there, the point is that I'm gonna be putting out updated info and maybe even small test cases to show off the problems I noticed, and anytime I receive 'undefined behavior', because I'm sure someone with a more in depth knowledge of all this could have already done something really nasty with what I've found so far.  Bottom line I don't know how much work it would take but we are absolutely going to have to get rid of these binding names as windows properties.  A good example is an rss feed, bindings are added and provided through the windows properties that are never seen in regular content, which is why I've been trying to get some script injection into a feed to somehow pass a reference of those bindings to a regular content window, the more binding names you can access the more malicious stuff you can do bottom line.  I feel I'm very close to being able to manually set the value property of a file input element now, just going to take a little more finagling and time.  I believe this will end up being multiple issues by the time it all plays out and I'm beginning to focus on the wrapper generation too because multiple times lately I have been able to somehow make one of these cloned XUL elements become its XRAY equivalent even at content level while still maintaining access to its properties functions and attributes but it happens randomly usually when switching a clone of an XUL element between being an anonymous child of anonymous content, and actually just appending that element to a regular HTML content document.

So much for keeping that short, just so many issues and I don't actually collaborate with you all as I should which I'm going to start working on.  Ill post a few testcases by tomorrow evening showing click-jacking, the firing of events, modifying the src attribute of an anonymous SOW protected iframe, and maybe if I'm lucky some script injection into an rss feed but even if I get that getting anything I could gain with that to a regular content level window would be a trick in itself.  Well, that's it until tomorrow evening.
This grabs the scale/slider from the audio controls as it plays.  document.getBindingParent(anonScale) will then return the video controls. The scale is not protected by SOW protections as it should be(no idea why.)  It's also worth noting that even though the video controls should not be able to be cloned using the observe function from XMLPrettyPrint.xml bindings, with a little work it can be.
Attachment #804928 - Attachment mime type: text/plain → text/html
Attachment #804928 - Attachment description: Accessing true native anonymous content. → Accessing true native anonymous content. EDIT: should be true(er) I guess since taking better note of comment 8
(In reply to Bobby Holley (:bholley) from comment #10)
> So, this is what I thought, but smaug was adamant in a thread in December
> that this wasn't strictly accurate.

Not accurate in what way?

> Indeed, I don't see anywhere that we actually set the bit for video
> controls. Is there one that I'm missing? If not, this seems to be the

It's complicated. nsVideoFrame implements nsIAnonymousContentCreator, so when we create a video frame, we go through and mark all of the anonymous content as "native anonymous". See <http://mxr.mozilla.org/mozilla-central/source/layout/base/nsCSSFrameConstructor.cpp?rev=1a895d95c88b#3678> for the gory details. One thing I'm not sure of off the top of my head is how this interacts with the binding in html.css. Is it possible that we actually create *two* sets of anonymous content?

> issue AFAICT. Probably better to just land bug 912322 and embargo this bug.

It seems like bug 912322 landed and cody's still getting his hands on unwrapped anonymous content?

> Modifying pluginProblem AC would indeed be a problem, but AFAICT the exploit
> here only allows it to be read/cloned.

That seems all right, then?
Flags: needinfo?(mrbkap)
Ill touch on this tomorrow, my mind is all over the place tonight so sleep it is.  I will say though that after bug 912322 landed there's still a hole there by remapping the valueChanged function, and using the boxObject API once you have access to the anonymous content you can probably get to everything else.
Ill be putting off going over this possibly, as a new bug has came to my attention that is definitely going to take priority over anything like this.  Right now with it im already able to load a window to the 'chrome://browser/content/browser.xul' from content, and gain a reference to that window.  The window isnt rendered, but does exist and now I'm just going to be working on either finding a way to actually access some of its properties from the reference or go a completely different route and maybe try to load up a data URI hoping it will retain the chrome privileges and use that to access Components, and actually do a proper launch calc.exe poc.
For those interested the previously mentioned bug is up at bug 920515.  I'll CC anyone on this one, and soon I'll get back to properly following up on this.  Just one error showing up in console led me there, and I've noticed many other things.  There's also that crash that I mentioned from this bug, and I've noticed that the base pointer when trying to call a virtual function ends up null and causes the crash and my assembly is bad but that worries me, I found a couple of references to controlling ebp when instances of classes are trying to call virtual functions and having them instead call a malicious attacker supplied function.  I haven't forgotten any of these issues I've mentioned here, but there really is so many of them and I have so little time that it's a bit overwhelming.
I'm back to my normal schedule as of now, but I expect some time to get back around to this later this week/this coming weekend.  Sorry for taking so long.
Take your time.  Thanks for looking into this.
So, I'm pretty sure we should just mostly stop exporting XBL implementations methods/properties to content scopes. I'm going to prototype a whitelist-based implementation.
Assignee: nobody → bobbyholley+bmo
Depends on: 822425
(In reply to codyc from comment #22)
> (In reply to Bobby Holley (:bholley) from comment #21)
Uh oh, that sounds like a very different (and potentially dangerous) bug. Please file it and CC me.

In general, it's better not to talk about multiple security issues in the same bug, because we'll open this bug open when the fix lands on all branches, and then we might accidentally expose unpatched issues. I've marked your comment as 'private' to make sure we're safe.
Ah appreciate you taking care of that for me man, Ill be more careful in the future.  I can see your proposed fix taking care of that too, because you need an XULElement to apply the proper class to trigger the bindings and once there is no longer access to remap functions on binding names obtaining an XULElement at content level will be tricky.  Ill put together a few simple testcases here shortly, I have to admit though that I haven't been doing the most recent testing in nightly, so Ill see how they hold up there, but I think the issue will still be there. Ill be in touch.
This is green on linux64 modulo one test that I fixed. Uploading patches and flagging for review.
We also remove test_menubar_gtk.xul, which is unreferenced by the build system
and references a file that doesn't exist in the tree.
Attachment #818433 - Flags: review?(bugs)
This will no longer be the default after the ensuing patch. And while this
fixup doesn't seem necessary to make the tests pass, I'm concerned that this
could cause weirdness for someone using synthesizeMouseEvent (which creates
untrusted events) if the event somehow got bounced to the video NAC. I've
audited these handlers and they're fine to expose.
Attachment #818441 - Flags: review?(bugs)
Even though a precise exploit was never attached, I think we should pay cody some sort of bounty for demonstrating that our protections from bug 912322 were insufficient in a number of ways.
Flags: needinfo?(codycrews00) → sec-bounty?
Comment on attachment 818434 [details] [diff] [review]
Part 2 - Expose an API to copy a single property across compartments. v1

Review of attachment 818434 [details] [diff] [review]:
-----------------------------------------------------------------

r=me, but with lots of comments to address.

Pre-existing scariness: I had forgotten this API existed. It's moderately-to-severely scary to me that we copy properties that have JSPropertyOp getters and setters. Do you think we could possibly:

  - only copy properties between objects that have the same JSClass; or

  - refuse to copy properties with JSPropertyOp getters/setters, with or without an error; or

  - some combination of the two,

enforced by run-time checks?

If so, please do. If not, what's the particular use case that needs us to do that?

::: js/src/jsfriendapi.h
@@ +176,5 @@
> +/*
> + * Copies all |own| properties from |obj| to |target|.
> + *
> + * On entry, |cx| should be in the compartment of |target|.
> + */

Thanks for backfilling comments!

Nits: s/should/must/ and I think remove the ||s around "own".

::: js/src/jsobj.cpp
@@ +1755,5 @@
> +static bool
> +CopyProperty(JSContext *cx, HandleObject target, HandleObject obj,
> +             HandleShape shape)
> +{
> +    // NB: |shape| is generally not same-compartment here.

|shape| and |obj| both.

Micro-nit: I think "NB:" is a little obscure; I'd go with "Note:" or just remove it. But there's a shade of meaning there, so I won't insist.

@@ +1783,5 @@
> +    assertSameCompartment(cx, target);
> +    RootedObject obj2(cx);
> +    RootedShape shape(cx);
> +    {
> +        JSAutoCompartment ac(cx, obj);

Inside js/src, use AutoCompartment.

@@ +1788,5 @@
> +        if (!JSObject::lookupGeneric(cx, obj, id, &obj2, &shape))
> +            return false;
> +    }
> +    MOZ_ASSERT(shape && obj == obj2);
> +    return CopyProperty(cx, target, obj, shape);

How perf-sensitive is this? Instead of (or in addition to) asserting, can we check and report an error here if the asserted condition is false?

It is only safe to use |shape| if |obj| is native. Either MOZ_ASSERT(obj->isNative()) or change the behavior. Depending on your goals here it might be better to do:
  - js::GetOwnPropertyDescriptor() to populate a PropertyDescriptor
  - the rewrapping
  - js::DefineOwnProperty() using the signature that takes a PropertyDescriptor

lookupGeneric consults the prototype chain if there's no own property obj[id], and that's observable by proxies on the prototype chain. GetOwnPropertyDescriptor avoids that. It also works even if obj is non-native.

@@ +1801,5 @@
>  
>      // If we're not native, then we cannot copy properties.
>      JS_ASSERT(target->isNative() == obj->isNative());
>      if (!target->isNative())
>          return true;

This is another place where I'd be happier checking both at runtime even in non-debug builds.
Attachment #818434 - Flags: review?(jorendorff) → review+
(In reply to Jason Orendorff [:jorendorff] from comment #39)
> Pre-existing scariness: I had forgotten this API existed. It's
> moderately-to-severely scary to me that we copy properties that have
> JSPropertyOp getters and setters. Do you think we could possibly:
> 
>   - only copy properties between objects that have the same JSClass;

We can't do that, because when transplanting reflectors, we temporarily copy properties onto a property holder object whose JSClass doesn't match that of the reflector.

>   - refuse to copy properties with JSPropertyOp getters/setters, with or
> without an error

I think we could do this. JSPropertyOp accessor props appear as value props to script, so there's no way that script could define them on an unexpecting object, right?

This will alter behavior when transplanting anything that has a resolve hook that defines a JSPropertyOp accessor on the reflector - we'll need to skip silently in those cases. This should be fine, since the resolve hook will presumably just re-run in the target compartment.

Can we do this in a followup bug though? This stack of patches is already risky enough as-is.
 
> @@ +1788,5 @@
> > +        if (!JSObject::lookupGeneric(cx, obj, id, &obj2, &shape))
> > +            return false;
> > +    }
> > +    MOZ_ASSERT(shape && obj == obj2);
> > +    return CopyProperty(cx, target, obj, shape);
> 
> How perf-sensitive is this? Instead of (or in addition to) asserting, can we
> check and report an error here if the asserted condition is false?

I'd really rather not - in my mind, the only use cases for this function happen when the caller is absolutely sure that the property exists. If it doesn't, that's a logic bug in the caller that we should find out about sooner rather than later. I don't see any more value in handling this condition than in handling the case in which the compartment invariants are not what we expect. We should just crash early.

> 
> It is only safe to use |shape| if |obj| is native. Either
> MOZ_ASSERT(obj->isNative()) or change the behavior. Depending on your goals
> here it might be better to do:
>   - js::GetOwnPropertyDescriptor() to populate a PropertyDescriptor
>   - the rewrapping
>   - js::DefineOwnProperty() using the signature that takes a
> PropertyDescriptor
> 
> lookupGeneric consults the prototype chain if there's no own property
> obj[id], and that's observable by proxies on the prototype chain.
> GetOwnPropertyDescriptor avoids that. It also works even if obj is
> non-native.

I will MOZ_ASSERT(obj->isNative()) for the new API, and continue no-oping in the old API for non-native objects. I'll do the GetOwnPropertyDescriptor thing in the followup.
Blocks: 928616
Filed bug 928616 for the followup work.
(In reply to Bobby Holley (:bholley) from comment #40)
> Can we do this in a followup bug though? This stack of patches is already
> risky enough as-is.

Sure.

> > > +    MOZ_ASSERT(shape && obj == obj2);
> > 
> > How perf-sensitive is this? Instead of (or in addition to) asserting, can we
> > check and report an error here if the asserted condition is false?
> 
> I'd really rather not - in my mind, the only use cases for this function
> happen when the caller is absolutely sure that the property exists. If it
> doesn't, that's a logic bug in the caller [...] We should just crash early.

Great. My concern is about letting it pass in opt builds.

You know better than I do how nervous to be about these invariants. To me they seem security-sensitive, and rather custom-purpose in terms of API design, and thus possibly error-prone for users. We could check the asserted preconditions even in opt builds, and MOZ_CRASH() if it fails. Your call.
Comment on attachment 818435 [details] [diff] [review]
Part 3 - Introduce an explicit opt-in attribute for exposing methods and properties to untrusted content. v1

>+    else if (localName == nsGkAtoms::exposeToUntrustedContent) {
>+      exposeToUntrustedContent = true;
>+    }
We should explicitly test that the value is true.
Otherwise someone may put exposeToUntrustedContent="false" and assume the property is not exposed

> 
>   const PRUnichar* name = nullptr;
>+  const PRUnichar* ignored = nullptr;
>   if (FindValue(aAtts, nsGkAtoms::name, &name)) {
>     mMethod = new nsXBLProtoImplMethod(name);
>+    if (FindValue(aAtts, nsGkAtoms::exposeToUntrustedContent, &ignored)) {
>+      mMethod->SetExposeToUntrustedContent(true);
>+    }
Please check the value also here.

>-  nsXBLProtoImplMember(const PRUnichar* aName) :mNext(nullptr) { mName = ToNewUnicode(nsDependentString(aName)); }
>+  nsXBLProtoImplMember(const PRUnichar* aName)
>+    : mNext(nullptr)
>+    , mExposeToUntrustedContent(false)
>+  { mName = ToNewUnicode(nsDependentString(aName)); }
    {
      mName = ToNewUnicode(nsDependentString(aName));
    }
Attachment #818435 - Flags: review?(bugs) → review-
Attachment #818437 - Flags: review?(bugs) → review+
Attachment #818438 - Flags: review?(bugs) → review+
Comment on attachment 818441 [details] [diff] [review]
Part 9 - Explicitly mark <video> mouse event handlers as allowuntrusted="true". v1

Well, we shouldn't have ever exposed these listeners to untrusted events.
Are you worried about some web page actually sending synthetic
events to video?
Sounds very unlikely.
Attachment #818441 - Flags: review?(bugs) → review-
Comment on attachment 818433 [details] [diff] [review]
Part 1 - Convert toolkit widget xul tests to chrome tests. v1

I think chrome.ini is ok, but I'm not familiar with those .ini files.
Attachment #818433 - Flags: review?(bugs) → review+
Attachment #818440 - Flags: review?(bugs) → review+
Comment on attachment 818436 [details] [diff] [review]
Part 4 - Install XBL members in the safe scope, then clone them into content (rather than the reverse). v1

funciton

(but splitting the patch to so many pieces makes it hard to review. I'll continue tomorrow.)
Attaching a rollup patch for smaug.
Comment on attachment 820962 [details] [diff] [review]
Part 3 - Introduce an explicit opt-in attribute for exposing methods and properties to untrusted content. v2

I would use
nsDependentString(aAtts[1]).EqualsLiteral("true")
and similar also in the other case.
Attachment #820962 - Flags: review?(bugs) → review+
Status: UNCONFIRMED → NEW
Ever confirmed: true
Comment on attachment 818436 [details] [diff] [review]
Part 4 - Install XBL members in the safe scope, then clone them into content (rather than the reverse). v1


>   JS::Rooted<JSObject*> targetScriptObject(cx, holder->GetJSObject());
> 
>-  JSAutoCompartment ac(cx, targetClassObject);
>+  // We want to define the canonical set of members in a safe place. If we're
>+  // using a separate XBL scope, we want to define them there first (so that
>+  // they'll be available for Xray lookups, among other things), and then copy
>+  // the properties to the content-side proptotype as needed. We don't need to
proptotype?

>+  // bother about the field accessors here, since we don't use/support those
>+  // for in-content bindings.
I don't understand this comment. Why don't support xbl fields? How do we handle them?

>+  // Walk our member list and install each one in turn on the XBL scope object.
>+  for (nsXBLProtoImplMember* curr = mMembers;
>+       curr;
>+       curr = curr->GetNext())
>+    curr->InstallMember(cx, propertyHolder);


>+  // From here on out, work in the scope of the bound element.
>+  JSAutoCompartment ac2(cx, targetClassObject);
>+
>+  // Now, if we're using a separate XBL scope, enter the compartment of the
>+  // bound node and copy the properties to the prototype there. This rewraps
>+  // them appropriately, which should result in cross-compartment funciton
function

>   // Install all of our field accessors.
>   for (nsXBLProtoImplField* curr = mFields;
>        curr;
>        curr = curr->GetNext())
... here we anyway deal with all the fields

The setup looks a bit hard to follow. First we enter to xbl scope, install members, enter to bound element scope copy members
and then for each field enter to xbl scope again and install it.

Could we not install all the stuff first and then do the possible non-xbl-scope copying/field setter/getter setup
I think what would make the setup easier to follow.
Feel free to disagree (with some explanation) and re-ask review
Attachment #818436 - Flags: review?(bugs) → review-
(In reply to Olli Pettay [:smaug] from comment #50)
> I don't understand this comment. Why don't support xbl fields? How do we
> handle them?

We kinda support them, but not really. They're just data properties on the reflector, so for the in-content case (where we use XBL scopes), they're invisible to the XBL without waiving the Xray wrapper. I weaned our use cases off of them in bug 848939.


> and then for each field enter to xbl scope again and install it.

No, fields are installed in the bound scope.

The issue is that fields need to be installed on the bound node, and members need to be installed in the XBL compartment, because, in later patches, we only copy _some_ of them over to the bound scope (but leave them all available to the XBL scope).

The eventual ordering here (see the rollup) is:

1 - Install members in XBL scope
2 - Copy exposed members into content scope
3 - Install fields in the content scope

We could interchange the order of 2 and 3, but I'm not convinced it would be better.
Attachment #818436 - Flags: review- → review?(bugs)
Comment on attachment 818443 [details] [diff] [review]
Part 10 - Make in-content XBL event handlers allowuntrusted=false by default. v1


>+
>+  if (!mBoundElement)
>+    return;
I know XBL code isn't consistent, but {} with if, please
Attachment #818443 - Flags: review?(bugs) → review+
> The eventual ordering here (see the rollup) is:
> 
> 1 - Install members in XBL scope
> 2 - Copy exposed members into content scope
> 3 - Install fields in the content scope
I don't see that in the patch - the patch doesn't change nsXBLProtoImplField::InstallAccessors.
But yeah, indeed we install in bound scope but create functions in xbl scope.
Could we do that all with less compartment entering. First in xbl scope do whatever we need to
and then enter to bound element scope and do there what we need to.
(performance should be better and hopefully also code readability.)
Attachment #818439 - Flags: review?(bugs) → review+
Comment on attachment 818436 [details] [diff] [review]
Part 4 - Install XBL members in the safe scope, then clone them into content (rather than the reverse). v1

I guess my complain isn't really about this bug but the setup is too
complicated even without this patch.
Could you file a followup to clean up this stuff.
Attachment #818436 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] from comment #53)
> > 1 - Install members in XBL scope
> > 2 - Copy exposed members into content scope
> > 3 - Install fields in the content scope
> I don't see that in the patch - the patch doesn't change
> nsXBLProtoImplField::InstallAccessors.

Because fields have always been installed in the bound scope, so there's nothing to change.

> Could we do that all with less compartment entering. First in xbl scope do
> whatever we need to
> and then enter to bound element scope and do there what we need to.

That's exactly what the world looks like after these patches.

(In reply to Olli Pettay [:smaug] from comment #54)
> I guess my complain isn't really about this bug but the setup is too
> complicated even without this patch.
> Could you file a followup to clean up this stuff.

This setup is complicated, for sure. But it's solving a very complicated problem, and it's not obvious to me how to make it simpler. If you have any suggestions (or patches) to improve it, I'd certainly welcome them.
Comment on attachment 820963 [details] [diff] [review]
rollup (for context - don't review)

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

Not easily. This just locks down a lot of XBL stuff on an architectural level.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

I don't think so.

Which older supported branches are affected by this flaw?

All.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

Depending on the branch, backports may be tricky. But ESR24 is probably doable.

How likely is this patch to cause regressions; how much testing does it need?

This has a high probability of breaking in-content XBL bindings. There aren't many of those that we know of, but we should still be careful.

I recommend landing this on m-c immediately. If there are no regressions within 10 days, we should uplift to aurora. After the next merge, we should then uplift to ESR24, so that this hits release on all channels simultaneously.
Attachment #820963 - Flags: sec-approval?
This is a sec-other? Is it really rated correctly as that?
(In reply to Al Billings [:abillings] from comment #59)
> This is a sec-other? Is it really rated correctly as that?

Yeah, it's hard to say. There aren't explicit testcases in this bug that I'm worried about, but the issue these patches solve is a general issue that I'm worried about. I'm going to very hand-wavily label this sec-high.
Keywords: sec-othersec-high
Comment on attachment 820963 [details] [diff] [review]
rollup (for context - don't review)

sec-approval+.

What about taking it on Beta after Aurora? We've already branched.

I am concerned about landing tests.
Attachment #820963 - Flags: sec-approval? → sec-approval+
(In reply to Al Billings [:abillings] from comment #61)
> Comment on attachment 820963 [details] [diff] [review]
> What about taking it on Beta after Aurora? We've already branched.

Well, it's a somewhat risky change, hence my recommendation at the bottom of comment 57. I wouldn't put up the fight if the release drivers want it for beta though.

I'll nominate this for aurora in 1 week.

> I am concerned about landing tests.

These tests don't reveal anything interesting security-wise. They just verify the behavior that these patches very obviously implement (that bindings must explicitly annotate any member that they wish to expose to content).
All right, land the tests. I think we'll probably skip Beta.
Whiteboard: [uplift needs bug 822425]
Flags: sec-bounty? → sec-bounty+
Comment on attachment 818443 [details] [diff] [review]
Part 10 - Make in-content XBL event handlers allowuntrusted=false by default. v1

It's been at least a week without any reported regressions. Let's get this on aurora. Next cycle, we'll get this onto esr.

See comment 57 for analysis.
Attachment #818443 - Flags: approval-mozilla-aurora?
What about b2g26?
(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #69)
> What about b2g26?

Yeah, we should it on there as well.
(In reply to Bobby Holley (:bholley) from comment #70)
> (In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #69)
> > What about b2g26?
> 
> Yeah, we should it on there as well.

Would it be easier from a partner standpoint to do that this cycle as opposed to next cycle?
(In reply to Bobby Holley (:bholley) from comment #57)
> Comment on attachment 820963 [details] [diff] [review]
> rollup (for context - don't review)
> 
> [Security approval request comment]
> How easily could an exploit be constructed based on the patch?
> 
> Not easily. This just locks down a lot of XBL stuff on an architectural
> level.
> 
> Do comments in the patch, the check-in comment, or tests included in the
> patch paint a bulls-eye on the security problem?
> 
> I don't think so.
> 
> Which older supported branches are affected by this flaw?
> 
> All.
> 
> Do you have backports for the affected branches? If not, how different, hard
> to create, and risky will they be?
> 
> Depending on the branch, backports may be tricky. But ESR24 is probably
> doable.
> 
> How likely is this patch to cause regressions; how much testing does it need?
> 
> This has a high probability of breaking in-content XBL bindings. There
> aren't many of those that we know of, but we should still be careful.

Is this something that QA can help with ? I am adding mwobensmith as a QA contact here, will be helpful if you can discuss testcases with him to avoid missing out on corner cases.
> 
> I recommend landing this on m-c immediately. If there are no regressions
> within 10 days, we should uplift to aurora. After the next merge, we should
> then uplift to ESR24, so that this hits release on all channels
> simultaneously.
QA Contact: mwobensmith
Attachment #818443 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(In reply to bhavana bajaj [:bajaj] from comment #72)
> > This has a high probability of breaking in-content XBL bindings. There
> > aren't many of those that we know of, but we should still be careful.
> 
> Is this something that QA can help with ? I am adding mwobensmith as a QA
> contact here, will be helpful if you can discuss testcases with him to avoid
> missing out on corner cases.

The testcase in this bug isn't really relevant to my concerns.

Basically, the testcase indicates that we tend to expose more stuff to content than is really safe, so my patches make XBL API exposure to content be opt-in (via a special attribute) instead of opt-out. If anyone was previously trying to expose APIs via in-content XBL bindings, this will break them (though I don't know if anyone does that).

So on the QA side, I think we more or less just need to see if anyone reports broken addons.
Preparing for merge and final beta so we'll go with the suggestion in comment 74 of direct-landing to b2g26 rather than take churn on beta/release branches today.
Bobby, are we ready to move on esr24/b2g26/b2g18 patches?
Flags: needinfo?(bobbyholley+bmo)
(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #76)
> Bobby, are we ready to move on esr24/b2g26/b2g18 patches?

Yep! The reminder for this actually just showed up in my calendar this morning. ;-)

We can't do b2g18 though, because it doesn't have bug 821850. We'll have to embargo until b2g18 EOL.

I'll write the approval request now. Let me know if the backports are at all hair and I'll do them. :-)
Flags: needinfo?(bobbyholley+bmo)
Whiteboard: [uplift needs bug 822425] → [uplift needs bug 822425][embargo until b2g18 EOL]
Comment on attachment 818443 [details] [diff] [review]
Part 10 - Make in-content XBL event handlers allowuntrusted=false by default. v1

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: security issues.
Fix Landed on Version: mozilla-28 (and backported to mozilla-27).
Risk to taking this patch (and alternatives if risky): Moderate risk of breaking addons and intranet remote XUL apps (though the latter is only a problem if I screwed up somewhere). No alternatives.
String or UUID changes made by this patch: 

Note that this uplift requires bug 822425.
Attachment #818443 - Flags: approval-mozilla-esr24?
Attachment #818443 - Flags: approval-mozilla-b2g26?
With any luck, b2g26 will go OK. I'm not overly optimistic about esr24 though.
b2g26 rebase: https://tbpl.mozilla.org/?tree=Try&rev=cc2d6fd0c0f6

esr24 is going to require more love than I can give.
Attachment #818443 - Flags: approval-mozilla-esr24? → approval-mozilla-esr24+
try push for b2g27 while we wait for approval:

https://tbpl.mozilla.org/?tree=Try&rev=7a34a013d06e
(In reply to Bobby Holley (:bholley) from comment #82)
> try push for b2g27 while we wait for approval:

Er, b2g26. And Ryan says we actually have auth on these patches, so I'm pushing directly and canceling the try push:

https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/pushloghtml?changeset=129ad3c335a5
Attachment #818443 - Flags: approval-mozilla-b2g26?
Depends on: 950909
I don't think this needs QA verification. If anyone thinks that's a mistake please remove the [qa-] whiteboard tag and add the verifyme keyword.
Whiteboard: [uplift needs bug 822425][embargo until b2g18 EOL] → [uplift needs bug 822425][embargo until b2g18 EOL][qa-]
Whiteboard: [uplift needs bug 822425][embargo until b2g18 EOL][qa-] → [uplift needs bug 822425][embargo until b2g18 EOL][qa-][adv-main27+][adv-esr24.3+]
Bobby, what can we say here in an advisory that doesn't break the embargo for 1.1 b2g devices?
Flags: needinfo?(bobbyholley+bmo)
(In reply to Al Billings [:abillings] from comment #85)
> Bobby, what can we say here in an advisory that doesn't break the embargo
> for 1.1 b2g devices?

This bug is basically just about bug 911864 not going far enough. It doesn't make sense to do an advisory for this bug without one for that bug. I honestly think we should just wait until everything without these fixes is EOLed.
Flags: needinfo?(bobbyholley+bmo)
(In reply to Bobby Holley (:bholley) from comment #86)

> This bug is basically just about bug 911864 not going far enough. It doesn't
> make sense to do an advisory for this bug without one for that bug. I
> honestly think we should just wait until everything without these fixes is
> EOLed.

This bug is bug 911864. Did you mean bug 912322?
Sorry, I meant bug 821850.
Alias: CVE-2014-1479
Group: core-security
You need to log in before you can comment on or make changes to this bug.