359675 - provide an option to manually fill forms and log in

Status: VERIFIED FIXED

(Toolkit :: Password Manager, enhancement, P3)

Description

[Martijn Wargers (dead)]

This came from Hendrix feedback (see url):
http://groups.google.com/group/mozilla.feedback/browse_thread/thread/3440f6868c5a8f40/d3925c6a13107523#d3925c6a13107523

"
I think though that the autofill when only one username/password combo
exists in the password-"database", should be optional (checkbox) or
turned off. The reason for this, is security and I'll explain how:
If a site is XSS-exploitable, the user's session might be stolen. But if
the user has autofill, this may be exploited to actually steal the users
username/password combo. I've created a test page, showing this on:
http://www.oftedal.no/~erlend/test/
I wanted to send this to you guys instead of sending it to the masses. 
"

This is also a problem in Seamonkey, so I guess also a Seamonkey specific bug should be filed about this.

Comment 1

[Jesse Ruderman]

On the other hand, having an easy-to-use password manager has security advantages.  For one thing, it makes phishing less likely to succeed, because users don't have to type their passwords all the time.

IMO, XSS is a site's problem, not the browser's problem.  I don't think we should sacrifice usability this much just to slightly mitigate the effect of a successful XSS attack.

Comment 2

[Erlend Oftedal]

(In reply to comment #1)
> On the other hand, having an easy-to-use password manager has security
> advantages.  For one thing, it makes phishing less likely to succeed, because
> users don't have to type their passwords all the time.
> 
> IMO, XSS is a site's problem, not the browser's problem.  I don't think we
> should sacrifice usability this much just to slightly mitigate the effect of a
> successful XSS attack.

I agree that having a password manager is a good thing for security. I don't have a problem with the password manager as is. IE and Opera has similar features. However the password should not be autofilled when there is only one username/password combination in the database for the site. The user should always be required to actually click the username-field and choose his username from a dropdown list. This is what happens if you have more than one username/password combination registered for the site. The user thus won't have to write the password, only select the username/password combo.

Comment 3

[Daniel Veditz [:dveditz]]

The concern is not unreasonable. On the other hand if the site is vulnerable to XSS an attack could probably steal your data or change your password if you're already logged in, with or without this problem.

Comment 4

[Erlend Oftedal]

Yes, they probably could steal some data from the profile, or change the user's password. Most sites hash their passwords, and the password is thus not available in the profile, but even good sites can have XSS-mistakes. Stealing the actual password can allow the attacker to login to other sites as users tend to use the same password everywhere. 

Comment 5

[Robert Chapin]

(In reply to comment #0)
> I've created a test page

see XSS Abusing Password Management at http://www.infosecwriters.com/hhworld/hh8/csstut.htm

This is inherent to the feature.

Robert Chapin
Chapin Information Services, Inc.

Comment 6

[Martijn Wargers (dead)]

I guess this is basically the same as bug 360493.

Comment 7

[Jesse Ruderman]

No, it's not.  This is about XSS.  Bug 360493 is not about XSS.

Comment 8

[Erlend Oftedal]

(In reply to comment #7)
> No, it's not.  This is about XSS.  Bug 360493 is not about XSS.

I agree with Martijn here. The cause for the problem is somewhat the same. Firefox fills in the password because it thinks the login form is a valid one. If the bug in 360493 was not there, the XSSed login form would not work.

Comment 9

[Robert Chapin]

(In reply to comment #8)
> If the bug in 360493 was not there, the XSSed login form would not work.
 
XSS can steal anything in any form within scope, browser bugs not required.

Comment 10

[Justin Dolske [:Dolske]]

Part of the work to 360493 was to add a preference (signon.autofillForms) to allow disabling the automatic filling of forms. Given that we can only do so much about XSS (which is what this bug is about) without completely crippling features, I think this is now fixed.

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Daniel, do we have an UI where the user can deselect the automatic filling of forms? I just ask because you added this keyword and I cannot find a checkbox in Firefox 2.0.0.4. So dunno if this is really fixed.

Comment 12

[Daniel Veditz [:dveditz]]

We have a back-end pref but no practical way to enable it for most normal users. I think this is still a problem in Firefox, but I'll change it to an enhancement request.

Comment 13

[Justin Dolske [:Dolske]]

I will take another look at this and see if there's something we can do for Firefox 3 to improve things.

While completely disabling password autofill (as the default) probably isn't a change that would be made right now, at a minimum we should look at improving things for users/extensions interested in not having autofill enabled. [Eg expose the pref in the UI, an optional toolbar wand button, etc]

Comment 14

[Daniel Veditz [:dveditz]]

I have to say the signon.prefillForms pref (which is what I think Justin meant in comment 10) doesn't work all that well. There's no point in exposing UI for that if even a paranoid like me is unhappy with it -- no normal user will want to run in that mode.

One better idea we batted around in the PwMgr security review is to open an infobar on a page that contains a password we know we can fill in, with a button to go ahead and do so.
 - this prevents XSS silently stealing passwords. If the infobar pops
   up and you weren't expecting it and don't see a password form anywhere
   you _know_ something is wrong.
 - If you follow a link to your "bank" and the infobar does NOT pop up
   that's a good clue you may not be at your bank (although they could
   have simply changed the page enough that we can't replay)

This approach would fit well with the change to use infobars for saving the passwords in the first place (bug 226735).

Comment 15

[Justin Dolske [:Dolske]]

I wonder if the fix to bug 376668 will help in this respect -- using the existing autocomplete UI as the way to manually fill in a form? It looks like we already always attach the autocomplete dropdown (even when autofill is disabled, and even when there's only 1 matching login). We'd probably want to double check to ensure untrusted content cannot trigger and select an autocomplete entry, though.

That may be a non-starter, though, if the goal is not just to help avoid XSS but also to strictly isolate doing *any* form processing in Login Manager until the user take some specific manual action.

Comment 16

[Justin Dolske [:Dolske]]

Untargeting... I think at this point the best that's going to make FF3 is what I mentioned in comment #15. The signon.prefillForms pref isn't completely useless, because the user can continue to use the autocomplete dropdown to select the login.

An infobar or something might be better, and the uiwanted flag is set here, but I suspect there won't be cycles to design something until post-FF3. :(

Comment 18

[Justin Dolske [:Dolske]]

Random half-thought...

It might be interesting to only have pwmgr autofill a password when the document is *not* in a frame (or if the document's eTLD == parents eTLDs). That would help mitigate the case of evilsite.com embedding an iframe with an XSS'd gmail.com. A malicious site could still open a window with the XSS'd site, of course. Maybe check window.opener, and don't autofill if eTLDs are different?

Comment 19

[georgi - hopefully not receiving bugspam]

(In reply to comment #18)
> Random half-thought...
> 
> It might be interesting to only have pwmgr autofill a password when the
> document is *not* in a frame (or if the document's eTLD == parents eTLDs). That

not sure this is correct.
visiting http://target/XSS-ed
or location.replace("http://target/XSS-ed")

doesn't involve any frames, does it?

Comment 20

[Justin Dolske [:Dolske]]

True. Like I said, half-thought. :-)

Comment 21

[:Gavin Sharp [email: [e-mail]]]

It wouldn't prevent that case (location.replace), but it would solve dveditz's scenario of loading multiple XSS-vulnerable sites in a (potentially hidden) frame and collecting passwords. File a bug on it?

Comment 22

[georgi - hopefully not receiving bugspam]

(In reply to comment #21)
> It wouldn't prevent that case (location.replace), but it would solve dveditz's
> scenario of loading multiple XSS-vulnerable sites in a (potentially hidden)
> frame and collecting passwords. File a bug on it?
> 

imo this is an ugly kludge.
what's the problem to chain a lot of XSSed sites:
[1] site_A: steal pass, do location.replace() or form.post() to site_B
[2] A = B, B = C goto [1]

same effect afaict, just a little slower

Comment 24

[Daniel Veditz [:dveditz]]

It's after FF3 (per comment 16), time to try again.

My design principal for what I want: if the user hasn't made an explicit action indicating they want the password replayed, DON'T. Requiring at least clicking or typing in the user/pass field--as long as we reject synthetic events--would be a start, though potentially spoofable (there's a pref that sort of gives me this, but it's got enough problems that we shouldn't just turn it on).

My preference at this point would be opening an infobar (matching the new remember password style) that tells the user we know a password for that site, with a button that lets them replay the password and submit the form in one click.

Edge case 1: Multiple remembered usernames. For these use the bar just as an announcement for consistency, but otherwise use the multi-user behavior that requires the user to click in the form and start typing to bring up an autocomplete.

Edge case 2: Multiple password forms. The button in the infobar could autofill all of them but not submit, or we could use autocomplete like the multiple-username case when the user clicks interacts with their chosen form. I lean toward the former because it lends itself to working with password-only forms which the current autocompleting code doesn't handle.

Comment 25

[Johnathan Nightingale [:johnath]]

(In reply to comment #24)
> It's after FF3 (per comment 16), time to try again.
> 
> My design principal for what I want: if the user hasn't made an explicit action
> indicating they want the password replayed, DON'T. Requiring at least clicking
> or typing in the user/pass field--as long as we reject synthetic events--would
> be a start, though potentially spoofable (there's a pref that sort of gives me
> this, but it's got enough problems that we shouldn't just turn it on).

So, can we agree that in a world without XSS, this would be a clear downgrade in usability?  I mean, I am obviously interested in mitigating XSS where possible, but I want us to be clear that we're talking about trading one off against the other here, because having to do extra work is not desirable in and of itself.  Trading those things off is a thing we can do, but that is the question at hand, right?

I'm still thinking it through, but I'll admit that my gut reaction is to agree with Jesse in comment 1 when he says that the mitigation this offers is slight, and that we have to be careful about making pwmgr less friendly, since that creates its own risks.

> My preference at this point would be opening an infobar (matching the new
> remember password style) that tells the user we know a password for that site,
> with a button that lets them replay the password and submit the form in one
> click.

Yeah, and we could also do the Opera thing, where there's some kind of widget, a magic wand or a keyboard symbol or a key, attached to the text fields which populates them when clicked.  I dunno, I'm not opposed to making this better, but these solutions feel like they take an easy thing and make it harder, and we don't really want to turn people off of the feature.  Maybe you could do something like seeding the text fields with gray text that said "Click to auto-fill" or "password saved" but the current situation is that it "just works" so I guess anything is going to feel like an intrusion, at this point. 

I'll be honest, I actually really like the current behaviour from an end-user perspective.  Not only is it easy to use, but it also creates an important ambient cue -- "this is my brokerage, clearly, because it remembers my information."  I find it quite disconcerting when Firefox doesn't remember those credentials if I'm on my work machine or something; makes me triple-check that I'm where I think I am.

I'm gonna assign this back to nobody@ for now, since I'm certainly not "actively working on it." I am interested in finding a way to make everyone happy here, and I certainly don't have a problem with you throwing bugs my way in general, but realistically a solution here might not implemented by me anyhow - dolske knows this code far better than I, for instance.

Comment 26

[Justin Dolske [:Dolske]]

I don't think we'd want to disable automatic filling by default, but it seems like a legitimate use case for those who want this as an option. At the very least by adding some flags to the existing code, so an extension could drive the UI side. [Eg, currently the password manager form-fill code can only be triggered by page loads, but it should be easy to expose this as an interface for extensions.]

Supporting this as an optional mode helps with a few other things too:

* Pages that dynamically add login forms with JS (like hulu.com) are missed by pwmgr. Detecting this automatically would be best, but is likely difficult and carries perf risk. A "fill in passwords for this page" widget somewhere would be a workaround.

* We honor autocomplete=off now, but there are a few edge cases where it's awkward/difficult for the user to manually get their login filled in anyway. This mechanism could possible ignore autocomplete=off, and provide a consistent way for filling logins in such cases.

Comment 27

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

I'm working on this, as per Justin's suggestion.  It doesn't exactly solve the XSS issue, but it will make it possible for the user to turn off the autofillin and still have an extension do that.

Essentially, just adding a public nsLoginManager#fillDocument and adding a parameter to nsLoginManager#_fillDocument.  No functionality is compromised.

Comment 28

[georgi - hopefully not receiving bugspam]

i think requiring user action for filling a password is necessary even if it affects usability.

and i don't think XSS gonna die anytime soon.

Comment 29

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Attachment: Patch v1

Created patch to expose this function as I mentioned above.

Comment 30

[Justin Dolske [:Dolske]]

Comment on attachment 324865 [details] [diff] [review]
Patch v1

>+    /**
>+     * Fill in forms in the document if we can have them.
>+     *
>+     * @param aDocument
>+     *        The document to parse looking for login forms
>+     */
>+    void fillDocument(in nsIDOMDocument aDocument);

So, this patch basically looks fine, but now I think we should actually expose form filling per-form, instead of per-document. EG, we should implement a "void fillForm(in nsIDOMHTMLFormElement aForm)".

Callers can still fill the whole document [just iterate over each form and call fillForm()]. But now a caller can specify just a specific form to fill, perhaps if the action was triggered from a specific login field...

>+     * fillDocument
>+     *
>+     * Can be called by public, exposes private _fillDocument.
>+     * For each form in the document,
>+     * we check to see if it can be filled with a stored login.

Nit: Doesn't need to be described as public, text is line-wrapping at an odd point.

>+        // We check the pref if told to do so (typical use case).
>+        // Otherwise we assume that we should auto fill form.
>+        // This is the case of the public fillDocument is called.

Nit: a briefer "use the pref if told to, otherwise default to true"


>+Login Manager test: simple form fill 2
...
>+/** Test for Login Manager: simple form fill 2 **/

Nit: "simple form fill with autofillForms disabled"

>+var pwmgr = Components.classes["@mozilla.org/login-manager;1"].
>+                        getService(Components.interfaces.nsILoginManager);
>+var prefs = Components.classes["@mozilla.org/preferences-service;1"].
>+                        getService(Components.interfaces.nsIPrefService);

Nit: style is to line up getService with the "C" in "Components" in the line above.

Comment 31

[Mike Connor [:mconnor]]

In addition to dolske's comments, I think we want to figure out how to signal that there are forms that can be filled with passwords (i.e. where we check autofillForm we already know there's logins, so we can do something (notification, set a member var... somewhere, etc) so that extensions that are doing something like "show an infobar" or "have a login button in the statusbar" can enable/trigger whatever UI is appropriate.

Comment 32

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Attachment: Patch v2

Does what dolske said should happen. Public interface is now to a specific form as opposed to the whole document.

Comment 33

[Justin Dolske [:Dolske]]

Comment on attachment 325053 [details] [diff] [review]
Patch v2

>-                return this._pwmgr._fillDocument(domDoc);
>+                return this._pwmgr._fillDocument(domDoc, true);
...
>-                    this._pwmgr._fillDocument(doc);
>+                    this._pwmgr._fillDocument(doc, true);

Those changes shouldn't be needed now.

>+        var foundLogins = null;
> 
>         for (var i = 0; i < forms.length; i++) {
>             var form = forms[i];
>-
>+            
>             // Heuristically determine what the user/pass fields are
>             // We do this before checking to see if logins are stored,
>             // so that the user isn't prompted for a master password

Seems like that's where foundLogins should have been declared anyway, but you've still got the old declaration there too. Remove it.

Also, looks like you accidentally added a space on a blank line here.

>+    _fillForm : function (form, autofillForm, foundLogins) {
>+        // Repeating code from _fillDocument

Hmm. I think we can avoid the code duplication (and yet retain the optimizations) by having fillForm return the list of logins it fetched (and having the duplicated code only live in fillForm). Something like:

_fillDoc() {
  var logins = null;
  foreach form {
    if (actionOrigin changed)
      logins = null;
    logins = fillForm(logins);
  }
}

_fillForm(logins) {
  // get login fields, bail if no pw field
  ...
  if (logins == null)
    logins = findLogins()
  ...
  return logins;
}

So, basically, _fillForm will get a list of logins if it wasn't provided, and _fillDoc will only provide the last list of login if they're still valid.


>+    fillForm : function (form) {
>+        // Some code repetition from _fillDocument
>+        var formOrigin = this._getPasswordOrigin(form.ownerDocument.documentURI);
>+        var actionOrigin = this._getActionOrigin(form);
>+        var foundLogins = this.findLogins({}, formOrigin, actionOrigin, null);
>+        this._fillForm(form, true, foundLogins)
>     },

With my suggestion, fillForm would be simpler too:

fillForm(form) {
  this._fillForm(form, true, null);
}

Hmm, I suppose we should document in the IDL for fillForm() that it's not affected by the autofillForms preference. Lest anyone somehow expect that to be the case.

>+SimpleTest.waitForExplicitFinish();
>+</script>
>+</pre>
>+</body>
>+</html>
>+

Nit: trailing newline. :-)

Comment 34

[Justin Dolske [:Dolske]]

(In reply to comment #31)
> In addition to dolske's comments, I think we want to figure out how to signal
> that there are forms that can be filled with passwords

Yeah. I spun this idea off to bug 439365, to keep the scope of this bug narrow. Paul can implement that next. :-)

Comment 35

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Attachment: Patch v3

I cleaned up the dumb things I was doing, and also, all of the code repetition is gone.  Since the call to get logins isn't happening in _fillDocument, we don't have to check for a password field there, so that was taken out of _fillDocument and just left in _fillForm.

I also changed the log messages to use "form[id]" instead of "form[i]", since i is no longer in scope.

Comment 36

[Justin Dolske [:Dolske]]

Comment on attachment 325206 [details] [diff] [review]
Patch v3

Almost there! Just a few small nits...

>+    /**
>+     * Fill a form with login information if we have it. This method will fill
>+     * aForm regardless of signon.autofillForms preference.

...of *the* signon....

>+                foundLogins = null;
>+                previousActionOrigin = actionOrigin;
>+            }
>+            foundLogins = this._fillForm(form, autofillForm, foundLogins);
>+        } // foreach form

Before the call to fillForm, log "Processing form[i]...".

And then the log calls in _fillForm can be simplified to not bother with form[i/id].

I like the idea of having the ID from external callers though -- in the fillForm() wrapper, go ahead and log something like "fillForm processing form[id=X]".

>+    /*
>+    * _fillform
>+    *
>+    * Fill the form with login information if we can find it.
>+    */

Add a brief note here about the return value, and why we're passing in + returning logins.

Nit: The "*"s should align. (astrology pun?)

>+    _fillForm : function (form, autofillForm, foundLogins) {
...
>+            foundLogins =
>+                this.findLogins({}, formOrigin, actionOrigin, null);

Might that squeeze onto 1 line now? (<80 columns)

Comment 37

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Attachment: Patch v4

Comment 38

[Justin Dolske [:Dolske]]

Comment on attachment 325273 [details] [diff] [review]
Patch v4

Just a few more trivial nits, in debug messages. I'll just fix them when checking in.

>+            this.log("_fillDocument Processing form[" + i + "]");

s/Processing/processing/ :-)

>+            this.log("not filled, has autocomplete=off");

"form not filled [...]", lest it not be clear without context.

>+    fillForm : function (form) {
>+        this.log("fillForm Processing form[id=" + form.id + "]");

s/Processing/processing/

Comment 39

[Justin Dolske [:Dolske]]

Pushed http://hg.mozilla.org/mozilla-central/index.cgi/rev/055a716092aa

Great job!

Comment 40

[DigitAl56K]

Suggest status change to REOPENED.

I would like to refer to bug #389208, which was closed as a duplicated of this bug and which has not been addressed. In summary, if I have a master password enabled but have not yet entered it and visit a URL that contains a login form for which there is a stored password the master password dialog pops up, whether or not I actually want to login to a site. This is quite infuriating when you have the privacy options set to clear private data when you close Firefox - every time you open a new instance and start browsing certain sites the master password dialog will keep popping open. Visiting any pages from Digg.com's RSS feed is a prime example.

Because the title of this bug suggests that it is related to this behavior, and because my bug was closed as a dupe of this bug, I was expecting this problem to have been fixed given that the status of this bug is "FIXED". However, it is not fixed in Firefox 3 and it is not fixed in the June 21 2008 nightly build, which I assume incorporates Justin's June 16 2008 commit.

Please either REOPEN this bug or correct the status of bug #389208 so that this problem does not get lost in the depths of bugzilla.

Comment 41

[Chris Lawson (gone)]

I'm not sure what nightly build you're referring to, but you should ensure it came from this directory:

ftp://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-mozilla-central/

Firefox 3 does not have this change in it; the only builds that do are from the mozilla-central repository.

Comment 42

[DigitAl56K]

Chris,

From that FTP location I installed firefox-3.1a1pre.en-US.win32.installer.exe .

I then went to this URL:

http://digg.com/general_sciences/Preparing_for_the_Katrina_of_earthquakes

.. and immediately received a popup from the software security device requesting the master password, despite not having clicked any form field on the page.

Comment 43

[Jesse Ruderman]

Digit, are you still seeing unwanted master password dialogs pop up after you set signon.autofillForms to false?

Comment 44

[DigitAl56K]

Jesse, yes, the master password dialog still pops up even when signon.autofillForms is false. The only signon option that is "true" is signon.rememberSignons . I ran the minefield version in safe mode to be sure all extensions were disabled.

Same problem happens here at bugzilla.mozilla.org btw, so you should be able to reproduce easily.

Comment 45

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Not everyone is using a master password to encrypt the stored data. So I believe it's better to do the remaining work in bug 389208 which was duped against this one. I can also see the same behavior with the latest 3.1 nightly build on OS X.

One more thing. The uiwanted keyword is set. Are there plans to have it integrated into the security preferences pane? Or was it getting lost here?

Comment 46

[DigitAl56K]

"Not everyone is using a master password to encrypt the stored data."

I'm not sure that is relevant. It is functionality covered by this bug when a UI-visible feature of Firefox is enabled. The other bug doesn't have nearly as many developers/testers because it was closed long ago. My worry is that this will not be fixed, or will not be fixed for a long time.

FYI, SecureLogin addon used to solve this problem on FF2, it no longer appears to solve the problem on FF3, which might influence your prioritization - I don't know of any applicable workaround.

Comment 47

[Justin Dolske [:Dolske]]

(In reply to comment #44)
> Jesse, yes, the master password dialog still pops up even when
> signon.autofillForms is false.

That's bug 400680.

This bug is fixed. Other issues should not be handled here.

Comment 51

[Frank]

(In reply to Justin Dolske [:Dolske] from comment #47)
> (In reply to comment #44)
> > Jesse, yes, the master password dialog still pops up even when
> > signon.autofillForms is false.
> 
> That's bug 400680.
> 
> This bug is fixed. Other issues should not be handled here.

Sorry, but the bug is not fixed, in year 2013 (5 years later) the bug still exist.

Look at https://bugzilla.mozilla.org/show_bug.cgi?id=777725

Comment 54

[alex_mayorga]

¡Hola Daniel!

I ended up here upon testing for https://freedom-to-tinker.com/2017/12/27/no-boundaries-for-user-identities-web-trackers-exploit-browser-login-managers/ on Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:59.0) Gecko/20100101 Firefox/59.0 ID:20180102110609

The demo at https://senglehardt.com/demo/no_boundaries/loginmanager/index.html does show me the e-mail and password.

Shall I be worried?

Should this be reopened or a new bug filed?

¡Gracias!
Alex

Comment 55

[Daniel Veditz [:dveditz]]

(In reply to alex_mayorga from comment #54)
> Shall I be worried?

Yes.

> Should this be reopened or a new bug filed?

Comment 47 still stands: "This bug is fixed. Other issues should not be handled here." An option was provided. What we want (or you and I, anyway) is to change the default for signon.autofillForms to false (or equivalent behavior). The behavior is much improved since my grumpy comment 14 from ten years ago so I hope it is something we can do now. That's requested in bug 1427543.

Comment 56

[Daren Jeffcott]

[redacted - mhoye]