Closed Bug 371360 (CVE-2007-1095) Opened 18 years ago Closed 17 years ago

[FIX]scripts can tailgate departing users with onUnload

Categories

(Core :: Security, defect)

Product:
Core
Component:
Security
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla1.9alpha6

People

(Reporter: lcamtuf, Assigned: bzbarsky, NeedInfo)

References

(Blocks 1 open bug,
URL
)

Details

(4 keywords, Whiteboard: [sg:low spoof/info-leak] verify nsDocShell::InternalLoad is fixed on branch, too)

Attachments

(3 files, 6 obsolete files)

Alternate approach without load stopping
12.89 KB, patch
Biesinger
: review+
jst
: superreview+
Details | Diff | Splinter Review
1.8 branch patch including the hunk from bug 364461
8.88 KB, patch
Details | Diff | Splinter Review
1.8.0 branch patch including the hunk from bug 364461
8.72 KB, patch
Details | Diff | Splinter Review
It is possible for onUnload handler to abuse document.write(), manipulate location DOM hierarchy, read back the URL for which the user is departing, and possibly redirect the browser to a typosquatting-type phishing site instead. The user is unlikely to notice: http://lcamtuf.coredump.cx/ietrap/ff/ I have a gut feeling that this can be abused in a far more sinister way than just redirection, although document and window hierarchies are not directly accessible. Still, this is a variant of a more serious MSIE7 vulnerability (http://lcamtuf.coredump.cx/ietrap/). The MSIE7 example behaves on FF in an interesting way by itself (we end up with a page that has a favicon and URL bar entry of the target site, Ctrl-U shows attacker-generated code, but no text is displayed). It is my impression that onUnload/onBeforeUnload handlers shouldn't be able to effectively use document.write() or manipulate location.* (alternatively, these changes shouldn't take precedence over loading of a new document).
Flags: blocking1.8.1.3?
This appears to be limited to URL changes as the script security context of the original page is transfered to the subsequent included script code. 1. <body onunload="entrapment()"> 2. entrapment() creates a new document through document.open(), document.write('<html><body><script src="http://lcamtuf.coredump.cx/ietrap/ff/execthere.js"></script>'), document.close() 3. execthere.js sets location.href to a CGI script which receives a HTTP_Referer header containing the URL of the page that was being navigated to. It's not possible to reference the DOM in any way inside part 3, any references to alert/eval/document/etc throw a security error. You can't read the document.cookie or location.href properties. If you attempt to set location.href to a relative path in step 3 instead of an absolute path then those changes are relative to the page being navigated to.
So a solution would be to disable setting location on unload? Like mentioned bug 247660, comment 6.
Shouldn't this bug be marked as "Severity: major" ?
Keywords: privacy
Whiteboard: [sg:low spoof]
Flags: blocking1.8.1.3?
Since I personally think that this is a "Major public security bug", it would be a good idea to nominate it in order to be fixed for 2.0.0.3 (or maybe 2.0.0.4) as I read in: http://developer.mozilla.org/devnews/index.php/2007/03/04/firefox-2003-and-2004/
This basically allows the page to read the URL you're going to. While unfortunate, this is not a "major public security bug". A "major public security bug" would be a privilege escalation or remote code execution bug... Of course the link you point to explicitly says "sg:high and sg:critical" whereas this bug is "sg:low"...
Any changes to onunload will need more baking than we have time for. It is a fairly low level of info that is obtained, hence sg:low
Flags: blocking1.8.1.3? → blocking1.8.1.3-
What about the re-direction aspect? It effectively re-directs my browser.
Just thought I'd share, if network.http.sendRefererHeader is set to "0," the re-direction does not work.
> What about the re-direction aspect? You don't really need onunload for that. You can always set document.location.href. It's true that this may allow certain kinds of more-targeted spoofs if the user first goes to an evil site and then in the same window goes to a bank page or something. So it should be fixed. No one's arguing about it. It just doesn't need to be fixed by tomorrow (which is what 2.0.0.3 is all about).
(In reply to comment #10) > > What about the re-direction aspect? > > You don't really need onunload for that. You can always set > document.location.href. > > It's true that this may allow certain kinds of more-targeted spoofs if the user > first goes to an evil site and then in the same window goes to a bank page or > something. So it should be fixed. No one's arguing about it. It just doesn't > need to be fixed by tomorrow (which is what 2.0.0.3 is all about). > Whole-heartedly agree. I know this isn't that urgent, just no one else was speaking of it, so I thought I might direct the conversation to that aspect.
I can verify the comment from Jordan M, setting network.http.sendRefererHeader not only prevents the HTTP header from being sent but also prevents document.referrer from being set. In either case, you can't read the URL the user is navigating to. The whiteboard says [sg:low spoof], but I don't agree that this is primarily a spoofing issue. The spoofing part is just one part of the original proof-of-concept, this is a generic information disclosure vulnerability. Instead of redirecting you to a spoofed version of the original site, which is highly visible, you could also be silently tracking the users surfing habits.
So as mentioned, one way to approach this (and bug 372666, bug 247660, bug 251944, and breaking bug 356923 completely, which I think is desirable) is to disallow all docshell operations and document.open() from inside the unload handler. The more I think about that, the more I like it... jst, sicking, Blake, biesi, what do you think of doing that? Do other UAs allow any history navigation, form submission, or document.write from unload handlers?
Assignee: nobody → dveditz
Product: Firefox → Core
QA Contact: firefox → toolkit
Version: 2.0 Branch → Trunk
To me the scary part here is that the page can read the new location. Is that something we can fix instead?
That doesn't address the other issues, in particular bug 372666. It wouldn't be trivial to fix either -- by the time unload fires the docshell has the new URI.
Definitely wanted on the branch, but not going to commit to it until we see the scope of the patch required when it's fixed on trunk.
Assignee: dveditz → jst
Flags: wanted1.8.1.x+
Flags: blocking1.9?
Whiteboard: [sg:low spoof] → [sg:low spoof/info-leak]
renominate when Gecko1.9 has the fix.
Flags: blocking1.8.1.4?
Blocks: 372666
I'm sorry, didn't noted that Boris Zbarsky had already sent this link.
Any chance you have cycles left for this one bz?
Assignee: jst → bzbarsky
Flags: blocking1.9? → blocking1.9+
I don't have the cycles to test behavior of other UAs, for sure. I might have the cycles for implementing comment 13, but I franly don't know when. At this point, "not in May". :(
I actually think we should go ahead and just do this, no matter what other UAs do.
Target Milestone: --- → mozilla1.9alpha5
Target Milestone: mozilla1.9alpha5 → mozilla1.9alpha6
Blocks: 247660, 251944, 356923
Attached patch Proposed patch (obsolete) — — Splinter Review
The good: This patch should fix this bug, bug 372666, bug 247660, bug 251944, and breake bug 356923 completely, as per comment 13. The bad: I can't reproduce this bug on trunk (the testcase in comment 0 doesn't behave as advertised). The patch needs a LOT of testing on these various bugs and the various other bugs that have popped up in this very fragile code. We need testcases for mochitest, probably. We need testing of various onunload scenarios, and we need testing of various types of channels (e.g. view-source: channels in content, javascripy: channels, etc). The ugly: I'm not in a position to do this testing in time to get this landed in 1.9a6 (and hence in 1.9). So if there's anyone who can help with that, that would be much appreciated! I'll try to put up a branch patch in a few minutes as well.
Attached patch With a minor tweak (obsolete) — — Splinter Review
Attachment #267203 - Attachment is obsolete: true
Attachment #267205 - Flags: superreview?(jst)
Attachment #267205 - Flags: review?(cbiesinger)
Summary: scripts can tailgate departing users with onUnload → [FIX]scripts can tailgate departing users with onUnload
Flags: in-testsuite?
Attached patch 1.8 branch patch version (obsolete) — — Splinter Review
Attachment #267213 - Flags: superreview?(jst)
Attachment #267213 - Flags: review?(cbiesinger)
Attachment #267213 - Flags: approval1.8.1.5?
Attached patch 1.8.0 branch patch (obsolete) — — Splinter Review
Attachment #267214 - Flags: superreview?(jst)
Attachment #267214 - Flags: review?(cbiesinger)
Attachment #267214 - Flags: approval1.8.0.13?
Perhaps OnLinkClick should also be checking whether we're in the middle of unload? Might be worth checking what happens if unload calls form.submit().
On branch, we might not need to stop loads like that -- we need it on trunk, I think, because javascript: loads are async and don't stop things when they start... But on branch they're sync, so that should be OK... worth double-checking, I guess. :(
I think this should still fix all the testcases, except variants of bug 372666 on trunk. I'll post a separate patch to cover those. Note that the document.open change is only really needed because document.open stops existing requests in the loadgroup. So without that change it takes two tries to navigate away from a page whose unload does document.open. So on branch we could even skip on that, possibly...
Attachment #267292 - Flags: superreview?(jst)
Attachment #267292 - Flags: review?(cbiesinger)
Oh, and I'll hold off on branch variants of the alternate approach until I have some feedback from reviewers and testing.
I've been running with this patch applied to my win32 builds for a couple days now and haven't noticed any obvious regressions so far.
Comment on attachment 267292 [details] [diff] [review] Alternate approach without load stopping I think I prefer this approach. Less code, the stopping code looks potentially tricky to get right. r=jst for this one.
Attachment #267292 - Flags: superreview?(jst) → superreview+
Been using the alternate patch for nearly a week now with no obvious side effects.
Attached patch Alternate approach 1.8 branch version (obsolete) — — Splinter Review
Attachment #267205 - Attachment is obsolete: true
Attachment #267213 - Attachment is obsolete: true
Attachment #267214 - Attachment is obsolete: true
Attachment #268378 - Flags: superreview?(jst)
Attachment #268378 - Flags: review?(cbiesinger)
Attachment #267205 - Flags: superreview?(jst)
Attachment #267205 - Flags: review?(cbiesinger)
Attachment #267214 - Flags: superreview?(jst)
Attachment #267214 - Flags: review?(cbiesinger)
Attachment #267214 - Flags: approval1.8.0.13?
Attachment #267213 - Flags: superreview?(jst)
Attachment #267213 - Flags: review?(cbiesinger)
Attachment #267213 - Flags: approval1.8.1.5?
Attached patch Alternate approach 1.8.0 branch version (obsolete) — — Splinter Review
Attachment #268379 - Flags: superreview?(jst)
Attachment #268379 - Flags: review?(cbiesinger)
Attachment #268378 - Flags: approval1.8.1.5?
Attachment #268379 - Flags: approval1.8.0.13?
Attachment #267292 - Flags: review?(cbiesinger) → review+
Attachment #268378 - Flags: review?(cbiesinger) → review+
Attachment #268379 - Flags: review?(cbiesinger) → review+
Checked in on trunk.
Status: NEW → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Attachment #268378 - Flags: superreview?(jst) → superreview+
Attachment #268379 - Flags: superreview?(jst) → superreview+
Depends on: 384977
Note: this exposed underlying bug 384977. We probably need to fix that on the 1.8 branch too. :(
Actually, that was already covered by bug 364461. Still need to fix it, though.
Depends on: 364461
No longer depends on: 384977
Note that this also still needs some serious testing before I'd be happy putting it on branches...
Keywords: qawanted
what kind of "serious testing"? Anything targeted, or just broadly "use lots of js-heavy sites"?
Comment 23 has some details about the sort of testing we need. In addition to that, I'd test whether setting .location works in various stages of page load and unload, possibly with extensions which might be doing it at bizarre times (e.g. off of progress listener notifications). At least if we have such extensions. Do we?
Whiteboard: [sg:low spoof/info-leak] → [sg:low spoof/info-leak] need trunk testing
qawanted: please test this heavily on the trunk (see comment 23 and 41)
Comment on attachment 268378 [details] [diff] [review] Alternate approach 1.8 branch version Given an accellerated 1.8.1.5 we'll have to wait for this one.
Attachment #268378 - Flags: approval1.8.1.5? → approval1.8.1.6?
When landing on branches, should remember the InternalLoad change from bug 364461.
Depends on: 388579
Attachment #268379 - Flags: approval1.8.0.13? → approval1.8.0.14?
Flags: blocking1.8.1.7+
Blocks: CVE-2007-1256
Comment on attachment 268378 [details] [diff] [review] Alternate approach 1.8 branch version >+interface nsIDocShell_MOZILLA_1_8_BRANCH2 : nsISupports why a second _MOZILLA_1_8_BRANCH ? I think you could have used the original one, we're not promising not to change those, we're just using those to keep the original 1.8 branch untouched. Doesn't much matter I guess. approved for 1.8.1.7, a=dveditz for release-drivers
Attachment #268378 - Flags: approval1.8.1.7? → approval1.8.1.7+
Comment on attachment 268379 [details] [diff] [review] Alternate approach 1.8.0 branch version approved for 1.8.0.14, a=dveditz for release-drivers
Attachment #268379 - Flags: approval1.8.0.14? → approval1.8.0.14+
Need another patch to incorporate the nsDocShell::InternalLoad change not checked in with bug 364461.
Flags: blocking1.8.0.14?
Whiteboard: [sg:low spoof/info-leak] need trunk testing → [sg:low spoof/info-leak] need nsDocShell::InternalLoad fix too
Whiteboard: [sg:low spoof/info-leak] need nsDocShell::InternalLoad fix too → [sg:low spoof/info-leak] verify nsDocShell::InternalLoad is fixed on branch, too
> we're not promising not to change those As I understand it, we are promising API immutability on branch. We can add APIs, but not change existing ones.
Attachment #268378 - Attachment is obsolete: true
Attachment #268379 - Attachment is obsolete: true
Checked in on both branches.
Flags: blocking1.8.0.14?
Keywords: fixed1.8.0.14, fixed1.8.1.7
Flags: blocking1.8.0.14+
Verified fixed, using: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.8pre) Gecko/20071006 BonEcho/2.0.0.8pre I can see the bug in Firefox2.0.0.7.
Keywords: fixed1.8.1.8 → verified1.8.1.8
Alias: CVE-2007-1095
Broken "onunload" behavior? I do an onload=document.form.submit() to submit a form when a page loads. I then do an onunload=window.location.replace to redirect to another page. I do this to prevent the submit page from showing up in the history and prevent a re-submit if the back button is clicked. This no longer works. In my testing, onunload does not execute window.location.replace.
onunload=window.location.replace works fine in Firefox 2.0.0.7 and earlier. Must have been broken by this patch in 2.0.0.8.
That is the intended effect of this patch. It's unfortunate for your use case, but it can also be abused to fool people into going somewhere else. IE7 also blocks this in an onunload now.
What Daniel said. That is in fact the intended behavior.
I tested with IE7 7.0.5730.11 and it executes window.location.replace. Is there a newer IE I don't know about? IE must be trapping a malicious redirect differently. I admit it's a bit of a kludge, but the case I described is used by MANY sites to mask a submission page.
Interesting. Does IE7 allow setting location.href in that situation? I was told that it did not.
I do have to ask.... Did the testing I mentioned in comment 23 ever happen? I assumed that a= meant it had.
Re: Boris' question in comment #58: I just tested onunload=location.href with IE7 7.0.5730.11. It works as well.
Ah, bug 372666 comment 18 says: > That is something Safari and Konqueror and newer versions (apparently > 9.1+) of Opera seem to do, but IIRC not IE. I still think this is the right behavior for us. There's no way to tell apart malicious and non-malicious loads in onunload, really...
Yes, I noticed the same location set behavior with onunload in Safari. So I'll take this as, "We're not gonna change it, change your code." Right?
Unless magic happens somehow, probably. It _would_ be nice to not lose IE compat here, though, if possible....
Bummer... this leads to even more convoluted js code. Security just gets more and more painful. I'll monitor this thread in case you guys change your mind. Thanks for your time.
Depends on: 401611
OK. So I've been thinking about this... perhaps we can do the following: 1) Continue to disallow document.open from unload 2) Allow loads to happen from unload but a) Perform the loads async (that is, if InternalLoad is called during unload, post an event to do the actual load) b) Only allow loads that are to a URI which is same-origin with the page being loaded when unload fires. In particular, no javascript: URIs allowed. (1) fixes this bug as originally files. (2b) fixes bug 372666, bug 247660, bug 251944. (2a) should allow the use cases that seem to have broken as a result of this change, right? We'd still want to block loads if the window is being closed, I think. Thoughts?
So from what I understand the intent of the usecase in comment 54 is to prevent submission pages from ending up in history. Is that still going to be the case if we do the load off an event?
Yes. The history effect is due to using location.replace, which does a history-replace load.
Are there any other possible side effects if doing the load asynchronously? I don't want to rob Peter to pay Paul. If not, it works for me as long as location.replace replaces the submit page with the redirect page in the history. Thanks for spending the time and reconsidering. I'd be glad to thoroughly test any interim build if change is implemented.
> Are there any other possible side effects if doing the load asynchronously? Yes. If the page being loaded when unload fires has, say, a meta refresh, it's maybe possible for that to fire and cancel the load started in the unload handler. I'm hoping this won't come up much. ;) I'll try to get a patch together sometime next week, but my Mozilla time is sadly pretty limited right now. :(
(In reply to comment #65) > In particular, no javascript: URIs allowed. > data: URIs are kind of similar to javascript URIs
Verified for 1.8.0 with Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.0.14pre) Gecko/20071210 Firefox/1.5.0.13pre.
Keywords: fixed1.8.0.14 → verified1.8.0.14
I filed bug 409888 on comment 65 so we can track blocking status for it, etc.
Depends on: 409888
Raul da Silva, could you maybe retest your use case in tomorrow's trunk build (which should have the fix for bug 409888) and comment with the results? That would be very helpful!
Boris- I downloaded firefox-2.0.0.12pre.en-US.win32 dated Jan 26 and tested against my use case described in comment #53. window.location.replace seems to still not be executed onunload. Sorry for my ignorance of your build process, but I downloaded the version from http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2008-01-26-03-mozilla1.8/... is this the correct build? If not, please point me to the correct one and I'll gladly retest.
Hi Raul, it is fixed on trunk build, so Boris asked you to test with that: http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-trunk/ It's not fixed on the branch yet.
Thanks Martijn - I saw that trunk directory, but wasn't sure because it was labeled 3 beta. I just tested with firefox-3.0b3pre.en-US.win32 from /pub/mozilla.org/firefox/nightly/latest-trunk dated 26-Jan-2008 05:58. window.location.replace still does not execute with onunload.
Raul, do you have a publicly available testcase that shows your problem? The situation you describe is working for me as far as I can tell, as long as the URI passed to replace() is on the same server as the URI being navigated to when unload fired...
Boris- Fill out the form (you can use bogus data) at: http://www.workday.com/resources/whitepapers/whitepaper_form.php and submit... you will be directed to a download page. If you hit the back button you *should* be returned to the form if window.location.replace was fired. If it wasn't fired, when you hit the back button the form will be resubmitted and you will end up on the download page again. (note: for each retest, you must delete the 'wdcontent' cookie associated with the workday.com site) Try the same form with IE to see 'as designed' behavior. Thanks.
Ah, that seems to be the same issue as bug 409888 comment 21 and following... I hadn't realized the replace() replaced the page being _unloaded_ when I wrote comment 65. See the details in bug 409888. I guess it's a matter of which is worse from your point of view: the replace() doing nothing at all, or replacing the page being loaded when unload fires... At least in the near term. :(
Also, what did Firefox 2 do in this regard? Did things work exactly as you want them there, or does it behave like Firefox 3 does now with the latest round of fixes?
Firefox 2.0.0.7 and prior work as wanted. 2.0.0.8 and later and 3 beta all function incorrectly with my testcase.
I guess this means we're back where we started. If this can't be fixed, we'll need to redesign our submission functionality soon. Because of Firefox's rising market share :-) we're getting increasingly higher rates of duplicate submissions. Let me know if anything changes. Thanks.
Raul, I just checked in a fix that might fix your issue (at least it does over here). Want to try tomorrow's trunk nightly and let me know?
Boris- Thanks for your continued interest in this problem... I really do appreciate it. But unfortunately, no joy yet. I just tested the 01/30 nightly trunk at http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-trunk/firefox-3.0b3pre.en-US.win32 and onunload is still not executing window.location.replace.
Raul, that's very odd. I finally got a chance to try your page, and it is indeed not fixed. But "testcase 3" from bug 409888 is fixed, and is testing more or less what your page is doing, as far as I can tell... I guess I'll dig more tonight.
Boris- Could the problem be that the submit page we want to hide/replace is doing a post to a servlet on a server in a different domain? Re-reading 2b in your Comment 65 lead me to think this may be the problem. Sorry for overlooking this.
Boris- Could the problem be that the submit page we want to hide/replace is doing a post to a servlet on a server in a different domain? Re-reading 2b in your Comment 65 leads me to think this may be the problem. Sorry for overlooking this.
Yes, if the address that's being navigated to is not on the same server as the URI passed to replace() when unload fires, the replace() will not happen. I'm really not sure we want to relax that restriction; if we do, we need to solve the corresponding security bugs in some other way (and mark bug 251944 wontfix).
Well, that's it then... and it's the one thing that we absolutely can't change. We must post to this particular server. I overlooked this since the replace() *is* a page on the originating server. Just today it was decided our site will move to a new framework in the next few months; so much functionality will be rewritten anyway. I guess we'll just have to tough it out until then. Just wondering, does this mean that IE is susceptible to this tailgating problem or are they trapping the problem another way?
IE is definitely susceptible to bug 251944, last I checked.
This should fix the problem for those doing an onunload=replace() in the same domain. Which will probably be the majority of the legitimate use cases. At least your effort wasn't wasted. Thanks again for your help.
Based on the age of QAWANTED request on this bug, is QAWANTED still wanted?
Yes, the tests for this still need to be written...
Is there a manual testcase that can be written for this (i.e. Litmus)? If I am right, I think no. Based on the language of this comment thread I'm guessing only automated testing is desired (i.e. mochitests)...
One can write a litmnus test for this, but mochitest tests would be vastly preferable.
What would the manual test for this look like? Code a minimized test to attempt a redirect onUnload and have the Litmus tester run the code sample?
There's a test linked in the url of this bug. But there are also various comments in this bug describing the tests that are needed (in addition to that test linked in the url field).
Added a testcase to Litmus: https://litmus.mozilla.org/show_test.cgi?id=7886 in-litmus+
Flags: in-litmus+
Depending on an external page for our tests is really bad form (not to mention not very future-proof....)
(In reply to comment #99) > Depending on an external page for our tests is really bad form (not to mention > not very future-proof....) Yes it is. However, our procedure for writing test cases is to get a working test case then get it checked into Litmus storage so we don't have to rely on third party URLs. This is not possible in all cases, but we try to do this wherever possible.
Blocks: 278418
No longer blocks: 251944
Blocks: backtraps

To stay on topic, I think sendBeacon is a great thing - too bad IE and mobile Safari don't support it

You need to use navigator.sendBeacon(), so check out the spec at https://developer.mozilla.o...

Do note that it won't work with IE or some of the older browsers, so you should add a fallback to <a href="https://onlineitguru.com/tableau-online-training-placement.html">POST/GET</a>

Flags: needinfo?(meenatibiswalcynixit)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: