Last Comment Bug 803870 - (CVE-2013-0793) XSS using timed document navigations
(CVE-2013-0793)
: XSS using timed document navigations
Status: VERIFIED FIXED
[adv-main20+][adv-esr1705+]
: sec-high
Product: Core
Classification: Components
Component: Document Navigation (show other bugs)
: 16 Branch
: All All
: -- normal (vote)
: mozilla22
Assigned To: Bobby Holley (:bholley) (busy with Stylo)
:
Mentors:
Depends on: 801576
Blocks:
  Show dependency treegraph
 
Reported: 2012-10-20 11:17 PDT by Mariusz Mlynski
Modified: 2014-07-24 14:38 PDT (History)
18 users (show)
dveditz: sec‑bounty+
bobbyholley: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
tef+
+
wontfix
+
wontfix
+
verified
+
verified
+
verified
wontfix
20+
verified
20+
fixed
wontfix
fixed


Attachments
Proof of concept (1.06 KB, application/java-archive)
2012-10-20 11:18 PDT, Mariusz Mlynski
no flags Details
modified version of navigate.htm (1.08 KB, text/html)
2012-12-16 21:57 PST, moz_bug_r_a4
no flags Details
Compare subject with the outer window when invoking methods. v1 (2.72 KB, patch)
2013-02-18 15:08 PST, Bobby Holley (:bholley) (busy with Stylo)
bzbarsky: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
lukasblakk+bugs: approval‑mozilla‑beta+
akeybl: approval‑mozilla‑esr17+
akeybl: approval‑mozilla‑b2g18+
abillings: sec‑approval+
Details | Diff | Splinter Review
Tests. v1 (2.80 KB, patch)
2013-02-18 15:09 PST, Bobby Holley (:bholley) (busy with Stylo)
bzbarsky: review+
Details | Diff | Splinter Review

Description Mariusz Mlynski 2012-10-20 11:17:39 PDT
It is possible to load an arbitrary website with the document's baseURI pointing to a malicious server, which allows script injection or data theft when the page accesses a relative path.

The exploit performs several navigations to achieve a stable result. It seems to me that the crucial part is the following sequence:

opener.history.go(0);
opener.history.forward();
opener.location.hash='a';

In certain circumstances, the late hash navigation appears to confuse the browser to adopt the same BFCache and/or share a history entry between the 2 adjacent, cross-origin pages.
Comment 1 Mariusz Mlynski 2012-10-20 11:18:45 PDT
Created attachment 673604 [details]
Proof of concept

It opens https://bug757376.bugzilla.mozilla.org/attachment.cgi?id=626233 which loads script.js from a relative path after a few seconds. The script will be fetched from whatever location you upload this testcase to, so don't run it from the file system.

Tested on the current release build (16.0.1) and the latest Nightly (19.0a1, 2012-10-20).
Comment 2 Mariusz Mlynski 2012-10-20 11:47:52 PDT
Comment on attachment 673604 [details]
Proof of concept

Ugh, this doesn't appear to work from jar:, presumably due to identical URLs getting a different "t" parameter on consecutive loads -- maybe it spoils the caching somehow. Just assume to test it from another web server.
Comment 3 Boris Zbarsky [:bz] 2012-10-20 12:01:05 PDT
Sounds awfully like bug 775009 and friends...
Comment 4 Justin Lebar (not reading bugmail) 2012-10-20 12:24:09 PDT
Oh jeez.
Comment 5 Justin Lebar (not reading bugmail) 2012-10-20 12:27:05 PDT
Bug 801576 should make this a benign bug, rather than a security bug.
Comment 7 Daniel Veditz [:dveditz] 2012-10-24 10:24:28 PDT
(In reply to Boris Zbarsky (:bz) from comment #3)
> Sounds awfully like bug 775009 and friends...

Same reporter, guess the fix there wasn't sufficient.
Comment 8 Daniel Veditz [:dveditz] 2012-12-06 16:32:48 PST
(In reply to Justin Lebar [:jlebar] from comment #5)
> Bug 801576 should make this a benign bug, rather than a security bug.

That bug is a web compat change, can we take it on ESR-17?
Comment 9 Alex Keybl [:akeybl] 2012-12-06 16:33:56 PST
Also, is ESR10 affected?
Comment 10 Bobby Holley (:bholley) (busy with Stylo) 2012-12-06 16:40:37 PST
(In reply to Daniel Veditz [:dveditz] from comment #8)
> (In reply to Justin Lebar [:jlebar] from comment #5)
> > Bug 801576 should make this a benign bug, rather than a security bug.
> 
> That bug is a web compat change, can we take it on ESR-17?

Possibly, but only once that bug bubbles all the way out to the release channel and we can confirm that there are no major web compat regressions. Otherwise, I don't think it's appropriate.

(In reply to Alex Keybl [:akeybl] from comment #9)
> Also, is ESR10 affected?

AFAICT yes.

As for this bug itself, I'm not the appropriate assignee.
Comment 11 Justin Lebar (not reading bugmail) 2012-12-09 13:00:21 PST
Until B2G calms down, I'm really not going to be able to give this bug the attention it deserves.

I'm also hesitant to spend a lot of time on this bug because we've already fixed three previous incarnations of it.  It seems likely that if we fix this bug, there will be yet another hole.

Moreover a direct fix here will be risky for branches, because it's hard to tell when we're breaking an edgecase in docshell.  We've regressed peoples' banking sites at least once trying to fix a similar bug.

Bug 801576 seems like a reasonable fix to me.  If we cannot wait for that to percolate to branches, I'm likely the wrong person to write a fix for this bug for branches.
Comment 12 Boris Zbarsky [:bz] 2012-12-10 21:00:36 PST
Bobby, thoughts on last paragraph of comment 11?
Comment 13 Bobby Holley (:bholley) (busy with Stylo) 2012-12-10 22:48:14 PST
(In reply to Boris Zbarsky (:bz) from comment #12)
> Bobby, thoughts on last paragraph of comment 11?

As noted in comment 10, I don't think its appropriate to uplift bug 801576.

As for this bug, I don't understand what's going on. But it seems to be continuing drama from an issue that Justin seems to understand, so I'll defer to him on whether revoking cross-origin History access is enough here. Or should I spend some time and dig in here?
Comment 14 Boris Zbarsky [:bz] 2012-12-10 23:00:34 PST
Revoking cross-origin history access is sufficient to fix this bug, yes.
Comment 15 Justin Lebar (not reading bugmail) 2012-12-11 08:02:47 PST
> Bug 801576 seems like a reasonable fix to me.  If we cannot wait for that to percolate to 
> branches, I'm likely the wrong person to write a fix for this bug for branches.

To be clear, I wasn't arguing that we should or shouldn't take bug 801576 on branches, just that it's the path of least resistance for fixing this bug and the myriad ones like it (or at least, turning them into regular bugs instead of security bugs).
Comment 16 Daniel Veditz [:dveditz] 2012-12-13 16:45:11 PST
(In reply to Justin Lebar [:jlebar] from comment #11)
> Bug 801576 seems like a reasonable fix to me.

If that's a fix for this bug then this should be fixed in 19 and 20 already. Can we verify that?
Comment 17 Daniel Veditz [:dveditz] 2012-12-13 16:50:39 PST
Since we're worried about web-compat I can't see us taking this on ESR10. After we've gotten some beta exposure for the fix in bug 801576 we can then uplift to ESR-17 in the Fx19 timeframe if we don't break too much on the web.
Comment 18 moz_bug_r_a4 2012-12-16 21:55:21 PST
(In reply to Daniel Veditz [:dveditz] from comment #16)
> (In reply to Justin Lebar [:jlebar] from comment #11)
> > Bug 801576 seems like a reasonable fix to me.
> 
> If that's a fix for this bug then this should be fixed in 19 and 20 already.
> Can we verify that?

The fix in bug 801576 does not entirely revoke cross-origin history access.  Once a script gets a history object from a same-origin window, the script can call back(), forward(), and go() methods even when that window is no longer same-origin.

I'll attach a modified version of the reporter's testcase.
Comment 19 moz_bug_r_a4 2012-12-16 21:57:39 PST
Created attachment 692846 [details]
modified version of navigate.htm

I can reproduce the reporter's testcase on fx19 and fx20 by replacing the original file with this file.
Comment 20 Justin Lebar (not reading bugmail) 2012-12-17 07:40:07 PST
Bobby, is that intentional behavior?
Comment 21 Bobby Holley (:bholley) (busy with Stylo) 2012-12-17 10:58:42 PST
(In reply to Justin Lebar [:jlebar] from comment #20)
> Bobby, is that intentional behavior?
Which behavior, exactly? History operating on the outer window (rather than the inner)? What would you prefer it to do? Throw if its window isn't the current inner?
Comment 22 Justin Lebar (not reading bugmail) 2012-12-17 11:12:10 PST
> What would you prefer it to do? Throw if its window isn't the current inner?

Throw if the caller's principal doesn't match the inner window's principal?  That is, throw iff the caller's reading |openee.location| would throw?
Comment 23 Bobby Holley (:bholley) (busy with Stylo) 2012-12-17 13:00:51 PST
(In reply to Justin Lebar [:jlebar] from comment #22)
> > What would you prefer it to do? Throw if its window isn't the current inner?
> 
> Throw if the caller's principal doesn't match the inner window's principal? 
> That is, throw iff the caller's reading |openee.location| would throw?

Ok, that would involves a couple of simple C++ security checks in nsHistory.cpp (in back, forward, and go, I'd imagine?). Just adding a static method that compares GetWindow to do_QueryInterface<nsPIDOMWindow>(GetDocShell())->GetCurrentInnerWindow() should be sufficient.

Note that this approach is only landable on branches that have bug 801576. That is to say, it only gets us back to where we thought we were before comment 18.
Comment 24 Matt Wobensmith [:mwobensmith][:matt:] 2012-12-17 14:56:22 PST
As moz_bug_r_a4 states above, this issue can be reproduced in 20 and 19 with a modified test case.
Comment 25 Alex Keybl [:akeybl] 2013-01-23 13:29:18 PST
We're only a couple of weeks away from the release of FF19. Are we still hoping to fix in that timeframe?
Comment 26 Justin Lebar (not reading bugmail) 2013-01-23 16:44:20 PST
I would have to drop some high-priority B2G work in order to get a fix for this in docshell.  But moreover, every one of the past three fixes for this sort of bug I've come up with have been worked around, so it seems sisyphean to continue like that when we have an alternative fix.

IOW, I don't think I'm the right assignee for this bug.
Comment 27 Bobby Holley (:bholley) (busy with Stylo) 2013-01-24 03:27:42 PST
(In reply to Justin Lebar [:jlebar] from comment #26)
> I would have to drop some high-priority B2G work in order to get a fix for
> this in docshell.  But moreover, every one of the past three fixes for this
> sort of bug I've come up with have been worked around, so it seems sisyphean
> to continue like that when we have an alternative fix.

What is the alternate fix? We have bug 801576, which will be released in 19, but per comment 23 that still won't fix things unless we do a little bit of extra work, right?
Comment 28 Justin Lebar (not reading bugmail) 2013-01-24 06:53:03 PST
> but per comment 23 that still won't fix things unless we do a little bit of extra work, right?

Right.
Comment 29 David Bolter [:davidb] 2013-01-31 11:36:48 PST
Any good alternative assignees for the extra work?
Comment 30 Daniel Veditz [:dveditz] 2013-02-07 13:37:06 PST
bobby: are you the right one to do what you suggested in comment 23?
Comment 31 Bobby Holley (:bholley) (busy with Stylo) 2013-02-08 05:46:48 PST
Ok, I'll write a patch.
Comment 32 Lukas Blakk [:lsblakk] use ?needinfo 2013-02-13 11:29:38 PST
Batch edit: Bugs marked status-b2g18: affected after 2/13 branching of v1.0.1 are now also status-b2g18-v1.0.1: affected
Comment 33 Bobby Holley (:bholley) (busy with Stylo) 2013-02-18 15:08:57 PST
Created attachment 715257 [details] [diff] [review]
Compare subject with the outer window when invoking methods. v1
Comment 34 Bobby Holley (:bholley) (busy with Stylo) 2013-02-18 15:09:10 PST
Created attachment 715258 [details] [diff] [review]
Tests. v1
Comment 35 Bobby Holley (:bholley) (busy with Stylo) 2013-02-18 15:11:30 PST
https://tbpl.mozilla.org/?tree=Try&rev=79b1430a1b2a
Comment 36 Boris Zbarsky [:bz] 2013-02-19 11:28:25 PST
Comment on attachment 715257 [details] [diff] [review]
Compare subject with the outer window when invoking methods. v1

r=me
Comment 37 Boris Zbarsky [:bz] 2013-02-19 11:28:47 PST
Oh, and this probably needs to have a spec issue raised.
Comment 38 Boris Zbarsky [:bz] 2013-02-19 11:29:30 PST
Comment on attachment 715258 [details] [diff] [review]
Tests. v1

r=me
Comment 39 Bobby Holley (:bholley) (busy with Stylo) 2013-02-19 17:04:33 PST
Comment on attachment 715257 [details] [diff] [review]
Compare subject with the outer window when invoking methods. v1

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

It's clear that we're preventing access to History if the inner is not the current active Window. However, these methods were accessible by any frame, even cross-origin, until very recently (bug 801576). This patch doesn't do much to reveal the issue in comment 0.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

See above.

Which older supported branches are affected by this flaw?

All of them, I think.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

No, but it should be straightforward. Note that backporting this to esr17 would also involve backporting bug 801576, which is a behavior change. But given that we haven't heard any fallout, that might be acceptable? Probably worth asking akeybl.

How likely is this patch to cause regressions; how much testing does it need?

Not risky.
Comment 40 Bobby Holley (:bholley) (busy with Stylo) 2013-02-20 08:37:44 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/bd115faef0ee
Comment 41 Bobby Holley (:bholley) (busy with Stylo) 2013-02-20 08:39:07 PST
Comment on attachment 715257 [details] [diff] [review]
Compare subject with the outer window when invoking methods. v1

[Approval Request Comment]
Bug caused by (feature/regressing bug #): longstanding
User impact if declined: security bugs
Testing completed (on m-c, etc.): Just pushed to m-i
Risk to taking this patch (and alternatives if risky): not risky.
String or UUID changes made by this patch: none

Note that landing this on esr17 requires backporting bug 801576, which is another discussion. Flagging just m-a and m-b for now.
Comment 42 Ryan VanderMeulen [:RyanVM] 2013-02-21 07:13:58 PST
https://hg.mozilla.org/mozilla-central/rev/bd115faef0ee
Comment 43 Daniel Veditz [:dveditz] 2013-02-21 16:45:50 PST
(In reply to Bobby Holley (:bholley) from comment #41)
> Note that landing this on esr17 requires backporting bug 801576, which is
> another discussion.

I'd still like to take it. Not worried about the compat on the public web since mainline Firefox will force updates there, but slight risk of compat issues for internal apps.
Comment 44 Justin Lebar (not reading bugmail) 2013-02-21 17:12:07 PST
Thanks a lot for fixing this bug, Bobby.
Comment 45 Bobby Holley (:bholley) (busy with Stylo) 2013-02-22 08:05:21 PST
(In reply to Daniel Veditz [:dveditz] from comment #43)
> (In reply to Bobby Holley (:bholley) from comment #41)
> > Note that landing this on esr17 requires backporting bug 801576, which is
> > another discussion.
> 
> I'd still like to take it. Not worried about the compat on the public web
> since mainline Firefox will force updates there, but slight risk of compat
> issues for internal apps.

Ok, I'll flag that bug for uplift.

(In reply to Justin Lebar [:jlebar] from comment #44)
> Thanks a lot for fixing this bug, Bobby.

No problem. :-)
Comment 46 Lukas Blakk [:lsblakk] use ?needinfo 2013-02-22 15:10:39 PST
Comment on attachment 715257 [details] [diff] [review]
Compare subject with the outer window when invoking methods. v1

Approving for uplift to branches, let the discussion of ESR continue...
Comment 48 Ioana (away) 2013-02-28 06:39:07 PST
Verified as fixed on:
Mozilla/5.0 (X11; Linux i686; rv:20.0) Gecko/20100101 Firefox/20.0
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:20.0) Gecko/20100101 Firefox/20.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:20.0) Gecko/20100101 Firefox/20.0
Comment 49 Bobby Holley (:bholley) (busy with Stylo) 2013-03-01 09:00:57 PST
Comment on attachment 715257 [details] [diff] [review]
Compare subject with the outer window when invoking methods. v1

Bug 801576 has approval-esr17 now, so flagging this for uplift as well. Once this is approved, I'll push them both together.
Comment 50 Bobby Holley (:bholley) (busy with Stylo) 2013-03-03 16:59:17 PST
Ryan successfully uplifted bug 801576, so the uplift here should be relatively straightforward. If you want to take a crack at it Ryan, feel free. :-)
Comment 51 Ryan VanderMeulen [:RyanVM] 2013-03-06 08:16:47 PST
https://hg.mozilla.org/releases/mozilla-esr17/rev/f1cb8d49c4d7

I assume you'll be landing the test on the branches once Fx20 is released? Also, still wondering about whether this needs to land on the b2g18 branch(es) or not.
Comment 52 Bobby Holley (:bholley) (busy with Stylo) 2013-03-06 08:47:45 PST
Comment on attachment 715257 [details] [diff] [review]
Compare subject with the outer window when invoking methods. v1

(In reply to Ryan VanderMeulen [:RyanVM] from comment #51)
> https://hg.mozilla.org/releases/mozilla-esr17/rev/f1cb8d49c4d7
> 
> I assume you'll be landing the test on the branches once Fx20 is released?

Yes, I'm watching my in-testsuite? bugs and will batch land the tests at some point.

> Also, still wondering about whether this needs to land on the b2g18
> branch(es) or not.

Let's find out. Nominating.
Comment 53 Ryan VanderMeulen [:RyanVM] 2013-03-08 07:53:53 PST
https://hg.mozilla.org/releases/mozilla-b2g18/rev/49ed11e7cb79
Comment 54 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2013-03-08 15:03:41 PST
Flagging for verification in Firefox 21, 22, and esr17.
Comment 55 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2013-03-09 07:27:46 PST
(In reply to Anthony Hughes, Mozilla QA (:ashughes) from comment #54)
> Flagging for verification in Firefox 21, 22, and esr17.

Kamil, can you please help us out with verifying this bug?
Comment 56 Matt Wobensmith [:mwobensmith][:matt:] 2013-03-12 15:10:18 PDT
Verified on m-c 22, 2013-03-12.
Comment 57 Matt Wobensmith [:mwobensmith][:matt:] 2013-03-12 15:42:14 PDT
Verified on Aurora 21, 2013-03-12.
Comment 58 Matt Wobensmith [:mwobensmith][:matt:] 2013-03-12 17:06:52 PDT
Verified on ESR 17.0.3, 2013-03-08.
Comment 59 Alex Keybl [:akeybl] 2013-04-02 14:50:50 PDT
Since Firefox 20 is now released, v1.0.1 branches are still open, and this is considered low risk, we can uplift to v1.0.1. Marking as tef+ and flipping status-b2g18-v1.0.1 to affected.
Comment 60 Ryan VanderMeulen [:RyanVM] 2013-04-02 18:45:10 PDT
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/fdc04dffa97b

I also pushed bug 801576 with a=akeybl over IRC.
Comment 61 Bobby Holley (:bholley) (busy with Stylo) 2013-11-19 17:06:58 PST
I'm going to fold the tests for this into my tests for bug 940783.

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