Last Comment Bug 745254 - Security UI needs to stop assuming that bytes transferred == bytes rendered
: Security UI needs to stop assuming that bytes transferred == bytes rendered
Status: VERIFIED FIXED
[sg:high][qa!][advisory-tracking+]
: sec-high
Product: Core
Classification: Components
Component: Security: PSM (show other bugs)
: Trunk
: x86 Mac OS X
: -- normal (vote)
: mozilla15
Assigned To: Honza Bambas (:mayhemer)
:
Mentors:
Depends on: 748809 CVE-2012-3976 773582
Blocks: CVE-2012-0479 752211
  Show dependency treegraph
 
Reported: 2012-04-13 10:49 PDT by Boris Zbarsky [:bz]
Modified: 2012-07-13 03:21 PDT (History)
10 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
wontfix
+
verified
+
verified
verified
13+
verified
wontfix


Attachments
v1 (9.08 KB, patch)
2012-04-25 11:22 PDT, Honza Bambas (:mayhemer)
kaie: review+
akeybl: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑beta+
akeybl: approval‑mozilla‑esr10+
Details | Diff | Splinter Review

Description Boris Zbarsky [:bz] 2012-04-13 10:49:17 PDT
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!
Comment 1 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-04-16 17:35:49 PDT
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.
Comment 2 Daniel Veditz [:dveditz] 2012-04-20 13:22:47 PDT
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.
Comment 3 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2012-04-20 13:57:38 PDT
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.
Comment 4 Honza Bambas (:mayhemer) 2012-04-21 15:04:32 PDT
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.
Comment 5 Honza Bambas (:mayhemer) 2012-04-21 15:18:11 PDT
(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.
Comment 6 Honza Bambas (:mayhemer) 2012-04-23 13:12:09 PDT
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
Comment 7 Honza Bambas (:mayhemer) 2012-04-24 17:26:32 PDT
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.
Comment 8 Honza Bambas (:mayhemer) 2012-04-25 11:22:21 PDT
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
Comment 9 Kai Engert (:kaie) 2012-05-01 11:13:35 PDT
Comment on attachment 618367 [details] [diff] [review]
v1

r=kaie
Comment 10 Honza Bambas (:mayhemer) 2012-05-01 11:15:02 PDT
Thanks Kai!
Comment 12 Daniel Veditz [:dveditz] 2012-05-03 17:07:44 PDT
Please request approval for aurora and beta.
Comment 13 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-05-03 17:43:10 PDT
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 :/
Comment 14 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-05-03 17:54:45 PDT
Taking a change to the underlying logic for all of our security UI on beta seems rather scary to me.
Comment 15 Honza Bambas (:mayhemer) 2012-05-04 05:31:43 PDT
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.
Comment 16 Honza Bambas (:mayhemer) 2012-05-04 05:33:34 PDT
(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.
Comment 17 Justin Wood (:Callek) (Away until Aug 29) 2012-05-05 07:08:43 PDT
Serge is working on the code/test changes for SeaMonkey for this -- CCing him
Comment 18 Alex Keybl [:akeybl] 2012-05-06 18:55:44 PDT
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.
Comment 20 Jason Smith [:jsmith] 2012-05-17 17:09:30 PDT
Could I get clarification on how to verify this bug? I'm a bit puzzled on what needs to be done here.
Comment 21 Boris Zbarsky [:bz] 2012-05-17 17:16:31 PDT
Jason, see comment 1?
Comment 22 Jason Smith [:jsmith] 2012-05-17 22:50:27 PDT
(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
Comment 23 Boris Zbarsky [:bz] 2012-05-17 23:03:23 PDT
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?
Comment 24 Jason Smith [:jsmith] 2012-05-17 23:47:14 PDT
(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
Comment 25 Jason Smith [:jsmith] 2012-05-17 23:47:41 PDT
(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.
Comment 26 Jason Smith [:jsmith] 2012-05-17 23:50:14 PDT
(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?)
Comment 27 Jason Smith [:jsmith] 2012-05-17 23:51:59 PDT
(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.
Comment 28 Jason Smith [:jsmith] 2012-05-17 23:58:19 PDT
Verified on beta.
Comment 29 Jason Smith [:jsmith] 2012-05-18 00:07:07 PDT
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.

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