1105045 - (CVE-2015-4502) Receiver passed to proxy get hook is not outerized when proxy is on the window's proto chain

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Boris Zbarsky [:bzbarsky]]

Attachment: Testcase

Credit for finding this goes to André Bargull (cced).

If you have a proxy that has a handler like this:

  {
    get: function(t,pk,r) {
    },
    has: function(t,pk) {
      return pk == 'try_to_get_global_object';
    }
  }

on the proto chain of the window and then you evaluate the bareword "try_to_get_global_object" then the "r" that's passed to the get hook is the inner window.  The attached testcase shows this clearly: r === window tests false, but calling a getter on "r" that just returns "this" (which just serves to outerize) returns something that's equal to "window".

Comment 1

[Jason Orendorff [:jorendorff]]

My bug for sure. Recent regression partly attributable to TC39 ignoring the global/WindowProxy distinction.

I'm not sure what *should* happen here; possibly the only people who have ever considered it are cc'd on this bug...

Comment 2

[Boris Zbarsky [:bzbarsky]]

I think we should outerize just like we outerize the thisval for setters defined on the global's proto chain.

Comment 3

[Jason Orendorff [:jorendorff]]

But for all proxies? Or only scripted proxies? What about other non-native objects?

Comment 4

[Boris Zbarsky [:bzbarsky]]

Oh, hmm....

I don't know.  :(

If this only affects proxies/non-natives on proto chains, then the only significant internal consumer we need to worry about is the global scope polluter, I suspect.  And as far as I can tell it's not actually getting a receiver passed to any of the hooks it implements, so doesn't care what happens with the receiver.  So I think we could just always outerize the receiver for proxy hooks and be ok, as long as that happens _after_ we find the right object on the proto chain where the prop lives.

Comment 5

[David Bolter [:davidb] (NeedInfo me for attention)]

Looks like we are not sure of next steps here and it might be a spec bug? Critsmash team looked at rating this but are not sure of severity other than 'bad'. Anyone know how bad?

Comment 6

[Boris Zbarsky [:bzbarsky]]

Per spec, we should be outerizing for scripted proxies (which is the only really spec-observable thing here).

Comment 7

[Daniel Veditz [:dveditz]]

bz: see comment 5

Comment 8

[Boris Zbarsky [:bzbarsky]]

If the question is how bad it is that script can get its hands on an inner window, the answer is I don't know.  Bobby might...

Comment 9

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky [:bz] from comment #8)
> If the question is how bad it is that script can get its hands on an inner
> window, the answer is I don't know.  Bobby might...

Much less bad than it used to be. I'd guess sec-moderate.

Comment 10

[Jason Orendorff [:jorendorff]]

Attachment: Outerize the receiver passed to proxy hooks

Comment 11

[Eric Faust [:efaust]]

Comment on attachment 8645020 [details] [diff] [review]
Outerize the receiver passed to proxy hooks

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

This looks good to me. We should think about how to keep more of these kinds of cases from biting us. Jason and I talked a little on IRC. There's not much we can do immediately better. We should try, in future, to force good test coverage with Window.

Another suggestion Jason had was to rename "inner" and "outer" windows (or at least the functions that go between them) in terms of "global" and "window proxy". To me, that does seem intuitively better, though perhaps it's hard to understandh whether you have the inner or outer one in your hand, if you're not familiar with the system.

Comment 12

[Jason Orendorff [:jorendorff]]

url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/dc21224de25bd42bb8ad4728c71988dfdb26ea26
changeset:  dc21224de25bd42bb8ad4728c71988dfdb26ea26
user:       Jason Orendorff <[e-mail]>
date:       Fri Aug 07 10:23:05 2015 -0500
description:
Bug 1105045 - Outerize the receiver passed to proxy hooks. r=efaust.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/dc21224de25b

Based on when this was filed, I guess this affects at least 37+. What other releases does this affect? Please nominate this for uplift as well.

Comment 14

[Jason Orendorff [:jorendorff]]

Comment on attachment 8645020 [details] [diff] [review]
Outerize the receiver passed to proxy hooks

Approval Request Comment
[Feature/regressing bug #]: One of the bugs under meta bug 1139700, which included a rewrite of GetProperty and SetProperty for native objects.
[User impact if declined]: inner window objects can be obtained. sec-moderate, no known exploit
[Describe test coverage new/current, TreeHerder]: the patch landed in Nightly, with tests
[Risks and why]: Not risky, as the fix is localized and straightforward. Landed in Nightly without incident.
[String/UUID change made/needed]: none

Comment 15

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8645020 [details] [diff] [review]
Outerize the receiver passed to proxy hooks

Security fix, taking it.

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/eb7622882d75

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out in https://hg.mozilla.org/releases/mozilla-aurora/rev/d8d62e273a8c because of merge conflicts. This seems to need bug 1192297 uplifted also to land cleanly.

Can we either get a branch-specific patch uploaded or get approval for 1192297 to be uplifted?

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Calling esr38 wontfix since this is only sec-moderate and most of the work hanging off bug 1139700 landed on 39+ anyway.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Jason rebased and landed this.
https://hg.mozilla.org/releases/mozilla-aurora/rev/226332faa005
https://hg.mozilla.org/releases/mozilla-beta/rev/34a6fcd4793a

Jason, for the future, don't forget that pulsebot a) can't mark s-s bugs and b) doesn't mark uplifts.