The default bug view has changed. See this FAQ.
Bug 514232 (CVE-2009-3985)

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

RESOLVED FIXED in mozilla1.9.2

Status

()

Core
Document Navigation
P2
normal
RESOLVED FIXED
8 years ago
7 years ago

People

(Reporter: Jordi Chancel, Assigned: mayhemer)

Tracking

({verified1.9.0.16, verified1.9.1})

unspecified
mozilla1.9.2
All
Windows Vista
verified1.9.0.16, verified1.9.1
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.2 +
blocking1.9.0.16 +
wanted1.9.0.x +

Firefox Tracking Flags

(status1.9.2 beta3-fixed, blocking1.9.1 .6+, status1.9.1 .6-fixed)

Details

(Whiteboard: [sg:moderate] spoof )

Attachments

(5 attachments, 8 obsolete attachments)

(Reporter)

Description

8 years ago
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
(Reporter)

Comment 1

8 years ago
Created attachment 398176 [details]
TEST
(Reporter)

Updated

8 years ago
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: --- → ?
status1.9.1: --- → wanted
Component: Location Bar and Autocomplete → Document Navigation
Depends on: 451898
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+
(Reporter)

Updated

8 years ago
(Reporter)

Updated

8 years ago
(Reporter)

Comment 4

8 years ago
Created attachment 399307 [details]
Screenshot
(Reporter)

Comment 5

8 years ago
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
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
(Reporter)

Updated

8 years ago
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...
(Assignee)

Comment 12

8 years ago
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?
Assignee: bzbarsky → honzab.moz
Status: NEW → ASSIGNED
Attachment #410242 - Flags: review?(bzbarsky)
(Assignee)

Comment 13

8 years ago
Created attachment 410253 [details] [diff] [review]
v1 + test
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...
(Assignee)

Comment 15

8 years ago
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.
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.
(Assignee)

Updated

8 years ago
Attachment #410242 - Attachment is obsolete: true
(Assignee)

Updated

8 years ago
Attachment #410253 - Attachment is obsolete: true
(Assignee)

Updated

8 years ago
Attachment #410531 - Attachment is obsolete: true
(Assignee)

Comment 18

8 years ago
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.
(Reporter)

Updated

8 years ago
Summary: URL spoofing bug involving Firefox's error pages, document.write and document.location → URL spoofing bug involving Firefox's error pages, document.location
(Assignee)

Comment 20

8 years ago
Created attachment 410609 [details] [diff] [review]
v3

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...
(Assignee)

Comment 22

8 years ago
Created attachment 410752 [details] [diff] [review]
v3.1

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+
(Assignee)

Comment 24

8 years ago
(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
(Assignee)

Comment 27

8 years ago
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.
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.
(Assignee)

Comment 29

8 years ago
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]
(Assignee)

Updated

8 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Honza: Can you get this on 1.9.2 today as well?
(Assignee)

Comment 31

8 years ago
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.
Attachment #411792 - Flags: approval1.9.2?
(Assignee)

Updated

8 years ago
Attachment #411744 - Flags: approval1.9.2?
(Assignee)

Updated

8 years ago
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?
(Assignee)

Comment 33

8 years ago
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 TXul and DHTML talos wins?

http://groups.google.com/group/mozilla.dev.tree-management/browse_thread/thread/840c1cf7f812d450#
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
(Assignee)

Updated

8 years ago
status1.9.2: --- → final-fixed
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?
(Assignee)

Comment 38

8 years ago
Samuel, going to do it now.
(Assignee)

Comment 39

8 years ago
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
(Assignee)

Updated

8 years ago
status1.9.1: wanted → .6-fixed
Depends on: 529119
This caused a Tp4 increase on Linux, can't remember if that was known or understood or not.
(Assignee)

Comment 41

7 years ago
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
(Assignee)

Comment 42

7 years ago
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
(Assignee)

Comment 43

7 years ago
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
(Assignee)

Comment 44

7 years ago
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.
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+
(Assignee)

Comment 46

7 years ago
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]
(Assignee)

Updated

7 years ago
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)).
Keywords: fixed1.9.0.16 → verified1.9.0.16, verified1.9.1
Alias: CVE-2009-3985
Group: core-security
(Reporter)

Updated

7 years ago
Attachment #398176 - Attachment is obsolete: true
(Reporter)

Updated

7 years ago
Attachment #399319 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.