1127567 - In LoginManagerParent.doAutocompleteSearch, return logins for both the HTTP and HTTPS versions of the provided formOrigin

Status: RESOLVED DUPLICATE of bug 667233

(Toolkit :: Password Manager, defect, P1)

Description

[Chris Karlof [:ckarlof]]

Why? In this path, the user is intentionally searching for her login credentials for the site (i.e., autofill) and we should provide her with all the reasonably relevant logins.

Proposed approach:

In LoginManagerParent.doAutocompleteSearch (http://mxr.mozilla.org/mozilla-central/source/toolkit/components/passwordmgr/LoginManagerParent.jsm#157), change the behavior to collect the union of stored logins for both the HTTPS and the HTTP version (default ports). Then dedup the list based on username. If we have an entry for both HTTP and HTTPS with the same username with different passwords, we could:
  - choose the one most recently used (this gets wacky with Sync, since it doesn't sync the lastUsed timestamps), or
  - choose the HTTPS one

I expect the solution here to have similarities to the one for bug 667233.

Comment 1

[Chris Karlof [:ckarlof]]

> Why? In this path, the user is intentionally searching for her login credentials for the site (i.e., autofill) and we should provide her with all the reasonably relevant logins.

Typo in the first sentence: "login credentials for the site (i.e., *not* autofill)"

Comment 2

[Chris Karlof [:ckarlof]]

> If we have an entry for both HTTP and HTTPS with the same username with different passwords, we could:

:mattN suggests lastChanged might be better since lastUsed is updated on failed password attempts too.

Comment 3

[Chris Karlof [:ckarlof]]

IRL discussion, we could alternatively do the exact same approach as in https://bugzilla.mozilla.org/show_bug.cgi?id=667233#c26 (i.e., don't offer HTTPS saved passwords on HTTP pages during autocomplete).

Comment 4

[Tanvi Vyas[:tanvi]]

Is this bug about the password manager UI that contains all saved passwords and where the user can search through the list of sites?  Or is this during the multiple username flow where we don't autofill because we aren't sure which username to use?

Comment 5

[Chris Karlof [:ckarlof]]

Neither.

This is for when

* the user loads an HTTP page with a login form and has an HTTPS login for the same domain
* the user invokes the autocomplete UI by typing the username field

At this point, the user has started the login process, and we should help her complete it if we can.

Comment 6

[Allison Naaktgeboren :ally]

So my understanding of this bug is that we want to:

1)LoginManagerParent.doAutocompleteSearch to find both https & http 
2) remove duplicate entries keyed on username
3)if there exists both http & https entries AND a difference between the http & https passwords, autocomplete with the https password.

Comment 7

[Chris Karlof [:ckarlof]]

> 3)if there exists both http & https entries AND a difference between the http & https passwords, autocomplete with the https password.

If we have a http and https entries for the same username, we should use the one that was most recently updated.

Confirm, Tanvi?

Comment 8

[Allison Naaktgeboren :ally]

I am concerned about Man In The Middle attacks raised by Jesse Ruderman with respect to using https credentials on http logins, which this bug (see plan in comment 6) would do.

"I agree strongly, but only in one direction.
Passwords saved for http should also work on https. Not doing this severely punishes sites that switch to https. Ideally we'd change the password to https-only at some point (when the form is submitted? when the site enables HSTS, a la bug 1119555?).
Passwords saved for https should NOT also work on http. That would enable really dangerous MITM attacks."

https://bugzilla.mozilla.org/show_bug.cgi?id=986609#c1

I'm inclined to morph this bug in this direction. Chris, what do you think?

Comment 9

[Chenxia Liu [:liuche]]

I think comment #5 clarifies your question - if the user has already started typing a username into the login form, they're going to want to sign in and we shouldn't block them. They'll just reset their password, or be angry that we didn't save it correctly - not supplying it as an autocomplete option isn't going to stop them from logging them, and we shouldn't actively obstruct them.

We should not *autofill* https entries (on page load), however, because the user hasn't taken an action.

Comment 10

[Chris Karlof [:ckarlof]]

What Chenxia said.

Comment 11

[Allison Naaktgeboren :ally]

irc discusion summary: yes it opens MITM attack surface, but helps users login more, and the team has voted to make that tradeoff.

liuche
ally: we should try to make http/https more clear to users, but that's not the scope of that bug, i think

ckarlof
ally: liuche’s response in the bug is my opinion as well

ally
liuche, ckarlof comment 5 doesnt address the concern Jesse raises. If its an http site, and we have login credentials for http & https, even if its the most recently used, we should not supply the https password

liuche
"At this point, the user has started the login process, and we should help her complete it if we can."

ally
liuche:  and ive got her http password
liuche:  so give her that

liuche
but if she's updated her https password more recently, that's probably the right one
supplying the wrong password is probably worse than supplying no password at all

ally
liuche:  it probably is, but is it worth safety risk?

MattN
yes, the user would just type it themselves anyways

liuche
i mean, do you think the user is going to say, oops, wrong password, guess i'm not going to use facebook today

MattN
(if Firefox doesn't help)

liuche
they'll just reset their password, and be more pissed off at password managers (and less inclined to use them or have a positive opinion of them)

ally
I'm not very comfortable with this approach, but I appear to be outvoted

liuche 
ally: basically, we should emphasize HTTP vs HTTPS better, but that's not the scope of the password manager

Comment 12

[Tanvi Vyas[:tanvi]]

It is a bit difficult to distinguish between this bug and Bug 1127579.  This bug seems to be about fill while Bug 1127579 is about capture, but the decisions we make on each effect each other.

I've been trying to formulate a policy around how to handle HTTP/HTTPS logins - see https://bugzilla.mozilla.org/show_bug.cgi?id=1127579#c9.  From a security standpoint, the third option listed is most secure.  And easiest to implement, but might cause the user to have to go through the autocomplete flow more often.  We have to gather data to see how frequently that might be.  (Or if we are okay with user's having to autocomplete on HTTP pages where an HTTPS record exists, then lets just go with that).

Anyway, to act on this bug now we could the following on an HTTP page:

1) LoginManagerParent.doAutoCompleteSearch finds an HTTP record and autofills.  (If we have an HTTP record, we don't even look for the HTTPS one)
2) If no HTTP record exists, but an HTTPS record does, offer user autocomplete experience to fill in the HTTPS record.

OR if we want to use the most recently updated when both records exist:

1)LoginManagerParent.doAutocompleteSearch to find both https & https records 
2)Select the most recently updated from the http and the https entry.  If most recently updated is HTTP, autofill HTTP record.  If the most recently updated is HTTPS, autocomplete experience to fill the HTTPS record.

Similar logic follows for HTTPS pages, except there you can always autofill.

For resolving duplicate entries and changing the way we capture data, that would happen in bug 1127579.

Comment 13

[Chenxia Liu [:liuche]]

That sounds good Tanvi, thanks for laying everything out so clearly.

Regarding password capture though, I'm concerned that this should depend on bug 1127579 (or include some manner of overriding how we save credentials).

Consider the following case:

1) User visits the HTTP version of a login site (which also has an HTTPS page).
2) User starts typing in the username for a login that has both an HTTP and HTTPS password. The HTTPS password is the most recently updated, so it's provided as the (deduplicated) autocomplete option.
3) User logs in with HTTPS password.
4) Firefox detects that the login for this HTTP page has a new username/password combination and offers to Save/Update the login.

If we don't change any of this handling, we will copy the HTTPS password to HTTP login, which could lead to autofilling a password that the user has never explicitly typed on the HTTP page.

This seems like something that we should explicitly address (either in this bug, or by making this bug depend on bug 1127579), because it crosses the lines set by our model of:
Autofill = same origin
Autocomplete = deduplicate based on recency/origin
because we will save an HTTPS password into HTTP which can then be autofilled on HTTP.

Comment 14

[Tanvi Vyas[:tanvi]]

Clearing needinfo.  The policy is defined here https://etherpad.mozilla.org/same-domain-http-https-passwords4

Comment 15

[Tanvi Vyas[:tanvi]]

(Sorry this is so long!  Wanted to capture all of this somewhere.)
LoginManagerParent.doAutocompleteSearch returns a list of usernames to provide in a drop down menu when a user is going through the autocomplete experience.  It is responsible for filling in the username the user selects, but not the password.

When a password field is first observed, onDOMFormHasPassword is called.  If the Password Manager finds exactly one login it is autofilled.  If it finds 0 it returns and doesn’t go on.  If it finds one or more logins, it marks the username field as a username field.

When the user interacts with a username field that has been marked, we invoke doAutocompleteSearch (see https://pastebin.mozilla.org/8833687 for full details).  It returns login data that is used to provide the user with a drop down of username options.

Once the user has selected a username, we call LoginManagerContent:onUsernameInput which will populate the password if we have one for the matching username.  This goes through the same code path as onDOMFormHasPassword.  Namely, onDOMFormHasPassword and onUsernameInput both end up calling sendLoginDataToChild().

When we go through onUserNameInput->sendLoginDataToChild() we’d like sendLoginDataToChild() to return login data for a different scheme if the scheme provided in formOrigin doesn’t find any logins (HTTP or HTTPS).  But for onDOMFormHasPassword->sendLoginDataToChild() we do not want to return HTTPS passwords for an HTTP formOrigin.  Hence, we need to send more information to sendLoginDataToChild() to communicate this.  We can pass a flag indicating whether the caller is onDOMFormHasPassword (the user hasn’t interacted with the form) or the caller is onUsernameInput (the user has interacted with the form).  We can pass it via the options parameter in _getLoginDataFromParent() http://mxr.mozilla.org/mozilla-central/source/toolkit/components/passwordmgr/LoginManagerContent.jsm#219

While the onDOMFormHasPassword code is always called when we see a password field, doAutocompleteSearch isn’t always called for a username field.  This is because the username field might not be marked.  If onDOMFormHasPassword doesn’t return HTTPS logins for HTTP pages and the HTTP page doesn't have any saved HTTP logins, then we will not get a chance to provide the user with the autocomplete experience.  If we always mark the username field (regardless of how many logins were found), then we overwrite users form history with a potentially empty username list.  We either have to fix this by returning both the users form history and the username list (deduped) or find a way around this.

To get around this, another option is to let sendLoginDataToChild() always return HTTP and HTTPS logins.  But also pass up a flag for autofillForm flag and set it to false when appropriate (ex: using an HTTPS login on an HTTP page).  This would simplify things and avoid a situation where the username field doesn’t get marked.  Also means that we don't have to send a flag to sendLoginDataToChild and return different data based on who's calling it.  But I’m not sure how to pass autoFillForm information back up to onDOMFormHasPassword.  Can I set a global similar to gAutoFillForms that is per form?  That doesn’t really work because the global would be created in the parent but needs to be read by the child.

Comment 16

[:Paolo Amadini]

(In reply to Tanvi Vyas [:tanvi] from comment #15)
> (Sorry this is so long!  Wanted to capture all of this somewhere.)

Great description of the flow! It helped me a lot as well.

We should look into moving this (including the pastebin) to a wiki.m.o page. The current version of Matt's high-level diagram could be placed there as well. Even the page will become obsolete soon, it's still a starting point and we don't have to commit to keeping it updated.

Comment 17

[Tanvi Vyas[:tanvi]]

I talked this over with Matt.  There are 3 options (and I'm going to go with option 3).

1) Mark all username fields detected by onDOMFormHasPassword (even when 0 logins are found).  When providing users with the list of autocomplete options, append the formHistory to the username list and dedup (Bug 1166112).
Then, change the way we fill password fields after the username is autocompleted.  Instead of making a second call to LoginManager to get data to populate the password field, use the data returned when nsLoginManager.js:autoCompleteSearchAsync was called on the username (Bug 1166113).

If we do the above, doAutocompleteSearch can return both HTTP and HTTPS logins.  While _getLoginDataFromParent won't return an HTTPS password to an HTTP page for autofill use.  We will get the experience we would like in this bug, without touching the code that affects bug 667233.

2) Set a flag in sendLoginDataToChild that indicates whether the returned login can or cannot be autofilled.  If it is an HTTP login on HTTPS page, the value is true.  If it is an HTTPS login on an HTTP page, the value is false.  Honor that value in onDOMFormHasPassword and return an empty array to loginsFound if it is false and there is only 1 login returned.

3) Let _getLoginDataFromParent return both HTTP and HTTPS logins[1].  In onDOMFormHasPassword, do a check to see if there is only one login and if the login is an HTTPS login for an HTTP page.  If so, then mark the username field and return an empty array to loginsFound.  This form will now go through the autocomplete experience.
This means that all callers of _getLoginDataFromParent have to understand that they will get back data for both HTTP and HTTPS schemes, and they will have to be careful to handle them accordingly.  Right now there are only two callers, so not a big deal.  But something to note for the future.

[1] As per https://etherpad.mozilla.org/same-domain-http-https-passwords4, if an HTTP login exists for an HTTP page, we only return the HTTP login.  If an HTTPS login exists for an HTTPS page, we only return the the HTTPS login.  We only check the other scheme when the formOrigins scheme returns no results.  We can enhance this in the future to return both sets of logins deduped - Bug 1166111.

Comment 18

[Tanvi Vyas[:tanvi]]

> 3) Let _getLoginDataFromParent return both HTTP and HTTPS logins[1].  In
> onDOMFormHasPassword, do a check to see if there is only one login and if
> the login is an HTTPS login for an HTTP page.  If so, then mark the username
> field and return an empty array to loginsFound.  This form will now go
> through the autocomplete experience.

This is implemented as part of bug 667233.  Because the implementations were dependent on each other, it wasn't feasible to separate them into two separate patches.  Marking this bug as a duplicate of bug 667233.