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

RESOLVED FIXED in Firefox 48

Status

()

Firefox
Security
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: mossop, Assigned: bgrins)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 48
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox48 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments)

(Reporter)

Description

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

Updated

2 years ago
Blocks: 1245571
(Reporter)

Comment 1

2 years ago
Created attachment 8737973 [details]
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/
Attachment #8737973 - Flags: review?(MattN+bmo)
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.
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.
(Reporter)

Comment 5

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

Comment 7

2 years ago
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/
Attachment #8737973 - Attachment description: MozReview Request: Bug 1262009: Don't attempt to display identity information when we don't have any. r?MattN → MozReview Request: Bug 1262009: Don't attempt to display identity information when we don't have any.
Attachment #8737973 - Flags: review?(MattN+bmo)
(Reporter)

Updated

2 years ago
Attachment #8737973 - Flags: review?(MattN+bmo)
(Reporter)

Updated

2 years ago
Attachment #8737973 - Flags: review?(paolo.mozmail)
(Reporter)

Updated

2 years ago
Attachment #8737973 - Flags: review?(paolo.mozmail) → review?(bgrinstead)
(Assignee)

Comment 8

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

Comment 9

2 years ago
Created attachment 8739263 [details]
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/
Attachment #8739263 - Flags: review?(tanvi)
Attachment #8739264 - Flags: review?(MattN+bmo)
(Assignee)

Comment 10

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

Comment 11

2 years ago
Dave, can you confirm that these patches fix your test?
Flags: needinfo?(dtownsend)
(Assignee)

Comment 12

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

Comment 13

2 years ago
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
Attachment #8737973 - Flags: review?(bgrinstead)
(Reporter)

Comment 14

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

Yes, this fixes it, thanks!
Flags: needinfo?(dtownsend)
(Reporter)

Updated

2 years ago
Assignee: dtownsend → bgrinstead
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
Attachment #8739264 - Flags: review?(MattN+bmo) → review+
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?
Attachment #8739263 - Flags: review?(tanvi)
Attachment #8739263 - Flags: review+
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!
(Assignee)

Comment 18

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

Comment 19

2 years ago
(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?
Flags: needinfo?(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!
Flags: needinfo?(tanvi)

Comment 23

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/3f43e85a0cfd
https://hg.mozilla.org/mozilla-central/rev/f8ec60f2667e
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48

Updated

2 years ago
Depends on: 1266196
You need to log in before you can comment on or make changes to this bug.