Bug 775009 (CVE-2012-3992)

History state error with late navigation involving a hash change

RESOLVED FIXED in Firefox 16

Status

()

Core
Document Navigation
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: Mariusz Mlynski, Assigned: Justin Lebar (not reading bugmail))

Tracking

({sec-high})

14 Branch
mozilla17
sec-high
Points:
---
Bug Flags:
sec-bounty +
in-testsuite ?

Firefox Tracking Flags

(firefox14 affected, firefox15+ wontfix, firefox16+ fixed, firefox17+ fixed, firefox-esr1016+ fixed)

Details

(Whiteboard: [advisory-tracking+])

Attachments

(3 attachments)

(Reporter)

Description

5 years ago
Docshell is still vulnerable to the history state issue described in bug 757376 if the write to location.hash is substituted for a history navigation involving a hash change. Suppose the history object is in the following state:

[0]: http://attacker
[1]: http://attacker#hash (active)
[2]: http://victim

Calling history.forward and history.back in a rapid succession will cause the targeted website to load into the existing session history entry 0:

[0]: http://attacker (active)
[1]: http://attacker#hash
[2]: http://victim

A subsequent call to history.forward changes |baseURI|, which allows the attacker to inject a script loaded from a relative path (XSS) or intercept data posted to a location specified with a relative path.
(Reporter)

Comment 1

5 years ago
Created attachment 643279 [details]
Proof of concept

It opens https://bug757376.bugzilla.mozilla.org/attachment.cgi?id=626233 which loads script.js 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.
(Reporter)

Updated

5 years ago
Attachment #643279 - Attachment mime type: application/octet-stream → application/java-archive
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: sec-high
(Assignee)

Comment 2

5 years ago
Somehow we get into a state where the following happens (I'm loading HN, instead of bugzilla).

Load http://localhost/~jlebar/PoC/navigate.htm
Load http://localhost/~jlebar/PoC/index.htm#h
Load http://news.ycombinator.com/
Go back to http://localhost/~jlebar/PoC/index.htm#h, aSHEntry=0x141e74a10, shentrySpec=http://localhost/~jlebar/PoC/index.htm#h
Go forward to http://news.ycombinator.com/, aSHEntry=0x117c21790, shentrySpec=http://news.ycombinator.com/
Go back to http://localhost/~jlebar/PoC/index.htm, aSHEntry=0x10d4ed160, shentrySpec=http://localhost/~jlebar/PoC/index.htm
Go forward to http://localhost/~jlebar/PoC/index.htm#h, aSHEntry=0x141e74a10, shentrySpec=http://localhost/~jlebar/PoC/index.htm#h
Now the location bar says localhost, but the content displayed is hacker news.

Notice that when we go back for the second time, it's not to the right page.

In addition to hacking around this as we have in the past, I wonder if we can detect that docshell has become confused and either fix things or, as Jesse has suggested, assert so his fuzzer will notice.
Assignee: nobody → justin.lebar+bug
(Assignee)

Comment 3

5 years ago
This bug appears to be different from the other ones we've seen, in that it doesn't rely on session history confusion.

Instead, it appears that we're merely allowing an existing channel to finish loading after we've performed a history load.  That is, we do:

  a. load attack.html#h
  b. start loading news.ycombinator.com
  c. go back
  d. news.ycombinator finishes loading, into the wrong shentry.

Normally we'd call stop() when we went back, but I think what's messing us up is the fact that we can't call stop in step (3) of the following case:

  1. load page.html
  2. start loading page2.html
  3. load page.html#h

So I think what's happening is, the back in step (c) doesn't cause us to call Stop.
(Assignee)

Comment 4

5 years ago
Created attachment 653199 [details] [diff] [review]
Patch, v1
Attachment #653199 - Flags: review?(bzbarsky)
(Assignee)

Comment 5

5 years ago
I still feel like we're skirting around this issue, but I'm not sure how to detect this more generally.

Perhaps we should periodically assert that the document's channel is same-origin as the document's URI.
Comment on attachment 653199 [details] [diff] [review]
Patch, v1

r=me.  Seems reasonable.

Please raise a spec issue, though?  :(
Attachment #653199 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 7

5 years ago
> Please raise a spec issue, though?  :(

I made a note to bring it up after this bakes for a few days.

https://tbpl.mozilla.org/?tree=Try&rev=26c20a0ae5d3
(Assignee)

Comment 8

5 years ago
> TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/whatwg/test_bug500328.html | Test timed out.

I wrote that test, too.  :(

Sorry, I should have pushed to try before asking for review.
(Assignee)

Comment 9

5 years ago
It looks like that's the only test failure.

Now the question is: Do we think that this change will break the web (as it did the last time that we had a test fail because of one of these changes), or do we want to fix the test?

I'm not sure how to know whether this will break the web without pushing it, so maybe I should just fix the test.
I think we just fix the test and hope.  :(

Do we have any idea whether other UAs issue a stop in this case?
(Assignee)

Comment 11

5 years ago
> Do we have any idea whether other UAs issue a stop in this case?

(Interestingly, it seems that Chrome doesn't throw a catchable exception when you touch a cross-origin opener.location.href.  Instead, it returns undefined and prints a warning to the console.)

Anyway, if you work around that, Chrome cancels the load.  The shistory is

  attack.htm    <-- oldest
  attack.htm#h  <-- current
  news.ycombinator.com <-- newest

which matches our behavior with this patch.

I'll see what other browsers do.
(Assignee)

Comment 12

5 years ago
Safari unsurprisingly matches Chrome.

Opera doesn't even let me do cross-origin history.back, which I suppose solves this problem quite nicely.  :)

Now for IE.
(Assignee)

Comment 13

5 years ago
IE9 does the same thing as Opera: It disallows cross-origin history.back.
(Assignee)

Comment 14

5 years ago
Created attachment 654816 [details] [diff] [review]
Test fix, v1

I didn't look too closely at this horrible, horrible test I wrote.  It was hitting "already in a generator" on this gGen.next() call, so this was the obvious fix.
Attachment #654816 - Flags: review?(bzbarsky)
(Assignee)

Comment 15

5 years ago
I just verified that all versions are affected by this bug.

We'll likely not want to land this on beta now, given how close we are to release, and given that we have close to no idea if this will break the web.
status-firefox-esr10: --- → affected
status-firefox14: --- → affected
status-firefox15: --- → affected
status-firefox16: --- → affected
status-firefox17: --- → affected
tracking-firefox-esr10: --- → ?
tracking-firefox15: --- → ?
tracking-firefox16: --- → ?
tracking-firefox17: --- → ?

Updated

5 years ago
status-firefox15: affected → wontfix
tracking-firefox-esr10: ? → ---
tracking-firefox15: ? → +
tracking-firefox16: ? → +
tracking-firefox17: ? → +
Comment on attachment 654816 [details] [diff] [review]
Test fix, v1

r=me
Attachment #654816 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 17

5 years ago
I've landed this even though I think there's a significant chance of a regression, because it's trivial to back out on Aurora (comment out one line), and we need test coverage as quickly as possible so we can know whether it's safe to push to beta.
(Assignee)

Comment 18

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/b431f498a9ba
https://hg.mozilla.org/integration/mozilla-inbound/rev/7ef5b8b2c2c7
https://hg.mozilla.org/mozilla-central/rev/b431f498a9ba
https://hg.mozilla.org/mozilla-central/rev/7ef5b8b2c2c7
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
(Assignee)

Comment 20

5 years ago
Comment on attachment 653199 [details] [diff] [review]
Patch, v1

[Approval Request Comment]
Bug caused by (feature/regressing bug #): been around for a long time, maybe bug 500328 or related.
User impact if declined: xss any page
Testing completed (on m-c, etc.): pushed to m-c, will soon be uplifted to aurora.
Risk to taking this patch (and alternatives if risky): docshell changes always risk web regressions, and this one is particularly risky, judging from past experience with similar changes.  We should consider carefully what's the right amount of time to allow this to bake before pushing to Beta.
String or UUID changes made by this patch: none
Attachment #653199 - Flags: approval-mozilla-esr10?
Attachment #653199 - Flags: approval-mozilla-beta?
(Assignee)

Comment 21

5 years ago
Comment on attachment 653199 [details] [diff] [review]
Patch, v1

erm, we don't need the ESR? flag, since 17 is going to be uplifted to ESR.
Attachment #653199 - Flags: approval-mozilla-esr10?
(In reply to Justin Lebar [:jlebar] from comment #20)
> Risk to taking this patch (and alternatives if risky): docshell changes
> always risk web regressions, and this one is particularly risky, judging
> from past experience with similar changes.  We should consider carefully
> what's the right amount of time to allow this to bake before pushing to Beta.

If we'd like to get this sec-high fix into FF16, I think we should land it no later than Beta 2. That'll at least ensure that we get over a week of regression finding on Nightly/Aurora, while still giving enough bake time on Beta to find regressions at scale before release.

Any tips for QA to do exploratory regression testing in the meantime would be helpful.
(Assignee)

Comment 23

5 years ago
> If we'd like to get this sec-high fix into FF16, I think we should land it no later than 
> Beta 2

Do you have an approximate date for Beta 2?

> Any tips for QA to do exploratory regression testing in the meantime would be helpful.

With similar changes in the past, we had problems with people logging into their banks (e.g. bug 739478).  I'd expect to see regressions here in similar situations, where you do some automatic navigation of Firefox from one page to another.
(In reply to Justin Lebar [:jlebar] from comment #23)
> > If we'd like to get this sec-high fix into FF16, I think we should land it no later than 
> > Beta 2
> 
> Do you have an approximate date for Beta 2?

Beta 2 will go to build next Tuesday, so we can approve/land Monday if that's OK by you.
(Assignee)

Comment 25

5 years ago
> Beta 2 will go to build next Tuesday, so we can approve/land Monday if that's OK by you.

That feels a bit aggressive to me; I'm not sure a week is long enough to identify any potential problems.  OTOH this is a serious bug that we shouldn't ship unless we can't help it.  So I guess I'm OK landing on Monday so long as you're OK possibly backing out on beta late in the cycle.
If this gets uplifted to Beta we'll want to take it on ESR as well for 10.0.8
tracking-firefox-esr10: --- → 16+
(In reply to Justin Lebar [:jlebar] from comment #25)
> > Beta 2 will go to build next Tuesday, so we can approve/land Monday if that's OK by you.
> 
> That feels a bit aggressive to me; I'm not sure a week is long enough to
> identify any potential problems.  OTOH this is a serious bug that we
> shouldn't ship unless we can't help it.  So I guess I'm OK landing on Monday
> so long as you're OK possibly backing out on beta late in the cycle.

I'm OK with that. Waiting any longer would delay Beta feedback about regressions from this bug till the fourth week of the beta (pretty late).

Updated

5 years ago
Attachment #653199 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(Assignee)

Comment 28

5 years ago
Pushed to Beta 16.

https://hg.mozilla.org/releases/mozilla-beta/rev/2269e70e9ec1
https://hg.mozilla.org/releases/mozilla-beta/rev/a3fa0ab85787
(Assignee)

Comment 29

5 years ago
Comment on attachment 653199 [details] [diff] [review]
Patch, v1

[Approval Request Comment]
I take it from lsblakk's earlier comment that we want this for ESR.
Attachment #653199 - Flags: approval-mozilla-esr10?

Updated

5 years ago
Attachment #653199 - Flags: approval-mozilla-esr10? → approval-mozilla-esr10+

Updated

5 years ago
status-firefox16: affected → fixed
status-firefox17: affected → fixed
(Assignee)

Comment 30

5 years ago
Looks like we managed to do this on our first try without breaking the web!

I still owe you a html spec bug...
(Assignee)

Comment 31

5 years ago
https://hg.mozilla.org/releases/mozilla-esr10/rev/cf5c29f97019
https://hg.mozilla.org/releases/mozilla-esr10/rev/149fc18b680b
status-firefox-esr10: affected → fixed
(Assignee)

Comment 32

5 years ago
I just sent mail to the whatwg about this issue.
Depends on: 791011
(Assignee)

Comment 33

5 years ago
Should we be backing this out on branches due to bug 791011, or should we try to backport the fix as well?
I think backporting the fix might be safer, actually.
Created attachment 663197 [details]
Bounty Awarded $3000 [paid] 10/3/2012
Whiteboard: [advisory-tracking+]
Alias: CVE-2012-3992
Group: core-security
Flags: sec-bounty+
You need to log in before you can comment on or make changes to this bug.