I encountered this twice during all the plugin related work I've been doing recently and was finally able to reproduce it. I usually use embed elements to load plugins and the key point here is that you have to use an object element. When using an object element to preview a plugin like pdfjs anonymous content is only generated as you plugin, where as with an embed element its there either way. This can lead to an interesting situation in which you load a normal URL into an object after loading a pdf document as a plugin and as a result the object actually has two windows associated with it, one directly as the window provided by the object, and another that's provided by the anonymous iframe generated when loading a plugin. If done in the proper order and with a window that's accessible to you being loaded in the anonymous iframe(using the view-source: protocol trick), the frameElement of the window accessible through window[#] becomes fully accessible and is no longer protected by a SOW. Here I take advantage of this to use the insertAdjacentHTML function and inject an iframe into the anonymous content with an onload attribute that provides script execution in the context of the Sandbox associated with the bindings that have provided the anonymous content. Lol I know that's tough to follow and it's why it took me a little time to figure out. Here I show accessing Components.interfaces which would allow QI'ing DOM objects into other interfaces they support. I also create a sandboxEval function on the window which allows access to most any anonymous content and even functions associated with the anonymous content that should only be visible to chrome level code. This one actually makes me a little sad, because it brings bug 1043787 back into play but even stronger than before the last release update. Once that bug is back in play bug 928211 also comes back into play fully. What made me sad about this is it seems that you can do even more nasty things with this SOW bypass than you could before all the extensive XBL work that had just finally given me some hope of most if not all the holes being filled. I'll let you guys gauge it from here in terms of importance and all that but I can tell you that this one is very very serious. With this you wouldn't even need the WindowProperties named window trick I previous used in 1043587. There's a real mess with these plugins honestly and I haven't even gotten to properly fuzzing the pdfjs code itself yet.

I hope you guys can understand that with that mess I threw together there. Was up too early today mainly because I nailed this one and wanted to get to the testcase, I found this last night. Let me know if you need anything clarified. It's still unclear to me why the iframe is no longer protected by a SOW, so also post what you find out about it because I'm still beyond curious.

Cody, We really appreciate all the work your doing, and it would be super helpful if you could condense things down. The wall of text makes if very hard to distill exactly what is trying to be reported and for us to then confirm. If you could take a look at https://developer.mozilla.org/en-US/docs/Mozilla/QA/Bug_writing_guidelines and use the guidelines there when filling bugs at a bare minimum the "Writing precise steps to reproduce" and "Writing a clear summary" would be enormously helpful. We can handle getting into correct components and what not after that.

Sorry. There's been a steady decline in the quality of my bugs filed there's actually a reason but I'll try to do better from here on out.

This is a playpreview bug. John is currently handling these (which might involve just doing bug 558184 and embargoing elsewhere). Curtis - If you're having trouble filing a codyc bug, please needinfo me and I'll try to take a guess as to who should own it. Cody's quite clever, and I've asked him to file partial testcases of "interesting" behavior rather than waiting until he can bundle several of them up into a sec-crit. His stuff is always worth a good look by the right developer.

bug 558184 will remove uses of these anonymous iframes for sure, but why anonymous content is sticking around after the XBL binding dies and losing its wrapper is worrying. We still use anonymous content via XBL for e.g. plugin errors, so if it's possible to leak unwrapped things to the page that code probably needs a considerable audit, as, like pdfjs, its probably assuming that its safe in XBL-magic-land.

:bholley or :johns can either of you confirm this bug?

I'll let John handle this one.

There is definitely new badness here

I thought I would edit the title to reflect the fact that as bobby pointed out to me there are no SOWs anymore. This one was kind of like a unicorn for me so I filed as soon as I nailed down the steps to reproduce, sorry about that.

(In reply to John Schoenick [:johns] from comment #5) > bug 558184 will remove uses of these anonymous iframes for sure, but why > anonymous content is sticking around after the XBL binding dies and losing > its wrapper is worrying. We still use anonymous content via XBL for e.g. > plugin errors, so if it's possible to leak unwrapped things to the page that > code probably needs a considerable audit, as, like pdfjs, its probably > assuming that its safe in XBL-magic-land. One of the other times I encountered this behavior relied on having setTimout execute some javascript in the window associated with the anonymous iframe, so that it had script executing in it at the time of the binding dying. I believe here the use of the data URI "data:application/x-moz-playpreview-pdfjs;," has the same affect. Sorry about being so late on seeing that.

How likely is it this could be used to execute chrome-privileged JS? We're looking to rate this.

This appears to reliably execute code in a higher-than-content scope, but I don't know enough about XBL magic to say how far that is from chrome execution

(In reply to John Schoenick [:johns] from comment #12) > This appears to reliably execute code in a higher-than-content scope You mean the XBL scope (i.e. an nsExpandedPrincipal)? That's not a full exploit in itself, but it removes a major layer of defense-in-depth, and so we've historically classified it as sec-high (see bug 865947). John, assuming that you can confirm that this is indeed executing arbitrary code in an XBL scope, let's call this sec-high.

Marking sec-high per comment 12 and 13. Feel free to change it as appropriate, John, if I'm misunderstanding.

Can we get some sec-bounty goodness going on soon? My moneys were low, now they're getting lonely even.

(In reply to codyc from comment #15) > Can we get some sec-bounty goodness going on soon? My moneys were low, now > they're getting lonely even. I'm pretty sure that the policy is to not pay bounties until the fix actually lands on central (since that's the only sure-fire way to make sure someone has investigated the bug sufficiently). John, do you have a rough ETA on this bug?

I'll *try* to come up with a quick fix for this later this evening. I'm thinking something along the lines of using isCallerChrome and IsInAnonymousSubtree to return null for the frameElement property of a window if the frame element is anonymous and the caller is content level. I've been working with my C++ quite a bit lately, but no promises. The overall size of the source still makes me hesitant. This will be good practice for an idea I have about a built in fuzzer interface that will use WebIDL definitions to always be able to fuzz new functions and changes as they are implemented. Sound like a plan? Now let's hope I can actually get it done.

I seem to have the above fix in place, and I think this could serve as a point fix until bug 558184 is worked out which will probably change how all this works. I'm going to do some more testing for a minute, then probably get a patch up, unless you guys would rather handle this somehow else.

Sorry to hit you for a review on such a raw patch, but here you go Bobby. I'm still getting accustomed to mercurial so take it easy on me. Should I comment about what the change is for above it?

This is a revised patch with comments in place for the change. I flagged Bobby for review without thinking before, who should review this? It's such a minimal change that I can't see any problems with it. I'll leave it up to you guys.

Bobby, is this something you could review? Thanks.

Maybe I wasn't entirely clear here but this patch should only be seen as a point fix for this one known scenario that can be exploited. I just sent Bobby an email telling him my thoughts on this and since I should have a little time, I'll be digging through the source hard to find the actual root cause. I'll try to see what other anonymous content one can actually gain a reference to at this point. I've been running this minor change without problems for some time, and I can't find a single use case that it breaks. Sorry to be pushy about this, but I do need some kind of financial bump, plus I would be really stoked to actually have a patch land on mc even if it's only temporary. I should be back in touch by Tuesday with my findings. If not something else has just taken priority, it's been a crazy couple of weeks for me.

I didn't have a chance to get to this yet. Take your time with this now guys, my situation has changed. I'll get back to this as soon as I can, as of right now I have two major projects that have taken priority besides all the stuff I already had going on. I'll be back active as soon as I can.

See bug 1043778 comment 11.

see bug 1043778 comment 13

I think :johns isn't working on this any more. I'll talk to jst about it.

Boris is working on this stuff now.

Any progress here? There haven't been any updates in over a month. Thanks.

No progress so far.

This is almost a year old. Have you had a chance to look at this, Boris?

Peter and I are actively working on this, yes, as part of the general move-shumway-out-of-process stuff.

(From talking to jst, it sounds like this is actually an enormous project)

This is the core playPreview nastiness behind bug 1178058. It's basically fixed because we stopped using playPreview, but I'm still investigating how exactly this anonymous content is getting revealed.

Hey Cody, I never ended up getting the time to dig into exactly how the anonymous content was getting de-anonymized here. Obviously it's less of a concern since we eliminated playPreview, but I'd still like to make sure we understand what was happening in case it might happen in some other context. Whenever you get the chance, would you mind taking a look and letting me know how it was happening? This isn't particularly urgent, so no rush here. Also, if this isn't the kind of investigation you feel comfortable doing, just let me know and I'll put it back on my list.

I'll look into this so need to put it back on your list. One key thing IIRC and thought I even put in the comments of the testcase is that for this to work nothing from the content page can even touch the frameElement property of the window associated with the anonymous iframe until after the binding has died. That tells me this may in fact be a wrapper generation issue. I need to dig into this just to be sure but I believe that's where an issue is going to be found here.

I'm going to call this WFM thanks to the removal of playPreview in bug 1192831. However, we should still finish the investigation in comment 36.

I have a rough idea I believe of what is going on here. I'm not in the best position to be properly debugging here so I could be wrong. It looks at least a key part of what is happening is as follows: See http://mxr.mozilla.org/mozilla-release/source/js/xpconnect/wrappers/WrapperFactory.cpp#260 Running a debug build I get warned when running the original testcase from here that the call to WrapNativeToJSVal before this actually fails. I think the frameElement in this instance somehow gets treated like it can be wrapped to a JSVal which fails. After this failure there is this line: obj.set(&v.toObject()); So it seems to me that even though v isn't actually a wrapper but just an object that ends up returned in wrapScope that even without the call to WaiveXray it's still returning an object that isn't properly wrapped. Please anyone who has eyes on this look into this some. It's probably taken me 5 times longer than it would have normally to come up with this since I can't properly debug.

(In reply to Cody Crews from comment #38) > I have a rough idea I believe of what is going on here. I'm not in the best > position to be properly debugging here so I could be wrong. It looks at > least a key part of what is happening is as follows: > > See > http://mxr.mozilla.org/mozilla-release/source/js/xpconnect/wrappers/ > WrapperFactory.cpp#260 > > Running a debug build I get warned when running the original testcase from > here that the call to WrapNativeToJSVal before this actually fails. I think > the frameElement in this instance somehow gets treated like it can be > wrapped to a JSVal which fails. After this failure there is this line: > > obj.set(&v.toObject()); Yes, but the NS_ENSURE_SUCCESS will early-return in the case where it fails, and we'll just refuse to create the wrapper, so I doubt that's what's going on here. What seems more likely to me is that the frameElement reflector isn't being created in the XBL scope like it should. But I'm not sure exactly how that'd be happening. Feel free to wait on this one until you have better hardware if that's coming. :P

Ah, it's going to be really nice to able to link in the source with a debugger and step through it again. Never thought I would be missing the headache that debugging things like this can give me. It's much more of a headache just using mxr.mozilla.org and trying to visualize things. I have some friends locally that I might could talk to to see if I could just hold on to a mid-range system until I can get things sorted because I'm not feeling like I'm being very useful right now.

OK - Again, no hurry on this one - waiting until you've got proper hardware here is totally fine. NI cody just to make it easier to track whose court this ball is in.

What I see here first is the fact that in nsXBLBinding::ChangeDocument the call to nsXBLBinding::UninstallAnonymousContent isn't called in some situations(might be totally unrelated). This *might* be why the anonymous content lives past the binding dying. I've been trying to fix up the logic involved to ensure that UninstallAnonymousContent is called, without much luck yet. That will still leave the other issue of the frameElement reflector not being created as Bobby pointed out above. I'm trying to get the logic right first, and then move on to the second issue of the reflector not being properly created. I'm thinking that this bug could be triggered even with playPreview removed, which is bad.

(In reply to Cody Crews from comment #42) > I'm thinking that this bug could be triggered even with playPreview removed, > which is bad. That was my concern. Please continue your investigation!

Hmm, after spending quite some time dredging through the c++ side of things looking for the cause of this, I found that it's actually much more simple than anyone probably thought. What is happening here is that the anonymous iframe that plugins use is generated by code in PluginContent.jsm on the fly. This not only causes the iframe to not have the flag NODE_CHROME_ONLY_ACCESS but apparently can cause confusion if the binding dies around the same time its generated. I'll throw together the most forward patch here shortly, which is a two line alteration to pluginProblem.xml. I'll also be looking over the c++ side of things to see if there's any way that this could be handled better from that end.

Nice someone has already made these changes :-) At least I've now read through pretty much all the XBL C++ code(at least 3 times) and have walked through the most of the DOM bindings code too. Sorry I was dragging getting to this guys. After reading through a lot of the XBL code there's a few things which I believe might could lead to some problems but I'll have to do some more leg work to see. I was also going to try to make some changes with the aim of making sure that when a node is added to an anonymous subtree that it properly inherits everything it should. Before I dig too deep on that, has anyone already been working on it?

(In reply to Cody Crews from comment #45) > Nice someone has already made these changes :-) > > At least I've now read through pretty much all the XBL C++ code(at least 3 > times) and have walked through the most of the DOM bindings code too. > > Sorry I was dragging getting to this guys. After reading through a lot of > the XBL code there's a few things which I believe might could lead to some > problems but I'll have to do some more leg work to see. > > I was also going to try to make some changes with the aim of making sure > that when a node is added to an anonymous subtree that it properly inherits > everything it should. Before I dig too deep on that, has anyone already > been working on it? -> needinfo back on bholley

I'll wait and see what Bobby says, and be digging into the C++ side anyway. I just looked at pluginProblem.xml again in the newest nightly. It's not actually the patch I was going to submit here, but I assume that the iframe generated at runtime is gone because work on jsplugins has advanced. Just to state it clearly the whole reason that the iframe here became accessible is because it is generated at runtime and then appended to the anonymous content which is properly setup as chromeOnlyContent. This runtime generation is triggered from a custom event, and has nothing to do with XBL content generation(or C++ code). With the race type setup in the testcase, I assume the binding was already dead or marked for death by the time the iframe was even generated. There's some other content in there generated at runtime, but I cant see any possible way for someone to get to it. Simply replacing the comment which mentioned that the iframe would be generated at runtime with proper iframe tags which had the right attributes stopped the problem demonstrated in the testcase completely. I will be looking over how a node which has been appended to anonymous content could automatically inherit all the relevant special things like being treated like NAC.

(In reply to Cody Crews from comment #47) > I will be > looking over how a node which has been appended to anonymous content could > automatically inherit all the relevant special things like being treated > like NAC. I'm not sure how much will break if we do this in the general anonymous content case. There are cases where we use non-anon content inside anon elements (e.g. Firefox UI <tab> elements are non-anon inside a bundle of anon stuff inside (non-anon) <tabs>). I don't know offhand how many things like that happen with XBL inside unprivileged pages, but it seems like there such behaviour is likely to always be a bug.

I believe that the bindings from pluginProblem.xml are the only in content ones which use chromeOnlyContent which makes them be treated like NAC. Also the ui for media elements IIRC gets walked through and flagged to be treated like NAC. Does anybody know of any other instances of unprivileged anonymous content being treated as if it was native? If not, I think we could take care of any problems this change would make. On the other hand maybe we don't want to encourage people to randomly generate content at runtime and append that to NAC(whether true NAC or not). I'm thinking about addon/extension/plugin people.

Nice work cody! So it sounds like the root of the problem here is the dynamic insertion of children into protected in-content subtrees. I assume that our replacement mechanism for PlayPreview doesn't do that? The instances of in-content XBL that I'm aware of are: * audio/video * marquee * pluginProblem pluginProblem is the only one that leverages chromeOnlyContent. audio/video achieve this effect by parenting the subtree to a real NAC element. I don't believe that we hoist the marquee anonymous content, but we probably should use chromeOnlyContent there as well. Cody, can you file a followup on that? It seems like we should also confirm that there are no other callers in the tree that insert dynamic children into the pluginProblem (or audio/video) <content>. I would hope that the frontend team will not add any new uses of in-content XBL to the tree. SO once we do both of those things, I think we can call this matter settled.

(In reply to Bobby Holley (:bholley) from comment #50) > pluginProblem is the only one that leverages chromeOnlyContent. audio/video > achieve this effect by parenting the subtree to a real NAC element. I don't > believe that we hoist the marquee anonymous content, but we probably should > use chromeOnlyContent there as well. Cody, can you file a followup on that? Yeah I can handle that and I'll actually generate the patch for this. There's also the pretty print bindings, I assume we want to go ahead and use chromeOnlyContent there too? There's a few other things generated at runtime in pluginProblem, but I don't think they would be vulnerable to any exploitation as the only way one could gain a reference to them is if the binding were still alive and they're there to be clicked on(then one could use document.activeElement).

The followup bug is bug 1220901. I didn't mark it security sensitive because it should seem routine, but if it should be then please change it for me. I'll only add the patch for the change to pluginProblem.xml here. I'm going submit the patches for the other in content bindings there, and also throw up some experimental patches which *should* help prevent this sort of problem in the future.

This adds the anonymous iframe to the <content> of pluginProblem.xml so that it isn't generated at runtime.

Sorry that it took a minute, I had to setup a local repository of the current release version. I've uploaded a patch over in bug 1220901 that adds chromeOnlyContent where it was missing. I'll try to have something to look from the C++ side uploaded there tomorrow.

(In reply to Cody Crews from comment #51) > There's a few other things generated at runtime in pluginProblem, but I > don't think they would be vulnerable to any exploitation as the only way one > could gain a reference to them is if the binding were still alive and > they're there to be clicked on(then one could use document.activeElement). I don't think we should rely on same-compartment "can't ever get a reference to this" invariants - there are always tricky was that we don't anticipate (your activeElement trick, for example, is a very clever one that I never thought of). Let's file security-sensitive bugs on any places where we do this. If they're non-trivial to fix we can find owners for them.

Ok, I made the followup restricted too. For other things generated at runtime see: http://mxr.mozilla.org/mozilla-release/search?string=set+at+runtime&find=pluginProblem.xml&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-release I'll look through and see if those others are only filling in text or actually adding elements. I'll also dig through as much as I can to see if there's any other runtime generation going on elsewhere.

(In reply to Cody Crews from comment #56) > Ok, I made the followup restricted too. > > For other things generated at runtime see: > > http://mxr.mozilla.org/mozilla-release/ > search?string=set+at+runtime&find=pluginProblem. > xml&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-release > > I'll look through and see if those others are only filling in text or > actually adding elements. I'll also dig through as much as I can to see if > there's any other runtime generation going on elsewhere. Yeah, I wouldn't rely on the comments though. I think we basically need to look through all the anonids and see if they're referenced somewhere. Also, can we remove previewPluginContent, now that it should be unused?

Yeah I'll take care of that. I should have thought about that more as a made that patch. That's only for my testing(dont know what I was thinking). I'm also going to have to have a version that once again generates the previewPluginContent at runtime to see if the changes I make on the C++ side actually prevent this bug. BTW I didn't expect any of us to rely on the comments, I was just showing you the total count and the fact that their placement means two of them are not likely to be elements(one is just the text for the href attribute of an element.)

First off above that should be an I in place of a. Just had to get that out of the way. I thought I should say more clearly that I've been through well over 90% of this code more than once with work I've done relating to plugins. Those generations are most likely going to come from PluginContent.jsm too. I'll definitely be doing the work to dig through everything to see if I can find any pieces of code doing similar things also. Something definitely has to be done on the native side of things, or even if we make these changes addon/extension developers are sure to do exactly what they shouldn't. Bobby from what I've been reading I can see at least two different ways(i think) to handle things in native XBL code when it comes to nodes being added/appended to anonymous content. That would be to implement nsIMutationObserver and watch for content mutations or capture the DOMNode*/DOMSubtreeModified events that get fired. I think a mutation observer is supposed to be better performance wise than those events. There may be a much easier way this could be done that I'm just overlooking. The binding manager already implements nsIMutationObserver, but the binding manager is only watching the addition/removal of <content> elements from XBL. I'm trying to find the most minimal change that could be made and still accomplish what we want done here. It might be time to just start hacking away at the current XBL implementation and see what I can come up with though. Oh yeah I'll generate the revised version of that patch and have it uploaded here shortly.

(In reply to Cody Crews from comment #59) > First off above that should be an I in place of a. Just had to get that out > of the way. > > I thought I should say more clearly that I've been through well over 90% of > this code more than once with work I've done relating to plugins. Those > generations are most likely going to come from PluginContent.jsm too. I'll > definitely be doing the work to dig through everything to see if I can find > any pieces of code doing similar things also. > > Something definitely has to be done on the native side of things, or even if > we make these changes addon/extension developers are sure to do exactly what > they shouldn't. I don't think we should bend over backwards for addons here, TBH. XUL addons are going away anyway, and in-content XBL is certainly one of the most esoteric features they might be using. > Bobby from what I've been reading I can see at least two different ways(i > think) to handle things in native XBL code when it comes to nodes being > added/appended to anonymous content. That would be to implement > nsIMutationObserver and watch for content mutations or capture the > DOMNode*/DOMSubtreeModified events that get fired. I think a mutation > observer is supposed to be better performance wise than those events. There > may be a much easier way this could be done that I'm just overlooking. The > binding manager already implements nsIMutationObserver, but the binding > manager is only watching the addition/removal of <content> elements from XBL. > > I'm trying to find the most minimal change that could be made and still > accomplish what we want done here. It might be time to just start hacking > away at the current XBL implementation and see what I can come up with > though. Yeah, I think all of this is probably too complicated. Could we just disallow appending children to native anonymous subtrees entirely?

Yeah that's fine too, it's kind of my fault I've been all over the XBL code and wanted to see what I could do without breaking any of it. (In reply to Cody Crews from comment #49) > On the other hand maybe we don't want to encourage people to randomly > generate content at runtime and append that to NAC(whether true NAC or not). > I'm thinking about addon/extension/plugin people. As you can see, I was thinking something along those lines. I was just looking through PluginContent.jsm and so far the iframe is the only actual element it is generating. How much of this is going to be made pointless by jsplugins? I'll get rid of any runtime generation happening that I can find even if it means having to make different extended bindings based on the same base binding(probably why the ui people did it that way in the first place).

I meant to get back to this yesterday, but something came up. Here is PluginContent.jsm and pluginPreview.xml with all references to playPreview being removed. There's still two messages generated at runtime stored on existing anonymous nodes through node.textContent, and one existing anonymous node which has its href attribute set at runtime.

I believe that about wraps things up here. Now no actual elements are generated at runtime. With that done even if we were to bring playPreview back this should be locked down. I'm working on making the proper changes on the native side so that nothing can be appended to native anonymous subtrees. I'll upload that over in bug 1220901 hopefully with meaningful tests included tomorrow if not by later today.

(In reply to Cody Crews from comment #62) > Created attachment 8683146 [details] [diff] [review] > removePlayPreviewContentGeneration.patch This looks great. Can you file it as a separate bug and flag me for review?