Last Comment Bug 768568 - (CVE-2012-3976) Incorrect EV identity display (loading new page, identity not updated)
: Incorrect EV identity display (loading new page, identity not updated)
[STR: comment 8][qa?][advisory-tracki...
: regression, sec-high
Product: Core Graveyard
Classification: Graveyard
Component: Security: UI (show other bugs)
: 10 Branch
: All All
: -- critical (vote)
: mozilla17
Assigned To: Honza Bambas (:mayhemer)
Depends on:
Blocks: 745254 784060
  Show dependency treegraph
Reported: 2012-06-26 11:20 PDT by Mark Poticha
Modified: 2016-09-27 13:03 PDT (History)
11 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---

Über Mozilla Firefox_2012-06-26_20-04-05 #1.png (100.80 KB, image/png)
2012-06-26 11:20 PDT, Mark Poticha
no flags Details
left https://comdirect go to https://moneyou (48.56 KB, image/png)
2012-06-26 11:22 PDT, Mark Poticha
no flags Details
wrong certificate in the address input box (122.60 KB, image/png)
2012-06-26 11:23 PDT, Mark Poticha
no flags Details
v1 (4.21 KB, patch)
2012-06-28 16:47 PDT, Honza Bambas (:mayhemer)
kaie: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
lukasblakk+bugs: approval‑mozilla‑beta+
lukasblakk+bugs: approval‑mozilla‑esr10+
honzab.moz: checkin+
Details | Diff | Splinter Review

Description Mark Poticha 2012-06-26 11:20:00 PDT
Created attachment 636798 [details]
Ü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 2012-06-26 11:22:16 PDT
Created attachment 636799 [details]
left https://comdirect go to https://moneyou
Comment 2 Mark Poticha 2012-06-26 11:23:07 PDT
Created attachment 636800 [details]
wrong certificate in the address input box
Comment 3 Daniel Veditz [:dveditz] 2012-06-26 17:11:56 PDT
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) 2012-06-27 02:23:10 PDT
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) 2012-06-27 02:25:00 PDT
I cannot reproduce the problem yet, but some links:

On page
on the middle right, click "Demokonto starten" to get into the banking area
for demonstration purposes.

I couldn't find a link from comdirect to moneyou in the public area,
waiting for feedback from Mark.
Comment 6 Mark Poticha 2012-06-27 02:54:32 PDT
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) 2012-06-27 03:33:57 PDT
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) 2012-06-27 03:49:37 PDT
Here are steps to reproduce:

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

paste this link:
after the page loads, paste this link:

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

Paste this link into address bar:
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

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) 2012-06-27 04:09:59 PDT
I can reproduce using Firefox 10.0.5, Firefox 13, Firefox 14.

Firefox 8 was good.
Comment 10 Kai Engert (:kaie) 2012-06-27 04:38:56 PDT
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) 2012-06-27 05:28:45 PDT
I cannot reproduce using Firefox 10.0.4

Maybe the regression was caused by the fix for bug 745254.
Comment 12 Mark Poticha 2012-06-27 05:50:38 PDT
btw.: Great work, guys! Thank you for the bigest browser!
Comment 13 Kai Engert (:kaie) 2012-06-27 06:34:04 PDT
Yes, backing out attachment 618367 [details] [diff] [review] (from bug 745254) on esr10 branch fixes this issue.
Comment 14 :Gavin Sharp [email:] 2012-06-27 09:48:32 PDT
(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) 2012-06-27 12:19:59 PDT
> When you say "Firefox 10" here, do you mean the ESR version?

Comment 16 Alex Keybl [:akeybl] 2012-06-27 15:47:20 PDT
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) 2012-06-27 18:38:30 PDT
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 <> but cached host is <>.

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) 2012-06-28 16:26:41 PDT
Simpler str:
- open, let load
- open a new blank tab
- load any page (even non-secure), let load
- close the tab
- enter, 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
- 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) 2012-06-28 16:47:09 PDT
Created attachment 637731 [details] [diff] [review]

Very simple patch.
Comment 20 Lukas Blakk [:lsblakk] use ?needinfo 2012-07-02 11:16:17 PDT
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) 2012-07-09 11:35:40 PDT
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) 2012-07-18 10:48:11 PDT
Comment on attachment 637731 [details] [diff] [review]

Brina, will you get to this?
Comment 23 Honza Bambas (:mayhemer) 2012-07-18 10:48:32 PDT
sorry: s/Brina/Brian/ :)
Comment 24 Honza Bambas (:mayhemer) 2012-07-18 15:39:34 PDT
Comment on attachment 637731 [details] [diff] [review]

Brian doesn't feel confident to do this review.  Kai, it's up to you.
Comment 25 Daniel Veditz [:dveditz] 2012-07-19 16:24:27 PDT
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) 2012-07-20 03:57:01 PDT
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) 2012-07-20 05:16:35 PDT
Comment on attachment 637731 [details] [diff] [review]


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] 2012-07-26 17:23:05 PDT
Can this land so that we can consider uplift to FF15 beta and FF16 aurora if deemed low risk?
Comment 29 Honza Bambas (:mayhemer) 2012-07-30 08:29:40 PDT
(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:
Comment 31 Honza Bambas (:mayhemer) 2012-07-31 12:03:57 PDT
Comment 32 Ed Morley [:emorley] 2012-08-01 02:59:32 PDT
Comment 33 Honza Bambas (:mayhemer) 2012-08-01 11:36:08 PDT
Comment on attachment 637731 [details] [diff] [review]

[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 2012-08-01 17:38:21 PDT
Comment on attachment 637731 [details] [diff] [review]

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] 2012-08-08 16:30:45 PDT
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) 2012-08-09 09:04:23 PDT
Comment 39 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-08-14 15:17:42 PDT
Should this have a testcase in-testsuite? does it need a manual test?
Comment 40 Honza Bambas (:mayhemer) 2012-11-27 05:16:57 PST
Comment on attachment 637731 [details] [diff] [review]

Doesn't make sense anymore.

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