361463 - Address security concerns with keychain password auto-fill in forms

Status: RESOLVED FIXED

(Camino Graveyard :: Security, defect)

Description

[Stuart Morgan]

Camino is going to have to resolve bug 360493 and related issues separately from any Firefox/core decisions.

One option that's been discussed there is to not fill any form where the form action host doesn't match the page host. That's probably a good step, although we need to figure if there are a significant number of sites doing that legitimately (e.g., bar.foo.com's form submits to login.foo.com). However, there's a class of attack that that approach doesn't address.

Another option, which I've tossed out in 360493 is as follows:

Remember passwords for a domain, but also remember white-/black-lists of URLs for each one, and prompt the user for any new URL. E.g., if a user logs in to foo.com/login.html and stores the password, the password would auto-fill on foo.com/login.html. If the user goes to foo.com/login-try-again.html, they would get an alert, and choose whether or not they want to fill there (with an option to remember).

The warning is a weak point since it involves a user decision about security, but it could at least be made very understandable (and with a "no" default); something to the effect of:

-------------
This page has a log-in form. If you are trying to log in to foo.com, you can fill it with your saved account information. If you are not trying to log to foo.com, this may be an attempt to steal your password.

[Fill Form] [Don't Fill Form]
| | remember this decision
-------------

The biggest issue here would be user-management of the URL white-/black-lists, but I'm not sure it's the sort of thing that would be wrong enough that we'd actually need to worry too much about it (since the user would have to check the remember box explicitly).

Comment 1

[Stuart Morgan]

Apparently bug 359675 is more relevant to some of that discussion.

In the short term, I guess we should probably keep a list of target realms per keychain item, adding to it whenever the user manually fills a form to that realm, and only auto-fill in forms that have that have one of those targets (that should give us safety without breaking autofill on sites that legitimately submit to a different realm). That fixes the more dangerous subset that the exploit discussed that bug 360493 is specifically about.

We'll have to allow existing passwords to fill blindly the first time, both for backward compatibility and for dealing with keychains that we didn't create one the sharing code is in.

Comment 2

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #0)

> One option that's been discussed there is to not fill any form where the form
> action host doesn't match the page host. That's probably a good step, although
> we need to figure if there are a significant number of sites doing that
> legitimately (e.g., bar.foo.com's form submits to login.foo.com). 

Yahoo's properties spring to mind here.

Comment 3

[Stuart Morgan]

(In reply to comment #2)
> Yahoo's properties spring to mind here.

Yahoo appears to redirect to a form on login.yahoo.com that posts to login.yahoo.com, then redirects the user back, so I don't think it would be in that category.  Either way, it won't matter if we remember the target when the user fills it manually rather than assuming it has to match the current realm.

Comment 4

[Paul Borokhov (lensovet)]

i'll echo a comment that i posted over on the firefox bug. namely, ask the
user if they want the password filled in.Also, give a pref option (possibly a hidden one, so that "Average Joe" doesn't know how to set it) that will turn on autofill as it works now. for those that want to risk it and actually know what the hell they are doing, let them do their thing. also, as was mentioned earlier, provide some sort of visual indication that the autofill has in fact occurred.

Comment 5

[Stuart Morgan]

There's no need to echo any of the bug 360493 discussion here; no decision will be made about what to do in Camino until we know what the Firefox policy decision is going to be.

Comment 6

[Stuart Morgan]

Looks like storing the action domain is the Firefox plan, so I'll start working on a patch to do the same.  I'm thinking we can use kSecSecurityDomainItemAttr, since it doesn't have any other use I can see for web forms.

Doing this for 1.0.4 will be interesting; we'll either have to either have to land KS on the 1.0.4 branch, or I'll have to write this patch twice two completely different ways.

Comment 7

[Paul Borokhov (lensovet)]

(In reply to comment #6)
> Looks like storing the action domain is the Firefox plan, so I'll start working
> on a patch to do the same.  I'm thinking we can use kSecSecurityDomainItemAttr,
> since it doesn't have any other use I can see for web forms.

just out of curiosity, what happens when the site changes (legitimately) the action domain? i know this is rare, but still.
i also don't see why we necessarily _have_ to go the same way fx does; i'm def glad that we allow autocomplete=off to be overridden while they don't.

Comment 8

[Chris Lawson (gone)]

Is this going to be entirely under the hood, or is there going to be nib/string work involved too?

cl

Comment 9

[Stuart Morgan]

That depends on whether we alert the user and offer a choice in the case of a form with a different action, or whether we silently refuse to fill the form. I'm leaning toward the former, but there are some potential user experience issues that make me hesitate.

Comment 10

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

For details on the two options and some limitations, see the discussion in our meeting log http://wiki.caminobrowser.org/Status_Meetings:2007-01-17:Log (beginning at 12:36pm).

Comment 11

[Stuart Morgan]

Attachment: branch fix

Branch patch.
- Adds a new attribute to KeychainItem. I'm abusing the security domain field of the keychain a little, but as I said I don't see any other need for it in the case of HTML forms. The only downside I see is that once we do domains for HTTP auth, they will be dealt with in terms of single-element arrays rather than strings, but that's not a big deal.
- Does some related cleanup: fixes member variable naming, removes an unnecessary level of calling for the confirmation dialogs, and moves the convenience function for getting NSWindows out of the class, since it wasn't at all class-specific.

Comment 12

[Stuart Morgan]

Attachment: new nib

Corresponding nib. The language may need work.

Comment 13

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

At the very least, the interrogative needs to use "your Keychain" to be in sync with the others.  I'm still pondering the rest of the text.

Comment 14

[Josh Aas]

Comment on attachment 254537 [details] [diff] [review]
branch fix

From the nib file:

"If you are trying to log in on this page, you can fill the form with your saved password."

This sentence does not need a comma.

Comment 15

[Mark Mentovai]

Comment on attachment 254537 [details] [diff] [review]
branch fix

"If you are trying to log in on this page, you can fill the form with your
saved password."

I think the comma should stay.  I also think that "fill form" doesn't read cleanly.  I don't think of forms as things that are filled as much as things that are filled IN.

Do our first-stringers have any thoughts?  The text from the attached nib is:

--
*Fill log-in information from Keychain?*

This page has a log-in form that is different from the one you used to store your password for this site. If you are trying to log in on this page, you can fill the form with your saved password. If not, or you are not sure, you should not fill the form.

(Fill Form) ((Don't Fill Form))
--

Comment 16

[Paul Borokhov (lensovet)]

grammatically, as well as "flow-wise", the comma should in fact stay.
as for "fill form", i think it will read a bit weirdly if we change it to "Fill in the log-in form" – too many ins. consider that (i think) it is necessary for wording everywhere to be consistent ("fill log-in", "fill form", "don't fill form", and don't forget in the menu, Edit->Fill Form (cmd-opt-f) – all of those should use the same wording, whichever is finally decided here). Also, is the dash necessary? I've always thought of it as "login" form.

Comment 17

[Stuart Morgan]

Here's what we came up with in channel:

*Use saved password from your Keychain?*
This page has a log-in form that changed since you saved your password.  If you are trying to log in on this page, you can use the password saved in your Keychain.  If not, or if you are not sure, you should not use the saved password, as this could be an attempt to steal your password.
(Use Saved Password) ((Don't Use Saved Password))


(Also, note to self: change 'stored' to 'saved' in the update dialog.)

Comment 18

[Paul Borokhov (lensovet)]

(In reply to comment #17)
> Here's what we came up with in channel:
> ...
> This page has a log-in form that changed since you saved your password.
that HAS changed

Comment 19

[Paul Borokhov (lensovet)]

also
> If not, or if you are not sure, you
just change it to "Otherwise, you"...

Comment 20

[Stuart Morgan]

Checked in on trunk and MOZILLA_1_8_BRANCH (with Smokey's color on the bike
shed).

Comment 21

[Stuart Morgan]

(In reply to comment #19)
> > If not, or if you are not sure, you
> just change it to "Otherwise, you"...

Those are not equivalent in terms of user response.

This has already been checked in with the strings we decided on in channel.

Comment 22

[Mark Mentovai]

> that HAS changed

I go back and forth on this one, but the committee HAS spoken.

Comment 23

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

We'll want this for 1.0.5.

Comment 24

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Clearing nomination; 1.8.0 fix will be in bug 379438.