Note: There are a few cases of duplicates in user autocompletion which are being worked on.
Bug 800666 (CVE-2012-4194)

Location can be spoofed using |valueOf|

RESOLVED FIXED

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: Mariusz Mlynski, Assigned: bholley)

Tracking

({regression, sec-high})

16 Branch
regression, sec-high
Points:
---
Dependency tree / graph
Bug Flags:
sec-bounty +

Firefox Tracking Flags

(firefox16+ verified, firefox17+ fixed, firefox18+ fixed, firefox19+ verified, firefox-esr10 unaffected)

Details

(Whiteboard: [Must Land in 17.0])

Attachments

(2 attachments)

(Reporter)

Description

5 years ago
Created attachment 670644 [details]
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.
(Reporter)

Updated

5 years ago
Attachment #670644 - Attachment mime type: text/plain → text/html

Comment 1

5 years ago
confirmed with Firefox 16.0.1 on OSX
Status: UNCONFIRMED → NEW
Ever confirmed: true

Comment 2

5 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

5 years ago
It appears to be a regression from bug 720619:

https://hg.mozilla.org/mozilla-central/rev/634a2b9859ab

Comment 5

5 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

5 years ago
Blocks: 720619

Updated

5 years ago
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 793969
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. :-)
Keywords: regression, sec-high
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 → ---
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)
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

5 years ago
tracking-firefox16: ? → +

Updated

5 years ago
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
Last Resolved: 5 years ago5 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.
Created attachment 674879 [details] [diff] [review]
Testcase for bug #800666 - shadowing location.valueOf

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.
status-firefox16: affected → verified
status-firefox17: affected → fixed
status-firefox18: affected → fixed
status-firefox19: affected → verified
Flags: in-testsuite?
(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?
You need to log in before you can comment on or make changes to this bug.