Closed Bug 541530 (CVE-2010-0170) Opened 15 years ago Closed 15 years ago

Restore paranoid location object protecting code

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
Tracking Status
blocking2.0 --- beta1+
blocking1.9.2 --- .2+
status1.9.2 --- .2-fixed
status1.9.1 --- unaffected

People

(Reporter: mrbkap, Assigned: mrbkap)

References

Details

(Keywords: regression, verified1.9.2, Whiteboard: [sg:high] maybe critical if plugins can write to disk when loaded locally)

Attachments

(3 files, 1 obsolete file)

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.
Attached patch Fix (deleted) — Splinter Review
Assignee: nobody → mrbkap
Status: NEW → ASSIGNED
Attachment #423080 - Flags: superreview?(brendan)
Attachment #423080 - Flags: review?(jst)
Attached patch Mochitest (obsolete) (deleted) — Splinter Review
I had to remove the mochitest that tested the *opposite* of this bug, too. Jonas: any more cases you can think of?
Attachment #423088 - Flags: review?(jonas)
Comment on attachment 423080 [details] [diff] [review] Fix sr=me contingent on r=jst. /be
Attachment #423080 - Flags: superreview?(brendan) → superreview+
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
Attachment #423088 - Flags: review?(jonas) → review+
Attachment #423080 - Flags: review?(jst) → review+
blocking2.0: ? → alpha1
(superfun: mozilla1.9.2.2 will be Firefox 3.6.1)
blocking1.9.2: ? → .2+
Attachment #423080 - Flags: approval1.9.2.1?
Not blocking alpha1, we'll synchronize landing this on branches etc once we're ready for that.
blocking2.0: alpha1 → beta1
Should we also add tests for testing other properties, like domain, port, etc?
Is it OK to land this on 1.9.2 without landing on trunk?
Whiteboard: [sg:high] maybe critical if plugins can write to disk when loaded locally
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.
Attachment #423080 - Flags: approval1.9.2.2? → approval1.9.2.2+
(In reply to comment #9) > Approved for 1.9.1.8, a=dveditz for release-drivers Meant a1.9.2.2+
But it won't be very obvious who depends on this or how to exploit it.
OS: Linux → All
Hardware: x86 → All
Any reason why we haven't landed this yet? Sounds like comment 11 is saying that comment 9 isn't a concern.
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.
Didn't break the 1.9.2 branch, but looks like test_bug534362.html doesn't exist on that branch.
Blocks: 534362, 492713
Yeah, I backed this out because test_bug534362.html was failing.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
http://hg.mozilla.org/mozilla-central/rev/5cc72b7b63ee (with the offending test backed out).
Status: REOPENED → RESOLVED
Closed: 15 years ago15 years ago
Resolution: --- → FIXED
mrbkap says he'll post up a local page to test his mochitest run here. Thanks mrbkap.
Ah, Clint is trying to build to run the mochitest but that would be quicker.
Attached patch Mochitest (deleted) — Splinter Review
I took this opportunity to update the mochitest to sicking's comments.
Attachment #423088 - Attachment is obsolete: true
Attachment #434069 - Flags: review+
Attached file Page for testing (deleted) —
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.
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
Status: RESOLVED → VERIFIED
Keywords: verified1.9.2
(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.
Alias: CVE-2010-0170
Group: core-security
Keywords: regression
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: