775009 - (CVE-2012-3992) History state error with late navigation involving a hash change

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect)

Description

[Mariusz Mlynski]

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.

Comment 1

[Mariusz Mlynski]

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

Comment 2

[Justin Lebar (not reading bugmail)]

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.

Comment 3

[Justin Lebar (not reading bugmail)]

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.

Comment 4

[Justin Lebar (not reading bugmail)]

Attachment: Patch, v1

Comment 5

[Justin Lebar (not reading bugmail)]

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 6

[Boris Zbarsky [:bzbarsky]]

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

r=me.  Seems reasonable.

Please raise a spec issue, though?  :(

Comment 7

[Justin Lebar (not reading bugmail)]

> 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

Comment 8

[Justin Lebar (not reading bugmail)]

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

Comment 9

[Justin Lebar (not reading bugmail)]

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.

Comment 10

[Boris Zbarsky [:bzbarsky]]

I think we just fix the test and hope.  :(

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

Comment 11

[Justin Lebar (not reading bugmail)]

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

Comment 12

[Justin Lebar (not reading bugmail)]

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.

Comment 13

[Justin Lebar (not reading bugmail)]

IE9 does the same thing as Opera: It disallows cross-origin history.back.

Comment 14

[Justin Lebar (not reading bugmail)]

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

Comment 15

[Justin Lebar (not reading bugmail)]

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.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 654816 [details] [diff] [review]
Test fix, v1

r=me

Comment 17

[Justin Lebar (not reading bugmail)]

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.

Comment 18

[Justin Lebar (not reading bugmail)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b431f498a9ba
https://hg.mozilla.org/integration/mozilla-inbound/rev/7ef5b8b2c2c7

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b431f498a9ba
https://hg.mozilla.org/mozilla-central/rev/7ef5b8b2c2c7

Comment 20

[Justin Lebar (not reading bugmail)]

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

Comment 21

[Justin Lebar (not reading bugmail)]

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.

Comment 22

[Alex Keybl [:akeybl]]

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

Comment 23

[Justin Lebar (not reading bugmail)]

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

Comment 24

[Alex Keybl [:akeybl]]

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

Comment 25

[Justin Lebar (not reading bugmail)]

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

Comment 26

[Lukas Blakk [:lsblakk] use ?needinfo]

If this gets uplifted to Beta we'll want to take it on ESR as well for 10.0.8

Comment 27

[Alex Keybl [:akeybl]]

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

Comment 28

[Justin Lebar (not reading bugmail)]

Pushed to Beta 16.

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

Comment 29

[Justin Lebar (not reading bugmail)]

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.

Comment 30

[Justin Lebar (not reading bugmail)]

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

I still owe you a html spec bug...

Comment 31

[Justin Lebar (not reading bugmail)]

https://hg.mozilla.org/releases/mozilla-esr10/rev/cf5c29f97019
https://hg.mozilla.org/releases/mozilla-esr10/rev/149fc18b680b

Comment 32

[Justin Lebar (not reading bugmail)]

I just sent mail to the whatwg about this issue.

Comment 33

[Justin Lebar (not reading bugmail)]

Should we be backing this out on branches due to bug 791011, or should we try to backport the fix as well?

Comment 34

[Boris Zbarsky [:bzbarsky]]

I think backporting the fix might be safer, actually.

Comment 35

[Daniel Veditz [:dveditz]]

Attachment: Bounty Awarded $3000 [paid] 10/3/2012