514232 - (CVE-2009-3985) URL spoofing bug involving Firefox's error pages, document.location

Status: RESOLVED FIXED

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

Description

[Jordi Chancel]

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]

Attachment: TEST

Comment 2

[Daniel Veditz [:dveditz]]

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]]

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]

Attachment: Screenshot

Comment 5

[Jordi Chancel]

Attachment: 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 [:bzbarsky]]

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)]

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)]

Blocking 1.9.2+.

Comment 9

[Daniel Veditz [:dveditz]]

Will try to catch this next time once we figure out how to fix it for 1.9.2

Comment 10

[Boris Zbarsky [:bzbarsky]]

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)]

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)]

Attachment: 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)]

Attachment: v1 + test

Comment 14

[Boris Zbarsky [:bzbarsky]]

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)]

Attachment: 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 [:bzbarsky]]

Comment on attachment 410531 [details] [diff] [review]
v2

Subject principal isn't system during a location set....

Comment 17

[Boris Zbarsky [:bzbarsky]]

And more importantly, we can't allow the racing-load situation, period, ever.  If it happens, bad things happen.

Comment 18

[Honza Bambas (:mayhemer)]

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 [:bzbarsky]]

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)]

Attachment: v3

Delayed current uri assignment and history entry creation. A bit tricky but should be ok.

Comment 21

[Boris Zbarsky [: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...

Comment 22

[Honza Bambas (:mayhemer)]

Attachment: v3.1

As you suggest.

Comment 23

[Boris Zbarsky [:bzbarsky]]

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)]

(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 [:bzbarsky]]

In general, yes, but right now you have an mLoadType check right above this line...

Comment 26

[Daniel Veditz [:dveditz]]

(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)]

Attachment: 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 [:bzbarsky]]

Please replace this:

>+    // for what these objects are needed.

With:

  // for which these objects are needed.

Comment 29

[Honza Bambas (:mayhemer)]

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)]

Honza: Can you get this on 1.9.2 today as well?

Comment 31

[Honza Bambas (:mayhemer)]

Attachment: 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)]

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)]

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]]

Unexpected TXul and DHTML talos wins?

http://groups.google.com/group/mozilla.dev.tree-management/browse_thread/thread/840c1cf7f812d450#

Comment 35

[Boris Zbarsky [:bzbarsky]]

Unexpected is right.  If we're hitting this code during Txul/Tdhtml, something is very wrong.

Comment 36

[Daniel Veditz [:dveditz]]

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)]

Honza: Any update on getting this landed on 1.9.1 and 1.9.0?

Comment 38

[Honza Bambas (:mayhemer)]

Samuel, going to do it now.

Comment 39

[Honza Bambas (:mayhemer)]

Attachment: 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]]

This caused a Tp4 increase on Linux, can't remember if that was known or understood or not.

Comment 41

[Honza Bambas (:mayhemer)]

Attachment: 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)]

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)]

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)]

Attachment: 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]]

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)]

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 - ex-MoCo]]

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

Comment 48

[Ali]

(In reply to Daniel Veditz [:dveditz] from comment #2)

This is public on his blog, not sure making it a private bug makes sense.
http://vxcasino.bet/