Bug 768568 (CVE-2012-3976)

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

RESOLVED FIXED in Firefox 15

Status

Core Graveyard
Security: UI
--
critical
RESOLVED FIXED
5 years ago
7 months ago

People

(Reporter: Mark Poticha, Assigned: mayhemer)

Tracking

({regression, sec-high})

10 Branch
mozilla17
regression, sec-high
Dependency tree / graph

Firefox Tracking Flags

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

Details

(Whiteboard: [STR: comment 8][qa?][advisory-tracking+])

Attachments

(4 attachments)

(Reporter)

Description

5 years ago
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
(Reporter)

Comment 1

5 years ago
Created attachment 636799 [details]
left https://comdirect go to https://moneyou
(Reporter)

Comment 2

5 years ago
Created attachment 636800 [details]
wrong certificate in the address input box
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

Comment 4

5 years ago
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

5 years ago
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.
(Reporter)

Comment 6

5 years ago
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

5 years ago
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true

Comment 8

5 years ago
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

5 years ago
I can reproduce using Firefox 10.0.5, Firefox 13, Firefox 14.

Firefox 8 was good.
OS: Windows 7 → All
Hardware: x86_64 → All

Updated

5 years ago
tracking-firefox-esr10: --- → ?
tracking-firefox13: --- → ?
tracking-firefox14: --- → ?
tracking-firefox15: --- → ?
tracking-firefox16: --- → ?
Whiteboard: [STR: comment 8]

Updated

5 years ago
Summary: Certificate visualization failure → Incorrect EV identity display

Comment 10

5 years ago
Firefox 9 works, too.

Firefox 10 is the first to show this broken behaviour.

I can still reproduce with Aurora15 and Nightly16.
Keywords: regression, regressionwindow-wanted
Version: 13 Branch → 10 Branch

Updated

5 years ago
Summary: Incorrect EV identity display → Incorrect EV identity display (loading new page, identity not updated)

Comment 11

5 years ago
I cannot reproduce using Firefox 10.0.4

Maybe the regression was caused by the fix for bug 745254.
Depends on: 745254
(Reporter)

Comment 12

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

Comment 13

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

Updated

5 years ago
Severity: normal → critical
Keywords: regressionwindow-wanted → sec-high
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

Comment 15

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

yes
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?
tracking-firefox-esr10: ? → 14+
tracking-firefox14: ? → +
tracking-firefox15: ? → +
tracking-firefox16: ? → +
(Assignee)

Comment 17

5 years ago
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...)
(Assignee)

Comment 18

5 years ago
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.
(Assignee)

Comment 19

5 years ago
Created attachment 637731 [details] [diff] [review]
v1

Very simple patch.
Assignee: nobody → honzab.moz
Status: NEW → ASSIGNED
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.
tracking-firefox13: ? → +
(Assignee)

Comment 21

5 years ago
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.

Updated

5 years ago
tracking-firefox-esr10: 14+ → ?
(Assignee)

Comment 22

5 years ago
Comment on attachment 637731 [details] [diff] [review]
v1

Brina, will you get to this?
Attachment #637731 - Flags: review?(kaie) → review?(bsmith)
(Assignee)

Comment 23

5 years ago
sorry: s/Brina/Brian/ :)
(Assignee)

Comment 24

5 years ago
Comment on attachment 637731 [details] [diff] [review]
v1

Brian doesn't feel confident to do this review.  Kai, it's up to you.
Attachment #637731 - Flags: review?(bsmith) → review?(kaie)
status-firefox-esr10: --- → affected
status-firefox14: --- → wontfix
status-firefox15: --- → affected
status-firefox16: --- → affected
status-firefox17: --- → affected
tracking-firefox-esr10: ? → 15+
tracking-firefox17: --- → +
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

5 years ago
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

5 years ago
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.
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?
(Assignee)

Comment 29

5 years ago
(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
(Assignee)

Comment 30

5 years ago
Comment on attachment 637731 [details] [diff] [review]
v1

https://hg.mozilla.org/integration/mozilla-inbound/rev/36bf4862be19
Attachment #637731 - Flags: checkin+
(Assignee)

Comment 31

5 years ago
https://tbpl.mozilla.org/?tree=Try&rev=6c65467433ca
https://hg.mozilla.org/mozilla-central/rev/36bf4862be19
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
status-firefox17: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
(Assignee)

Comment 33

5 years ago
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.
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]
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.
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?
(Assignee)

Comment 36

5 years ago
Today.
(Assignee)

Comment 37

5 years ago
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
(Assignee)

Comment 38

5 years ago
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
(Assignee)

Updated

5 years ago
status-firefox-esr10: affected → fixed
status-firefox15: affected → fixed
status-firefox16: affected → fixed
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
(Assignee)

Comment 40

5 years ago
Comment on attachment 637731 [details] [diff] [review]
v1

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