Closed
Bug 800666
(CVE-2012-4194)
Opened 12 years ago
Closed 12 years ago
Location can be spoofed using |valueOf|
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
People
(Reporter: marius.mlynski, Assigned: bholley)
References
Details
(Keywords: regression, sec-high, Whiteboard: [Must Land in 17.0])
Attachments
(2 files)
(deleted),
text/html
|
Details | |
(deleted),
patch
|
bholley
:
review-
|
Details | Diff | Splinter Review |
When Adobe Flash Player checks the page location to apply the SOP, it reads the return value of javascript:top.location+"__flashplugin_unique__". When an object is joined with a string, its |valueOf| method is called before |toString|, and content can redefine the former. This appears to have regressed in Firefox v16.0.1.
Reporter | ||
Updated•12 years ago
|
Attachment #670644 -
Attachment mime type: text/plain → text/html
Comment 1•12 years ago
|
||
confirmed with Firefox 16.0.1 on OSX
Status: UNCONFIRMED → NEW
Ever confirmed: true
![]() |
||
Comment 2•12 years ago
|
||
Dup of bug 793969? Though it's odd that behavior here changed in 16. Bobby?
The behavior did not change in 16, it changed in 16.0.1.
Reporter | ||
Comment 4•12 years ago
|
||
It appears to be a regression from bug 720619: https://hg.mozilla.org/mozilla-central/rev/634a2b9859ab
![]() |
||
Comment 5•12 years ago
|
||
> The behavior did not change in 16, it changed in 16.0.1.
Oh, gah. Why, why do we not have tests for this stuff? :(
It sounds like we didn't use to be calling the content-defined valueOf, but now we end up in ToPrimitive or DefaultValue (where did we end up before?) and lose?
![]() |
||
Updated•12 years ago
|
Blocks: CVE-2012-4193
![]() |
||
Updated•12 years ago
|
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → DUPLICATE
Assignee | ||
Comment 7•12 years ago
|
||
To clarify, this isn't a full dupe, given that the other bug was theoretical and this is a clear regression in 16.0.1. We are investigating. Thanks a million Mariusz for point this one out. :-)
Updated•12 years ago
|
Keywords: regression,
sec-high
Comment 8•12 years ago
|
||
Since this is a regression I'd rather reopen this and make it "depend on" the bug where you're patching it, just as bug 799952 was not a dupe of bug 720619 but was fixed by it. If nothing else we can use this bug to hold the testcase for this symptom and check it in after the fix is released.
Comment 9•12 years ago
|
||
For historical testcases see http://www.mozilla.org/security/announce/2012/mfsa2012-82.html (bug 765527) http://www.mozilla.org/security/announce/2012/mfsa2012-59.html (bug 756719) bug 735073 (incorrectly part of http://www.mozilla.org/security/announce/2012/mfsa2012-20.html -- should have had it's own CVE) http://www.mozilla.org/security/announce/2011/mfsa2011-38.html (bug 665548) http://www.mozilla.org/security/announce/2010/mfsa2010-10.html (bug 541530)
Comment 10•12 years ago
|
||
This testcase does not work on ESR 10.0.9 which contains the fix for bug 720619. But there are a lot of other differences between ESR-10 and trunk (for example, the fix for bug 754202)
status-firefox-esr10:
--- → unaffected
status-firefox16:
--- → affected
status-firefox17:
--- → affected
status-firefox18:
--- → affected
status-firefox19:
--- → affected
tracking-firefox16:
--- → ?
tracking-firefox17:
--- → +
tracking-firefox18:
--- → +
tracking-firefox19:
--- → +
Updated•12 years ago
|
Updated•12 years ago
|
Attachment #670838 -
Attachment description: Bug Bounty Nomination → Bug Bounty Awarded $3000
Updated•12 years ago
|
Whiteboard: [Must Land in 17.0]
Comment 11•12 years ago
|
||
Assigning to Bobby since he is owning bug 793969. Hope that's okay? The relationship between these bugs is confusing but is discussed in previous comments.
Assignee: nobody → bobbyholley+bmo
Updated•12 years ago
|
Status: REOPENED → NEW
Comment 12•12 years ago
|
||
Matt, this SHOULD be fixed in the 16.0.2. Can you please verify that this is the case?
Updated•12 years ago
|
Alias: CVE-2012-4194
Comment 13•12 years ago
|
||
Verified fixed on build 2012-10-24, 10.0.10esr Verified fixed on build 2012-10-24, 16.0.2 Mac 10.8.2, Windows 7 and Ubuntu 11.10
Status: NEW → RESOLVED
Closed: 12 years ago → 12 years ago
Resolution: --- → FIXED
Comment 14•12 years ago
|
||
More specifically, the test case throws a new exception: "Error: Error: Permission denied to shadow native property" Nicely done. I will make a Mochitest for this.
Comment 15•12 years ago
|
||
This Mochitest is pretty much Mariusz' PoC verbatim. If anyone feels it needs to be beefed up more, I'm open to suggestions.
Attachment #674879 -
Flags: review?(bobbyholley+bmo)
Assignee | ||
Comment 16•12 years ago
|
||
Comment on attachment 674879 [details] [diff] [review] Testcase for bug #800666 - shadowing location.valueOf I appreciate turning the PoC into a checkin-worthy test here, but this test seems like it's subsumed by the test I wrote in bug 793969 (the patch with the fix). As such, I don't see any reason to check it in, but feel free to correct me if I'm missing something. >+var result = ""; >+try { >+ location.valueOf = function(){return "anything"}; >+ result = (top.location+"__flashplugin_unique__"); >+} catch (e) { >+ result = e.toString() >+} When writing testcases like this, it's less important to reproduce the PoC verbatim, and more important to test a variety of cases and mechanisms. See my tests in bug 793969 for an example. >+is(result, "Error: Permission denied to shadow native property", I'd generally shy away from hard-coding the exact error message, because that might change. At the same time, I agree that it's good to make sure a security exception (and not something else) is what's being thrown here. I've been doing /denied|insecure/.exec(e), which seems to be a good balance (most security exceptions include the string "denied", with the exception of returning NS_ERROR_DOM_SECURITY_ERR, which prints "the operation is insecure").
Attachment #674879 -
Flags: review?(bobbyholley+bmo) → review-
Comment 17•12 years ago
|
||
Thanks Bobby, I'm 100% in agreement. Your tests already cover this, and a whole lot more. And points taken about not hard-coding an error message that might change... keying on "denied" seems smarter. No problem on rejecting this test, and thanks for instructing me.
Comment 18•12 years ago
|
||
Looks like we still need uplift to branches here - though not sure how this got to 16.0.2 without landing to branches already. Please confirm if this is just flagging out of date status flags or nominate for uplift?
Comment 19•12 years ago
|
||
The patch for this is in bug 793969 (the "Depends on" bug) and it has been checked in everywhere. I'll add "verified" where that's true according to Matt (comment 15) and "fixed" elsewhere, although it's possible this was being left open to track landing the testcase. If so it's probably better to track the testcase in a spin-off bug.
Flags: in-testsuite?
Assignee | ||
Comment 20•12 years ago
|
||
(In reply to Daniel Veditz [:dveditz] from comment #21) > although it's possible this was > being left open to track landing the testcase. If so it's probably better to > track the testcase in a spin-off bug. No, the testcase we want to check in is in bug 793969.
Assignee | ||
Comment 21•12 years ago
|
||
Clearing the in-testsuite? flag per comment 22, since the testcase we want to check in is already being tracked over there.
Flags: in-testsuite?
Updated•12 years ago
|
Attachment #670838 -
Attachment description: Bug Bounty Awarded $3000 → Bug Bounty Awarded $3000 [paid]
Updated•12 years ago
|
Group: core-security
Flags: sec-bounty+
Flags: in-testsuite?
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•