803870 - (CVE-2013-0793) XSS using timed document navigations

Status: VERIFIED FIXED

(Core :: DOM: Navigation, defect)

Description

[Mariusz Mlynski]

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]

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

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 [:bzbarsky]]

Sounds awfully like bug 775009 and friends...

Comment 4

[Justin Lebar (not reading bugmail)]

Oh jeez.

Comment 5

[Justin Lebar (not reading bugmail)]

Bug 801576 should make this a benign bug, rather than a security bug.

Comment 7

[Daniel Veditz [:dveditz]]

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

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

Also, is ESR10 affected?

Comment 10

[Bobby Holley (:bholley)]

(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)]

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 [:bzbarsky]]

Bobby, thoughts on last paragraph of comment 11?

Comment 13

[Bobby Holley (:bholley)]

(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 [:bzbarsky]]

Revoking cross-origin history access is sufficient to fix this bug, yes.

Comment 15

[Justin Lebar (not reading bugmail)]

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

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

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]

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

Attachment: 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)]

Bobby, is that intentional behavior?

Comment 21

[Bobby Holley (:bholley)]

(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)]

> 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)]

(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:]]

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

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)]

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)]

(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)]

> 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] (NeedInfo me for attention)]

Any good alternative assignees for the extra work?

Comment 30

[Daniel Veditz [:dveditz]]

bobby: are you the right one to do what you suggested in comment 23?

Comment 31

[Bobby Holley (:bholley)]

Ok, I'll write a patch.

Comment 32

[Lukas Blakk [:lsblakk] use ?needinfo]

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)]

Attachment: Compare subject with the outer window when invoking methods. v1

Comment 34

[Bobby Holley (:bholley)]

Attachment: Tests. v1

Comment 35

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=79b1430a1b2a

Comment 36

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 715257 [details] [diff] [review]
Compare subject with the outer window when invoking methods. v1

r=me

Comment 37

[Boris Zbarsky [:bzbarsky]]

Oh, and this probably needs to have a spec issue raised.

Comment 38

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 715258 [details] [diff] [review]
Tests. v1

r=me

Comment 39

[Bobby Holley (:bholley)]

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)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bd115faef0ee

Comment 41

[Bobby Holley (:bholley)]

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

https://hg.mozilla.org/mozilla-central/rev/bd115faef0ee

Comment 43

[Daniel Veditz [:dveditz]]

(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)]

Thanks a lot for fixing this bug, Bobby.

Comment 45

[Bobby Holley (:bholley)]

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

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 47

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/193316bdc446
https://hg.mozilla.org/releases/mozilla-beta/rev/98579b64434a

Comment 48

[Ioana (away)]

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)]

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)]

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

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)]

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

https://hg.mozilla.org/releases/mozilla-b2g18/rev/49ed11e7cb79

Comment 54

[u279076]

Flagging for verification in Firefox 21, 22, and esr17.

Comment 55

[u279076]

(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:]]

Verified on m-c 22, 2013-03-12.

Comment 57

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

Verified on Aurora 21, 2013-03-12.

Comment 58

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

Verified on ESR 17.0.3, 2013-03-08.

Comment 59

[Alex Keybl [:akeybl]]

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

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)]

I'm going to fold the tests for this into my tests for bug 940783.