Closed Bug 768568 (CVE-2012-3976) Opened 7 years ago Closed 7 years ago

Incorrect EV identity display (loading new page, identity not updated)


(Core Graveyard :: Security: UI, defect, critical)

10 Branch
Not set


(firefox13+, firefox14+ wontfix, firefox15+ fixed, firefox16+ fixed, firefox17+ fixed, firefox-esr1015+ fixed)

Tracking Status
firefox13 + ---
firefox14 + wontfix
firefox15 + fixed
firefox16 + fixed
firefox17 + fixed
firefox-esr10 15+ fixed


(Reporter: mozilla_mp, Assigned: mayhemer)



(Keywords: regression, sec-high, Whiteboard: [STR: comment 8][qa?][advisory-tracking+])


(4 files)

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
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?
Component: Untriaged → Security: UI
Product: Firefox → Core
QA Contact: untriaged → ui
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?
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.
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.
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.
Ever confirmed: true
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".
I can reproduce using Firefox 10.0.5, Firefox 13, Firefox 14.

Firefox 8 was good.
OS: Windows 7 → All
Hardware: x86_64 → All
Whiteboard: [STR: comment 8]
Summary: Certificate visualization failure → Incorrect EV identity display
Firefox 9 works, too.

Firefox 10 is the first to show this broken behaviour.

I can still reproduce with Aurora15 and Nightly16.
Version: 13 Branch → 10 Branch
Summary: Incorrect EV identity display → Incorrect EV identity display (loading new page, identity not updated)
I cannot reproduce using Firefox 10.0.4

Maybe the regression was caused by the fix for bug 745254.
Depends on: 745254
btw.: Great work, guys! Thank you for the bigest browser!
Yes, backing out attachment 618367 [details] [diff] [review] (from bug 745254) on esr10 branch fixes this issue.
Severity: normal → critical
Whiteboard: [STR: comment 8] → [STR: comment 8][sg:high]
Whiteboard: [STR: comment 8][sg:high] → [STR: comment 8]
(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?
Blocks: 745254
No longer depends on: 745254
> When you say "Firefox 10" here, do you mean the ESR version?

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?
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...)
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.
Attached patch v1Splinter Review
Very simple patch.
Assignee: nobody → honzab.moz
Attachment #637731 - Flags: review?(kaie)
Will set tracking to + for 13 in case we have any chemspills between now and when 14 is released so we can consider this.
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 on attachment 637731 [details] [diff] [review]

Brina, will you get to this?
Attachment #637731 - Flags: review?(kaie) → review?(bsmith)
sorry: s/Brina/Brian/ :)
Comment on attachment 637731 [details] [diff] [review]

Brian doesn't feel confident to do this review.  Kai, it's up to you.
Attachment #637731 - Flags: review?(bsmith) → review?(kaie)
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)
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 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.
Attachment #637731 - Flags: review?(kaie) → review+
Can this land so that we can consider uplift to FF15 beta and FF16 aurora if deemed low risk?
(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:
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
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.
Attachment #637731 - Flags: approval-mozilla-release?
Attachment #637731 - Flags: approval-mozilla-esr10?
Attachment #637731 - Flags: approval-mozilla-beta?
Attachment #637731 - Flags: approval-mozilla-aurora?
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.
Attachment #637731 - Flags: approval-mozilla-esr10?
Attachment #637731 - Flags: approval-mozilla-esr10+
Attachment #637731 - Flags: approval-mozilla-beta?
Attachment #637731 - Flags: approval-mozilla-beta+
Attachment #637731 - Flags: approval-mozilla-aurora?
Attachment #637731 - Flags: approval-mozilla-aurora+
Are we fixing this in current beta and aurora, where it is approved (as well as ESR)? When can we expect a checkin?
Should this have a testcase in-testsuite? does it need a manual test?
Whiteboard: [STR: comment 8] → [STR: comment 8][qa?]
Whiteboard: [STR: comment 8][qa?] → [STR: comment 8][qa?][advisory-tracking+]
Alias: CVE-2012-3976
Group: core-security
Comment on attachment 637731 [details] [diff] [review]

Doesn't make sense anymore.
Attachment #637731 - Flags: approval-mozilla-release?
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.