757376 - (CVE-2012-1955) History navigation error with late location.hash changes

Status: VERIFIED FIXED

(Core :: DOM: Navigation, defect)

Description

[Mariusz Mlynski]

This is basically another flavor of bug 687745, the impact is identical.

It is possible to confuse the URL navigation in the browser, such that the user input on a victim site is redirected to a location on the attacker's server. The bug exists in the history navigation, which incorrectly handles calls to history.forward and history.back that are immediately followed by a write to location.hash. Suppose the history object is in the following state:

[0]: 'http://attacker/' (active)
[1]: 'http://victim/'

Calling history.forward on the attacker's website will redirect a user to the victim website. If location.hash is set before the attacker's script is terminated, the history object will be in the following state after opening http://victim/:

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

There's a discrepancy between the displayed page/URL bar and the address stored in history. Calling history.back() will set baseURI back to http://attacker/ (thus all user input will be posted there), but since that's only a hash change in the "confused" history object, the page will not be reloaded and will still display the content of the victim's website. The Larry badge for HTTPS websites will also continue to show the victim's domain name.

Although, as Daniel noted in bug 687745 comment 21, as a spoof it's on par with a typical phishing attack (though with a convincing Larry badge), the attacker may delay the call to history.back until the user is busy filling out the forms. The URL shift is completely seamless and will not reset any input. I've implemented this in the PoC as a 7 second delay between the loading of Mozilla Sync login page and navigating back to change the baseURI.

Verified to work with the latest Nightly (15.0a1 2012-05-21) and the release build (12.0).

Comment 1

[Mariusz Mlynski]

Attachment: proof of concept

Comment 2

[Boris Zbarsky [:bzbarsky]]

This sounds really familiar.  In particular, sounds a lot like bug 737307.  Justin, do you have time to look into this?

Comment 3

[Justin Lebar (not reading bugmail)]

> Justin, do you have time to look into this?

I'll make time.

I can't reproduce this bug, though.  I briefly see the login page appear, and then we're back at redirect.htm#h, which is blank.  After a few seconds we go to redirect.htm, which is also blank.  The URL bar matches the content shown.

Mariusz, if you can't make the testcase work by posting HTML files directly to bugzilla, can you host the testcase at some hard-to-find URL on your own server?

Comment 4

[Mariusz Mlynski]

(In reply to Justin Lebar [:jlebar] from comment #3)
> Mariusz, if you can't make the testcase work by posting HTML files directly
> to bugzilla, can you host the testcase at some hard-to-find URL on your own
> server?

Sure, I'll hack something up later.

Comment 5

[Mariusz Mlynski]

Attachment: just an auxiliary file I'll use

Comment 6

[Mariusz Mlynski]

(In reply to Justin Lebar [:jlebar] from comment #3)
> I can't reproduce this bug, though.  I briefly see the login page appear,
> and then we're back at redirect.htm#h, which is blank.  After a few seconds
> we go to redirect.htm, which is also blank.  The URL bar matches the content
> shown.

OK, I know what's going on. This is due to my PoC being too optimistic about dom.min_background_timeout_value. I have this set to 0 locally, but the default config is different. I'll provide fixed PoCs (and a video) in a few hours.

Comment 7

[Justin Lebar (not reading bugmail)]

Okay, I can reproduce this with dom.min_background_timeout set to 0.  It doesn't work if I load from http://localhost, but it does if I load from my local IP.

Comment 8

[Mariusz Mlynski]

In case there are any more problems, I've broken the test down into individual steps. The buttons do the following:

step 1: open the victim site in the redirected window and navigate back.
step 2: call window.forward() and write a hash to confuse the history object.
step 3: call window.back() to change the baseURI.

Videos (use HD and fullscreen):

https://www.youtube.com/watch?v=FnTqwi1YgkQ -- a step-by-step reproduction of the issue in the latest Nightly (clean profile).
https://www.youtube.com/watch?v=shbgu9NSUPA -- this video shows how this bug can be used to hijack script fetches using relative paths (based on https://bugzilla.mozilla.org/attachment.cgi?id=626233)

Comment 9

[Mariusz Mlynski]

Attachment: updated PoC's

Comment 11

[Justin Lebar (not reading bugmail)]

So I think this is extremely similar to bug 737307.  It appears that, like in bug 737307, we're loading into a SHEntry when we should be creating a new one.

The testcase does

  1) window.location = http://youtube.com
  2) Go back as soon as reading opener.location.href throws.
  <Pause>
  3) history.forward()
  4) location.hash = "#h"

I think (2) is the key step here.  It appears that we create a SHEntry for youtube without ever loading the page.  Not entirely sure how that works yet, but I presume the back() cancels the load.  

So then when we do |history.forward(); location.hash=#h;|, the change to location.hash creates a new session history entry, and then the history.forward() loads into the new entry.

Without the specific timing in (2), youtube would presumably be bfcached, which would break the testcase.  (Not that bfcache would mitigate the attack, of course, because bfcache doesn't always work, but it would make it harder to reproduce.)

Notice that this gets around the fix to bug 737307 because the two navigations have the same load type (they're both history loads).

Comment 12

[Justin Lebar (not reading bugmail)]

Attachment: Straw-man patch.

This works to fix the security problem here, although it's not the right fix.

What happens with this patch is:

  0) At the beginning, our session history is
       A: Attack page  <-- current
       B: Youtube

  1) We do history.forward.  Now we start loading youtube into shentry B.

  2) We do location.hash = "#h".  This rewrites session history so that it's now
       A: Attack page
       C: Attack page #h.  <-- current

  3) Youtube finishes loading into shentry B.  But B is not in the SHistory chain!

The net effect is, Youtube loads fine, and the URL bar is fine.  But our
SHistory is inconsistent; if you go back then forward, you end up at attack#h
(SHEntry C).

We have four options here:

  I)   Cancel the youtube load when you modify the hash.
       Result: SHistory A, B.
  II)  Overwrite/remove the SHEntry for #h when youtube loads.
       Result: SHistory A, C.
  III) Insert the youtube SHEntry before #h.
       Result: SHistory A, B, C.
  IV)  Insert the youtube SHEntry after #h.
       Result: SHistory A, C, B.

We established in bug 737307 that we can't do (I) when step (1) above is
location.replace instead of history.forward, so we probably shouldn't do (I) in
this case either.

(III) is good because it makes SHistory respect the order the page did things
in.  But A and C share a document, so it's super-weird for them to be non-adjacent
in SHistory.  (In fact, I don't think HTML5 allows this at all.)

(IV) is not great because the shistory doesn't match the order the page did things in.

Which leaves us with (II).  This isn't great, but it's similar to how
navigating before onload doesn't create a new SHEntry, so maybe it's OK.

Comment 13

[Justin Lebar (not reading bugmail)]

>  I)   Cancel the youtube load when you modify the hash.
>       Result: SHistory A, B.
>  II)  Overwrite/remove the SHEntry for #h when youtube loads.
>       Result: SHistory A, C.

The results here are backwards; the first one results in A, C, and the second one results in A, B.

I was surprised to discover that Chrome does (I).  If you replace history.forward() with location.replace, then Chrome does (IV).

Safari matches Chrome in both cases.

Opera does (IV) with or without location.replace.

IE does (II) with or without location.replace.

So...that's about as unenlightening as we could have hoped for.  :)

Comment 14

[Justin Lebar (not reading bugmail)]

bz, any idea how I can tell whether a network load is pending?  I can look at GetCurrentDocChannel(), but I don't think that tells me whether the network load has called back into InternalLoad yet...

Modulo getting the right condition for network-load-is-pending, it seems straightforward to make any hashchanges (and pushstates) which happen during network-load-is-pending use replacement.  Just switch the load flags to LOAD_NORMAL_REPLACE.

The downside is that this would not match any other browser's behavior; the result would be C, B.  Also, it would mean that pushState effectively becomes replaceState during network-load-is-pending.

Comment 15

[Boris Zbarsky [:bzbarsky]]

You could compare GetCurrentDocChannel() to mDocumentRequest (or GetDocumentChannel() if you want to stick to the getters), perhaps?

We really need to spec this stuff.  :(

Comment 16

[Justin Lebar (not reading bugmail)]

> You could compare GetCurrentDocChannel() to mDocumentRequest (or GetDocumentChannel() if you want to 
> stick to the getters), perhaps?

Unfortunately at the critical moment GetCurrentDocChannel() still points to the old doc, while mDocumentRequest points to the new doc.  (This actually makes sense, since location.hash='#h' operates on the current doc.)

I can add yet another magic boolean, if nothing else.

Comment 17

[Boris Zbarsky [:bzbarsky]]

> Unfortunately at the critical moment GetCurrentDocChannel() still points to the old doc,
> while mDocumentRequest points to the new doc.

Right.  That's the "we started a load, but that load has not become the current document we're showing yet" condition: when GetCurrentDocChannel() and mDocumentRequest are different.

Comment 18

[Justin Lebar (not reading bugmail)]

Attachment: Patch v1

I'm not too happy about this; I'm afraid we're just dancing around the real problem here.

I'm not sure if I should leave the AutoRestore of mLSHE in there.  The whole point of this patch is that we do replacement loads so we don't need that.  But it may turn future security bugs into merely weird-navigation bugs.

If we do leave the AutoRestore in, I'm not sure if we should be restoring mLSHE via something like AddToSessionHistory.  It's not clear what the right behavior is because it's not clear when it would ever have an effect!

Comment 19

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 628459 [details] [diff] [review]
Patch v1

You shouldn't need the static_cast, but other than that this looks good.

Perhaps leave the AutoRestore in but also assert at the end of the block that mLSHE has not actually changed?

Comment 20

[Justin Lebar (not reading bugmail)]

Hm, maybe these AutoRestore(mLSHE)'s are wrong.

Suppose we're loading a document, but JustStartedNetworkLoad() is false, because we're past that point.  Suppose we call pushState, or change the hash.  Shouldn't mLSHE change?  I have no idea if it matters -- maybe mLSHE is never read past that point.  But changing mLSHE in that case certainly seems right...

Comment 21

[Justin Lebar (not reading bugmail)]

Attachment: Bug 757376.

Removing auto-restore of mLSHE, per last comment.

Comment 22

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 629768 [details] [diff] [review]
Bug 757376.

r=me

Comment 23

[Justin Lebar (not reading bugmail)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1b34d689a6ba

I admit I'm setting these "affected" flags without testing the relevant versions.  I strongly suspect they're affected, however.

Experience suggests we should let this bake for a few days before considering it for branches.

Comment 24

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/1b34d689a6ba

Comment 25

[Alex Keybl [:akeybl]]

Tracking FF14 and up since this is a recently and externally reported sg:high issue.

Comment 26

[Justin Lebar (not reading bugmail)]

Comment on attachment 629768 [details] [diff] [review]
Bug 757376.

[Approval Request Comment]
Security bug, no string changes.  This is a docshell change, so it is risky as always.

Rather than waiting before asking for approval for branches, can I get approval to land, say, Friday June 9, assuming we don't hear about any regressions before then?  I could land on Aurora and then Beta/ESR some time later, if we wanted to further minimize risk.

Comment 27

[Al Billings [:abillings - ex-MoCo]]

Verified both tests in attached PoC no longer work in 6/7 nightly trunk build. Compared against shipped Firefox 13.

Comment 28

[Alex Keybl [:akeybl]]

Comment on attachment 629768 [details] [diff] [review]
Bug 757376.

[Triage Comment]
I think it makes more sense to land this on both Aurora and Beta at the same time, given the risk here. That'll get this fix into our second FF14 beta (please land today). I'd rather have more feedback sooner than protect our beta users from this risk.

Comment 29

[Justin Lebar (not reading bugmail)]

(In reply to Alex Keybl [:akeybl] from comment #28)
> I think it makes more sense to land this on both Aurora and Beta at the same
> time, given the risk here. That'll get this fix into our second FF14 beta
> (please land today). I'd rather have more feedback sooner than protect our
> beta users from this risk.

That sounds quite reasonable to me.  I'll land asap.

Comment 30

[Alex Keybl [:akeybl]]

Comment on attachment 629768 [details] [diff] [review]
Bug 757376.

Actually - I take that back. To ensure that we don't cause a regression in this next FN beta, let's start with Aurora. We'll land for the next FF15 beta.

Comment 31

[Justin Lebar (not reading bugmail)]

Fixed in Aurora 15 and ESR 10:

https://hg.mozilla.org/releases/mozilla-aurora/rev/851b070766ad
https://hg.mozilla.org/releases/mozilla-esr10/rev/e1f98eb2476e

I'll land on Beta 14 on Wednesday.

Comment 32

[Justin Lebar (not reading bugmail)]

I didn't notice that my approval flags had been rescinded; I already landed on ESR.  I guess I'll wait to land on beta until I get a+ again.

Comment 33

[Alex Keybl [:akeybl]]

Comment on attachment 629768 [details] [diff] [review]
Bug 757376.

You're good to go for landing on Beta.

Comment 34

[Justin Lebar (not reading bugmail)]

Can you please mark approval-esr-10, since I already landed based on the old a+?

Comment 35

[Justin Lebar (not reading bugmail)]

Landed in Beta 14: https://hg.mozilla.org/releases/mozilla-beta/rev/c6680c9f29fe

Comment 36

[u279076]

I'm a bit confused in the expected behaviour here. Can someone please clarify so I can verify this fixed?

BZ testcase with Firefox 13.0.1:
1. Load 'bz' testcase and click the 'click' button
2. A window opens stating "please wait while redirecting"
3. Redirects to index.htm#h then back to index.htm
4. The secondary window closes

YT testcase with Firefox 13.0.1:
1. Load 'yt' testcase and click the 'click' button
2. A window opens with three 'step' buttons
3. Clicking 'step 1' appears to do nothing
4. Clicking 'step 2' loads youtube.com in the main window
5. Clicking 'step 3' puts index.htm in the location bar of the main window but youtube is still loaded

Thanks in advance for your help.

Comment 37

[Mariusz Mlynski]

(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #36)
> BZ testcase with Firefox 13.0.1:
> 1. Load 'bz' testcase and click the 'click' button
> 2. A window opens stating "please wait while redirecting"
> 3. Redirects to index.htm#h then back to index.htm
> 4. The secondary window closes

After a few seconds, an alert dialog should appear (see the second video in comment 8).

> YT testcase with Firefox 13.0.1:
> 1. Load 'yt' testcase and click the 'click' button
> 2. A window opens with three 'step' buttons
> 3. Clicking 'step 1' appears to do nothing
> 4. Clicking 'step 2' loads youtube.com in the main window
> 5. Clicking 'step 3' puts index.htm in the location bar of the main window
> but youtube is still loaded

The location bar/loaded content mismatch you see in point 5 is the expected (ie. bad) behavior.

Comment 38

[u279076]

(In reply to Mariusz Mlynski from comment #37)
> After a few seconds, an alert dialog should appear (see the second video in
> comment 8).
I'm not seeing an alert dialog in 13.0.1. I expect to see one given that it's status-firefox13:affected.

> > YT testcase with Firefox 13.0.1:
> The location bar/loaded content mismatch you see in point 5 is the expected
> (ie. bad) behavior.
At least I can reproduce this, so I'll be able to verify the fix on this case. However, is this case alone enough to call this bug verified fixed?

Comment 39

[Mariusz Mlynski]

(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #38)
> (In reply to Mariusz Mlynski from comment #37)
> > After a few seconds, an alert dialog should appear (see the second video in
> > comment 8).
> I'm not seeing an alert dialog in 13.0.1. I expect to see one given that
> it's status-firefox13:affected.

Ah, do you run it from the file system? It won't work from file: because mozilla.org is not allowed to load local files, and baseURI ends up being whatever the location of the testcase is. Try to upload the /bz/ directory to some remote HTTP server and run it from there.

> > > YT testcase with Firefox 13.0.1:
> > The location bar/loaded content mismatch you see in point 5 is the expected
> > (ie. bad) behavior.
> At least I can reproduce this, so I'll be able to verify the fix on this
> case. However, is this case alone enough to call this bug verified fixed?

Both test cases rely on the same mechanism, if one fails then the other won't work either.

Comment 40

[u279076]

Before I go to all the trouble of setting up a webserver so I can upload the testcase to test this, are you already set up to test this? If so, can you assist with the verification of this fix? The immediate need is verifying this against the latest Firefox 14 Beta and the latest mozilla-esr10 nightly.

Alternatively, if you can set this up on a remote server temporarily so I can test it, that would help.

Thanks

Comment 41

[Justin Lebar (not reading bugmail)]

> Before I go to all the trouble of setting up a webserver so I can upload the testcase to test this,

You don't have a people.mozilla.org account?

Comment 42

[u279076]

No, I personally do not. I'm willing to take the time to get done what's needed for me to test this. I just think it would be better, for the sake of getting this verified before we release, for someone that's already set up to test this to do it in parallel.

Comment 45

[Mariusz Mlynski]

If you don't see any alert dialog, it's good.

Comment 46

[u279076]

Okay, thanks, marking my two comments which reference your server IP private. Please keep the test server live until I've verified on Aurora, Beta, and ESR.

Comment 47

[u279076]

Verified fixed for Firefox 14, 15, 16 and 10.0.6esr. Mariusz, you can take down your test server if you want.

Comment 48

[:Gavin Sharp [email: [e-mail]]]

FWIW, all MoCo employees with an ssh pubkey in LDAP (criteria you satisfy since you have commit access) have access to people.m.o - just |ssh [e-mail]|.

Comment 49

[Justin Lebar (not reading bugmail)]

When will we be able to un-protect this bug?  We already have opened bug 687745, which is basically the same but with a jankier testcase and no link to the fixing cset.

Comment 50

[Raymond Forbes[:rforbes]]

rforbes-bugspam-for-setting-that-bounty-flag-20130719