Last Comment Bug 541530 - (CVE-2010-0170) Restore paranoid location object protecting code
(CVE-2010-0170)
: Restore paranoid location object protecting code
Status: VERIFIED FIXED
[sg:high] maybe critical if plugins c...
: regression, verified1.9.2
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Blake Kaplan (:mrbkap) (please use needinfo!)
:
Mentors:
Depends on:
Blocks: 492713 534362
  Show dependency treegraph
 
Reported: 2010-01-22 15:25 PST by Blake Kaplan (:mrbkap) (please use needinfo!)
Modified: 2010-11-09 18:35 PST (History)
13 users (show)
mrbkap: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
beta1+
.2+
.2-fixed
unaffected


Attachments
Fix (2.38 KB, patch)
2010-01-22 15:29 PST, Blake Kaplan (:mrbkap) (please use needinfo!)
jst: review+
brendan: superreview+
dveditz: approval1.9.2.2+
Details | Diff | Review
Mochitest (5.28 KB, patch)
2010-01-22 15:53 PST, Blake Kaplan (:mrbkap) (please use needinfo!)
jonas: review+
Details | Diff | Review
Mochitest (3.01 KB, patch)
2010-03-22 16:28 PDT, Blake Kaplan (:mrbkap) (please use needinfo!)
mrbkap: review+
Details | Diff | Review
Page for testing (1.95 KB, text/html)
2010-03-22 16:31 PDT, Blake Kaplan (:mrbkap) (please use needinfo!)
no flags Details

Description Blake Kaplan (:mrbkap) (please use needinfo!) 2010-01-22 15:25:31 PST
In bug 534362 and bug 492713, we removed some code protecting the location object (on both the document and the window) because it isn't needed anymore for either web content or extensions (web pages are allowed to confuse themselves to their heart's content). In doing this, we forgot that plugins also use location.href to figure out what page they've been embedded in.

The real fix for this bug would be to provide an API in NPAPI to allow plugins to figure out where they are, but in the meantime, we need to re-overprotect the location object.
Comment 1 Blake Kaplan (:mrbkap) (please use needinfo!) 2010-01-22 15:29:50 PST
Created attachment 423080 [details] [diff] [review]
Fix
Comment 2 Blake Kaplan (:mrbkap) (please use needinfo!) 2010-01-22 15:53:19 PST
Created attachment 423088 [details] [diff] [review]
Mochitest

I had to remove the mochitest that tested the *opposite* of this bug, too. Jonas: any more cases you can think of?
Comment 3 Brendan Eich [:brendan] 2010-01-22 16:00:57 PST
Comment on attachment 423080 [details] [diff] [review]
Fix

sr=me contingent on r=jst.

/be
Comment 4 Jonas Sicking (:sicking) 2010-01-22 16:07:58 PST
Comment on attachment 423088 [details] [diff] [review]
Mochitest

>+var orig = window;
>+window = {};
>+
>+ok(window === orig, "can't override window");
>+ok(window.location === location, "properties are properly aliased");
>+ok(document.location === location, "properties are properly aliased");

Do these tests at the end too. But grab a copy of location first.

>+try {
>+    __defineGetter__('window', function() {});
>+    ok(false, "should not be able to defineGetter(window)");
>+} catch (e) {
>+    ok(true, "can't defineGetter(window)");
>+}

The last 'ok' here (and the other ones below like it) seems pretty useless.

r=me with that fixed
Comment 5 Mike Beltzner [:beltzner, not reading bugmail] 2010-01-26 00:58:06 PST
(superfun: mozilla1.9.2.2 will be Firefox 3.6.1)
Comment 6 Johnny Stenback (:jst, jst@mozilla.com) 2010-02-01 14:58:44 PST
Not blocking alpha1, we'll synchronize landing this on branches etc once we're ready for that.
Comment 7 Johnny Stenback (:jst, jst@mozilla.com) 2010-02-01 15:00:34 PST
Should we also add tests for testing other properties, like domain, port, etc?
Comment 8 Daniel Veditz [:dveditz] 2010-02-05 13:31:45 PST
Is it OK to land this on 1.9.2 without landing on trunk?
Comment 9 Daniel Veditz [:dveditz] 2010-02-05 13:36:20 PST
Comment on attachment 423080 [details] [diff] [review]
Fix

Approved for 1.9.1.8, a=dveditz for release-drivers

ugh, the problem is going to be pretty obvious if people are watching the patches.
Comment 10 Daniel Veditz [:dveditz] 2010-02-05 13:40:09 PST
(In reply to comment #9)
> Approved for 1.9.1.8, a=dveditz for release-drivers

Meant a1.9.2.2+
Comment 11 Johnny Stenback (:jst, jst@mozilla.com) 2010-02-05 14:16:52 PST
But it won't be very obvious who depends on this or how to exploit it.
Comment 12 Mike Beltzner [:beltzner, not reading bugmail] 2010-03-03 21:47:31 PST
Any reason why we haven't landed this yet? Sounds like comment 11 is saying that comment 9 isn't a concern.
Comment 14 Daniel Veditz [:dveditz] 2010-03-04 23:30:14 PST
Also checked in: http://hg.mozilla.org/mozilla-central/rev/9912050f4a4a

I can see why you didn't want to land the new test--it gives away the problem--but you still needed to land the removal of the test for bug 534362; test are burning on trunk. I think roc has just backed you out, though.
Comment 15 Daniel Veditz [:dveditz] 2010-03-04 23:39:52 PST
Didn't break the 1.9.2 branch, but looks like test_bug534362.html  doesn't exist on that branch.
Comment 16 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2010-03-05 00:04:03 PST
Yeah, I backed this out because test_bug534362.html was failing.
Comment 17 Blake Kaplan (:mrbkap) (please use needinfo!) 2010-03-05 11:58:59 PST
http://hg.mozilla.org/mozilla-central/rev/5cc72b7b63ee (with the offending test backed out).
Comment 18 Tony Chung [:tchung] 2010-03-22 16:05:16 PDT
mrbkap says he'll post up a local page to test his mochitest run here.   Thanks mrbkap.
Comment 19 Al Billings [:abillings] 2010-03-22 16:08:11 PDT
Ah, Clint is trying to build to run the mochitest but that would be quicker.
Comment 20 Blake Kaplan (:mrbkap) (please use needinfo!) 2010-03-22 16:28:39 PDT
Created attachment 434069 [details] [diff] [review]
Mochitest

I took this opportunity to update the mochitest to sicking's comments.
Comment 21 Blake Kaplan (:mrbkap) (please use needinfo!) 2010-03-22 16:31:14 PDT
Created attachment 434072 [details]
Page for testing

This is just a modified version of the mochitest with the mochitest stuff stripped out, a stupid ok() implementation and an alert at the end to tell you that it passed. Hopefully, running this is self explanatory.
Comment 22 Tony Chung [:tchung] 2010-03-22 16:33:33 PDT
Thanks blake!

With the testpage from comment 21, 
Verified fix on Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.2.2) Gecko/20100316 Firefox/3.6.2
and 
Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.3a4pre) Gecko/20100321 Minefield/3.7a4pre
Comment 23 cmtalbert 2010-03-22 16:39:47 PDT
(In reply to comment #20)
> Created an attachment (id=434069) [details]
> Mochitest
> 
> I took this opportunity to update the mochitest to sicking's comments.

Applied this patch to 1.9.2 on Windows 7 build and all tests passed.

Built From: 
changeset:   33751:d1eb6b2b3ded
tag:         qtip
tag:         tip
tag:         mrbkap.diff
tag:         qbase
user:        Clint Talbert <ctalbert@mozilla.com>
date:        Mon Mar 22 16:32:44 2010 -0700
summary:     imported patch mrbkap.diff

changeset:   33750:d5bfbe40cf5f
tag:         qparent
user:        Ben Turner <bent.mozilla@gmail.com>
date:        Mon Mar 22 12:32:47 2010 -0700
summary:     Bug 551233. a1.9.2.3=beltzner.
Comment 24 Blake Kaplan (:mrbkap) (please use needinfo!) 2010-11-09 18:35:30 PST
http://hg.mozilla.org/mozilla-central/rev/0acbb0c1a645

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