Closed Bug 757376 (CVE-2012-1955) Opened 9 years ago Closed 8 years ago
History navigation error with late location
273 bytes, text/html
1.82 KB, application/java-archive
4.57 KB, patch
|Details | Diff | Splinter Review|
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: : 'http://attacker/' (active) : '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/: : 'http://attacker/' : '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).
This sounds really familiar. In particular, sounds a lot like bug 737307. Justin, do you have time to look into this?
Attachment #625947 - Attachment mime type: application/octet-stream → application/zip
> 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?
Attachment #625947 - Attachment mime type: application/zip → application/java-archive
(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.
Attachment #626233 - Attachment mime type: text/plain → text/html
(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.
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.
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)
Attachment #626357 - Attachment mime type: application/octet-stream → application/zip
Status: UNCONFIRMED → NEW
Ever confirmed: true
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).
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.
> 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. :)
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.
You could compare GetCurrentDocChannel() to mDocumentRequest (or GetDocumentChannel() if you want to stick to the getters), perhaps? We really need to spec this stuff. :(
> 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.
> 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.
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!
Attachment #628459 - Flags: review?(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?
Attachment #628459 - Flags: review?(bzbarsky) → review+
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...
Removing auto-restore of mLSHE, per last comment.
Attachment #629768 - Flags: review?(bzbarsky)
Comment on attachment 629768 [details] [diff] [review] Bug 757376. r=me
Attachment #629768 - Flags: review?(bzbarsky) → review+
Attachment #628459 - Attachment is obsolete: true
Attachment #626855 - Attachment is obsolete: true
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.
Tracking FF14 and up since this is a recently and externally reported sg:high issue.
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.
Verified both tests in attached PoC no longer work in 6/7 nightly trunk build. Compared against shipped Firefox 13.
Status: RESOLVED → VERIFIED
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.
Attachment #629768 - Flags: approval-mozilla-esr10+
Attachment #629768 - Flags: approval-mozilla-beta+
Attachment #629768 - Flags: approval-mozilla-aurora?
Attachment #629768 - Flags: approval-mozilla-aurora+
(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 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.
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.
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 on attachment 629768 [details] [diff] [review] Bug 757376. You're good to go for landing on Beta.
Attachment #629768 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Can you please mark approval-esr-10, since I already landed based on the old a+?
Landed in Beta 14: https://hg.mozilla.org/releases/mozilla-beta/rev/c6680c9f29fe
Attachment #629768 - Flags: approval-mozilla-esr10? → approval-mozilla-esr10+
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.
(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.
(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?
Attachment #626357 - Attachment mime type: application/zip → application/java-archive
(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.
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
> 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?
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.
If you don't see any alert dialog, it's good.
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.
Verified fixed for Firefox 14, 15, 16 and 10.0.6esr. Mariusz, you can take down your test server if you want.
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 firstname.lastname@example.org|.
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.
You need to log in before you can comment on or make changes to this bug.