768568 - (CVE-2012-3976) Incorrect EV identity display (loading new page, identity not updated)

Status: RESOLVED FIXED

(Core Graveyard :: Security: UI, defect)

Description

[Mark Poticha]

Attachment: Über Mozilla Firefox_2012-06-26_20-04-05 #1.png

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:13.0) Gecko/20100101 Firefox/13.0.1
Build ID: 20120614114901

Steps to reproduce:

I've bin changed form a one https encrypted website to another https encrypted  one. 


Actual results:

On the address input box was still the certificate of the first website in green visible


Expected results:

On the address input box have to be the certificate of the second website

Comment 1

[Mark Poticha]

Attachment: left https://comdirect go to https://moneyou

Comment 2

[Mark Poticha]

Attachment: wrong certificate in the address input box

Comment 3

[Daniel Veditz [:dveditz]]

Do you know how the one site transitioned to the other? What was the original URL and what did you do that caused the navigation?

Comment 4

[Kai Engert (:KaiE:)]

Mark, weißt Du noch auf welcher Seite der ComDirect du warst, direkt bevor Du zu Moneyou weiterleitet wurdest? Warst Du in deinem persönlichen Bereich eingeloggt?

Weißt Du noch, ob Du einen Link zu Moneyou geklickt hast? Oder hast Du die Adresse von moneyou in die Adresszeile eingegeben? Oder hast Du auf eine Werbung geklickt?

Comment 5

[Kai Engert (:KaiE:)]

I cannot reproduce the problem yet, but some links:

On page
  https://kunde.comdirect.de/lp/wt/login
on the middle right, click "Demokonto starten" to get into the banking area
for demonstration purposes.

https://secure.moneyou.de/exp/jsp/authenticationDE.jsp

I couldn't find a link from comdirect to moneyou in the public area,
waiting for feedback from Mark.

Comment 6

[Mark Poticha]

1. log off comdirect
2. typed the url of moneyou in the address inputbox from firefox.
   (or switchd it from the hint list of last visited pages)

that it.

Comment 7

[Kai Engert (:KaiE:)]

Confirming :(

I was able to reproduce it once. Now I cannot produce any more. 
I'm trying to find reliable steps to reproduce the bug.

Comment 8

[Kai Engert (:KaiE:)]

Here are steps to reproduce:


Create a new profile.
Prepare your browser, so that it has the necessary history entries.

paste this link: https://www.comdirect.de
after the page loads, paste this link: https://secure.moneyou.de/exp/jsp/authenticationDE.jsp

Now quit Firefox and restart. Now we are ready to reproduce the bug.

Paste this link into address bar: https://www.comdirect.de
After the page loaded, in the upper left area, click the green LOGIN
After login page loaded, on the right hand side, middle area, lower area, click "Demokonto starten", which will open a new tab.
Wait until the page in the new tab has loaded.
Close the tab.
You're now back in the original tab. Click into the address bar. Type three letters "sec". 
The list of recent pages will show. Use the down arrow keyboard key to select the entry with secure.moneyou.de/exp/jsp/authenticationDE.jsp

You now have reproduced the bug. Both page content and address shown in address bar show moneyou, but the green identity area still says "comdirect bank".

Comment 9

[Kai Engert (:KaiE:)]

I can reproduce using Firefox 10.0.5, Firefox 13, Firefox 14.

Firefox 8 was good.

Comment 10

[Kai Engert (:KaiE:)]

Firefox 9 works, too.

Firefox 10 is the first to show this broken behaviour.

I can still reproduce with Aurora15 and Nightly16.

Comment 11

[Kai Engert (:KaiE:)]

I cannot reproduce using Firefox 10.0.4

Maybe the regression was caused by the fix for bug 745254.

Comment 12

[Mark Poticha]

btw.: Great work, guys! Thank you for the bigest browser!

Comment 13

[Kai Engert (:KaiE:)]

Yes, backing out attachment 618367 [details] [diff] [review] (from bug 745254) on esr10 branch fixes this issue.

Comment 14

[:Gavin Sharp [email: [e-mail]]]

(In reply to Kai Engert (:kaie) from comment #10)
> Firefox 10 is the first to show this broken behaviour.

Bug 745254 only landed on ESR, and 13 and later. When you say "Firefox 10" here, do you mean the ESR version?

Comment 15

[Kai Engert (:KaiE:)]

> When you say "Firefox 10" here, do you mean the ESR version?

yes

Comment 16

[Alex Keybl [:akeybl]]

Tracking for 14 and up. From the STR it's not clear if this reproduces with links clicked from the first https page. If not, I probably wouldn't rush a fix into a FF13 dot release (if there is another one). What do you all think?

Comment 17

[Honza Bambas (:mayhemer)]

Kai, thanks for the STR.

This is indication of the cause:

-------------------------------
Timestamp: 6/28/2012 3:21:18 AM
Error: ASSERTION: browser.js host is inconsistent. Content window has <secure.moneyou.de> but cached host is <kunde.comdirect.de>.

Source File: chrome://browser/content/browser.js
Line: 9471
-------------------------------

Problem is that nsSecureBrowserUI::OnLocationChange (that calls its sink's OnSecurityChange) is (in this case only, apparently) fired sooner then XULBrowserWindow.onLocationChange.  Then XULBrowserWindow.onSecurityChange ignores the security change notification since it believes the host has not changed but the assertion failure indicates something is wrong.

This assertion fails also when I back patch for bug 745254 out.  But since we fire the second OnSecurityChange notification from OnStopRequest after the document has finished loading, the state updates correctly.

Next steps:
- figure out why this cannot be reproduced in common use cases and only when this complicated process is made (might lead to a solution)
- try to exchange the order of calls to OnLocationChange (may be hard)
- post the OnSecurityChange notification (may be racy...)

Comment 18

[Honza Bambas (:mayhemer)]

Simpler str:
- open https://www.paypal.com/, let load
- open a new blank tab
- load any page (even non-secure), let load
- close the tab
- enter https://www.verisign.com/, let load
=> larry still shows UI for paypal

Problem is that OnLocationChange is calling OnSecurityChange of the sink (the window) BEFORE OnLocationChange of the window is called.  

The browser window, the sink, is XULBrowserWindow class.

XULBrowserWindow.onLocationChange does:
- set XULBrowserWindow._hostChanged to true

XULBrowserWindow.onSecurityChange does:
- early return when !XULBrowserWindow._hostChanged && XULBrowserWindow._state (security state) == the newly reported state
otherwise:
- set XULBrowserWindow._hostChanged to false
- update the security UI

During page loading, first onSecurityChange is called.  Since onLocationChange of the window is called after it, it means _hostChanged is set to true.  On next call of onSecurityChange the UI gets updated thanks _hostChanged == true.  But then _hostChanged is dropped back to false.  (This is definitely wrong!)

But when another tab is closed, order of calls is backward: onLocationChange is called first, then onSecurityChange that drops _hostChanged back to false.

When user then manually changes the URL, first onSecurityChange is called and it is bypassed because _hostChanged == false && _state == aNewState.

This can affect any secure page.

I'll create a patch that will still keep bug 745254 fixed and also fix this bug by reverting the behavior to call onSecurityChange *also* from STATE_STOP STATE_DOCUMENT.

Comment 19

[Honza Bambas (:mayhemer)]

Attachment: v1

Very simple patch.

Comment 20

[Lukas Blakk [:lsblakk] use ?needinfo]

Will set tracking to + for 13 in case we have any chemspills between now and when 14 is released so we can consider this.

Comment 21

[Honza Bambas (:mayhemer)]

Kai, ping on review.  This bug is tracked and deadline is closing.  If you don't have time to do this, I can find other reviewer.  Just let me know.

Comment 22

[Honza Bambas (:mayhemer)]

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

Brina, will you get to this?

Comment 23

[Honza Bambas (:mayhemer)]

sorry: s/Brina/Brian/ :)

Comment 24

[Honza Bambas (:mayhemer)]

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

Brian doesn't feel confident to do this review.  Kai, it's up to you.

Comment 25

[Daniel Veditz [:dveditz]]

Given the steps required to reproduce in comment 8 I'd lean more toward sec-moderate, but maybe there's some way for web content to force this state as there was in the bug that regressed this (in which case sec-high is appropriate)

Comment 26

[Kai Engert (:KaiE:)]

currently testing...

nsDocLoader.cpp: In member function ‘void nsDocLoader::FireOnLocationChange(nsIWebProgress*, nsIRequest*, nsIURI*, PRUint32)’:
nsDocLoader.cpp:1390:5: error: cannot pass objects of non-trivially-copyable type ‘class nsCOMPtr<nsIWebProgressListener>’ through ‘...’

use listener.get() instead of listener

Comment 27

[Kai Engert (:KaiE:)]

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

r=kaie

Please fix the compilation issue I mention in the previous comment.

In one existing comment we have a typo "OnLacationChange", please change to "OnLocationChange" when you commit.

Comment 28

[Alex Keybl [:akeybl]]

Can this land so that we can consider uplift to FF15 beta and FF16 aurora if deemed low risk?

Comment 29

[Honza Bambas (:mayhemer)]

(In reply to Alex Keybl [:akeybl] from comment #28)
> Can this land so that we can consider uplift to FF15 beta and FF16 aurora if
> deemed low risk?

Working on landing this: https://tbpl.mozilla.org/?tree=Try&rev=6c65467433ca

Comment 30

[Honza Bambas (:mayhemer)]

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/36bf4862be19

Comment 31

[Honza Bambas (:mayhemer)]

https://tbpl.mozilla.org/?tree=Try&rev=6c65467433ca

Comment 32

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/36bf4862be19

Comment 33

[Honza Bambas (:mayhemer)]

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

[Approval Request Comment]
User impact if declined: potential risk of ssl status spoof
Fix Landed on Version: 17
Risk to taking this patch (and alternatives if risky): low, since it reverts behavior before we landed bug 745254 that caused this regression.
String or UUID changes made by this patch: none
Regression caused by (bug #): 745254 

Not sure whether to take this on release too, but that is tracked for v14 as well.  We have 3 and a half week till next merge.

Comment 34

[Lukas Blakk [:lsblakk] use ?needinfo]

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

Approving for all but mozilla-release, we are 3 weeks from the next merge but this was already wontfixed for 14 so it's not something we'd respin for.  Will leave the nom in case there was a reason to respin we'd consider this for a tagalong.

Comment 35

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

Are we fixing this in current beta and aurora, where it is approved (as well as ESR)? When can we expect a checkin?

Comment 36

[Honza Bambas (:mayhemer)]

Today.

Comment 37

[Honza Bambas (:mayhemer)]

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/49848d305daa
https://hg.mozilla.org/releases/mozilla-beta/rev/f27018523082
https://hg.mozilla.org/releases/mozilla-esr10/rev/191e0943b150

Comment 38

[Honza Bambas (:mayhemer)]

Plus a test fix (slippy bitch):

https://hg.mozilla.org/releases/mozilla-aurora/rev/6d0ad34230a0
https://hg.mozilla.org/releases/mozilla-beta/rev/972dd5cab80c
https://hg.mozilla.org/releases/mozilla-esr10/rev/0d1caf6fed20

Comment 39

[u279076]

Should this have a testcase in-testsuite? does it need a manual test?

Comment 40

[Honza Bambas (:mayhemer)]

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

Doesn't make sense anymore.