Filling a bug specifically for disabling autofill on HTTP pages. If the user has a saved password on an HTTP page, the Password Manager should not fill it automatically. Instead, the user should have to first type in or select their username. At that point (after user interaction) the Password Manager can fill the password. Given bug https://bugzilla.mozilla.org/show_bug.cgi?id=1179961 has landed, Firefox will be sending mixed messages if we both warn the user that the password could be compromised and fill it in for them. Hence, it's time to finally stop autofilling passwords on HTTP pages! +++ This bug was initially created as a clone of Bug #1118511 +++ When passwords are autofilled by the Password Manager, javascript on the page can read the filled in passwords even when the user has no intent to login to the page. Hence, pages that are vulnerable to javascript injection (via XSS or MITM) are also subject to password exfiltration attacks[1] (in some cases without any indication to the user that such an attack has taken place). To protect against this, we should not autofill passwords without user interaction / user intent to login. This protects users from a number of the drive by attacks and protects users who never planned to login. From a usability perspective, it is debatable whether or not autofill is a good user experience. It allows for an easy and frictionless login, but can also cause confusion (how did my password get filled in?). Discussion about a more active login experience (rather than a passive one) are underway with UX. If we do decide that we need to continue with an autofill experience, then there are cases where we definitely should not autofill. (Note that this will make a less consistent experience for users and won't protect us on a page with an XSS vulnerability.) In the below cases, we MUST not autofill: * For http sites (IE 11 has this security feature) * https sites that have mixed active content * https sites that require a cert override (chrome does this) * in iframed sites (safari does this for non-same origin, chrome does this for all frames). * Invisible form fields (visibility and opacity, although this isn't going to prevent clickjacking attacks to autofill the passwords) Previous bugs have been filed to address subsets of this problem. I've added them to the dependent list. [1] https://www.usenix.org/conference/usenixsecurity14/technical-sessions/presentation/silver

Is this as simple as adding && !hasInsecureLoginForms here: http://mxr.mozilla.org/mozilla-central/source/toolkit/components/passwordmgr/LoginManagerContent.jsm#501 let autofillForm = gAutofillForms && !PrivateBrowsingUtils.isContentWindowPrivate(doc.defaultView) && !hasInsecureLoginForms Paolo, does that look right?

We probably want to check whether the specific page where we are autofilling is secure or not, ignoring subframes. In any case, insecure subframes or sibling frames (in the mixed content case) may load asynchronously after we have already autofilled. If an HTTPS frame is loaded by an HTTP page, is it safe to autofill in the HTTPS frame? I guess so, since they are considered different origins, correct? In that case, we don't need to consider parent frames also.

This is NOT a good idea. My Password Manager database includes 32 entries for HTTP sites. None of these contain any information about me that would create a vulnerability regarding my identity or finances. Primarily, I have accounts at these sites that merely contain my preferences for formatting the displays.

If we're telling them in the address bar their password is insecure we shouldn't at the same time be autofilling the password field. It's a mixed message to the user.

Reviewed at the team planning meeting. Determined to be non-MVP work and moved to our maintenance meta.

Since we now show an insecure password warning for login forms over HTTP in both Dev Edition and Nightly, we are sending a mixed message by autofilling on HTTP. We need to stop autofilling as soon as someone has the cycles to pick up this bug. https://blog.mozilla.org/tanvi/2016/01/28/no-more-passwords-over-http-please/ Matt, you may be interested after your work on https://bugzilla.mozilla.org/show_bug.cgi?id=667233 is complete.

I strongly disagree with comment #6. There are still a number of HTTP Web sites that require logins, most of which involve no security risks. Instead, the act of logging-in merely retrieves the user's preferred configuration of the site. We waited a long time for other bugs to be implemented to autofill user IDs and passwords, even for HTTP sites. Please do not undo that.

In my opinion, bug 1289913, bug 1120037, and bug 376668 are blockers to shipping this since we need the be able to help the users complete the task at hand which is logging in to the website. Users will just type their passwords in manually if he don't help them so providing fallback fill UI isn't a major security drawback, especially once we have contextual warning UI (bug 1217162).

(In reply to Matthew N. [:MattN] from comment #9) > In my opinion, bug 1289913, bug 1120037, and bug 376668 are blockers to > shipping this since we need the be able to help the users complete the task > at hand which is logging in to the website. Users will just type their > passwords in manually if he don't help them so providing fallback fill UI > isn't a major security drawback, especially once we have contextual warning > UI (bug 1217162). I'm not sure all of these are blockers, but bug 376668 is the most important of the 3 in my opinion - show the autocomplete drop down on focus of the username field.

Is there anyway to determine usage of password manager for http sites, specifically?

(In reply to Tanvi Vyas [:tanvi] from comment #10) > (In reply to Matthew N. [:MattN] from comment #9) > > In my opinion, bug 1289913, bug 1120037, and bug 376668 are blockers to > > shipping this since we need the be able to help the users complete the task > > at hand which is logging in to the website. Users will just type their > > passwords in manually if he don't help them so providing fallback fill UI > > isn't a major security drawback, especially once we have contextual warning > > UI (bug 1217162). > > I'm not sure all of these are blockers, but bug 376668 is the most important > of the 3 in my opinion - show the autocomplete drop down on focus of the > username field. Without fixing all three we will have some form of UX regression for affected sites. (In reply to Peter Dolanjski [:pdol] from comment #11) > Is there anyway to determine usage of password manager for http sites, > specifically? Yes, we should have recorded at form submission time whether a form was secure (like I suggested in bug 1174333 comment 7) but instead only measured the existence of forms so we don't currently have that data. I know many self-hosted forum sites (e.g. phpBB/SMF) don't offer HTTPS and I suspect they are a large chunk of the 35% of insecure login forms and I would hate to break them (especially since there is no indication Chrome will at the moment).

So maybe this got lost but we would put this behind a pref anyway. We'd leave it off for now and I would add a telemetry probe to the patch that collects data on how many HTTP pages would get skipped, no matter the pref. Sounds fine?

(In reply to Johann Hofmann [:johannh] from comment #13) > So maybe this got lost but we would put this behind a pref anyway. I filed bug 1302474 to add the pref so we don't need to change the whole dependency tree. > We'd leave it off for now and I would add a telemetry probe to the patch that > collects data on how many HTTP pages would get skipped, no matter the pref. Adding the reason to `const AUTOFILL_RESULT` makes sense but that should only record the real reason for not filling meaning that it should honour the pref. I don't think it's necessary to add a probe to know how often we wouldn't fill since it doesn't sound like it will affect our decision to eventually disable this (and provide manual fill methods such as autocomplete) but I also won't stop you from adding a new probe.

Marking as P1 because this needs to land in Firefox 52.

This bug could also flip the security.insecure_field_warning.contextual.enabled pref too.

We still have to wait for a few more bugs to land, but I wanted to get this patch and review ready for when the dependencies land. https://treeherder.mozilla.org/#/jobs?repo=try&revision=957be548837f50ae54639992e1f674267d37f1d6

Comment on attachment 8809221 [details] [diff] [review] Bug1217152-11-09-16.patch Review of attachment 8809221 [details] [diff] [review]: ----------------------------------------------------------------- ::: modules/libpref/init/all.js @@ +4343,5 @@ > // Login Manager prefs > pref("signon.rememberSignons", true); > pref("signon.rememberSignons.visibilityToggle", true); > pref("signon.autofillForms", true); > +pref("signon.autofillForms.http", false); This will disable HTTP autofill for all toolkit applications but I'll need to think about whether the other applications will get the fallback UI (autocomplete) for free or whether we should just override this to false in firefox.js

Comment on attachment 8809221 [details] [diff] [review] Bug1217152-11-09-16.patch Review of attachment 8809221 [details] [diff] [review]: ----------------------------------------------------------------- ::: modules/libpref/init/all.js @@ +4343,5 @@ > // Login Manager prefs > pref("signon.rememberSignons", true); > pref("signon.rememberSignons.visibilityToggle", true); > pref("signon.autofillForms", true); > +pref("signon.autofillForms.http", false); I think it's fine for the toolkit defaults to be secure and another app can override this to true if they want a less secure application.

I am confused. Will there be an option to still get autofill of passwords on HTTP sites? There are a number of Web sites that are still not HTTPS that require logins. For me, those sites contain NO personal information. The login merely causes those sites to be formatted in a way I previously setup.

(In reply to David E. Ross from comment #20) > I am confused. Will there be an option to still get autofill of passwords > on HTTP sites? > > There are a number of Web sites that are still not HTTPS that require > logins. For me, those sites contain NO personal information. The login > merely causes those sites to be formatted in a way I previously setup. You can still fill logins via the context menu (right click). You could also simply flip the "signon.autofillForms.http" pref to true in your personal about:config (once we change it to false by default).

(In reply to Johann Hofmann [:johannh] from comment #21) > (In reply to David E. Ross from comment #20) > > I am confused. Will there be an option to still get autofill of passwords > > on HTTP sites? > > > > There are a number of Web sites that are still not HTTPS that require > > logins. For me, those sites contain NO personal information. The login > > merely causes those sites to be formatted in a way I previously setup. > > You can still fill logins via the context menu (right click). and (more obviously) the autocomplete popup.

[Tracking Requested - why for this release]: Enable the new feature Adrian, can you do a QE pass on the whole insecure field contextual warning and login manager autocomplete changes when this lands or by flipping the two prefs yourself. Known bug dependencies: https://mnoorenberghe.github.io/bz-dependency-buglist/?list=1304224 Tracked work: https://docs.google.com/spreadsheets/d/155k3Ja06QVgRj9F_DnAJxC8wfH_lWM4N9eZal4llefk/edit#gid=1492590763

Disabling insecure password form field warning and enabling HTTP autofill on selective tests. This got to be a bit excessive, so Matt suggested we just flip the prefs for the whole directory until HTTPS mochitests are supported. There are a handful that don't need them. I went through everything in http://searchfox.org/mozilla-central/source/toolkit/components/passwordmgr/test/mochitest/ up to test_prompt_http.html. Will provide a new patch that just updates the whole directory.

Disable insecure password warning and enable autofill on HTTP pages for password manager tests using pwmgr_common.js

Push to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bb466892361f4671691ebd1a88758d03cf416fc5

Rebased and update comment. Carrying over r+.

Fixed trailing whitespace.

Matt, I'm out next week. Can you review the test patch, and if the patch and try looks good, mark this checkin-needed and request uplift?

Try shows that updating https://treeherder.mozilla.org/#/jobs?repo=try&revision=bb466892361f4671691ebd1a88758d03cf416fc5&selectedJob=33416806 pwmgr_common.js doesn't work so well, since some tests need to change the pref values. Perhaps the code in pwmgr_common.js is getting called after the individual tests change the pref to the value they desire. Anyway, we might have to go with the individual test approach after all. So I've pushed my WIP test patch (which is attached to this bug) to try to see what remaining tests we need to individually update: https://treeherder.mozilla.org/#/jobs?repo=try&revision=267ec9cd52f3d832079c2daa19a9512232f39849

Fixing the trailing whitespace again.

Try shows 2 remaining tests that need to be updated: TEST-UNEXPECTED-FAIL | toolkit/components/passwordmgr/test/mochitest/test_recipe_login_fields.html | Test timed out. TEST-UNEXPECTED-FAIL | toolkit/components/passwordmgr/test/test_master_password.html | Test timed out. Something may also have to be done for this one: TEST-UNEXPECTED-FAIL | toolkit/components/passwordmgr/test/mochitest/test_basic_form_autocomplete.html Matt, hoping you can take this during the week!

(In reply to Matthew N. [:MattN] (PM me if requests are blocking you) from comment #23) > [Tracking Requested - why for this release]: Enable the new feature > > Adrian, can you do a QE pass on the whole insecure field contextual warning > and login manager autocomplete changes when this lands or by flipping the > two prefs yourself. > > Known bug dependencies: > https://mnoorenberghe.github.io/bz-dependency-buglist/?list=1304224 > Tracked work: > https://docs.google.com/spreadsheets/d/ > 155k3Ja06QVgRj9F_DnAJxC8wfH_lWM4N9eZal4llefk/edit#gid=1492590763 Hello Matt, few questions below: 1. Are the preferences in discussion: (a)"security.insecure_field_warning.contextual.enabled" and (b)"signon.autofillForms.http"? Their defaults today (12.27) in Nightly53 are: (a) - false and (b)- true. The defaults when this lands will be a-true and b -false as the title of this issue suggests? 2. I'm not sure what exactly you mean by the "login manager autocomplete changes" - could you specify a list of (meta)bugs that cover this? Would this also cover the email tread with the password autocomplete? guessing this would be it bug 1318203? 3. I see that the tracking is set for 52/53, so I'll treat this work as a bug verification work(exploratory testing and bug fix verification), but I'd prefer if the time allows to treat it as a fully fledged feature (this means test plan, test casing, etc). So, please let me know how are we faring on the time chapter. Meanwhile, I'll go with the 1st version - bug verification work(exploratory testing and bug fix verification).

Comment on attachment 8821699 [details] [diff] [review] Bug1217152-pwmgrcommon-tests.patch Review of attachment 8821699 [details] [diff] [review]: ----------------------------------------------------------------- r=me with the clearUserPref lines removed ::: toolkit/components/passwordmgr/test/pwmgr_common.js @@ +443,4 @@ > SimpleTest.registerCleanupFunction(() => { > SpecialPowers.popPrefEnv(); > + SpecialPowers.clearUserPref("signon.autofillForms.http"); > + SpecialPowers.clearUserPref("security.insecure_field_warning.contextual.enabled"); The reason this failed is because pushPrefEnv is finicky about other code touching the prefs it's controlling and the popPrefEnv() line above is supposed to do this for you. Removing these two lines makes the tests pass with and without e10s for me.

Pushing to inbound now with the two lines removed as the following are passing locally without them: > ./mach mochitest toolkit/components/passwordmgr/test/ --disable-e10s > ./mach mochitest toolkit/components/passwordmgr/test/

After bug 1286312 lands we can revert the test changes and make affected tests https except for ones testing the warning.

Pushed by mozilla@noorenberghe.ca: https://hg.mozilla.org/integration/mozilla-inbound/rev/f0d146fe7317 Flip a pref to disable autofilling saved password on HTTP pages. Flip a pref to enable showing insecure password warnings in the password field autocomplete drop down. r=MattN https://hg.mozilla.org/integration/mozilla-inbound/rev/bc1c406e17c8 disable insecure password warning and enable autofill on HTTP pages for password manager tests using pwmgr_common.js. r=MattN

(In reply to Adrian Florinescu [:AdrianSV] [No PTO left this year - NI me if required] from comment #33) > (In reply to Matthew N. [:MattN] (PM me if requests are blocking you) from > comment #23) > > [Tracking Requested - why for this release]: Enable the new feature > > > > Adrian, can you do a QE pass on the whole insecure field contextual warning > > and login manager autocomplete changes when this lands or by flipping the > > two prefs yourself. > > > > Known bug dependencies: > > https://mnoorenberghe.github.io/bz-dependency-buglist/?list=1304224 > > Tracked work: > > https://docs.google.com/spreadsheets/d/ > > 155k3Ja06QVgRj9F_DnAJxC8wfH_lWM4N9eZal4llefk/edit#gid=1492590763 > > Hello Matt, few questions below: > > 1. Are the preferences in discussion: > (a)"security.insecure_field_warning.contextual.enabled" and > (b)"signon.autofillForms.http"? Their defaults today (12.27) in Nightly53 > are: (a) - false and (b)- true. > The defaults when this lands will be a-true and b -false as the title of > this issue suggests? Yes, correct. See https://hg.mozilla.org/integration/mozilla-inbound/rev/f0d146fe7317 > 2. I'm not sure what exactly you mean by the "login manager autocomplete > changes" - could you specify a list of (meta)bugs that cover this? Would > this also cover the email tread with the password autocomplete? guessing > this would be it bug 1318203? I mean refactorings like bug 1296638 and bug 1294502 (plus follow-ups) that this work built on top of. I would recommend getting ideas from the spreadsheet and I've added these two bugs there: https://docs.google.com/spreadsheets/d/155k3Ja06QVgRj9F_DnAJxC8wfH_lWM4N9eZal4llefk/edit#gid=1492590763 Yes, it's related to the firefox-dev thread about password autocomplete so anything related to autocomplete of usernames, passwords or even non-login data (e.g. using datalist or regular form history) were affected. > 3. I see that the tracking is set for 52/53, so I'll treat this work as a > bug verification work(exploratory testing and bug fix verification), but I'd > prefer if the time allows to treat it as a fully fledged feature (this means > test plan, test casing, etc). So, please let me know how are we faring on > the time chapter. Meanwhile, I'll go with the 1st version - bug verification > work(exploratory testing and bug fix verification). Yeah, I think if you have time it would be good to treat it as a feature affecting autocomplete (and autofill on insecure pages). There are only a few more bugs to land and uplift in the spreadsheet so 52 is still attainable IMO.

Comment on attachment 8821698 [details] [diff] [review] Bug1217152-12-23-16.patch Approval Request Comment [Feature/Bug causing the regression]: Insecure login field contextual warning feature [User impact if declined]: Users won't be warned in a login field when it's insecure [Is this code covered by automated tests?]: Yes, mochitests and unit tests [Has the fix been verified in Nightly?]: Adrian is working on verification. [Needs manual test from QE? If yes, steps to reproduce]: See above [List of other uplifts needed for the feature/fix]: Depends on bug 1324918 getting uplifted to avoid issues on Linux [Is the change risky?]: Not really [Why is the change risky/not risky?]: It's easy to pref off again and riskier dependency parts have been already riding the trains. [String changes made/needed]: None

Comment on attachment 8821699 [details] [diff] [review] Bug1217152-pwmgrcommon-tests.patch Approval Request Comment: See above

https://hg.mozilla.org/mozilla-central/rev/f0d146fe7317 https://hg.mozilla.org/mozilla-central/rev/bc1c406e17c8

Updating patch so the right one is in the bug for aurora uplift.

Comment on attachment 8823490 [details] [diff] [review] Bug1217152-pwmgrcommon-tests2.patch Approval Request Comment See comment 39

Comment on attachment 8823490 [details] [diff] [review] Bug1217152-pwmgrcommon-tests2.patch disable password autofill on http for aurora52

Comment on attachment 8821698 [details] [diff] [review] Bug1217152-12-23-16.patch disable password autofill on http for aurora52

https://hg.mozilla.org/releases/mozilla-aurora/rev/872f6e1948f5 https://hg.mozilla.org/releases/mozilla-aurora/rev/a51cdeb88cbf

Don't forget that there is a contextual menu for autofill as well. Currently this says (No Login Suggestions), but I think it should show the list of what's saved.

Matt, 27-30 december last year, I’ve explored around on Nightly by switching the two preferences as discussed in comment 33 point 1. The behavior I got then was that any HTTP login would not offer at all any auto-fill/autocomplete/auto-suggest options for the saved users/passwords (behaviorA). If I redo the same actions today, (for HTTP) the autocomplete/auto-suggest and also the context menu will show my usernames/passwords as saved in the password manager(behaviorB). Going back to the 27-29 dec. builds, I get no trace of the behavior A and everything works as BehaviorB. I think we can exclude the example sites, machines and OS’es since I used more than one. Other than asking if you have any thought of how the above might be possible, I’d like to confirm that today’s behavior (B) is the correct and expected result of Password manager: - turning off “signon.autofillForms.http” - automatic autofill for http will be disabled, while manual autofill is still enabled Automatic autofill = user,password are filled in on page load (as the current https behaves) Manual autofill = user, password will be filled on user action (clicking on username will trigger the username list to choose from, etc). (In reply to Ryan Feeley [:rfeeley] from comment #47) > Don't forget that there is a contextual menu for autofill as well. Currently > this says (No Login Suggestions), but I think it should show the list of > what's saved. The context menu today (behaviorB) shows the login suggestions (for password as well). Though I remember it did show "No Login Suggestions" on behaviorA. Ryan, could you share the http login you've seen this result on?

(In reply to Ryan Feeley [:rfeeley] from comment #47) > Don't forget that there is a contextual menu for autofill as well. Currently > this says (No Login Suggestions), but I think it should show the list of > what's saved. Hey Ryan, I'm not sure what you are referring to. Can you elaborate?

(In reply to Adrian Florinescu [:AdrianSV] from comment #48) > Matt, 27-30 december last year, I’ve explored around on Nightly by switching > the two preferences as discussed in comment 33 point 1. The behavior I got > then was that any HTTP login would not offer at all any > auto-fill/autocomplete/auto-suggest options for the saved users/passwords > (behaviorA). > If I redo the same actions today, (for HTTP) the autocomplete/auto-suggest > and also the context menu will show my usernames/passwords as saved in the > password manager(behaviorB). Going back to the 27-29 dec. builds, I get no > trace of the behavior A and everything works as BehaviorB. I think we can > exclude the example sites, machines and OS’es since I used more than one. I don't really know what to say about the cause but behaviourA would have been a bug. It would have been useful to have password manager debug logging from when that happened: https://wiki.mozilla.org/Toolkit:Password_Manager/Debugging > Other than asking if you have any thought of how the above might be > possible, I’d like to confirm that today’s behavior (B) is the correct and > expected result of Password manager: > > - turning off “signon.autofillForms.http” - automatic autofill for http will > be disabled, while manual autofill is still enabled > Automatic autofill = user,password are filled in on page load (as the > current https behaves) > Manual autofill = user, password will be filled on user action (clicking on > username will trigger the username list to choose from, etc). BehaviourB is what is expected and it sounds like what you're saying above matches that.

(In reply to Tanvi Vyas - behind on bugmail [:tanvi] from comment #49) > (In reply to Ryan Feeley [:rfeeley] from comment #47) > > Don't forget that there is a contextual menu for autofill as well. Currently > > this says (No Login Suggestions), but I think it should show the list of > > what's saved. > > Hey Ryan, I'm not sure what you are referring to. Can you elaborate? When you right-click the username/password field, you can also fill that way. Not as discoverable as our new UI, but still needs to reflect state accurately.

Posted the site compatibility doc for web developers: https://www.fxsitecompat.com/en-CA/docs/2017/insecure-login-forms-now-disable-autofill-show-warning-beneath-input-control/ (not actually a compatibility stuff though)

Reproduced the issue on 53.0a1 20161227030213. I can confirm the bug is fixed on 53.0a2 (2017-02-27) and 52.0b9 (20170223185858), using Windows 10 x64, Ubuntu 16.04 x64 and Mac OS X 10.11.6. Setting the corresponding flags.

Documented; see details at https://bugzilla.mozilla.org/show_bug.cgi?id=1319119#c17

(In reply to Ryan Feeley [:rfeeley] from comment #47) > Don't forget that there is a contextual menu for autofill as well. Currently > this says (No Login Suggestions), but I think it should show the list of > what's saved. If this is still an issue, please file a followup bug. Clearing my needinfo.