Last Comment Bug 536466 - (CVE-2010-2751) SSL spoofing using location.href and location.reload() with history.back() and history.forward()
(CVE-2010-2751)
: SSL spoofing using location.href and location.reload() with history.back() an...
Status: RESOLVED FIXED
[sg:moderate spoof][3.6.x]
: fixed1.9.0.20, verified1.9.1, verified1.9.2
Product: Core
Classification: Components
Component: Security: UI (show other bugs)
: unspecified
: All All
: -- major (vote)
: ---
Assigned To: Honza Bambas (:mayhemer)
:
Mentors:
http://www.alternativ-testing.fr/6df4...
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2009-12-22 14:28 PST by Jordi Chancel
Modified: 2010-09-15 15:36 PDT (History)
16 users (show)
mbeltzner: blocking1.9.2-
mbeltzner: blocking1.9.0.19-
dveditz: wanted1.9.0.x+
dveditz: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
final+
needed
.7-fixed
needed
.11-fixed


Attachments
screen shot (44.85 KB, image/png)
2009-12-22 14:30 PST, Jordi Chancel
no flags Details
link testcase (141 bytes, text/html)
2009-12-24 08:08 PST, Jordi Chancel
no flags Details
testcase screen shot result (127.04 KB, image/png)
2009-12-24 08:11 PST, Jordi Chancel
no flags Details
Testcase2 part1 alert(document.location) (120 bytes, text/html)
2009-12-24 18:09 PST, Jordi Chancel
no flags Details
Link Testcase2 (170 bytes, text/html)
2009-12-24 18:29 PST, Jordi Chancel
no flags Details
Screen Shot [document.location comparative] (149.20 KB, image/png)
2009-12-24 18:38 PST, Jordi Chancel
no flags Details
v1 (7.44 KB, patch)
2010-01-20 11:01 PST, Honza Bambas (:mayhemer)
no flags Details | Diff | Review
branch v0.9 (12.93 KB, patch)
2010-01-27 05:22 PST, Honza Bambas (:mayhemer)
no flags Details | Diff | Review
v2 (12.97 KB, patch)
2010-01-27 11:08 PST, Honza Bambas (:mayhemer)
bzbarsky: review+
cbiesinger: superreview+
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] (1.16 KB, patch)
2010-05-11 09:03 PDT, Honza Bambas (:mayhemer)
dveditz: approval1.9.2.7+
dveditz: approval1.9.1.11+
dveditz: approval1.9.0.next+
Details | Diff | Review
Tests for v2 (11.97 KB, patch)
2010-05-11 09:04 PDT, Honza Bambas (:mayhemer)
no flags Details | Diff | Review

Description Jordi Chancel 2009-12-22 14:28:07 PST
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 2009-12-22 14:30:09 PST
Created attachment 418910 [details]
screen shot
Comment 2 Jordi Chancel 2009-12-24 08:08:21 PST
Created attachment 419123 [details]
link testcase

link of testcase ( http://www.alternativ-testing.fr/5df49s6q4s8d5zq-firefoxSSL/ssl3.php )
Comment 3 Jordi Chancel 2009-12-24 08:11:23 PST
Created attachment 419125 [details]
testcase screen shot result
Comment 4 Jordi Chancel 2009-12-24 18:09:22 PST
Created attachment 419145 [details]
Testcase2 part1 alert(document.location)
Comment 5 Jordi Chancel 2009-12-24 18:29:38 PST
Created attachment 419146 [details]
Link Testcase2
Comment 6 Jordi Chancel 2009-12-24 18:38:51 PST
Created attachment 419147 [details]
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] (use needinfo?) 2009-12-30 14:40:19 PST
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 [:bz] (Out June 25-July 6) 2009-12-30 15:58:21 PST
Somewhat shades of bug 514232.  Honza, do you want to take a look at this?
Comment 9 Boris Zbarsky [:bz] (Out June 25-July 6) 2009-12-30 15:58:41 PST
And more interestingly, is this a regression from, say, 1.9.0?
Comment 10 Honza Bambas (:mayhemer) 2009-12-31 07:27:04 PST
(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) 2010-01-02 09:30:17 PST
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) 2010-01-04 15:33:40 PST
(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) 2010-01-04 15:47:29 PST
I think this is not an NSS issue.
Comment 14 Boris Zbarsky [:bz] (Out June 25-July 6) 2010-01-04 16:26:38 PST
> 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] 2010-01-05 10:39:08 PST
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) 2010-01-05 14:50:54 PST
(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) 2010-01-05 15:32:27 PST
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) 2010-01-20 11:01:29 PST
Created attachment 422570 [details] [diff] [review]
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) 2010-01-25 11:04:27 PST
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] 2010-01-26 10:45:42 PST
Readjusting flags; we'll get this next release.
Comment 21 Honza Bambas (:mayhemer) 2010-01-26 10:56:02 PST
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) 2010-01-27 05:22:51 PST
Created attachment 423783 [details] [diff] [review]
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) 2010-01-27 11:08:42 PST
Created attachment 423827 [details] [diff] [review]
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] 2010-02-10 12:43:02 PST
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 [:bz] (Out June 25-July 6) 2010-03-08 15:24:21 PST
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) 2010-03-12 06:24:35 PST
Comment on attachment 423783 [details] [diff] [review]
branch v0.9

I don't think we need this patch.
Comment 27 Jordi Chancel 2010-04-14 10:01:55 PDT
Resolved for Firefox 3.6.4 or another ?
Comment 28 Reed Loden [:reed] (use needinfo?) 2010-04-14 10:07:22 PDT
Apparently, the ball was dropped here. Honza, poke?
Comment 29 Christian :Biesinger (don't email me, ping me on IRC) 2010-04-14 11:36:06 PDT
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) 2010-04-20 08:40:39 PDT
Is this ready to land on trunk?
Comment 31 Honza Bambas (:mayhemer) 2010-04-20 09:45:22 PDT
I'll address comment 29 and land.  Probably on Wednesday.
Comment 32 Honza Bambas (:mayhemer) 2010-05-11 09:03:51 PDT
Created 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/mozilla-central/rev/162098e94b35
Comment 33 Honza Bambas (:mayhemer) 2010-05-11 09:04:24 PDT
Created attachment 444676 [details] [diff] [review]
Tests for v2
Comment 34 Honza Bambas (:mayhemer) 2010-06-12 03:18:55 PDT
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) 2010-06-12 04:09:53 PDT
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 [On PTO until 6/29] 2010-06-21 17:34:28 PDT
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 2010-06-26 01:04:18 PDT
(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 [On PTO until 6/29] 2010-07-01 12:38:12 PDT
(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] 2010-07-22 19:30:15 PDT
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) 2010-07-23 12:03:50 PDT
Honza, do you mind if I go ahead and check this in for you on 1.9.0?
Comment 41 Honza Bambas (:mayhemer) 2010-07-25 03:59:43 PDT
(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) 2010-07-25 22:12:09 PDT
(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 2010-07-26 04:02:21 PDT
(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) 2010-07-26 11:42:34 PDT
Smokey, thank you.

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