Closed Bug 514232 (CVE-2009-3985) Opened 11 years ago Closed 10 years ago

URL spoofing bug involving Firefox's error pages, document.location

Categories

(Core :: DOM: Navigation, defect, P2)

All
Windows Vista
defect

Tracking

()

RESOLVED FIXED
mozilla1.9.2
Tracking Status
status1.9.2 --- beta3-fixed
blocking1.9.1 --- .6+
status1.9.1 --- .6-fixed

People

(Reporter: jordi.chancel, Assigned: mayhemer)

References

Details

(Keywords: verified1.9.0.16, verified1.9.1, Whiteboard: [sg:moderate] spoof )

Attachments

(5 files, 8 obsolete files)

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
Attached file TEST (obsolete) —
Whiteboard: [sg:moderate] spoof
Group: core-security
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
Assignee: nobody → bzbarsky
blocking1.9.1: --- → ?
Component: Location Bar and Autocomplete → Document Navigation
Depends on: CVE-2009-2654
Flags: wanted1.9.0.x+
Flags: blocking1.9.0.15?
Product: Firefox → Core
QA Contact: location.bar → docshell
Flags: blocking1.9.2?
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
blocking1.9.1: ? → .4+
Flags: blocking1.9.0.15? → blocking1.9.0.15+
Attached image Screenshot
Attached file testcase with invisible characters (obsolete) —
this POC demonstrates that it is possible to spoof the URL without the character that generates the invalid url is visible
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?
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...
Blocking 1.9.2+.
Flags: blocking1.9.2? → blocking1.9.2+
Priority: -- → P2
Target Milestone: --- → mozilla1.9.2
Will try to catch this next time once we figure out how to fix it for 1.9.2
blocking1.9.1: .4+ → .5+
Flags: blocking1.9.0.15+ → blocking1.9.0.16+
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.
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...
Attached patch v1 (obsolete) — Splinter Review
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?
Assignee: bzbarsky → honzab.moz
Status: NEW → ASSIGNED
Attachment #410242 - Flags: review?(bzbarsky)
Attached patch v1 + test (obsolete) — Splinter Review
Attachment #410242 - Flags: review?(bzbarsky) → review-
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...
Attached patch v2 (obsolete) — Splinter Review
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.
Attachment #410531 - Flags: review?(bzbarsky)
Comment on attachment 410531 [details] [diff] [review]
v2

Subject principal isn't system during a location set....
Attachment #410531 - Flags: review?(bzbarsky) → review-
And more importantly, we can't allow the racing-load situation, period, ever.  If it happens, bad things happen.
Attachment #410242 - Attachment is obsolete: true
Attachment #410253 - Attachment is obsolete: true
Attachment #410531 - Attachment is obsolete: true
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.
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.
Summary: URL spoofing bug involving Firefox's error pages, document.write and document.location → URL spoofing bug involving Firefox's error pages, document.location
Attached patch v3 (obsolete) — Splinter Review
Delayed current uri assignment and history entry creation. A bit tricky but should be ok.
Attachment #410609 - Flags: review?(bzbarsky)
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...
Attached patch v3.1 (obsolete) — Splinter Review
As you suggest.
Attachment #410609 - Attachment is obsolete: true
Attachment #410752 - Flags: review?(bzbarsky)
Attachment #410609 - Flags: review?(bzbarsky)
Whiteboard: [sg:moderate] spoof → [needs r=bzbarsky][sg:moderate] spoof
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!
Attachment #410752 - Flags: review?(bzbarsky) → review+
(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?
In general, yes, but right now you have an mLoadType check right above this line...
(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).
Whiteboard: [needs r=bzbarsky][sg:moderate] spoof → [needs landing][needs 1.9.1/1.9.0 patch][sg:moderate] spoof
r=bzbarsky

introduced changes from comment 23. will be landed w/o the test until the bug opens.
Attachment #410752 - Attachment is obsolete: true
Attachment #411744 - Flags: review+
Attachment #411744 - Flags: approval1.9.2?
Attachment #411744 - Flags: approval1.9.1.6?
Attachment #411744 - Flags: approval1.9.0.16?
Please replace this:

>+    // for what these objects are needed.

With:

  // for which these objects are needed.
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
Attachment #411744 - Attachment description: v3.2 → v3.2 [Checkin comment 29]
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Honza: Can you get this on 1.9.2 today as well?
Here it is, built+tested on 1.9.2.
Attachment #411792 - Flags: approval1.9.2?
Attachment #411744 - Flags: approval1.9.2?
Whiteboard: [needs landing][needs 1.9.1/1.9.0 patch][sg:moderate] spoof → [needs branch approval][sg:moderate] spoof
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.
Attachment #411792 - Flags: approval1.9.2?
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
Attachment #411792 - Attachment description: v3.3 (w/o a test) for 1.9.2 → v3.3 (w/o a test) for 1.9.2 [Checkin 1.9.2 comment 33]
Unexpected is right.  If we're hitting this code during Txul/Tdhtml, something is very wrong.
Attachment #411744 - Flags: approval1.9.1.6?
Attachment #411744 - Flags: approval1.9.1.6+
Attachment #411744 - Flags: approval1.9.0.16?
Attachment #411744 - Flags: approval1.9.0.16+
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
Whiteboard: [needs branch approval][sg:moderate] spoof → [sg:moderate] spoof
Honza: Any update on getting this landed on 1.9.1 and 1.9.0?
Samuel, going to do it now.
This caused a Tp4 increase on Linux, can't remember if that was known or understood or not.
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
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 on attachment 413088 [details] [diff] [review]
as landed on 1.9.0 [Checkin comment 41]

Backed out because of build bustage.
Attachment #413088 - Attachment is obsolete: true
This is adjusted for 1.9.0 and includes patch (fix+test) for regression bug 529119 for which I actually ask aproval.
Attachment #413157 - Flags: approval1.9.0.16?
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
Attachment #413157 - Flags: approval1.9.0.16? → approval1.9.0.16+
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
Attachment #413157 - Attachment description: v3.3 for 1.9.0 → v3.3 for 1.9.0 [Checkin 1.9.0 comment 46]
Keywords: fixed1.9.0.16
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)).
Alias: CVE-2009-3985
Group: core-security
Attachment #398176 - Attachment is obsolete: true
Attachment #399319 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.