The default bug view has changed. See this FAQ.

Security UI needs to stop assuming that bytes transferred == bytes rendered

VERIFIED FIXED in Firefox 13

Status

()

Core
Security: PSM
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: bz, Assigned: mayhemer)

Tracking

(Depends on: 1 bug, {sec-high})

Trunk
mozilla15
x86
Mac OS X
sec-high
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox12 wontfix, firefox13+ verified, firefox14+ verified, firefox15 verified, firefox-esr1013+ verified, status1.9.2 wontfix)

Details

(Whiteboard: [sg:high][qa!][advisory-tracking+])

Attachments

(1 attachment)

It's quite possible for data to be transferred but not rendered (e.g. when stream converters of any sort, including the unknown decoder, are involved).

The security UI code will treat data being transferred as a signal to update security state.

This means it's possible to leave the browser rendering page A while the security UI shows the state for page B.  This is obviously bad.

Bug 714631 had a specific instance of this that I papered over, but this is a general problem that needs fixing.  If we need to expose more information from docshell or necko or the DOM to the security UI, we should just do that as needed!
Some STR from bug 714631 comment 30:

> 1. Visit a non-HTTPS website
> 2. Enter https://bugzilla.mozilla.org/attachment.cgi?id=614200 in the address bar
>
> Contrary to the Twitter API, I get a download dialog. However, after I cancel that
> dialog, the site identity button is still spoofed.
Whiteboard: [sg:high]
We're releasing a fix (and advisory) for bug 714631 in Firefox 12, and Chrome fixed a similar problem in Chrome 16 (with blog from the reporter). There could be renewed interest in playing with this kind of attack so we really should let this go unaddressed.
Assignee: nobody → honzab.moz
status1.9.2: --- → wontfix
status-firefox-esr10: --- → affected
status-firefox12: --- → wontfix
status-firefox13: --- → affected
status-firefox14: --- → affected
tracking-firefox13: --- → +
tracking-firefox14: --- → +
Honza, let me know how I can help. I know you've wanted to fix security indicator problems and I'd be happy to talk through whatever ideas you have to see how we can fix them.
I'll start working on this on Monday.


(In reply to Brian Smith (:bsmith) from comment #3)
> Honza, let me know how I can help. I know you've wanted to fix security
> indicator problems and I'd be happy to talk through whatever ideas you have
> to see how we can fix them.

I wanted to do some changes, but this bug needs a quick fix.  

We may start some discussion on what we should change (not in this bug).  There are bugs, secure browser UI should be radically rewritten - that was what I wanted to do.
Status: NEW → ASSIGNED
(In reply to Jeroen van der Gun from Bug 714631 comment #30)
> This is also interesting:
> 
> 1. Visit a non-HTTPS website
> 2. Enter https://bugzilla.mozilla.org/attachment.cgi?id=614200 in the
> address bar
> 
> Contrary to the Twitter API, I get a download dialog. However, after I
> cancel that dialog, the site identity button is still spoofed.

My test:
- I'm at e.g. www.janbambas.cz (non-https)
- I enter https://mylocalhttpsserver/the-test-attachment.atom at the address bar(my local server where I host the attachment with '.atom' extension)

=> larry then shows blue UI, janbambas.cz and expanding it shows "you are connected to janbambas.cz, Verified by: my test CA" (that is my own test CA of my local server's cert)

The patch for bug 714631 doesn't fix that.
Just a tech summary:

- we load the .atom file
- content type is application/vnd.mozilla.maybe.feed
- we find FeedConverter able to convert the data to any type
- this may lead to display of the feed content on the screen, so this load is not considered a download (no LOAD_RETARGETED_DOCUMENT_URI flag set that would prevent security state update)
- the feed has a corrupted content that cannot be displayed
- feed converter then opens a new channel to the same URI at [1]
- content type is application/atom+xml this time
- no converter or handler found -> download the feed as a file

The first load breaks the UI security state since it is considered a top level document load and also has transferred data.  We don't update security state only in OnLocationChange since top level document load stop may be valid for changing security state while we don't change the location.  (I have to remember why exactly, I think wyciwyg channels are one of the cases.)

I need an advice whether there is some generic way how to determine the number of bytes that has actually been rendered (ns*ContentSink?).  


[1] http://hg.mozilla.org/mozilla-central/annotate/85abf12e5c83/browser/components/feeds/src/FeedConverter.js#l288
Simply avoiding call to EvaluateAndUpdateSecurityState when the request didn't render any data is not a solution here.  That breaks mixed content indicators.  To explain:

- I go to a secured page (https://server.com/secured.html)
- I navigate with an anchor on that page to a secured page that has a mixed content, e.g. an http image (https://server.com/mixed.html)
- I click the bad-feed link on the midex.html page
- that would trigger this bug (as stated in comment 6)
- but, when I instrument the code *not* to call EvaluateAndUpdateSecurityState from OnStateChange(STOP, DOCUMENT_LOAD) at [1] this bug disappears (*) ; state is still "no ssl"
- I go back to the initial secured page, state is back to "secure"
- I go forward to the mixed content page => state is unexpectedly "secure"

Reason:
- when I click the bad-feed link, sec UI in OnStateChange(START, DOCUMENT_LOAD) resets subrequest counters to all zeros (there are counters for non-secure, secure, etc - basis for mixed content UI)
- with my patch, when I later click back, these zeros are mis-stored to the associated security info of the https://server.com/mixed.html channel instead to the associated security info of the bad-feed ; mCurrentToplevelSecurityInfo is not assigned the bad-feed's info object because of the missing call to EvaluateAndUpdateSecurityState :(
- when navigating forward to the mixed.html page, associated content security is restored to all zeros => state is "secured"


I have to think of a solution here...  A long time ago I wanted to track the subrequests strictly only in the current secinfo object and therefor don't need to restore/switch tracking members, and ask it for the tracking info when telling UI the state.

I'll try to have a patch for it by tomorrow.


[1] http://hg.mozilla.org/mozilla-central/annotate/424cb3a6141b/security/manager/boot/src/nsSecureBrowserUIImpl.cpp#l1230

(*) The security state doesn't change after OnLocationChange (checked!), so invoking EvaluateAndUpdateSecurityState is redundant in updating the UI and internal top level load security state when called from OnStateChange(STOP, DOCUMENT_LOAD) ; clicking the bad-feed link doesn't invoke OnLocationChange, since nsDSURIContentListener is not used as the load consumer - this is expected.
Depends on: 748809
Created attachment 618367 [details] [diff] [review]
v1

- prevents redundant security state update and notification from OnStateChange(STOP | DOCUMENT) that is causing this bug

- update and onSecurityChange notification now invoked only from OnLocationChange

- I've checked the state doesn't change between OnLocationChange() and OnStateChange(STOP | DOCUMENT)

- if there is no OnLocationChange between OnStateChange(START | DOCUMENT) and OnStateChange(STOP | DOCUMENT) the content hasn't been targeted to a docshell and therefor it hasn't been rendered (RETARGETED flag isn't not set in case of the FeedConverter)

- if the target sink is changing in OnStateChange(STOP | DOCUMENT) (usually is being set, in a brand new window), we trigger the state update and notification even from OnStateChange(STOP | DOCUMENT)

- also, when there is no OnLocationChange leading to security state update and notification, we revert subrequest counters to the vales saved in OnStateChange(START | DOCUMENT) otherwise on navigating back in the session history would overwrite the potentially non-zero values of the counters with all zeros masking mixed state on going forward

- updates browser_alltabslistener.js test: redundant security notification is removed, only one onSecurityChange is expected
Attachment #618367 - Flags: review?(kaie)
Keywords: sec-high

Comment 9

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

r=kaie
Attachment #618367 - Flags: review?(kaie) → review+
Thanks Kai!
Comment on attachment 618367 [details] [diff] [review]
v1

https://hg.mozilla.org/mozilla-central/rev/625cac524c57
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla15
Please request approval for aurora and beta.
status-firefox15: --- → fixed
tracking-firefox-esr10: --- → 13+
I guess there isn't any existing test infrastructure to easily write tests for this? browser_alltabslistener.js is better than nothing, but seems like an area that would benefit from better, more targeted test coverage :/
Taking a change to the underlying logic for all of our security UI on beta seems rather scary to me.
Comment on attachment 618367 [details] [diff] [review]
v1

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: 
Fix Landed on Version:
Risk to taking this patch (and alternatives if risky): 
String changes made by this patch:

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

[Approval Request Comment]
Regression caused by (bug #): 
User impact if declined: 
Testing completed (on m-c, etc.): 
Risk to taking this patch (and alternatives if risky):
String changes made by this patch:

[Approval Request Comment]
Regression caused by (bug #): 
User impact if declined: 
Testing completed (on m-c, etc.): 
Risk to taking this patch (and alternatives if risky):
String changes made by this patch:

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: sg:high
User impact if declined: SSL state spoof
Fix Landed on Version: 15
Risk to taking this patch (and alternatives if risky): potential issues with mixed content and security state detection
String changes made by this patch: none

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

[Approval Request Comment]
Regression caused by (bug #): N/A - multiple bugs, present for a very long time
User impact if declined: SSL state spoof
Testing completed (on m-c, etc.): just landed
Risk to taking this patch (and alternatives if risky): same as for ESR above
String changes made by this patch: none

This should bake on m-c some time, since this code is quit complex.
Attachment #618367 - Flags: approval-mozilla-esr10?
Attachment #618367 - Flags: approval-mozilla-beta?
Attachment #618367 - Flags: approval-mozilla-aurora?
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #13)
> I guess there isn't any existing test infrastructure to easily write tests
> for this? browser_alltabslistener.js is better than nothing, but seems like
> an area that would benefit from better, more targeted test coverage :/

security/mixedcontent.  We have mochitests for this.  But not for this one particular case that needs atom involved.  I can write a test, but I'm more worried about not regressing the current overall functionality.
Serge is working on the code/test changes for SeaMonkey for this -- CCing him
Blocks: 752211
Comment on attachment 618367 [details] [diff] [review]
v1

[Triage Comment]
Approved for all branches based upon the security rating and where we are in the cycle.
Attachment #618367 - Flags: approval-mozilla-esr10?
Attachment #618367 - Flags: approval-mozilla-esr10+
Attachment #618367 - Flags: approval-mozilla-beta?
Attachment #618367 - Flags: approval-mozilla-beta+
Attachment #618367 - Flags: approval-mozilla-aurora?
Attachment #618367 - Flags: approval-mozilla-aurora+
Comment on attachment 618367 [details] [diff] [review]
v1

https://hg.mozilla.org/releases/mozilla-aurora/rev/cd5706ecd364
https://hg.mozilla.org/releases/mozilla-beta/rev/e4adf9011bfc
https://hg.mozilla.org/releases/mozilla-esr10/rev/eb07d47ae75a
status-firefox-esr10: affected → fixed
status-firefox13: affected → fixed
status-firefox14: affected → fixed
Whiteboard: [sg:high] → [sg:high][qa+]
Could I get clarification on how to verify this bug? I'm a bit puzzled on what needs to be done here.
Jason, see comment 1?

Updated

5 years ago
status-firefox13: fixed → verified

Updated

5 years ago
status-firefox13: verified → fixed
(In reply to Boris Zbarsky (:bz) from comment #21)
> Jason, see comment 1?

Hmm okay. Still a bit puzzled, as on ESR, I'm not seeing this fixed based on what I interpret in comment 1, but I may not be running the test correctly. Here's what I did:

1. Go to http://www.google.com
2. Enter https://bugzilla.mozilla.org/attachment.cgi?id=614200 into the address bar
3. Download dialog appears
4. Click cancel

Result: See screenshot - http://screencast.com/t/rkyyLNpjNz
Jason, that screenshot definitely looks like the bug is not fixed in whatever build you're testing.  That said, I just tried today's ESR nightly, and things seem to work correctly there.  Which ESR build are you testing?
(In reply to Boris Zbarsky (:bz) from comment #23)
> Jason, that screenshot definitely looks like the bug is not fixed in
> whatever build you're testing.  That said, I just tried today's ESR nightly,
> and things seem to work correctly there.  Which ESR build are you testing?

Firefox 10.0.4 ESR
(In reply to Jason Smith [:jsmith] from comment #24)
> (In reply to Boris Zbarsky (:bz) from comment #23)
> > Jason, that screenshot definitely looks like the bug is not fixed in
> > whatever build you're testing.  That said, I just tried today's ESR nightly,
> > and things seem to work correctly there.  Which ESR build are you testing?
> 
> Firefox 10.0.4 ESR

Oh, that would explain why I'm seeing that not working. I'm not running the latest ESR build.
(In reply to Jason Smith [:jsmith] from comment #25)
> (In reply to Jason Smith [:jsmith] from comment #24)
> > (In reply to Boris Zbarsky (:bz) from comment #23)
> > > Jason, that screenshot definitely looks like the bug is not fixed in
> > > whatever build you're testing.  That said, I just tried today's ESR nightly,
> > > and things seem to work correctly there.  Which ESR build are you testing?
> > 
> > Firefox 10.0.4 ESR
> 
> Oh, that would explain why I'm seeing that not working. I'm not running the
> latest ESR build.

Or maybe not. I don't see a Firefox 10.0.5 ESR build on ftp.mozilla.org (or maybe I'm not looking at the right spot?)
(In reply to Jason Smith [:jsmith] from comment #26)
> (In reply to Jason Smith [:jsmith] from comment #25)
> > (In reply to Jason Smith [:jsmith] from comment #24)
> > > (In reply to Boris Zbarsky (:bz) from comment #23)
> > > > Jason, that screenshot definitely looks like the bug is not fixed in
> > > > whatever build you're testing.  That said, I just tried today's ESR nightly,
> > > > and things seem to work correctly there.  Which ESR build are you testing?
> > > 
> > > Firefox 10.0.4 ESR
> > 
> > Oh, that would explain why I'm seeing that not working. I'm not running the
> > latest ESR build.
> 
> Or maybe not. I don't see a Firefox 10.0.5 ESR build on ftp.mozilla.org (or
> maybe I'm not looking at the right spot?)

Ah figured it out. My bad. I'll re-run this test with latest-mozilla-esr10.
Verified on beta.
status-firefox13: fixed → verified

Updated

5 years ago
status-firefox14: fixed → verified

Updated

5 years ago
status-firefox15: fixed → verified

Updated

5 years ago
Status: RESOLVED → VERIFIED
Verified on Firefox 10 ESR 5 (now I have the right build). I did what I said in comment 22, didn't look I hit issues. I also clicked identity button after I completed comment 22's steps, which reverted the location bar to the current webpage location (http://www.google.com) along with google's identification information. This seems valid.
status-firefox-esr10: fixed → verified
Whiteboard: [sg:high][qa+] → [sg:high][qa!]
Whiteboard: [sg:high][qa!] → [sg:high][qa!][advisory-tracking+]

Updated

5 years ago
Blocks: 768568
No longer blocks: 768568
Depends on: 768568
Group: core-security
Depends on: 773582
You need to log in before you can comment on or make changes to this bug.