1193341 - Detect presence of password fields in any subframe, flagging those on insecure connections

Status: RESOLVED FIXED

(Toolkit :: Password Manager, defect, P1)

Description

[:Paolo Amadini]

The front-end needs to know whether insecure password fields are present in any subframe of the current page. We can reuse the gathering mechanism of the Login Fill Doorhanger initially.

Comment 1

[Tanvi Vyas[:tanvi]]

I think InsecurePasswordUtils already does this for you.
https://mxr.mozilla.org/mozilla-central/source/toolkit/components/passwordmgr/InsecurePasswordUtils.jsm

Comment 2

[:Paolo Amadini]

(In reply to Tanvi Vyas [:tanvi] from comment #1)
> I think InsecurePasswordUtils already does this for you.
> https://mxr.mozilla.org/mozilla-central/source/toolkit/components/
> passwordmgr/InsecurePasswordUtils.jsm

We still need to send the information to the parent process and distill it for use by the front-end, it's a slightly different use case than logging to the console. I'd like to discuss the architecture with Matt as well, as I think we can consolidate things, let's see if maybe we can make some time tomorrow.

Comment 3

[Tanvi Vyas[:tanvi]]

Paolo, Matt, and I just talked about this.  I'd like to share some links that might be helpful, along with my understanding of the proposal (which is probably off in a couple places).

Paolo is going to attempt to use a node list to identity <input type=password> nodes in the platform.  Once one or more password fields is detected in a particular docshell, the docshell should be notified that docShellHasPassword (or something like that)[1].

That should invoke a method in a separate file (InsecurePasswords.cpp?) that takes a docShell that has the docShellHasPassword flag and does checks similar to the checks in InsecurePasswordUtils[2], but in c++.  It should check if the uri of the docshell against a list of URI flags.  It should also traverse up sameTypeParents to the root docshell[3], checking each docshell in between for the list of URI flags.  If we find the password is insecure, we should call onSecurityChange[4] and set a webProgressListener flag STATE_INSECURE_PASSWORD (that is set in addition to whatever state the security UI already has)[5].  In addition, we should get the rootDocument from the root DocShell and call rootDoc->SetHasInsecurePassword(true)[6].

Then in nsSecureBrowserUIImpl, we should check docShell->GetHasInsecurePassword() to make sure the STATE_INSECURE_PASSWORD does not get overriden[7].  We may also have to add that check in certain parts of nsMixedContentBlocker.cpp (but I'm not sure about this yet).

We have to add logic for STATE_INSECURE_PASSWORD to browser.js checkIdentity[8] and add a new identity mode that will show the lock with the strikethrough instead of the globe.

Doing this all in platform has the advantage that it will be easy for Fennec to make similar changes.  We could also probably remove InsecurePasswordUtils and just put webconsole messages in InsecurePasswords.cpp.

[1] http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsIDocShell.idl#546
http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#2211
http://mxr.mozilla.org/mozilla-central/source/dom/base/nsIDocument.h#526
[2] https://mxr.mozilla.org/mozilla-central/source/toolkit/components/passwordmgr/InsecurePasswordUtils.jsm
[3] docshell traversal http://mxr.mozilla.org/mozilla-central/source/dom/security/nsMixedContentBlocker.cpp#622
[4] onSecurityChange - http://mxr.mozilla.org/mozilla-central/source/dom/security/nsMixedContentBlocker.cpp#671 , http://mxr.mozilla.org/mozilla-central/source/dom/security/nsMixedContentBlocker.cpp#714
[5] http://mxr.mozilla.org/mozilla-central/source/uriloader/base/nsIWebProgressListener.idl - We are running out of bits here, so we should consider lumping this flag with something else.
[6] http://mxr.mozilla.org/mozilla-central/source/dom/security/nsMixedContentBlocker.cpp#667
[7] https://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/nsSecureBrowserUIImpl.cpp#253
[8] https://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#6850

Comment 4

[Matthew N. [:MattN]]

(In reply to Tanvi Vyas [:tanvi] from comment #3)
> That should invoke a method in a separate file (InsecurePasswords.cpp?) that
> takes a docShell that has the docShellHasPassword flag and does checks
> similar to the checks in InsecurePasswordUtils[2], but in c++.  It should
> check if the uri of the docshell against a list of URI flags. It should
> also traverse up sameTypeParents to the root docshell[3], checking each
> docshell in between for the list of URI flags.  If we find the password is
> insecure, we should call onSecurityChange[4] and set a webProgressListener
> flag STATE_INSECURE_PASSWORD (that is set in addition to whatever state the
> security UI already has)[5].  In addition, we should get the rootDocument
> from the root DocShell and call rootDoc->SetHasInsecurePassword(true)[6].

What I (and I think Paolo) proposed/understood at the end of the discussion is that there should not be any duplication of security checks in this password-related code platform code. The flag that gets sent with security change notifications simply indicates whether a password field is present. UI that wants to indicate insecure password fields simply checks for the existing security flags and also for the new STATE_PASSWORD_PRESENT (or whatever) flag. That avoids duplicating security and frame-traversal logic.

Comment 5

[Tanvi Vyas[:tanvi]]

(In reply to Matthew N. [:MattN] from comment #4)
> (In reply to Tanvi Vyas [:tanvi] from comment #3)
> What I (and I think Paolo) proposed/understood at the end of the discussion
> is that there should not be any duplication of security checks in this
> password-related code platform code. The flag that gets sent with security
> change notifications simply indicates whether a password field is present.
So the platform code will simply set STATE_PASSWORD_PRESENT on the security UI of the rootDocShell.  It also needs to set rootDoc->SetHasPassword(true), so that nsSecureBrowserUIImpl can check that flag and avoid overwriting the STATE_PASSWORD_PRESENT flag.

> UI that wants to indicate insecure password fields simply checks for the
> existing security flags and also for the new STATE_PASSWORD_PRESENT (or
> whatever) flag. 
The UI code will look for "STATE_IS_INSECURE" and "STATE_PASSWORD_PRESENT" and show the crossed out lock.

> That avoids duplicating security and frame-traversal logic.
Yeah; in that case we don't need to check URI flags or check each parent.  (We do still need to get the rootDoc and rootDocShell.)

This means that InsecurePasswordUtils still has a purpose.  And in some cases what it reports to the console may not match what's in control center (because a) it uses documentURI and b) I haven't looked to see if STATE_IS_INSECURE covers the URI flags that InsecurePasswordUtils checks for).  It's worth looking into which cases and protocols we use to set STATE_IS_INSECURE.

Comment 6

[:Paolo Amadini]

(In reply to Tanvi Vyas [:tanvi] from comment #3)
> Paolo is going to attempt to use a node list to identity <input
> type=password> nodes in the platform.

I started looking into this part in more detail. I thought I'd use an nsContentList [1] similar to document.forms [2] (see also document.formControls, not currently exposed) to have a list of password fields on the page. I vaguely remembered this to be the system used by plugin presence notifications but that's incorrect, it doesn't actually need a node list.

That's because the list of plugins for the page (implemented in bug 730318) is maintained directly by the BindToTree and UnbindFromTree methods of nsObjectLoadingContent [3] which invoke methods on the document [4] to add or remove items from a list stored for each document. When this list is accessed [5] all subdocuments are included in the query. Chrome code accesses that list for the root document (and all subdocuments) through the "plugins" getter on nsDOMWindowUtils [6].

Reading the list of plugins is triggered by various types of events [7] to which the JavaScript module in the content process listens.

The BindToTree method for HTMLInputElement is the one that raises the DOMInputPasswordAdded event that we use in the Logins code (in this case, the event is also raised when the type of an existing field is changed). A list of fields present for each document, as well as listening to page show events for sub-frame navigations when necessary, is basically what the Logins code already does. It seems that what we have in place here is already very similar to what we do for plugins.

[1] http://mxr.mozilla.org/mozilla-central/source/dom/base/nsContentList.h#195
[2] http://mxr.mozilla.org/mozilla-central/source/dom/html/nsHTMLDocument.cpp#2342
[3] http://mxr.mozilla.org/mozilla-central/source/dom/base/nsObjectLoadingContent.cpp#662
[4] http://mxr.mozilla.org/mozilla-central/source/dom/base/nsDocument.h#1182
[5] http://mxr.mozilla.org/mozilla-central/source/dom/base/nsDocument.cpp#10617
[6] http://mxr.mozilla.org/mozilla-central/source/dom/base/nsDOMWindowUtils.cpp#3130
[7] http://mxr.mozilla.org/mozilla-central/source/browser/modules/PluginContent.jsm#41
[8] http://mxr.mozilla.org/mozilla-central/source/dom/html/HTMLInputElement.cpp#4177

Comment 7

[:Paolo Amadini]

There's also a relevant difference between the insecure password fields checks and the mixed content blocking checks: the former are done at the DOM level, while the latter are more related to network loads.

I actually looked into the code for mixed content blocking from comment 3 and the approach currently used there. I don't think it fits with the type of checks we're doing for passwords. In general there are some things that make the code more complicated to read than it could, for example I've noticed that nsIWebProgressListener::STATE_LOADED_MIXED_ACTIVE_CONTENT is a transient flag representing a change in state, while other flags managed by nsSecureBrowserUIImpl are permanent. I'm not sure I'd like to add more notifications with that unexpected pattern.

I'll look into what I can do to integrate the check with what we already have in place for Logins. Except for the detection of removals, which is a separate bug and probably simple to implement using UnbindFromTree, we already have most of what we need.

Comment 8

[:Paolo Amadini]

Attachment: MozReview Request: Bug 1193341 - Detect presence of password fields in any subframe, flagging those on insecure connections. r=MattN

Bug 1193341 - Detect presence of password fields in any subframe, flagging those on insecure connections. r=MattN

Comment 9

[:Paolo Amadini]

I've posted a preliminary patch with an idea of how the detection logic could work.

Tanvi, do you know of any URL I can use to test this logic in various frame scenarios?

Matt, any suggestion on how to notify the UI of the change? We could have each window register a listener on LoginManagerParent, or more simply LoginManagerParent could call a refresh method on the browser UI directly.

I think in general we want a method on LoginManagerParent that can be called at any time to get the value from stateForBrowser.

We have to write automated tests for this code as well.

Comment 10

[Tanvi Vyas[:tanvi]]

https://reviewboard.mozilla.org/r/18495/#review16845

::: toolkit/components/passwordmgr/LoginManagerContent.jsm:419
(Diff revision 1)
> +    // Returns true if this window or any subframes have insecure login forms.

Does thisWindow.frames check the children frames and the grandchild frames that live within he children?

::: toolkit/components/passwordmgr/LoginManagerParent.jsm:577
(Diff revision 1)
> +    // Report the insecure login form state immediately because it's a simple UI

Not sure what you are referring to with the login fill doorhanger flicker?  Looks like this is just a console message.

Comment 11

[Tanvi Vyas[:tanvi]]

(In reply to :Paolo Amadini from comment #9)
> I've posted a preliminary patch with an idea of how the detection logic
> could work.
> 
> Tanvi, do you know of any URL I can use to test this logic in various frame
> scenarios?
I don't have a lot of test cases for password fields.  You could look at the mochitests in https://bugzilla.mozilla.org/show_bug.cgi?id=762593.

Or the following two test cases that work both over HTTP and HTTPS:
http://people.mozilla.org/~tvyas/password/password_test2.html (HTTP top level page posts to HTTPS)
http://people.mozilla.org/~tvyas/password/frame_password.html (HTTP page with HTTPS frame posts to HTTP)

https://people.mozilla.org/~tvyas/password/password_test2.html (HTTPS top level page posts to HTTPS)
https://people.mozilla.org/~tvyas/password/frame_password.html (HTTPS page with HTTPS frame posts to HTTP)


(In reply to :Paolo Amadini from comment #7)
> In general there are some things that
> make the code more complicated to read than it could, for example I've
> noticed that nsIWebProgressListener::STATE_LOADED_MIXED_ACTIVE_CONTENT is a
> transient flag representing a change in state, while other flags managed by
> nsSecureBrowserUIImpl are permanent.  I'm not sure I'd like to add more
> notifications with that unexpected pattern.
> 

Can you elaborate a little here (or perhaps we can discuss over irc).  I'm not sure what you mean.  The STATE_IS_SECURE flag is controlled by nsSecureBrowserUIImpl.  nsMixedContentBlocker may change that to broken if it finds mixed content and also adds more granular mixed content flags.

In order to change the globe to a crossed out lock, you will have to update the security UI and add an nsIWebProgressListener flag.  I don't think there is a way of getting around that.

Comment 12

[Matthew N. [:MattN]]

Comment on attachment 8658168 [details]
MozReview Request: Bug 1193341 - Detect presence of password fields in any subframe, flagging those on insecure connections. r=MattN

Looks good. We talked about possibly dispatching a chrome DOM event on the browser when the insecure password state changes and then the front-end (e.g. desktop or Android) can call a method on LoginManagerParent to get the updated state.

Comment 13

[:Paolo Amadini]

Tests are still missing, but I'll post a version for code review.

Comment 14

[:Paolo Amadini]

Comment on attachment 8658168 [details]
MozReview Request: Bug 1193341 - Detect presence of password fields in any subframe, flagging those on insecure connections. r=MattN

Bug 1193341 - Detect presence of password fields in any subframe, flagging those on insecure connections. r=MattN

Comment 15

[Matthew N. [:MattN]]

Comment on attachment 8658168 [details]
MozReview Request: Bug 1193341 - Detect presence of password fields in any subframe, flagging those on insecure connections. r=MattN

https://reviewboard.mozilla.org/r/18495/#review17655

::: toolkit/components/passwordmgr/LoginManagerContent.jsm:437
(Diff revision 2)
>      messageManager.sendAsyncMessage("RemoteLogins:updateLoginFormPresence", {
>        loginFormOrigin,
>        loginFormPresent: !!topState.loginFormForFill,
> +      hasInsecureLoginForms: hasInsecureLoginForms(topWindow, false),
>      });

Are you mostly sending this info along for testing or do you think we will actually use this?

Comment 16

[Tanvi Vyas[:tanvi]]

https://reviewboard.mozilla.org/r/18495/#review17893

::: toolkit/components/passwordmgr/LoginManagerParent.jsm:590
(Diff revision 2)
> +    browser.dispatchEvent(new browser.ownerDocument.defaultView

Hi Paolo,

Where will this event be consumed?  Is that tracked in a separate bug?

Comment 17

[Tanvi Vyas[:tanvi]]

> Hi Paolo,
> 
> Where will this event be consumed?  Is that tracked in a separate bug?
Nevermind, I see that here https://bugzilla.mozilla.org/show_bug.cgi?id=1179961

Comment 18

[:Paolo Amadini]

(In reply to Tanvi Vyas [:tanvi] from comment #11)
> (In reply to :Paolo Amadini from comment #7)
> > In general there are some things that
> > make the code more complicated to read than it could, for example I've
> > noticed that nsIWebProgressListener::STATE_LOADED_MIXED_ACTIVE_CONTENT is a
> > transient flag representing a change in state, while other flags managed by
> > nsSecureBrowserUIImpl are permanent.  I'm not sure I'd like to add more
> > notifications with that unexpected pattern.
> > 
> 
> Can you elaborate a little here (or perhaps we can discuss over irc).  I'm
> not sure what you mean.  The STATE_IS_SECURE flag is controlled by
> nsSecureBrowserUIImpl.  nsMixedContentBlocker may change that to broken if
> it finds mixed content and also adds more granular mixed content flags.

What I meant about STATE_LOADED_MIXED_ACTIVE_CONTENT being possibly "transient" is exemplified by what was changed in attachment 8647749 [details] [diff] [review] and attachment 8641935 [details] [diff] [review] of bug 1182551, that is by looking at the code it seemed to me that the OnSecurityChange notifications in the mixed content blocker were sent without keeping into account the existing state previously notified by nsSecureBrowserUIImpl.cpp, and possibly the reverse about nsSecureBrowserUIImpl not knowing about the state notified by the mixed content blocker. If the overall notified state is correct and not transient, it's because (as I noticed later) notifications are carefully constructed by each module to check each other's state. Adding a new state flag there for insecure passwords would have been much more error-prone in this situation.

As Olli said in bug 1182551, "It might make sense at some point to reorganize the code a bit so that
one would collect the right flags to some variable and then just call eventSink->OnSecurityChange in one place". I realize this is quite a bit of work though.

Comment 19

[:Paolo Amadini]

(In reply to Tanvi Vyas [:tanvi] from comment #17)
> > Where will this event be consumed?  Is that tracked in a separate bug?
> Nevermind, I see that here
> https://bugzilla.mozilla.org/show_bug.cgi?id=1179961

Sorry, it's easy to forget to set direct dependencies between bugs when they block the same meta-bug. Technically they're independent from each other but the best landing order is this one first. I'll also move the function to read the state back here from bug 1179961, to be used in unit tests.

Comment 20

[:Paolo Amadini]

Comment on attachment 8658168 [details]
MozReview Request: Bug 1193341 - Detect presence of password fields in any subframe, flagging those on insecure connections. r=MattN

Bug 1193341 - Detect presence of password fields in any subframe, flagging those on insecure connections. r=MattN

Comment 21

[:Paolo Amadini]

Comment on attachment 8658168 [details]
MozReview Request: Bug 1193341 - Detect presence of password fields in any subframe, flagging those on insecure connections. r=MattN

Matt, I've added the automated browser-chrome tests for review. The only way to test this asynchronous state reliably seems to be to wait for the exact number of events we expect to be raised for the scenario. This is implementation-dependent but is deterministic.

Tryserver build:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6a2113826937

Comment 22

[:Paolo Amadini]

Comment on attachment 8658168 [details]
MozReview Request: Bug 1193341 - Detect presence of password fields in any subframe, flagging those on insecure connections. r=MattN

Bug 1193341 - Detect presence of password fields in any subframe, flagging those on insecure connections. r=MattN

Comment 23

[:Paolo Amadini]

There was a race condition when running tests with --e10s due to the fact that openNewForegroundTab waited for the tab switch even if the aWaitForLoad argument was false.

New tryserver build:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3a9661e2e69f

Comment 24

[Matthew N. [:MattN]]

Comment on attachment 8658168 [details]
MozReview Request: Bug 1193341 - Detect presence of password fields in any subframe, flagging those on insecure connections. r=MattN

https://reviewboard.mozilla.org/r/18495/#review19129

Sorry about the delay

::: toolkit/components/passwordmgr/LoginManagerParent.jsm:576
(Diff revision 4)
> +   * on the browser element.
> +   */
> +  hasInsecureLoginForms(browser) {
> +    return this.stateForBrowser(browser).hasInsecureLoginForms;

Nit: You could add !! to ensure this is a boolean and then document it as @return {bool}

Comment 25

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/5d1ec6ca9374

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5d1ec6ca9374

Comment 27

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ee20b027ba8d

Comment 28

[:Paolo Amadini]

Comment on attachment 8658168 [details]
MozReview Request: Bug 1193341 - Detect presence of password fields in any subframe, flagging those on insecure connections. r=MattN

Bug 1193341 - Detect presence of password fields in any subframe, flagging those on insecure connections. r=MattN

Comment 29

[:Paolo Amadini]

Comment on attachment 8658168 [details]
MozReview Request: Bug 1193341 - Detect presence of password fields in any subframe, flagging those on insecure connections. r=MattN

It appears that importing InsecurePasswordUtils.jsm from LoginManagerContent.jsm caused a significant startup time regression (bug 1213364). In order to solve this I moved the checkIfURIisSecure function to LoginManagerContent. Matt, Tanvi, can you check whether this makes sense?

Comment 30

[:Paolo Amadini]

Quick link to the interdiff: https://reviewboard.mozilla.org/r/18495/diff/4-5/

Comment 31

[:Paolo Amadini]

Curious, the removal of _checkIfURIisSecure from InsecurePasswordUtils.jsm doesn't show up, but it's there.

It appears if you see the entire new patch:

https://reviewboard.mozilla.org/r/18495/diff/5/

It doesn't if you see the individual changes:

https://reviewboard.mozilla.org/r/18495/diff/4/
https://reviewboard.mozilla.org/r/18495/diff/4-5/

Comment 32

[Matthew N. [:MattN]]

Comment on attachment 8658168 [details]
MozReview Request: Bug 1193341 - Detect presence of password fields in any subframe, flagging those on insecure connections. r=MattN

https://reviewboard.mozilla.org/r/18495/#review19685

Good enough for me

Comment 33

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/cfcbb20f317d

Comment 34

[:Gijs (he/him)]

Attachment: MozReview Request: Bug 1193341 - Detect presence of password fields in any subframe, flagging those on insecure connections. r=MattN

Bug 1193341 - Detect presence of password fields in any subframe, flagging those on insecure connections. r=MattN

Comment 35

[:Gijs (he/him)]

Attachment: MozReview Request: Bug 1211805 - rework keyboard detection for on-screen keyboard, r?jaws

Bug 1211805 - rework keyboard detection for on-screen keyboard, r?jaws

Comment 36

[:Gijs (he/him)]

Um, wow, mozreview, that is very strange. I actually don't know why this happened or how to dissociate the review request I created. :-\

smacleod, can you help?

(In case it's not obvious, I meant to push a review just for bug 1211805, and used hg push -r . review, and it somehow picked up the public parent of that revision and pushed that as part of the review, too.)

Comment 37

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/cfcbb20f317d

Comment 38

[Steven MacLeod [:smacleod]]

(In reply to :Gijs Kruitbosch from comment #36)
> Um, wow, mozreview, that is very strange. I actually don't know why this
> happened or how to dissociate the review request I created. :-\
> 
> smacleod, can you help?
> 
> (In case it's not obvious, I meant to push a review just for bug 1211805,
> and used hg push -r . review, and it somehow picked up the public parent of
> that revision and pushed that as part of the review, too.)

TBH, I've come back to this a couple of times and given up investigating each time. I'm not really sure what happened, but I would assume the parent changeset would have been draft() at the time not public. I haven't come across a public changeset getting pulled along like this case since so I don't think there is anything more we can do to investigate.