User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Firefox/24.0 (Beta/Release) Build ID: 20130917102605 Steps to reproduce: 1. Pass an arbitrary `x` object (protected by a security wrapper) to `InstallTrigget.install` like this: `InstallTrigget.install({ name: { URL: x } }, callback)` 2. Or, pass an arbitrary `x` object (protected by a security wrapper) that looks like: `{ k1: 'str1', k2: 'str2', k3: 'str3' }` to `InstallTrigget.install` like this: `InstallTrigget.install(x, callback)` Actual results: `InstallTrigget.install` stringifies the passed objects, and passes the strings to the callback, even if the objects should be unaccessible from content. This can be used to access the `location` of not-same-origin iframes (first case), or to leak sensitive chrome objects of a certain shape (second case). Expected results: `InstallTrigget.install` should check if the passed object is well-formed (the value of the URL property is a string).

Attached example code for the second case.

It seems that the attached examples have to be downloaded first in order to work (I'll look into why...).

So Firefox prevents unknown sites from trying to install extensions by default, but this is not the case for local files.

bholley, when passing arbitrary objects to a chrome scope, is there a way to serialize/stringify them as they appear to the original caller, not to the chrome scope?

(In reply to Benjamin Smedberg [:bsmedberg] from comment #4) > bholley, when passing arbitrary objects to a chrome scope, is there a way to > serialize/stringify them as they appear to the original caller, not to the > chrome scope? Yes, they just need to be waived. Chrome can do Cu.waiveXrays(obj).toString() to get the exact same toString() that content would see.

I think it may be even simpler - we can just stop accepting objects. Dave: I don't *think* this will break anything on the web - all the documentation only mentions/shows using a string for the URL, and I don't remember seeing any case of anything but a string in the wild. But I wanted to get your thoughts before writing a test for this.

(In reply to Blair McBride [:Unfocused] from comment #6) > Created attachment 814769 [details] [diff] [review] > Patch v1 > > I think it may be even simpler - we can just stop accepting objects. This looks good for the first scenario, but doesn't work in the second (when you pass something like `{ k1: 'str1', k2: 'str2', k3: 'str3' }`).

Oh... right. I couldn't actually get that example working, and misinterpreted what was happening.

(In reply to Blair McBride [:Unfocused] from comment #6) > Created attachment 814769 [details] [diff] [review] > Patch v1 > > I think it may be even simpler - we can just stop accepting objects. > > Dave: I don't *think* this will break anything on the web - all the > documentation only mentions/shows using a string for the URL, and I don't > remember seeing any case of anything but a string in the wild. But I wanted > to get your thoughts before writing a test for this. I don't recall seeing anything else either

Ok, I'm stuck and confused. Ignore the talk of serializing the object - we do serialize things, but it's after processing. We need to see the passed object as the original caller long before that. We have a framescript (extensions-content.js) that essentially does this (assume __exposedPropers__, createObjectIn, makeObjectPropsNormal, etc all that are there): window.wrappedJSObject.InstallTrigger = { install: function(args, callback) { // |args| here needs to be seen as the webpage sees it args = Cu.waiveXrays(args); // All the properties of |args| are still accessible, // even though they're not accessible via the webpage } }; Any ideas?

Oh, I see. I misunderstood the issue here, and comment 5 isn't valid. What I should have said was that Cu.waiveXrays(obj).toString() will give you the same string that would be seen by script executing in whatever obj's compartment is. But the bug here is that content can pass some cross-origin object to InstallTrigger and get it to stringify it. So yeah. This is really the classic security pitfall of this kind of monkey-exposure. I think the best approach is to switch the content-side InstallTrigger stuff (extensions-content.js) to being exposed via WebIDL, which will solve this problem by enforcing stringification at the binding layer. AFAICT that should be pretty straightforward.

Oy. Ok, filed bug 926712 to switch to using WebIDL.

(In reply to Bobby Holley (:bholley) from comment #11) > So yeah. This is really the classic security pitfall of this kind of > monkey-exposure. I think the best approach is to switch the content-side > InstallTrigger stuff (extensions-content.js) to being exposed via WebIDL, > which will solve this problem by enforcing stringification at the binding > layer. AFAICT that should be pretty straightforward. It's not because of bug 863952. We could hack our way around that, but it'll be a bit ugly. If there's a quick workaround it might be better to use that for fixing this specific problem instead. Bobby: any alternatives?

The alternative is to do something along the lines of what was done in bug 856042 (before we had JS-implemented WebIDL and converted mozContacts to use that). It basically amounts to very careful manual sanitization of the inputs, which is super tricky to get right.

I've just reported a slightly different, and much more generic variation of this as https://bugzilla.mozilla.org/show_bug.cgi?id=928415 . Feel free to mark it as duplicate if you think that InstallTrigger.install is the only code that it affects and it will be fixed when transitioning InstallTrigger to WebIDL.

Cross-origin location reads are sec-high.

Note - I'm hoping bug 930091 will fix this.

@bobby how long until we should see 930091 completed? if it is not soon, maybe we should consider a specific fix for this?

Bug 930091 isn't going to fix this. Bug 926712 is the proper fix here.

@blair what is the status of 926712?

Still in progress, but stalled while I've been swamped with Australis. Can prioritise this now Australis is on Aurora.

Any updates Blair? We _really_ need to fix this. Some context: 18 months ago, we chemspilled for this exact same issue (cross-origin Location toString) in bug 720619, which, like this bug, was reported internally more than 6 months before. Dolske, if Blair is busy, can you find another owner?

I am working on this (yet) again, after being continually interrupted by "OMG we need to meet this deadline" demands. Though this week health issues and conferences are getting in the way... I hadn't realised this was chemspill worthy :(

(In reply to Blair McBride [:Unfocused] from comment #23) > I hadn't realised this was chemspill worthy :( Anything sec-high or sec-critical is chemspill-worthy.

This was one of the handful of bugs Blair and I talked about prioritizing a couple weeks back, so it should be on Blair's front-burner.

\o/

Because this bug (and the similar bug 928415 from the same reporter) opens up a new class of attacks that are not easy to fix (see bug 929542) we are awarding a higher-than usual bug bounty amount.

We simple _cannot_ just let this issue sit the way it has been. Doug, can you make this move?

Yes, this needs to get done. Over to Gijs to make that happen. But Mossop and I discussed this a bit over IRC, and I think we also need someone that knows webidl/DOM/xpconnect to help out with questions in that generic "expose an API to content" realm. At least, Dave and I are both confused from comments 11/13/14 here... Is finishing the work in bug 926712 going to fix this security bug or not? If not, exactly what else needs to be done? Who's the right person to help with that? Bobby?

(Also, Dave is willing to help out with any addon-manager questions.)

(In reply to Justin Dolske [:Dolske] from comment #30) > Yes, this needs to get done. Over to Gijs to make that happen. > > But Mossop and I discussed this a bit over IRC, and I think we also need > someone that knows webidl/DOM/xpconnect to help out with questions in that > generic "expose an API to content" realm. At least, Dave and I are both > confused from comments 11/13/14 here... Is finishing the work in bug 926712 > going to fix this security bug or not? Yes. Comment 11 indicates that Blair's previous (non-WebIDL) approach won't work. Comment 14 discusses as alternative that I don't endorse. > Who's the right person to help with that? Bobby? I am happy to help with any and all XPConnect/DOM/webidl questions. Thanks for jumping on this!

So I'm sure this is a dumb question, but why not this?

Uhm, should actually use the sanitized input instead of aArgs afterwards, although the JSON.stringify call actually kills the first poc to begin with because it throws a security exception. Oops.

Comment on attachment 8417947 [details] [diff] [review] , f?bholley Argh @ bugzilla / bzexport.

Per discussion on IRC with Boris, using the principal instead. This breaks both PoCs in my testing.

So, I spent some time sorting out a plan here with Gijs and Boris. My concern with the patch in this bug is that it tips our hand significantly about these kinds of bugs, and I'm concerned that we may have quite a bit more of them scattered throughout the platform. We need to make more progress on Slaughterhouse stuff in order to have confidence here. The advantage of doing this via bug 926712 is that "convert this interface to WebIDL" doesn't give people much of an indication of what the security issue is (or even that there is one at all). This, in a sense, is our saving grace with a lot of the API fixes we need to do for Slaughterhouse - converting them to WebIDL is a general forward-progress sort of thing, and is unlikely to raise suspicion. The problem, unfortunately, is that we can't actually securely implement the InstallTrigger API with the current WebIDL machinery, because the argument to install() is structured as follows: {fooAddon: {/* details */ }, barAddon: { /* details */ }}. This requires a kind of open-ended dictionary that people have been pushing for, but hasn't yet made it into the WebIDL spec. Boris and I concluded that the need for this kind of thing is pressing enough that we should create something for internal use without waiting for it to be specced, tentatively called MozMap. Once we have that, we can create a custom Dictionary called WebIDLInstallDetails, and pass a MozMap<WebIDLInstallDetails> to install(), which should give us all the proper type enforcement semantics we need. So here's the plan: * Gijs is going to finish up converting InstallTrigger to WebIDL in bug 926712, using a plain old |object| parameter for now, and get that landed. * Boris (or someone else) will try to quickly implement MozMap in bug 1007878. * Once both of the above hit trunk (ideally within 2 weeks), we uplift both to Aurora. * We avoid landing the Sandbox fix here, to avoid tipping our hand. Does this sound good to everyone?

So. Boris has heroically got the patches for MozMap done in bug 1007878. However, I was just reviewing the patch in bug 926712, and came across yet another hitch: the workaround that peter suggested for the lack of static methods ends up just exposing shum JS functions to content, and doing all of the WebIDL type coercion in privileged code. Boris, how far away of we from static WebIDL functions (bug 863952)? Is it realistic to block this security work on that as well, or should we just bail on the plan from comment 37 and do the Sandbox hack?

Or actually, maybe this is ok? I don't fully understand how this _create hack works. Maybe Boris should take a look at the patch

Peter's workaround is really neat. I wish I'd thought of it. ;) What it does is register a JavaScript-global-property with the name "InstallTrigger". The contract for such things is that in the resolve hook for the relevant name you createInstance using the given contract, then QI to nsIDOMGlobalPropertyInitializer, and if the QI succeeds call init() and if that returns an object use that object as the value of the property. And in this case the init() method returns a content-side implementation of the InstallTriggerImpl JS-implemented WebIDL interface. All _create() does is create the content-side object using the provided chrome-side object as the implementation (unlike the normal constructor case, which will try to get the chrome-side object by calling createInstance, etc). All of which is to say that with the patch window.InstallTrigger is a WebIDL object. InstallTrigger.install() is a non-static WebIDL method on that object's prototype. So using a MozMap for the first argument of install() would totally work as needed. If I read the code here correctly we'd do it like so: dictionary InstallTriggerData { DOMString URL; DOMString IconUrl; DOMString Hash; }; and boolean install(MozMap<(DOMString or InstallTriggerData)> installs, optional InstallTriggerCallback callback); Good thing you asked, because doing that uncovered a bug in the header includes bits of the patches in bug 1007878. ;)

Thank you Boris, that is _exceedingly_ helpful! To be clear - who is going to take ownership of the InstallTriggerData thing (along with the backport to Aurora)? Should be simple enough it sounds like, I just want to make sure we're crisp about ownership.

(In reply to Bobby Holley (:bholley) from comment #41) > Thank you Boris, that is _exceedingly_ helpful! > > To be clear - who is going to take ownership of the InstallTriggerData thing > (along with the backport to Aurora)? Should be simple enough it sounds like, > I just want to make sure we're crisp about ownership. I guess that because (a) I still own this bug, and (b) I expect bz has enough on his plate as-is, I will sign up for this bit, at least. I will try and get a patch together tomorrow. :-)

(In reply to :Gijs Kruitbosch from comment #42) > I guess that because (a) I still own this bug, and (b) I expect bz has > enough on his plate as-is, I will sign up for this bit, at least. I will try > and get a patch together tomorrow. :-) Sounds good! Please do the MozMap conversion in a separate bug, since it's a perfectly reasonable thing to do and will generally draw less attention to this.

What is the aurora plan? Are we backporting MozMap to aurora?

(In reply to Boris Zbarsky [:bz] from comment #44) > What is the aurora plan? Are we backporting MozMap to aurora? This was my understanding per comment #37 . Which is why I got approval for the installtrigger patch already...

Both bug 1007878 and bug 1011490 have landed on inbound, and I can confirm that both of the PoCs now fail.

(In reply to :Gijs Kruitbosch from comment #46) > Both bug 1007878 and bug 1011490 have landed on inbound, and I can confirm > that both of the PoCs now fail. \o/ As soon as bug 1011490 is fixed, we can mark this fixed.

Bug 1011490 and bug 1007878 have landed on both Aurora and central now, so marking this as fixed on all those branches.

Matt, iteration bugs with [qa+] are supposed to have an assigned QA contact. For this, that be you? Reassign at your discretion. Thanks.

Tracy: thanks, that is correct. Confirmed issue on Fx30. Verified fixed on Fx31, Fx32, 2014-06-05.

For reference, here's an email I just sent to Al when he asked about the embargo: > I think we should embargo it until all 3 dependencies of bug 928415 > (the more general version of this class of bug) are fixed everywhere. > The biggest dependency of that bug (which I've been working on for 16 months) > just landed yesterday. The other two dependencies should be pretty easy, and I > plan to have them both either done before the branch or uplifted so that they ship > in 33. That doesn't really help us for ESR though. At some point we'll have to decide > whether we want to uplift all the XrayToJS stuff that landed after esr31 in order to > mark this stuff fixed there, or whether to just wait it out. I'm focusing on the > normal trains first.

Could you open up this bug?

(In reply to Gábor Molnár from comment #53) > Could you open up this bug? Yep. Thanks for the great work! Posted a retrospective about this stuff here: http://bholley.net/blog/2016/the-right-fix.html

Thanks fixing it the right way, great job! :)