Closed
Bug 1105045
(CVE-2015-4502)
Opened 10 years ago
Closed 9 years ago
Receiver passed to proxy get hook is not outerized when proxy is on the window's proto chain
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
mozilla43
Tracking | Status | |
---|---|---|
firefox40 | --- | wontfix |
firefox41 | --- | fixed |
firefox42 | --- | fixed |
firefox43 | --- | fixed |
firefox-esr38 | --- | wontfix |
b2g-v2.0 | --- | unaffected |
b2g-v2.0M | --- | unaffected |
b2g-v2.1 | --- | unaffected |
b2g-v2.1S | --- | unaffected |
b2g-v2.2 | --- | unaffected |
b2g-v2.2r | --- | unaffected |
b2g-master | --- | fixed |
People
(Reporter: bzbarsky, Assigned: jorendorff)
Details
(Keywords: reporter-external, sec-moderate, Whiteboard: [reporter-external][post-critsmash-triage][adv-main41+])
Attachments
(2 files)
403 bytes,
text/html
|
Details | |
5.61 KB,
patch
|
efaust
:
review+
Sylvestre
:
approval-mozilla-aurora+
Sylvestre
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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".
Reporter | ||
Updated•10 years ago
|
Flags: needinfo?(efaustbmo)
Assignee | ||
Comment 1•10 years ago
|
||
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...
Assignee: nobody → jorendorff
Reporter | ||
Comment 2•10 years ago
|
||
I think we should outerize just like we outerize the thisval for setters defined on the global's proto chain.
Assignee | ||
Comment 3•10 years ago
|
||
But for all proxies? Or only scripted proxies? What about other non-native objects?
Reporter | ||
Comment 4•10 years ago
|
||
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.
Reporter | ||
Updated•10 years ago
|
Flags: sec-bounty?
Updated•10 years ago
|
Whiteboard: [reporter-external]
Comment 5•10 years ago
|
||
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?
Reporter | ||
Comment 6•10 years ago
|
||
Per spec, we should be outerizing for scripted proxies (which is the only really spec-observable thing here).
Reporter | ||
Comment 8•10 years ago
|
||
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...
Flags: needinfo?(bzbarsky) → needinfo?(bobbyholley)
Comment 9•10 years ago
|
||
(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.
Flags: needinfo?(bobbyholley)
Updated•10 years ago
|
Keywords: sec-moderate
Updated•10 years ago
|
Flags: sec-bounty? → sec-bounty+
Assignee | ||
Comment 10•9 years ago
|
||
Attachment #8645020 -
Flags: review?(efaustbmo)
Comment 11•9 years ago
|
||
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.
Attachment #8645020 -
Flags: review?(efaustbmo) → review+
Assignee | ||
Comment 12•9 years ago
|
||
url: https://hg.mozilla.org/integration/mozilla-inbound/rev/dc21224de25bd42bb8ad4728c71988dfdb26ea26
changeset: dc21224de25bd42bb8ad4728c71988dfdb26ea26
user: Jason Orendorff <jorendorff@mozilla.com>
date: Fri Aug 07 10:23:05 2015 -0500
description:
Bug 1105045 - Outerize the receiver passed to proxy hooks. r=efaust.
Comment 13•9 years ago
|
||
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.
Status: NEW → RESOLVED
Closed: 9 years ago
status-b2g-v2.2:
--- → affected
status-b2g-v2.2r:
--- → affected
status-b2g-master:
--- → fixed
status-firefox40:
--- → wontfix
status-firefox41:
--- → affected
status-firefox42:
--- → affected
status-firefox43:
--- → fixed
status-firefox-esr38:
--- → affected
Flags: needinfo?(jorendorff)
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Assignee | ||
Comment 14•9 years ago
|
||
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
Attachment #8645020 -
Flags: approval-mozilla-beta?
Attachment #8645020 -
Flags: approval-mozilla-aurora?
Comment 15•9 years ago
|
||
Comment on attachment 8645020 [details] [diff] [review]
Outerize the receiver passed to proxy hooks
Security fix, taking it.
Attachment #8645020 -
Flags: approval-mozilla-beta?
Attachment #8645020 -
Flags: approval-mozilla-beta+
Attachment #8645020 -
Flags: approval-mozilla-aurora?
Attachment #8645020 -
Flags: approval-mozilla-aurora+
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•9 years ago
|
||
Calling esr38 wontfix since this is only sec-moderate and most of the work hanging off bug 1139700 landed on 39+ anyway.
status-b2g-v2.0:
--- → unaffected
status-b2g-v2.0M:
--- → unaffected
status-b2g-v2.1:
--- → unaffected
status-b2g-v2.1S:
--- → unaffected
Comment 19•9 years ago
|
||
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.
Flags: needinfo?(jorendorff)
Flags: needinfo?(efaustbmo)
Updated•9 years ago
|
Group: core-security → core-security-release
Updated•9 years ago
|
Whiteboard: [reporter-external] → [reporter-external][post-critsmash-triage]
Updated•9 years ago
|
Whiteboard: [reporter-external][post-critsmash-triage] → [reporter-external][post-critsmash-triage][adv-main41+]
Updated•9 years ago
|
Alias: CVE-2015-4502
Updated•8 years ago
|
Group: core-security-release
Updated•4 months ago
|
Keywords: reporter-external
You need to log in
before you can comment on or make changes to this bug.
Description
•