Last Comment Bug 800666 - (CVE-2012-4194) Location can be spoofed using |valueOf|
: Location can be spoofed using |valueOf|
[Must Land in 17.0]
: regression, sec-high
Product: Core
Classification: Components
Component: DOM (show other bugs)
: 16 Branch
: All All
: -- normal (vote)
: ---
Assigned To: Bobby Holley (:bholley) (busy with Stylo)
: Andrew Overholt [:overholt]
Depends on: 793969
Blocks: CVE-2012-4193
  Show dependency treegraph
Reported: 2012-10-11 18:03 PDT by Mariusz Mlynski
Modified: 2014-07-24 14:37 PDT (History)
16 users (show)
dveditz: sec‑bounty+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Proof of concept (237 bytes, text/html)
2012-10-11 18:03 PDT, Mariusz Mlynski
no flags Details
Testcase for bug #800666 - shadowing location.valueOf (2.01 KB, patch)
2012-10-24 15:58 PDT, Matt Wobensmith [:mwobensmith][:matt:]
bobbyholley: review-
Details | Diff | Splinter Review

Description Mariusz Mlynski 2012-10-11 18:03:32 PDT
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.
Comment 1 Bob Clary [:bc:] 2012-10-11 18:13:54 PDT
confirmed with Firefox 16.0.1 on OSX
Comment 2 Boris Zbarsky [:bz] (still a bit busy) 2012-10-11 18:17:03 PDT
Dup of bug 793969?

Though it's odd that behavior here changed in 16.  Bobby?
Comment 3 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-10-11 18:21:28 PDT
The behavior did not change in 16, it changed in 16.0.1.
Comment 4 Mariusz Mlynski 2012-10-11 18:23:10 PDT
It appears to be a regression from bug 720619:
Comment 5 Boris Zbarsky [:bz] (still a bit busy) 2012-10-11 18:40:15 PDT
> 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 6 Boris Zbarsky [:bz] (still a bit busy) 2012-10-11 18:51:46 PDT

*** This bug has been marked as a duplicate of bug 793969 ***
Comment 8 Bobby Holley (:bholley) (busy with Stylo) 2012-10-12 05:55:32 PDT
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] 2012-10-12 09:56:13 PDT
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 12 Daniel Veditz [:dveditz] 2012-10-12 11:10:25 PDT
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] 2012-10-18 13:24:14 PDT
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] 2012-10-24 12:03:07 PDT
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:] 2012-10-24 14:27:23 PDT
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:] 2012-10-24 14:30:06 PDT
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:] 2012-10-24 15:58:32 PDT
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.
Comment 18 Bobby Holley (:bholley) (busy with Stylo) 2012-10-25 07:41:46 PDT
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:] 2012-10-25 09:36:04 PDT
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 2012-10-29 14:57:08 PDT
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] 2012-10-29 17:13:06 PDT
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) (busy with Stylo) 2012-10-30 10:16:47 PDT
(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) (busy with Stylo) 2012-11-04 07:17:50 PST
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) (busy with Stylo) 2013-03-11 11:07:18 PDT
Re-clearing in-testsuite? per comment 23.

Note You need to log in before you can comment on or make changes to this bug.