924329 - Reading wrapper-protected information using InstallTrigger

Status: VERIFIED FIXED

(Firefox :: General, defect)

Description

[Gábor Molnár]

Attachment: crossdomain_iframe_location_read.html

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).

Comment 1

[Gábor Molnár]

Attachment: Another POC

Attached example code for the second case.

Comment 2

[Gábor Molnár]

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

Comment 3

[Gábor Molnár]

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

Comment 4

[Benjamin Smedberg]

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?

Comment 5

[Bobby Holley (:bholley)]

(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.

Comment 6

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: 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.

Comment 7

[Gábor Molnár]

(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' }`).

Comment 8

[Blair McBride [:Unfocused] (UNAVAILABLE)]

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

Comment 9

[Dave Townsend [:mossop]]

(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

Comment 10

[Blair McBride [:Unfocused] (UNAVAILABLE)]

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?

Comment 11

[Bobby Holley (:bholley)]

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.

Comment 12

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Oy.

Ok, filed bug 926712 to switch to using WebIDL.

Comment 13

[Peter Van der Beken [:peterv]]

(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?

Comment 14

[Bobby Holley (:bholley)]

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.

Comment 15

[Gábor Molnár]

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.

Comment 16

[Bobby Holley (:bholley)]

Cross-origin location reads are sec-high.

Comment 17

[Bobby Holley (:bholley)]

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

Comment 18

[Raymond Forbes[:rforbes]]

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

Comment 19

[Bobby Holley (:bholley)]

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

Comment 20

[Raymond Forbes[:rforbes]]

@blair what is the status of 926712?

Comment 21

[Blair McBride [:Unfocused] (UNAVAILABLE)]

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

Comment 22

[Bobby Holley (:bholley)]

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?

Comment 23

[Blair McBride [:Unfocused] (UNAVAILABLE)]

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 :(

Comment 24

[Bobby Holley (:bholley)]

(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.

Comment 25

[Justin Dolske [:Dolske]]

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.

Comment 26

[Bobby Holley (:bholley)]

\o/

Comment 28

[Daniel Veditz [:dveditz]]

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.

Comment 29

[Bobby Holley (:bholley)]

We simple _cannot_ just let this issue sit the way it has been.

Doug, can you make this move?

Comment 30

[Justin Dolske [:Dolske]]

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?

Comment 31

[Justin Dolske [:Dolske]]

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

Comment 32

[Bobby Holley (:bholley)]

(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!

Comment 33

[:Gijs (out for now; he/him)]

Attachment: , f?bholley

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

Comment 34

[:Gijs (out for now; he/him)]

Attachment: , f?bholley

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 35

[:Gijs (out for now; he/him)]

Comment on attachment 8417947 [details] [diff] [review]
, f?bholley

Argh @ bugzilla / bzexport.

Comment 36

[:Gijs (out for now; he/him)]

Attachment: Use principal instead of 'content' per IRC discussion

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

Comment 37

[Bobby Holley (:bholley)]

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?

Comment 38

[Bobby Holley (:bholley)]

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?

Comment 39

[Bobby Holley (:bholley)]

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

Comment 40

[Boris Zbarsky [:bzbarsky]]

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.  ;)

Comment 41

[Bobby Holley (:bholley)]

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.

Comment 42

[:Gijs (out for now; he/him)]

(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. :-)

Comment 43

[Bobby Holley (:bholley)]

(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.

Comment 44

[Boris Zbarsky [:bzbarsky]]

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

Comment 45

[:Gijs (out for now; he/him)]

(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...

Comment 46

[:Gijs (out for now; he/him)]

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

Comment 47

[Bobby Holley (:bholley)]

(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.

Comment 48

[:Gijs (out for now; he/him)]

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

Comment 49

[Tracy Walker [:tracy]]

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

Comment 50

[Matt Wobensmith [:mwobensmith][:matt:]]

Tracy: thanks, that is correct.

Confirmed issue on Fx30.
Verified fixed on Fx31, Fx32, 2014-06-05.

Comment 51

[Bobby Holley (:bholley)]

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.

Comment 53

[Gábor Molnár]

Could you open up this bug?

Comment 54

[Bobby Holley (:bholley)]

(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

Comment 55

[Gábor Molnár]

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