Closed Bug 800666 (CVE-2012-4194) Opened 10 years ago Closed 10 years ago

Location can be spoofed using |valueOf|

Categories

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

16 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Tracking Status
firefox16 + verified
firefox17 + fixed
firefox18 + fixed
firefox19 + verified
firefox-esr10 --- unaffected

People

(Reporter: marius.mlynski, Assigned: bholley)

References

Details

(Keywords: regression, sec-high, Whiteboard: [Must Land in 17.0])

Attachments

(2 files)

Attached file Proof of concept
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.
Attachment #670644 - Attachment mime type: text/plain → text/html
confirmed with Firefox 16.0.1 on OSX
Status: UNCONFIRMED → NEW
Ever confirmed: true
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.
It appears to be a regression from bug 720619:

https://hg.mozilla.org/mozilla-central/rev/634a2b9859ab
> 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?
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → DUPLICATE
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. :-)
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.
Status: RESOLVED → REOPENED
Depends on: 793969
Resolution: DUPLICATE → ---
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)
Whiteboard: [Must Land in 17.0]
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
Status: REOPENED → NEW
Matt, this SHOULD be fixed in the 16.0.2. Can you please verify that this is the case?
Alias: CVE-2012-4194
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: 10 years ago10 years ago
Resolution: --- → FIXED
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.
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)
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-
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.
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?
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.
(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.
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?
Group: core-security
Flags: sec-bounty+
Flags: in-testsuite?
Re-clearing in-testsuite? per comment 23.
Flags: in-testsuite?
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.