Last Comment Bug 765527 - (CVE-2012-3994) Object.defineProperty can shadow |top|
(CVE-2012-3994)
: Object.defineProperty can shadow |top|
Status: RESOLVED FIXED
[advisory-tracking+]
: sec-high
Product: Core
Classification: Components
Component: DOM (show other bugs)
: 13 Branch
: All All
: -- normal (vote)
: mozilla17
Assigned To: Andrew McCreight [:mccr8]
:
:
Mentors:
Depends on: CVE-2012-1956 808404
Blocks: CVE-2012-4209
  Show dependency treegraph
 
Reported: 2012-06-16 18:11 PDT by Mariusz Mlynski
Modified: 2014-07-27 11:29 PDT (History)
14 users (show)
dveditz: sec‑bounty+
ryanvm: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
wontfix
+
wontfix
+
wontfix
+
fixed
+
fixed
fixed
16+
fixed


Attachments
shadow top.location test (300 bytes, text/html)
2012-06-16 18:11 PDT, Mariusz Mlynski
no flags Details
cargo culted patch (5.03 KB, patch)
2012-08-08 15:33 PDT, Andrew McCreight [:mccr8]
no flags Details | Diff | Splinter Review
friendlier version of the original test (532 bytes, text/html)
2012-08-08 16:06 PDT, Andrew McCreight [:mccr8]
no flags Details
v2, cargo culted patch (5.04 KB, patch)
2012-08-10 13:39 PDT, Andrew McCreight [:mccr8]
mrbkap: review+
justin.lebar+bug: feedback+
lukasblakk+bugs: approval‑mozilla‑beta+
lukasblakk+bugs: approval‑mozilla‑esr10+
Details | Diff | Splinter Review
ESR 10 version (4.94 KB, patch)
2012-09-10 11:33 PDT, Andrew McCreight [:mccr8]
no flags Details | Diff | Splinter Review
Add test for shadowing window.top. (2.43 KB, patch)
2014-07-25 14:30 PDT, Andrew McCreight [:mccr8]
no flags Details | Diff | Splinter Review

Description Mariusz Mlynski 2012-06-16 18:11:00 PDT
Created attachment 633854 [details]
shadow top.location test

Filing a new bug, as requested by Boris in Bug 756719 comment 10.

With bug 750307 fixed, the location property itself seems to be hardened well enough, but plugins may try to access it through |top.location| -- for instance, Adobe Flash Player opens javascript:top.location+"__flashplugin_unique__" to determine the page origin. And it is possible to shadow |top| using Object.defineProperty.

Incidentally, Google Chrome seems to disallow redefining |top|.
Comment 2 Blake Kaplan (:mrbkap) 2012-06-18 15:53:16 PDT
Can we give the flash player Xray wrappers by default or something?
Comment 3 Bobby Holley (:bholley) (busy with Stylo) 2012-06-19 00:53:32 PDT
(In reply to Blake Kaplan (:mrbkap) from comment #2)
> Can we give the flash player Xray wrappers by default or something?

That would involve running NPAPI JS in a separate compartment. Which would be great, but I'm not signing up for that...
Comment 4 Blake Kaplan (:mrbkap) 2012-06-21 14:41:04 PDT
Even before we get that far: does flash JS need to see things like global variables by default? If so, we're sunk before we start.
Comment 5 Daniel Veditz [:dveditz] 2012-06-21 16:43:22 PDT
Can we simply disallow mucking with top (and parent/owner while you're at it?).
Comment 6 Johnny Stenback (:jst, jst@mozilla.com) 2012-07-31 16:21:57 PDT
Andrew says he can look into this, and yes, I think the answer here is to make window.top permanent so it can't be mucked with.
Comment 7 Andrew McCreight [:mccr8] 2012-08-08 15:33:59 PDT
Created attachment 650340 [details] [diff] [review]
cargo culted patch

Per jst's suggestion, I just copied what is done for sNavigator_id. I'm going to try to understand what it actually is doing...

Anyways, with this patch, the test produces this error:

JavaScript error: file:///Users/amccreight/mz/tests/765527.html, line 3: can't redefine non-configurable property 'top'

and does not produce a pop alert.
Comment 8 Andrew McCreight [:mccr8] 2012-08-08 16:06:29 PDT
Created attachment 650351 [details]
friendlier version of the original test

The original test seemed to hang my browser. This does the same thing, it just checks top.location.toString() directly instead of using a plugin. Without the patch, it evaluates to the bogus string, with it, it produces the correct URL.
Comment 9 Andrew McCreight [:mccr8] 2012-08-10 13:39:55 PDT
Created attachment 650991 [details] [diff] [review]
v2, cargo culted patch

I was getting some browser element failures, but jlebar pointed out GetScriptableTop(), and switching to that fixed the problem with those tests.
Comment 10 Andrew McCreight [:mccr8] 2012-08-15 12:41:37 PDT
Comment on attachment 650991 [details] [diff] [review]
v2, cargo culted patch

I don't know this code very well.  If top changes, this isn't going to be right because we're caching the result, but I think that's not possible, unless it is possible for the weird scripted stuff.  If you could give feedback on the scripted top stuff, Justin (eg can it change) that would be good...
Comment 11 Andrew McCreight [:mccr8] 2012-08-15 12:47:45 PDT
Comment on attachment 650991 [details] [diff] [review]
v2, cargo culted patch

If Justin says that we can't just cache the top value, we'll need a different approach that replaces the getter or something, so I'm clearing the review flag for now.
Comment 12 Justin Lebar (not reading bugmail) 2012-08-15 12:49:31 PDT
Suppose I move an iframe from one document to another.  Does the iframe's contentWindow's outer window survive the re-parenting?  That is, if I do

  var w = iframe.contentWindow;
  [move iframe from one document to another]
  w.src = "http://google.com";

does that have any effect?  If so, then the outer window's .top changes, so GetScriptableTop should change to match.

If not, then the only other circumstance I can think of which would cause GetScriptableTop to change is if you change a docshell's is-browser or is-app property, which you shouldn't do, and will likely break all sorts of other things.
Comment 13 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2012-08-15 12:52:16 PDT
(In reply to Justin Lebar [:jlebar] from comment #12)
> Suppose I move an iframe from one document to another.
In that case we reload the iframe.
Comment 14 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2012-08-15 12:52:36 PDT
... and in fact re-create docshell too.
Comment 15 Justin Lebar (not reading bugmail) 2012-08-15 12:55:28 PDT
If we re-create the docshell, then this should be fine.
Comment 16 Andrew McCreight [:mccr8] 2012-08-15 14:32:20 PDT
Comment on attachment 650991 [details] [diff] [review]
v2, cargo culted patch

Thanks for the analysis!
Comment 17 Blake Kaplan (:mrbkap) 2012-08-15 15:10:00 PDT
Comment on attachment 650991 [details] [diff] [review]
v2, cargo culted patch

Who knows what evil lurks in the hearts of men?
Comment 18 Alex Keybl [:akeybl] 2012-08-23 15:47:59 PDT
Let's wait for this to be fixed on trunk before tracking for a specific ESR version again.
Comment 19 Alex Keybl [:akeybl] 2012-08-23 16:50:59 PDT
(In reply to Andrew McCreight [:mccr8] from comment #16)
> Comment on attachment 650991 [details] [diff] [review]
> v2, cargo culted patch
> 
> Thanks for the analysis!

Let's get this landed on m-c for possible aurora/beta 16 uplift.
Comment 20 Andrew McCreight [:mccr8] 2012-08-25 14:57:49 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/573c753b7bc8
Comment 21 Ryan VanderMeulen [:RyanVM] 2012-08-25 19:31:43 PDT
https://hg.mozilla.org/mozilla-central/rev/573c753b7bc8
Comment 22 Andrew McCreight [:mccr8] 2012-08-28 10:14:05 PDT
This should get a testcase checked in, once it is made public.
Comment 23 Andrew McCreight [:mccr8] 2012-08-30 20:54:28 PDT
Comment on attachment 650351 [details]
friendlier version of the original test

[Approval Request Comment]
User impact if declined: possible security risks with Flash
Bug caused by (feature/regressing bug #): none
Fix Landed on Version: it has been on 17 for a few days
Risk to taking this patch (and alternatives if risky): This changes web-exposed behavior which could potentially be bad, but Chrome already does this, so jst doesn't think it should be too bad. No real alternatives. I could leave it on Aurora and Nightly for another week or two to see if anything pops up.
String or UUID changes made by this patch: none
Comment 24 Andrew McCreight [:mccr8] 2012-08-30 20:55:21 PDT
Comment on attachment 650991 [details] [diff] [review]
v2, cargo culted patch

Err, that message was for this attachment, not the other one...
Comment 25 Andrew McCreight [:mccr8] 2012-08-30 20:57:06 PDT
The test will have "GOOD!" in it twice if it passes, in the popup, otherwise one or both will be "BAD!" instead.
Comment 26 Alex Keybl [:akeybl] 2012-08-31 15:49:24 PDT
(In reply to Andrew McCreight [:mccr8] from comment #23)
> Risk to taking this patch (and alternatives if risky): This changes
> web-exposed behavior which could potentially be bad, but Chrome already does
> this, so jst doesn't think it should be too bad. No real alternatives. I
> could leave it on Aurora and Nightly for another week or two to see if
> anything pops up.

Let's target this fix for Beta 3, so that we can have a week on Aurora 17 of bake time. Normally I'd approve a change like this as early as possible to find regressions, but I'm concerned that if we land it on Beta and then find regressions, we'll be committed to still fixing in 16.
Comment 27 Andrew McCreight [:mccr8] 2012-09-10 11:33:52 PDT
Created attachment 659796 [details] [diff] [review]
ESR 10 version

It works fine for beta, with minor adjustments. On ESR10, I had to change GetScriptedTop to GetTop, but even with that the test still fails with the patch applied, even though we are hitting the code I added. I'm not sure why that is.
Comment 28 Andrew McCreight [:mccr8] 2012-09-18 09:17:09 PDT
Ah, so, this isn't working on ESR10 because bug 756719 never landed on ESR10.
Comment 29 Andrew McCreight [:mccr8] 2012-09-18 09:53:07 PDT
I'll wait until that other bug lands on ESR10 to land this one there.
https://hg.mozilla.org/releases/mozilla-beta/rev/e7bcf3d49761
Comment 30 Andrew McCreight [:mccr8] 2012-10-02 17:21:50 PDT
I confirmed that it worked on ESR10 with bug 756719, but it ended up getting backed out, so I'll wait until that sorts itself out before landing.
Comment 31 Andrew McCreight [:mccr8] 2012-10-04 15:40:51 PDT
https://hg.mozilla.org/releases/mozilla-esr10/rev/913caf5a7cf0
Comment 32 Andrew McCreight [:mccr8] 2012-10-04 17:14:43 PDT
Backed out because bug 756719 (née bug 750307) was backed out:
  https://hg.mozilla.org/releases/mozilla-esr10/rev/96931ca23431
Comment 33 Andrew McCreight [:mccr8] 2012-10-05 11:29:03 PDT
Once more, with feeling.

https://hg.mozilla.org/releases/mozilla-esr10/rev/d3922c1c120e
Comment 34 Tracy Walker [:tracy] 2014-01-10 10:40:02 PST
mass remove verifyme requests greater than 4 months old
Comment 36 Andrew McCreight [:mccr8] 2014-07-25 14:30:28 PDT
Created attachment 8462846 [details] [diff] [review]
Add test for shadowing window.top.

I don't really know what I'm doing here, so I'd appreciate a review. I'm not sure how useful this test really is, as this will be much harder to mess up in the WebIDL world, but it is probably useful.
Comment 37 Andrew McCreight [:mccr8] 2014-07-27 11:29:05 PDT
Comment on attachment 8462846 [details] [diff] [review]
Add test for shadowing window.top.

test fails on try

Note You need to log in before you can comment on or make changes to this bug.