536466 - (CVE-2010-2751) SSL spoofing using location.href and location.reload() with history.back() and history.forward()

Status: RESOLVED FIXED

(Core Graveyard :: Security: UI, defect)

Description

[Jordi Chancel]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.1; fr; rv:1.9.1.6) Gecko/20091201 Firefox/3.5.6
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.1; fr; rv:1.9.1.6) Gecko/20091201 Firefox/3.5.6

When you enter an address with an automatic redirection enabled on an other adress (with SSL certificat) , 
enter again the same adress( with redirection disabled) , 
actualize again the same page (with automatic redirection enabled),
history.back() & history.forward().

Reproducible: Always

Steps to Reproduce:
Step 1=> enter an adress with an automatic redirection on an https website with SSL cetificat 
Step 2=> reopen the same adress but this time with redirection disabled
Step 3=> actualize (the redirection must be re-enabled automaticatly)
Step 4=> Go back & forward

or

Step 1=> Create an html file PoC with a special javascript and an other file with an activation/desactivation of the automatic rediction faith on two
Step 2=> Open the page
Actual Results:  
The adress (with redirection enabled/disabled) takes content and SSL certificat of the HTTPS website

Comment 1

[Jordi Chancel]

Attachment: screen shot

Comment 2

[Jordi Chancel]

Attachment: link testcase

link of testcase ( http://www.alternativ-testing.fr/5df49s6q4s8d5zq-firefoxSSL/ssl3.php )

Comment 3

[Jordi Chancel]

Attachment: testcase screen shot result

Comment 4

[Jordi Chancel]

Attachment: Testcase2 part1 alert(document.location)

Comment 5

[Jordi Chancel]

Attachment: Link Testcase2

Comment 6

[Jordi Chancel]

Attachment: Screen Shot [document.location comparative]

document.location and SSL indicat of https website(or http) is replaced by the document.location of redirection page(enabled/disabled faith on two)

Comment 7

[Reed Loden [:reed]]

Confirmed using Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.3a1pre) Gecko/20091230 Minefield/3.7a1pre.

Need a better summary...

Comment 8

[Boris Zbarsky [:bzbarsky]]

Somewhat shades of bug 514232.  Honza, do you want to take a look at this?

Comment 9

[Boris Zbarsky [:bzbarsky]]

And more interestingly, is this a regression from, say, 1.9.0?

Comment 10

[Honza Bambas (:mayhemer)]

(In reply to comment #8)
> Somewhat shades of bug 514232.  Honza, do you want to take a look at this?

Yes, I'm going to analyze this.

Comment 11

[Honza Bambas (:mayhemer)]

So, the only problem I can see here is that the URL in the address bar is replaced with the address of the page that leads to the target of the redirect. The same is then displayed in the Page info dialog.

It seems that security checks are made correctly against the 'https://target-of-redirect' principal. There is no way to modify the content directly via document.write or so.

I have an automated test.

Comment 12

[Honza Bambas (:mayhemer)]

(In reply to comment #9)
> And more interestingly, is this a regression from, say, 1.9.0?

Confirmed with Gran Paradiso, 3.0.17pre.

Comment 13

[Nelson Bolyard (seldom reads bugmail)]

I think this is not an NSS issue.

Comment 14

[Boris Zbarsky [:bzbarsky]]

> Confirmed with Gran Paradiso, 3.0.17pre.

I should have been clearer.  I meant 1.9.0.0 and in particular something from before we fixed bug 514232.

Comment 15

[Daniel Veditz [:dveditz]]

Optimistically putting this into 1.9.0.18/1.9.8 assuming that will match up with the 1.9.2.1, but we might have to adjust later.

Comment 16

[Honza Bambas (:mayhemer)]

(In reply to comment #14)
> > Confirmed with Gran Paradiso, 3.0.17pre.
> 
> I should have been clearer.  I meant 1.9.0.0 and in particular something from
> before we fixed bug 514232.

I have backed out http://hg.mozilla.org/mozilla-central/rev/05af3507e9c5 and 59ca9e3e4ef9 and this bug persists. So it seems to be older.

Comment 17

[Honza Bambas (:mayhemer)]

Test case ones again:
1. open window with URL of a page that redirects (redirect works now) -> we are on the redirect *target* page; first SHE created
2. do location.href = URL of a page that redirects (redirect now doesn't work) -> we are back at the page that redirects; second SHE created
3. do location.reload() (redirect now works) -> we get to the redirect *target* page; still using the second SHE

BUG => In case of location.reload() load type is LOAD_CMD_RELOAD and this code

http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#8874

doesn't update history, the current SH entry (the second one in this case) is not updated with the new URI of the redirect target. On back and forward we have the correct content but we update the session with the old URL.

Call stack this code gets invoked:

>	docshell.dll!nsDocShell::OnNewURI(nsIURI * aURI=0x06a62078, nsIChannel * aChannel=0x069dc3c0, nsISupports * aOwner=0x00000000, unsigned int aLoadType=2, int aFireOnLocationChange=0, int aAddToGlobalHistory=1)  Line 8852	C++
 	docshell.dll!nsDocShell::OnLoadingSite(nsIChannel * aChannel=0x069dc3c0, int aFireOnLocationChange=0, int aAddToGlobalHistory=1)  Line 8969 + 0x29 bytes	C++
 	docshell.dll!nsDocShell::CreateContentViewer(const char * aContentType=0x0629a078, nsIRequest * request=0x069dc3c0, nsIStreamListener * * aContentHandler=0x06657a70)  Line 7043 + 0x15 bytes	C++
 	docshell.dll!nsDSURIContentListener::DoContent(const char * aContentType=0x0629a078, int aIsContentPreferred=0, nsIRequest * request=0x069dc3c0, nsIStreamListener * * aContentHandler=0x06657a70, int * aAbortProcess=0x0013d5bc)  Line 138 + 0x20 bytes	C++
 	docshell.dll!nsDocumentOpenInfo::TryContentListener(nsIURIContentListener * aListener=0x069154c0, nsIChannel * aChannel=0x069dc3c0)  Line 736 + 0x41 bytes	C++
 	docshell.dll!nsDocumentOpenInfo::DispatchContent(nsIRequest * request=0x069dc3c0, nsISupports * aCtxt=0x00000000)  Line 434 + 0x39 bytes	C++
 	docshell.dll!nsDocumentOpenInfo::OnStartRequest(nsIRequest * request=0x069dc3c0, nsISupports * aCtxt=0x00000000)  Line 280 + 0x10 bytes	C++
 	necko.dll!nsHttpChannel::CallOnStartRequest()  Line 839 + 0x47 bytes	C++
 	necko.dll!nsHttpChannel::ProcessNormal()  Line 1145 + 0x8 bytes	C++
 	necko.dll!nsHttpChannel::ProcessResponse()  Line 1014 + 0x8 bytes	C++
 	necko.dll!nsHttpChannel::OnStartRequest(nsIRequest * request=0x05f1b1b8, nsISupports * ctxt=0x00000000)  Line 5162 + 0xb bytes	C++


At the moment no idea for a fix. Probably change mLoadType from RELOAD to NORMAL in case we detect a redirect?

Comment 18

[Honza Bambas (:mayhemer)]

Attachment: v1

This is my proposed fix for consideration.  There is a single drawback, when we set location to a page leading to a redirect, and redirect doesn't work, and later we refresh the page while redirect does work, there is created an SH entry with address of the page leading to the redirect.

I'm not sure how serious that is.

Comment 19

[Honza Bambas (:mayhemer)]

Smaug suggested to test with IE, so the result is that on refresh in step 3 of the test IE8 loads the content of the redirect target page, updates the security indicators to show an encrypted site and updates the URL in the address bar and doesn't crate a new history entry and either doesn't blow away forward history.

It also doesn't refresh the page (=is not doing a server request) on going back/forward.

I found an IE bug: when you do a test in a secondary open tab, the address bar is not updated after refresh, it is refreshed after back/forward or switch to another tab and then going back; just a note...

Going to study smaug's patch changing some SHE manipulation and potentially base a new patch up on his one.

Comment 20

[Mike Beltzner [:beltzner, not reading bugmail]]

Readjusting flags; we'll get this next release.

Comment 21

[Honza Bambas (:mayhemer)]

Comment on attachment 422570 [details] [diff] [review]
v1

This patch is wrong, I'm working on a different approach atm.

Comment 22

[Honza Bambas (:mayhemer)]

Attachment: branch v0.9

This is preview of my solution for branches: cancel the redirect when we reload or navigate.  The patch is based on m-c, merging is needed.

Comment 23

[Honza Bambas (:mayhemer)]

Attachment: v2

First, I was looking at Olli's patch to find API that I could use and I haven't found anything.

Then I spotted code handling LOAD_NORMAL_REPLACE load type here [1]. This piece of code and the LOAD_NORMAL_REPLACE type does exactly what I need.  This doesn't affect iframes, but this bug could not be reproduced when manipulating an iframe.

This is IMHO also a branch safe patch.

Also tested on top of patch for bug 462076, everything works.

[1] http://hg.mozilla.org/mozilla-central/annotate/5f631340007c/docshell/base/nsDocShell.cpp#l9098

Comment 24

[Mike Beltzner [:beltzner, not reading bugmail]]

Needed for 1.9.2, not blocking for 1.9.0 (since that branch is going out of support soon) but would definitely want the patch there when it gets reviewed.

Comment 25

[Boris Zbarsky [:bzbarsky]]

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

r=bzbarsky.  Nice approach!  Really sorry for the lag...

Do you need the v0.9 branch thing reviewed still, or do we just want to do this on branches?

Comment 26

[Honza Bambas (:mayhemer)]

Comment on attachment 423783 [details] [diff] [review]
branch v0.9

I don't think we need this patch.

Comment 27

[Jordi Chancel]

Resolved for Firefox 3.6.4 or another ?

Comment 28

[Reed Loden [:reed]]

Apparently, the ball was dropped here. Honza, poke?

Comment 29

[Christian :Biesinger (don't email me, ping me on IRC)]

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

+    if (~(aRedirectFlags & nsIChannelEventSink::REDIRECT_INTERNAL) && 

do you mean ! instead of ~?

sr=biesi

Comment 30

[Brandon Sterne (:bsterne)]

Is this ready to land on trunk?

Comment 31

[Honza Bambas (:mayhemer)]

I'll address comment 29 and land.  Probably on Wednesday.

Comment 32

[Honza Bambas (:mayhemer)]

Attachment: v2 as landed [Checkin comment 32] [Checkin 1.9.1 comment 34] [Checkin 1.9.2 comment 35] [Check-in 1.9.0 comment 42]

http://hg.mozilla.org/mozilla-central/rev/162098e94b35

Comment 33

[Honza Bambas (:mayhemer)]

Attachment: Tests for v2

Comment 34

[Honza Bambas (:mayhemer)]

Comment on attachment 444675 [details] [diff] [review]
v2 as landed [Checkin comment 32] [Checkin 1.9.1 comment 34] [Checkin 1.9.2 comment 35] [Check-in 1.9.0 comment 42]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/933ad42c0b77

Comment 35

[Honza Bambas (:mayhemer)]

Comment on attachment 444675 [details] [diff] [review]
v2 as landed [Checkin comment 32] [Checkin 1.9.1 comment 34] [Checkin 1.9.2 comment 35] [Check-in 1.9.0 comment 42]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/0fb56c49d1e9

Comment 36

[Al Billings [:abillings - ex-MoCo]]

The original STR is now 404. Can someone provide a link for manual verification since the tests aren't running yet?

Comment 37

[Jordi Chancel]

(In reply to comment #36)
> The original STR is now 404. Can someone provide a link for manual verification
> since the tests aren't running yet?

http://www.alternativ-testing.fr/6df4d587d9s-firefoxdocument-locationpersistbug/documentlocationfirefox.php

Comment 38

[Al Billings [:abillings - ex-MoCo]]

(In reply to comment #37)
> (In reply to comment #36)
> > The original STR is now 404. Can someone provide a link for manual verification
> > since the tests aren't running yet?
> 
> http://www.alternativ-testing.fr/6df4d587d9s-firefoxdocument-locationpersistbug/documentlocationfirefox.php

Used this to verify for 1.9.1 and 1.9.2. SSLspoofing doesn't work in 1.9.1.11 build 1 (Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.11) Gecko/20100701 Firefox/3.5.11 (.NET CLR 3.5.30729)) or 1.9.2.7 build 1 (Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2.7) Gecko/20100701 Firefox/3.6.7 (.NET CLR 3.5.30729)). Spoofing clearly occurs in 1.9.1.10 and 1.9.2.6.

Comment 39

[Daniel Veditz [:dveditz]]

Comment on attachment 444675 [details] [diff] [review]
v2 as landed [Checkin comment 32] [Checkin 1.9.1 comment 34] [Checkin 1.9.2 comment 35] [Check-in 1.9.0 comment 42]

Approved for 1.9.0.20, a=dveditz

Comment 40

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Honza, do you mind if I go ahead and check this in for you on 1.9.0?

Comment 41

[Honza Bambas (:mayhemer)]

(In reply to comment #40)
> Honza, do you mind if I go ahead and check this in for you on 1.9.0?

Feel free, but please test the patch works, i.e. run test case in comment 37 w/o the patch first and then w/ the patch to see the problem is fixed with the patch.

Comment 42

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #41)
> Feel free, but please test the patch works, i.e. run test case in comment 37
> w/o the patch first and then w/ the patch to see the problem is fixed with the
> patch.

Yep, I did that before I started working on the collection of 1.9.0 bugs :) (which is a good thing, because the testcase in comment 37 is 404 again).

Checking in docshell/base/nsDocShell.cpp;
/cvsroot/mozilla/docshell/base/nsDocShell.cpp,v  <--  nsDocShell.cpp
new revision: 1.919; previous revision: 1.918
done

Comment 43

[Jordi Chancel]

(In reply to comment #42)
> (In reply to comment #41)
> > Feel free, but please test the patch works, i.e. run test case in comment 37
> > w/o the patch first and then w/ the patch to see the problem is fixed with the
> > patch.
> 
> Yep, I did that before I started working on the collection of 1.9.0 bugs :)
> (which is a good thing, because the testcase in comment 37 is 404 again).
> 
> Checking in docshell/base/nsDocShell.cpp;
> /cvsroot/mozilla/docshell/base/nsDocShell.cpp,v  <--  nsDocShell.cpp
> new revision: 1.919; previous revision: 1.918
> done

http://www.alternativ-testing.fr/6df4d587d9s-testcasefirefoxdocument-location201001/documentlocationfirefox.php

Comment 44

[Honza Bambas (:mayhemer)]

Smokey, thank you.