Closed
Bug 803870
(CVE-2013-0793)
Opened 13 years ago
Closed 12 years ago
XSS using timed document navigations
Categories
(Core :: DOM: Navigation, defect)
Tracking
()
People
(Reporter: marius.mlynski, Assigned: bholley)
References
Details
(Keywords: reporter-external, sec-high, Whiteboard: [adv-main20+][adv-esr1705+])
Attachments
(4 files)
1.06 KB,
application/java-archive
|
Details | |
1.08 KB,
text/html
|
Details | |
2.72 KB,
patch
|
bzbarsky
:
review+
lsblakk
:
approval-mozilla-aurora+
lsblakk
:
approval-mozilla-beta+
akeybl
:
approval-mozilla-esr17+
akeybl
:
approval-mozilla-b2g18+
abillings
:
sec-approval+
|
Details | Diff | Splinter Review |
2.80 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•13 years ago
|
||
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).
Reporter | ||
Updated•13 years ago
|
Attachment #673604 -
Attachment mime type: application/octet-stream → application/java-archive
Reporter | ||
Comment 2•13 years ago
|
||
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.
Attachment #673604 -
Attachment mime type: application/java-archive → application/zip
![]() |
||
Comment 3•13 years ago
|
||
Sounds awfully like bug 775009 and friends...
Comment 4•13 years ago
|
||
Oh jeez.
Comment 5•13 years ago
|
||
Bug 801576 should make this a benign bug, rather than a security bug.
Comment 7•12 years ago
|
||
(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.
Updated•12 years ago
|
Flags: sec-bounty?
Updated•12 years ago
|
Attachment #673604 -
Attachment mime type: application/zip → application/java-archive
Updated•12 years ago
|
status-firefox18:
--- → affected
status-firefox19:
--- → affected
status-firefox20:
--- → affected
status-firefox-esr17:
--- → affected
tracking-firefox18:
--- → +
tracking-firefox19:
--- → +
tracking-firefox20:
--- → +
tracking-firefox-esr17:
--- → ?
Updated•12 years ago
|
Assignee: nobody → justin.lebar+bug
Comment 8•12 years ago
|
||
(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?
Assignee: justin.lebar+bug → bobbyholley+bmo
Comment 9•12 years ago
|
||
Also, is ESR10 affected?
Assignee | ||
Comment 10•12 years ago
|
||
(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.
Assignee: bobbyholley+bmo → justin.lebar+bug
Comment 11•12 years ago
|
||
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.
Updated•12 years ago
|
status-firefox-esr10:
--- → affected
![]() |
||
Comment 12•12 years ago
|
||
Bobby, thoughts on last paragraph of comment 11?
Assignee | ||
Comment 13•12 years ago
|
||
(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•12 years ago
|
||
Revoking cross-origin history access is sufficient to fix this bug, yes.
Comment 15•12 years ago
|
||
> 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•12 years ago
|
||
(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?
Flags: needinfo?(mwobensmith)
Comment 17•12 years ago
|
||
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.
Updated•12 years ago
|
Comment 18•12 years ago
|
||
(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•12 years ago
|
||
I can reproduce the reporter's testcase on fx19 and fx20 by replacing the original file with this file.
Comment 20•12 years ago
|
||
Bobby, is that intentional behavior?
Assignee | ||
Comment 21•12 years ago
|
||
(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•12 years ago
|
||
> 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?
Assignee | ||
Comment 23•12 years ago
|
||
(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•12 years ago
|
||
As moz_bug_r_a4 states above, this issue can be reproduced in 20 and 19 with a modified test case.
Flags: needinfo?(mwobensmith)
Updated•12 years ago
|
Attachment #692846 -
Attachment mime type: text/plain → text/html
Updated•12 years ago
|
Flags: sec-bounty? → sec-bounty+
Comment 25•12 years ago
|
||
We're only a couple of weeks away from the release of FF19. Are we still hoping to fix in that timeframe?
tracking-firefox-esr17:
19+ → ---
Comment 26•12 years ago
|
||
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.
Assignee: justin.lebar+bug → nobody
Assignee | ||
Comment 27•12 years ago
|
||
(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•12 years ago
|
||
> but per comment 23 that still won't fix things unless we do a little bit of extra work, right?
Right.
Comment 29•12 years ago
|
||
Any good alternative assignees for the extra work?
Comment 30•12 years ago
|
||
bobby: are you the right one to do what you suggested in comment 23?
Assignee: nobody → bobbyholley+bmo
status-b2g18:
--- → affected
Assignee | ||
Comment 31•12 years ago
|
||
Ok, I'll write a patch.
Comment 32•12 years ago
|
||
Batch edit: Bugs marked status-b2g18: affected after 2/13 branching of v1.0.1 are now also status-b2g18-v1.0.1: affected
status-b2g18-v1.0.1:
--- → affected
Assignee | ||
Comment 33•12 years ago
|
||
Attachment #715257 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 34•12 years ago
|
||
Attachment #715258 -
Flags: review?(bzbarsky)
Assignee | ||
Updated•12 years ago
|
Attachment #715257 -
Attachment description: Compare History with the outer window when invoking methods. v1 → Compare subject with the outer window when invoking methods. v1
Assignee | ||
Comment 35•12 years ago
|
||
![]() |
||
Comment 36•12 years ago
|
||
Comment on attachment 715257 [details] [diff] [review]
Compare subject with the outer window when invoking methods. v1
r=me
Attachment #715257 -
Flags: review?(bzbarsky) → review+
![]() |
||
Comment 37•12 years ago
|
||
Oh, and this probably needs to have a spec issue raised.
![]() |
||
Comment 38•12 years ago
|
||
Comment on attachment 715258 [details] [diff] [review]
Tests. v1
r=me
Attachment #715258 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 39•12 years ago
|
||
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.
Attachment #715257 -
Flags: sec-approval?
Updated•12 years ago
|
Attachment #715257 -
Flags: sec-approval? → sec-approval+
Updated•12 years ago
|
status-firefox21:
--- → affected
status-firefox22:
--- → affected
tracking-firefox21:
--- → +
tracking-firefox22:
--- → +
Assignee | ||
Comment 40•12 years ago
|
||
Assignee | ||
Comment 41•12 years ago
|
||
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.
Attachment #715257 -
Flags: approval-mozilla-beta?
Attachment #715257 -
Flags: approval-mozilla-aurora?
Comment 42•12 years ago
|
||
Status: NEW → RESOLVED
Closed: 12 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
Updated•12 years ago
|
Comment 43•12 years ago
|
||
(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.
Depends on: 801576
Comment 44•12 years ago
|
||
Thanks a lot for fixing this bug, Bobby.
Assignee | ||
Comment 45•12 years ago
|
||
(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•12 years ago
|
||
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...
Attachment #715257 -
Flags: approval-mozilla-beta?
Attachment #715257 -
Flags: approval-mozilla-beta+
Attachment #715257 -
Flags: approval-mozilla-aurora?
Attachment #715257 -
Flags: approval-mozilla-aurora+
Comment 47•12 years ago
|
||
Comment 48•12 years ago
|
||
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
Assignee | ||
Comment 49•12 years ago
|
||
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.
Attachment #715257 -
Flags: approval-mozilla-esr17?
Assignee | ||
Updated•12 years ago
|
Whiteboard: [leave uplift for bholley]
Assignee | ||
Comment 50•12 years ago
|
||
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. :-)
Whiteboard: [leave uplift for bholley]
Updated•12 years ago
|
Attachment #715257 -
Flags: approval-mozilla-esr17? → approval-mozilla-esr17+
Comment 51•12 years ago
|
||
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.
Assignee | ||
Comment 52•12 years ago
|
||
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.
Attachment #715257 -
Flags: approval-mozilla-b2g18?
Updated•12 years ago
|
Updated•12 years ago
|
Attachment #715257 -
Flags: approval-mozilla-b2g18? → approval-mozilla-b2g18+
Comment 53•12 years ago
|
||
Comment 55•12 years ago
|
||
(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•12 years ago
|
||
Verified on m-c 22, 2013-03-12.
Comment 57•12 years ago
|
||
Verified on Aurora 21, 2013-03-12.
Updated•12 years ago
|
Whiteboard: [adv-main20+][adv-esr1705+]
Updated•12 years ago
|
Alias: CVE-2013-0793
Comment 59•12 years ago
|
||
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.
blocking-b2g: --- → tef+
Comment 60•12 years ago
|
||
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/fdc04dffa97b
I also pushed bug 801576 with a=akeybl over IRC.
Assignee | ||
Comment 61•11 years ago
|
||
I'm going to fold the tests for this into my tests for bug 940783.
Flags: in-testsuite? → in-testsuite+
Updated•11 years ago
|
Group: core-security
Updated•11 months ago
|
Keywords: reporter-external
You need to log in
before you can comment on or make changes to this bug.
Description
•