800666 - (CVE-2012-4194) Location can be spoofed using |valueOf|

Status: RESOLVED FIXED

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

Description

[Mariusz Mlynski]

Attachment: 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.

Comment 1

[Bob Clary [:bc] (inactive)]

confirmed with Firefox 16.0.1 on OSX

Comment 2

[Boris Zbarsky [:bzbarsky]]

Dup of bug 793969?

Though it's odd that behavior here changed in 16.  Bobby?

Comment 3

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

The behavior did not change in 16, it changed in 16.0.1.

Comment 4

[Mariusz Mlynski]

It appears to be a regression from bug 720619:

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

Comment 5

[Boris Zbarsky [:bzbarsky]]

> 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?

Comment 8

[Bobby Holley (:bholley)]

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. :-)

Comment 10

[Daniel Veditz [:dveditz]]

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 11

[Daniel Veditz [:dveditz]]

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 12

[Daniel Veditz [:dveditz]]

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)

Comment 13

[David Bolter [:davidb] (NeedInfo me for attention)]

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.

Comment 14

[Al Billings [:abillings - ex-MoCo]]

Matt, this SHOULD be fixed in the 16.0.2. Can you please verify that this is the case?

Comment 15

[Matt Wobensmith [:mwobensmith][:matt:]]

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

Comment 16

[Matt Wobensmith [:mwobensmith][:matt:]]

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 17

[Matt Wobensmith [:mwobensmith][:matt:]]

Attachment: 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.

Comment 18

[Bobby Holley (:bholley)]

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").

Comment 19

[Matt Wobensmith [:mwobensmith][:matt:]]

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 20

[Lukas Blakk [:lsblakk] use ?needinfo]

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 21

[Daniel Veditz [:dveditz]]

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.

Comment 22

[Bobby Holley (:bholley)]

(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.

Comment 23

[Bobby Holley (:bholley)]

Clearing the in-testsuite? flag per comment 22, since the testcase we want to check in is already being tracked over there.

Comment 24

[Bobby Holley (:bholley)]

Re-clearing in-testsuite? per comment 23.