1262009 - Firefox attempts to update ssl information for chrome URLs that contain inner frames with secure content

Status: RESOLVED FIXED

(Firefox :: Security, defect)

Description

[Dave Townsend [:mossop]]

In a document like this:

<?xml version="1.0"?>
<?xml-stylesheet href="chrome://global/skin/" type="text/css"?>
<window xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">
  <browser id="frame" disablehistory="true" flex="1" type="content"
           src="https://www.example.com"/>
</window>

When loaded in a tabbrowser tab from a chrome: url it looks like the onSecurityChange handler is seeing the state from the inner frame and thinking that this is a secure document and then trying to get the identity information giving an error like:

TypeError: this._sslStatus is null at gIdentityHandler.getIdentityData@chrome://browser/content/browser.js:6560:9
refreshIdentityBlock@chrome://browser/content/browser.js:6736:56
refreshForInsecureLoginForms@chrome://browser/content/browser.js:6649:5
gBrowserInit._delayedStartup/<@chrome://browser/content/browser.js:1078:7
updateLoginFormPresence@resource://gre/modules/LoginManagerParent.jsm:470:5
LoginManagerParent.receiveMessage@resource://gre/modules/LoginManagerParent.jsm:93:9

I'm slightly concerned that the onSecurityChange handler isn't verifying that it is looking at a top-level webprogress but I don't know the consequences of changing that. There is however a quick fix here...

Comment 1

[Dave Townsend [:mossop]]

Attachment: MozReview Request: Bug 1262009: Don't attempt to display identity information when we don't have any.

Review commit: https://reviewboard.mozilla.org/r/44225/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/44225/

Comment 2

[Matthew N. [:MattN]]

Comment on attachment 8737973 [details]
MozReview Request: Bug 1262009: Don't attempt to display identity information when we don't have any.

https://reviewboard.mozilla.org/r/44225/#review40885

::: browser/base/content/browser.js:6722
(Diff revision 1)
>                          "rtl" : "ltr";
>  
> -    } else if (this._uriHasHost && this._isSecure) {
> +    } else if (this._uriHasHost && this._isSecure && this._sslStatus) {
>        this._identityBox.className = "verifiedDomain";
>        if (this._isMixedActiveContentBlocked) {
>          this._identityBox.classList.add("mixedActiveBlocked");

I don't think this is the right place to fix the problem as it will just hide future mistakes related to identity handling (e.g. I think it's a bug if `_sslStatus` is ever falsy when `_isSecure` is truthy).

I think a better solution would be to check the event target in the InsecureLoginFormsStateChange listener at: https://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js?rev=0dca990f8325#1077

```
if (!gBrowser.getTabForBrowser(evt.target)) {
 return;
}```

Basically if the event target (<browser>) isn't part of the gBrowser (<tabbrowser>) then do nothing.

Comment 3

[Matthew N. [:MattN]]

Tests to run: https://mxr.mozilla.org/mozilla-central/search?string=InsecureLoginFormsStateChange&find=test&findi=&filter=%5E%5B%5E%5C0%5D*%24&hitlimit=&tree=mozilla-central

Comment 4

[Matthew N. [:MattN]]

A faster check to return may be `gBrowser.selectedBrowser != evt.target` since I don't see why we would need to update UI for a tab that's not selected (other than race conditions with tab switching). I think comment 2 is a less risky change so would prefer that so we don't have to deal with possible races (especially with e10s) though it seems like we currently do unnecessary work by not looking to see if the event target is visible.

Comment 5

[Dave Townsend [:mossop]]

https://reviewboard.mozilla.org/r/44225/#review40885

> I don't think this is the right place to fix the problem as it will just hide future mistakes related to identity handling (e.g. I think it's a bug if `_sslStatus` is ever falsy when `_isSecure` is truthy).
> 
> I think a better solution would be to check the event target in the InsecureLoginFormsStateChange listener at: https://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js?rev=0dca990f8325#1077
> 
> ```
> if (!gBrowser.getTabForBrowser(evt.target)) {
>  return;
> }```
> 
> Basically if the event target (<browser>) isn't part of the gBrowser (<tabbrowser>) then do nothing.

So this suggestion doesn't help. The event is always dispatched to the top-level browser where the message originated: https://dxr.mozilla.org/mozilla-central/source/toolkit/components/passwordmgr/LoginManagerParent.jsm#92

It would also mean that refreshIdentityBlock is still broken in this case should we end up calling it elsewhere (there is already one other caller but turns out that caller "fixes" the _state property to the correct value) The underlying problem is that we set that _state property incorrectly so I guess I'll try fixing that.

Comment 6

[Matthew N. [:MattN]]

Ok, darn. I forgot that your <browser> is inside a regular tabbrowser browser.

Please flag Paolo, ttaubert, and/or bgrins for review of identity handler changes as they're more familiar.

Comment 7

[Dave Townsend [:mossop]]

Comment on attachment 8737973 [details]
MozReview Request: Bug 1262009: Don't attempt to display identity information when we don't have any.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44225/diff/1-2/

Comment 8

[Brian Grinstead [:bgrins]]

Tanvi and I discussed this on IRC.  The takeaway was that this case of a nested <browser> isn't explicitly handled in the UI and in the case that we know of it's it's on about:addons which is a whitelisted _isSecureInternalUI page that doesn't hit this condition.

There are various options to handle these states, like adding a new security state for chrome pages, disallowing http / mixed content on chrome pages and always showing green lock, and disallowing the security state from being 'upgraded' but allowing it to be downgraded.

Each has tradeoffs but it seems the simplest thing is to not allow the sec state to be upgraded by the <browser>, so do not allow isSecure to be true.  Sort of like the current patch on the bug, but more explicit that we are doing this for file resources instead of relying on sslStatus.  I've got patches incoming for this.

Comment 9

[Brian Grinstead [:bgrins]]

Attachment: MozReview Request: Bug 1262009 - Treat all file connections (including chrome uris) as unknown identities;r=tanvi

Review commit: https://reviewboard.mozilla.org/r/45157/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/45157/

Comment 10

[Brian Grinstead [:bgrins]]

Attachment: MozReview Request: Bug 1262009 - Refactor isURILoadedFromFile into a setURI function that sets all relevant properties in one place;r=MattN

Review commit: https://reviewboard.mozilla.org/r/45159/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/45159/

Comment 11

[Brian Grinstead [:bgrins]]

Dave, can you confirm that these patches fix your test?

Comment 12

[Brian Grinstead [:bgrins]]

(In reply to Brian Grinstead [:bgrins] from comment #10)
> Created attachment 8739264 [details]
> MozReview Request: Bug 1262009 - Refactor isURILoadedFromFile into a setURI
> function that sets all relevant properties in one place;r=MattN
> 
> Review commit: https://reviewboard.mozilla.org/r/45159/diff/#index_header
> See other reviews: https://reviewboard.mozilla.org/r/45159/

Matt, this is just a refactor to move all uri setting stuff into a function.  Should be pretty straightforward but let me know if you don't think we should do it (split it out from the bugfix which is the first patch in the series)

Comment 13

[Brian Grinstead [:bgrins]]

Comment on attachment 8737973 [details]
MozReview Request: Bug 1262009: Don't attempt to display identity information when we don't have any.

Clearing review for now as per Comment 8

Comment 14

[Dave Townsend [:mossop]]

(In reply to Brian Grinstead [:bgrins] from comment #11)
> Dave, can you confirm that these patches fix your test?

Yes, this fixes it, thanks!

Comment 15

[Matthew N. [:MattN]]

Comment on attachment 8739264 [details]
MozReview Request: Bug 1262009 - Refactor isURILoadedFromFile into a setURI function that sets all relevant properties in one place;r=MattN

https://reviewboard.mozilla.org/r/45159/#review41825

Comment 16

[Tanvi Vyas[:tanvi]]

Comment on attachment 8739263 [details]
MozReview Request: Bug 1262009 - Treat all file connections (including chrome uris) as unknown identities;r=tanvi

https://reviewboard.mozilla.org/r/45157/#review42103

::: browser/base/content/browser.js:6386
(Diff revision 1)
>    get _isBroken() {
>      return this._state & Ci.nsIWebProgressListener.STATE_IS_BROKEN;
>    },
>  
>    get _isSecure() {
> -    return this._state & Ci.nsIWebProgressListener.STATE_IS_SECURE;
> +    // chrome uris that embed a <browser> shouldn't show up as secure even if the browser is

This comment is a little hard to read/understand.  Can you rephrase it?

Comment 17

[Tanvi Vyas[:tanvi]]

Hey Brian,
Just to make sure I have this right - With these patches we will not show any security UI for chrome pages that have embedded http/https pages.  Regardless of whether they are fully secure, mixed, have a cert override, have an insecure password, or are over http.  Right?

Thanks!

Comment 18

[Brian Grinstead [:bgrins]]

(In reply to Tanvi Vyas [:tanvi] from comment #17)
> Hey Brian,
> Just to make sure I have this right - With these patches we will not show
> any security UI for chrome pages that have embedded http/https pages. 
> Regardless of whether they are fully secure, mixed, have a cert override,
> have an insecure password, or are over http.  Right?

With this patch it will still show 'broken' states, but never be promoted to a secure state.  We would have to special case _isBroken, _isMixedActiveContentLoaded, _isMixedActiveContentBlocked in a similar manner to the patch to ignore broken warnings.  It seems safer to cover up nested secure states than it is to cover up nested broken states.  It does 'penalize' https pages with mixed content compared with http pages, but in the same way we do it today when those pages are loaded in tabs.

Comment 19

[Brian Grinstead [:bgrins]]

(In reply to Tanvi Vyas [:tanvi] from comment #16)
> Comment on attachment 8739263 [details]
> MozReview Request: Bug 1262009 - Treat all file connections (including
> chrome uris) as unknown identities;r=tanvi
> 
> https://reviewboard.mozilla.org/r/45157/#review42103
> 
> ::: browser/base/content/browser.js:6386
> (Diff revision 1)
> >    get _isBroken() {
> >      return this._state & Ci.nsIWebProgressListener.STATE_IS_BROKEN;
> >    },
> >  
> >    get _isSecure() {
> > -    return this._state & Ci.nsIWebProgressListener.STATE_IS_SECURE;
> > +    // chrome uris that embed a <browser> shouldn't show up as secure even if the browser is
> 
> This comment is a little hard to read/understand.  Can you rephrase it?

How about "If a <browser> is included within a chrome document, then this._state will refer to the security state for the <browser> and not the top level document. In this case, don't upgrade the security state in the UI.".  Or, do you have other suggestions?

Comment 20

[Tanvi Vyas[:tanvi]]

(In reply to Brian Grinstead [:bgrins] from comment #19)
> (In reply to Tanvi Vyas [:tanvi] from comment #16)
> > Comment on attachment 8739263 [details]
> > MozReview Request: Bug 1262009 - Treat all file connections (including
> > chrome uris) as unknown identities;r=tanvi
> > 
> > https://reviewboard.mozilla.org/r/45157/#review42103
> > 
> > ::: browser/base/content/browser.js:6386
> > (Diff revision 1)
> > >    get _isBroken() {
> > >      return this._state & Ci.nsIWebProgressListener.STATE_IS_BROKEN;
> > >    },
> > >  
> > >    get _isSecure() {
> > > -    return this._state & Ci.nsIWebProgressListener.STATE_IS_SECURE;
> > > +    // chrome uris that embed a <browser> shouldn't show up as secure even if the browser is
> > 
> > This comment is a little hard to read/understand.  Can you rephrase it?
> 
> How about "If a <browser> is included within a chrome document, then
> this._state will refer to the security state for the <browser> and not the
> top level document. In this case, don't upgrade the security state in the
> UI.".  Or, do you have other suggestions?

Looks good with one tweak at the end:
If a <browser> is included within a chrome document, then this._state will refer to the security state for the <browser> and not the top level document. In this case, don't upgrade the security state in the UI with the secure state of the embedded <browser>.

Thanks Brian!

Comment 21

[Brian Grinstead [:bgrins]]

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d7886d1aeca7&selectedJob=19380944

Comment 22

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/3f43e85a0cfd
https://hg.mozilla.org/integration/fx-team/rev/f8ec60f2667e

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3f43e85a0cfd
https://hg.mozilla.org/mozilla-central/rev/f8ec60f2667e