Last Comment Bug 514232 - (CVE-2009-3985) URL spoofing bug involving Firefox's error pages, document.location
(CVE-2009-3985)
: URL spoofing bug involving Firefox's error pages, document.location
Status: RESOLVED FIXED
[sg:moderate] spoof
: verified1.9.0.16, verified1.9.1
Product: Core
Classification: Components
Component: Document Navigation (show other bugs)
: unspecified
: All Windows Vista
: P2 normal (vote)
: mozilla1.9.2
Assigned To: Honza Bambas (:mayhemer)
:
Mentors:
Depends on: CVE-2009-2654 529119
Blocks:
  Show dependency treegraph
 
Reported: 2009-09-02 10:52 PDT by Jordi Chancel
Modified: 2010-01-24 10:32 PST (History)
15 users (show)
jst: blocking1.9.2+
dveditz: blocking1.9.0.16+
dveditz: wanted1.9.0.x+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
beta3-fixed
.6+
.6-fixed


Attachments
TEST (453 bytes, text/html)
2009-09-02 10:53 PDT, Jordi Chancel
no flags Details
Screenshot (70.83 KB, image/jpeg)
2009-09-08 13:36 PDT, Jordi Chancel
no flags Details
testcase with invisible characters (1.17 KB, text/html)
2009-09-08 14:37 PDT, Jordi Chancel
no flags Details
v1 (885 bytes, patch)
2009-11-04 07:27 PST, Honza Bambas (:mayhemer)
bzbarsky: review-
Details | Diff | Splinter Review
v1 + test (3.92 KB, patch)
2009-11-04 08:28 PST, Honza Bambas (:mayhemer)
no flags Details | Diff | Splinter Review
v2 (4.21 KB, patch)
2009-11-05 09:28 PST, Honza Bambas (:mayhemer)
bzbarsky: review-
Details | Diff | Splinter Review
v3 (8.66 KB, patch)
2009-11-05 14:04 PST, Honza Bambas (:mayhemer)
no flags Details | Diff | Splinter Review
v3.1 (8.95 KB, patch)
2009-11-06 02:59 PST, Honza Bambas (:mayhemer)
bzbarsky: review+
Details | Diff | Splinter Review
v3.2 [Checkin comment 29] (9.05 KB, patch)
2009-11-11 11:35 PST, Honza Bambas (:mayhemer)
honzab.moz: review+
dveditz: approval1.9.1.6+
dveditz: approval1.9.0.16+
Details | Diff | Splinter Review
v3.3 (w/o a test) for 1.9.2 [Checkin 1.9.2 comment 33] (6.22 KB, patch)
2009-11-11 14:03 PST, Honza Bambas (:mayhemer)
no flags Details | Diff | Splinter Review
as landed on 1.9.1 [Checkin comment 39] (6.19 KB, patch)
2009-11-16 11:49 PST, Honza Bambas (:mayhemer)
no flags Details | Diff | Splinter Review
as landed on 1.9.0 [Checkin comment 41] (13.14 KB, patch)
2009-11-18 08:45 PST, Honza Bambas (:mayhemer)
no flags Details | Diff | Splinter Review
v3.3 for 1.9.0 [Checkin 1.9.0 comment 46] (14.64 KB, patch)
2009-11-18 12:33 PST, Honza Bambas (:mayhemer)
dveditz: approval1.9.0.16+
Details | Diff | Splinter Review

Description Jordi Chancel 2009-09-02 10:52:14 PDT
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.0; fr; rv:1.9.1.2) Gecko/20090729 Firefox/3.5.2 (.NET CLR 3.5.30729)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.0; fr; rv:1.9.1.2) Gecko/20090729 Firefox/3.5.2 (.NET CLR 3.5.30729)

This vulnerability is similare to http://www.mozilla.org/security/announce/2009/mfsa2009-44.html
The problem originated is from the creation of a link with javascript (window.open & document.location) . When you enter an incorrect address seems that the firefox can't
translate to address web and display it in blank page.
This blank page can be changed with javascript with making a spoofing attak.



Reproducible: Always

Steps to Reproduce:
Steps to Reproduce :
Step 1=> Create an html file with a special javascript or open http://www.seeon.fr/blog/public/spoofurl.html
Step 2=>Open the page

Actual Results:  
One can visualize the address unresolved and the fake page.


similary to http://www.mozilla.org/security/announce/2009/mfsa2009-44.html the problem is that with javascript window can be changed by altering the content and halting the loading of the page.

Function Javascript used :
window.open
window.stop
document.write
document.location
Comment 1 Jordi Chancel 2009-09-02 10:53:51 PDT
Created attachment 398176 [details]
TEST
Comment 2 Daniel Veditz [:dveditz] 2009-09-02 18:57:20 PDT
This is public on his blog, not sure making it a private bug makes sense.
http://www.seeon.fr/blog/index.php?post/2009/09/02/Mozilla-Firefox-3.*.*-URL-SPOOFING
Comment 3 Daniel Veditz [:dveditz] 2009-09-03 17:00:27 PDT
Meant to confirm the bug. The difference between this case and bug 451898 appears to be the addition of setting document.location after writing to the error page.
Comment 4 Jordi Chancel 2009-09-08 13:36:38 PDT
Created attachment 399307 [details]
Screenshot
Comment 5 Jordi Chancel 2009-09-08 14:37:08 PDT
Created attachment 399319 [details]
testcase with invisible characters

this POC demonstrates that it is possible to spoof the URL without the character that generates the invalid url is visible
Comment 6 Boris Zbarsky [:bz] 2009-09-09 12:46:19 PDT
OK, so the key is that setting location to something that triggers an error page load immediately changes the docshell's URI.  Then calling stop() stops the error page load, so we never show the error page.  But we've already changed the URI.

The document.write is only relevant in that it lets us inject content of our choosing.  A much simpler POC:

  javascript:window.location = 'https://www.google.com%';window.stop();void(0);

Run this on any page of your choice.  This bug page is a good testcase.

I think we really need to change the behavior where error pages eagerly set the URI.  Christian, thoughts?
Comment 7 Christian :Biesinger (don't email me, ping me on IRC) 2009-09-09 15:03:57 PDT
I suppose that should be fine. I kinda think that several issues could be avoided if we could make the error page load synchronous and not trigger any events, but that's probably not really possible...
Comment 8 Johnny Stenback (:jst, jst@mozilla.com) 2009-09-09 17:55:11 PDT
Blocking 1.9.2+.
Comment 9 Daniel Veditz [:dveditz] 2009-09-25 10:49:58 PDT
Will try to catch this next time once we figure out how to fix it for 1.9.2
Comment 10 Boris Zbarsky [:bz] 2009-10-11 06:48:07 PDT
Honza, if you want to take a crack at this just let me know.  Otherwise the plan is that I'll try to do it this next week.
Comment 11 Johnny Stenback (:jst, jst@mozilla.com) 2009-11-03 09:14:01 PST
bz, Honza says you guys talked about this and that you were going to fix this. Please work on this asap, we're getting close to 1.9.2 rc...
Comment 12 Honza Bambas (:mayhemer) 2009-11-04 07:27:34 PST
Created attachment 410242 [details] [diff] [review]
v1

Now sure you will like this patch, but I simply disallow stop of the error page load. When call to window.stop() is removed from the test case, we don't get the bug. Only problem I see is that mLSHE is wiped out but we are still loading the error page.

Is there a strong reason not to get progress from the error page load, to get EndPageLoad?
Comment 13 Honza Bambas (:mayhemer) 2009-11-04 08:28:16 PST
Created attachment 410253 [details] [diff] [review]
v1 + test
Comment 14 Boris Zbarsky [:bz] 2009-11-04 09:19:30 PST
Comment on attachment 410242 [details] [diff] [review]
v1

This makes the error page load also not stop if the user is trying to load something else (e.g. clicked the home button), which means the two loads can race and the error page can stomp on the thing the user is trying to load.  That's why I didn't do it that way in the first place...
Comment 15 Honza Bambas (:mayhemer) 2009-11-05 09:28:26 PST
Created attachment 410531 [details] [diff] [review]
v2

Another insane patch. This prevents stop of an error page load only when not called from chrome or system; i.e. it will not allow scripts stop the error page load.
Comment 16 Boris Zbarsky [:bz] 2009-11-05 09:32:56 PST
Comment on attachment 410531 [details] [diff] [review]
v2

Subject principal isn't system during a location set....
Comment 17 Boris Zbarsky [:bz] 2009-11-05 09:33:40 PST
And more importantly, we can't allow the racing-load situation, period, ever.  If it happens, bad things happen.
Comment 18 Honza Bambas (:mayhemer) 2009-11-05 11:14:31 PST
The way I want to try to go now: If I change the location assignment from "google.com%20%20 etc" to a valid address ("https://www.google.com/" for example) then the resulting URL in the address bar is the URL of the page trying to spoof (of the opener). To have this behavior also for cases while we are loading error page, we solve this bug.
Comment 19 Boris Zbarsky [:bz] 2009-11-05 11:16:35 PST
Right.  In other words, the error page load needs to not set the uri to the error page uri until the error page document viewer actually embeds itself.  Or something.
Comment 20 Honza Bambas (:mayhemer) 2009-11-05 14:04:43 PST
Created attachment 410609 [details] [diff] [review]
v3

Delayed current uri assignment and history entry creation. A bit tricky but should be ok.
Comment 21 Boris Zbarsky [:bz] 2009-11-05 19:03:52 PST
Yeah, that's more like what I was thinking!  Thank you!

A few comments offhand, without having dug into this in detail yet:

1)  Stop() should probably clear out at least mFailedURI and mFailedChannel,
    right?
2)  The comment seems to describe the exploit in detail.  Is that desirable?
3)  Might it make more sense to put this call next to the "normal" OnLoadingSite
    call in CreateContentViewer?  Presumably right before it...
Comment 22 Honza Bambas (:mayhemer) 2009-11-06 02:59:22 PST
Created attachment 410752 [details] [diff] [review]
v3.1

As you suggest.
Comment 23 Boris Zbarsky [:bz] 2009-11-10 07:19:07 PST
Comment on attachment 410752 [details] [diff] [review]
v3.1

>+++ b/docshell/base/nsDocShell.cpp
> nsDocShell::Stop(PRUint32 aStopFlags)
>     if (mLoadType == LOAD_ERROR_PAGE && mLSHE) {
...
>+        mFailedChannel = nsnull;
>+        mFailedURI = nsnull;

I think we want to do that even if !mLSHE.

>+        PRUint32 currentLoadType = mLoadType;

That's guaranteed to be LOAD_ERROR_PAGE.  Why have the variable at all?

>+        mFailedChannel = nsnull;
>+        mFailedURI = nsnull;

I would prefer we null those (by swapping into locals, probably) before the OnNewURI calls and such: those can trigger js execution which might start new loads and set a new mFailedChannel/mFailedURI...

>+++ b/docshell/base/nsDocShell.h
>+    // XXX Update the comment correctly

Yes, please do.

>+++ b/security/manager/ssl/tests/mochitest/bugs/test_bug514232.html

This should probably go in docshell mochitests instead, no?  And shouldn't get checked in until we open up the bug?

r=bzbarsky with those issues addressed.  Thanks for picking this up!
Comment 24 Honza Bambas (:mayhemer) 2009-11-10 07:27:42 PST
(In reply to comment #23)
> (From update of attachment 410752 [details] [diff] [review])
> >+        PRUint32 currentLoadType = mLoadType;
> 
> That's guaranteed to be LOAD_ERROR_PAGE.  Why have the variable at all?
> 

I just tough about a future-compatibility when some other flags would be specified during load (not just the LOAD_ERROR_PAGE constant). Don't you think this is safer?
Comment 25 Boris Zbarsky [:bz] 2009-11-10 07:29:15 PST
In general, yes, but right now you have an mLoadType check right above this line...
Comment 26 Daniel Veditz [:dveditz] 2009-11-10 16:35:05 PST
(In reply to comment #23)
> r=bzbarsky with those issues addressed.

Please attach a patch incorporating these changes when you request branch approval (so downstream distros don't take an incomplete patch).
Comment 27 Honza Bambas (:mayhemer) 2009-11-11 11:35:13 PST
Created attachment 411744 [details] [diff] [review]
v3.2 [Checkin comment 29]

r=bzbarsky

introduced changes from comment 23. will be landed w/o the test until the bug opens.
Comment 28 Boris Zbarsky [:bz] 2009-11-11 12:00:07 PST
Please replace this:

>+    // for what these objects are needed.

With:

  // for which these objects are needed.
Comment 29 Honza Bambas (:mayhemer) 2009-11-11 12:43:17 PST
Comment on attachment 411744 [details] [diff] [review]
v3.2 [Checkin comment 29]

http://hg.mozilla.org/mozilla-central/rev/59ca9e3e4ef9

Fixed according to comment 28
Comment 30 Samuel Sidler (old account; do not CC) 2009-11-11 12:46:08 PST
Honza: Can you get this on 1.9.2 today as well?
Comment 31 Honza Bambas (:mayhemer) 2009-11-11 14:03:30 PST
Created attachment 411792 [details] [diff] [review]
v3.3 (w/o a test) for 1.9.2 [Checkin 1.9.2 comment 33]

Here it is, built+tested on 1.9.2.
Comment 32 Johnny Stenback (:jst, jst@mozilla.com) 2009-11-11 14:11:01 PST
Comment on attachment 411792 [details] [diff] [review]
v3.3 (w/o a test) for 1.9.2 [Checkin 1.9.2 comment 33]

This is a blocker, so no need for explicit approval. Please land whenever possible.
Comment 33 Honza Bambas (:mayhemer) 2009-11-11 14:22:11 PST
Comment on attachment 411792 [details] [diff] [review]
v3.3 (w/o a test) for 1.9.2 [Checkin 1.9.2 comment 33]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/8467ea52b569
Comment 34 Mike Beltzner [:beltzner, not reading bugmail] 2009-11-11 20:53:46 PST
Unexpected TXul and DHTML talos wins?

http://groups.google.com/group/mozilla.dev.tree-management/browse_thread/thread/840c1cf7f812d450#
Comment 35 Boris Zbarsky [:bz] 2009-11-11 20:57:49 PST
Unexpected is right.  If we're hitting this code during Txul/Tdhtml, something is very wrong.
Comment 36 Daniel Veditz [:dveditz] 2009-11-13 11:17:21 PST
Comment on attachment 411744 [details] [diff] [review]
v3.2 [Checkin comment 29]

If this patch requires changes for 1.9.1 or 1.9.0 please attach "as-checked-in" patches.

Approved for 1.9.1.6 and 1.9.0.16, a=dveditz for release-drivers
Comment 37 Samuel Sidler (old account; do not CC) 2009-11-16 09:09:31 PST
Honza: Any update on getting this landed on 1.9.1 and 1.9.0?
Comment 38 Honza Bambas (:mayhemer) 2009-11-16 09:13:37 PST
Samuel, going to do it now.
Comment 39 Honza Bambas (:mayhemer) 2009-11-16 11:49:19 PST
Created attachment 412657 [details] [diff] [review]
as landed on 1.9.1 [Checkin comment 39]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/7e7d06c8547c
Comment 40 Mike Beltzner [:beltzner, not reading bugmail] 2009-11-18 07:44:49 PST
This caused a Tp4 increase on Linux, can't remember if that was known or understood or not.
Comment 41 Honza Bambas (:mayhemer) 2009-11-18 08:45:09 PST
Created attachment 413088 [details] [diff] [review]
as landed on 1.9.0 [Checkin comment 41]

Checking in docshell/base/nsDocShell.cpp;
/cvsroot/mozilla/docshell/base/nsDocShell.cpp,v  <--  nsDocShell.cpp
new revision: 1.916; previous revision: 1.915
done
Checking in docshell/test/Makefile.in;
/cvsroot/mozilla/docshell/test/Makefile.in,v  <--  Makefile.in
new revision: 1.16; previous revision: 1.15
done
RCS file: /cvsroot/mozilla/docshell/test/bug529119-window.html,v
done
Checking in docshell/test/bug529119-window.html;
/cvsroot/mozilla/docshell/test/bug529119-window.html,v  <--  bug529119-window.html
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/docshell/test/test_bug529119-1.html,v
done
Checking in docshell/test/test_bug529119-1.html;
/cvsroot/mozilla/docshell/test/test_bug529119-1.html,v  <--  test_bug529119-1.html
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/docshell/test/test_bug529119-2.html,v
done
Checking in docshell/test/test_bug529119-2.html;
/cvsroot/mozilla/docshell/test/test_bug529119-2.html,v  <--  test_bug529119-2.html
initial revision: 1.1
done
Comment 42 Honza Bambas (:mayhemer) 2009-11-18 08:55:39 PST
And missing header file changes, fixed build bustage:

Checking in docshell/base/nsDocShell.h;
/cvsroot/mozilla/docshell/base/nsDocShell.h,v  <--  nsDocShell.h
new revision: 1.228; previous revision: 1.227
done
Comment 43 Honza Bambas (:mayhemer) 2009-11-18 09:10:33 PST
Comment on attachment 413088 [details] [diff] [review]
as landed on 1.9.0 [Checkin comment 41]

Backed out because of build bustage.
Comment 44 Honza Bambas (:mayhemer) 2009-11-18 12:33:16 PST
Created attachment 413157 [details] [diff] [review]
v3.3 for 1.9.0 [Checkin 1.9.0 comment 46]

This is adjusted for 1.9.0 and includes patch (fix+test) for regression bug 529119 for which I actually ask aproval.
Comment 45 Daniel Veditz [:dveditz] 2009-11-18 17:57:00 PST
Comment on attachment 413157 [details] [diff] [review]
v3.3 for 1.9.0 [Checkin 1.9.0 comment 46]

Approved for 1.9.0.16, a=dveditz for release-drivers

When you land this please add the fixed1.9.0.16 keyword to bug 529119 as well
Comment 46 Honza Bambas (:mayhemer) 2009-11-19 09:01:56 PST
Comment on attachment 413157 [details] [diff] [review]
v3.3 for 1.9.0 [Checkin 1.9.0 comment 46]

Checking in docshell/base/nsDocShell.cpp;
/cvsroot/mozilla/docshell/base/nsDocShell.cpp,v  <--  nsDocShell.cpp
new revision: 1.918; previous revision: 1.917
done
Checking in docshell/base/nsDocShell.h;
/cvsroot/mozilla/docshell/base/nsDocShell.h,v  <--  nsDocShell.h
new revision: 1.230; previous revision: 1.229
done
Checking in docshell/test/Makefile.in;
/cvsroot/mozilla/docshell/test/Makefile.in,v  <--  Makefile.in
new revision: 1.18; previous revision: 1.17
done
Checking in docshell/test/bug529119-window.html;
/cvsroot/mozilla/docshell/test/bug529119-window.html,v  <--  bug529119-window.html
new revision: 1.3; previous revision: 1.2
done
Checking in docshell/test/test_bug529119-1.html;
/cvsroot/mozilla/docshell/test/test_bug529119-1.html,v  <--  test_bug529119-1.html
new revision: 1.3; previous revision: 1.2
done
Checking in docshell/test/test_bug529119-2.html;
/cvsroot/mozilla/docshell/test/test_bug529119-2.html,v  <--  test_bug529119-2.html
new revision: 1.3; previous revision: 1.2
done
Comment 47 Al Billings [:abillings] 2009-11-20 13:26:32 PST
Verified using attached manual testcases with 1.9.1.6 (Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.6pre) Gecko/20091119 Shiretoko/3.5.6pre (.NET CLR 3.5.30729)) and 1.9.0.16 (Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.16pre) Gecko/2009111921 GranParadiso/3.0.16pre (.NET CLR 3.5.30729)).

Note You need to log in before you can comment on or make changes to this bug.