As far as I can tell, this is how we detect insecure password fields: <https://dxr.mozilla.org/mozilla-central/source/toolkit/components/passwordmgr/LoginManagerContent.jsm#417>. It seems like before looking at the document URI, we should see if the password element is a descendant of a form element and if so, look at its action attribute in order to determine whether the password will be submitted to a non-secure page. Fixing this for example will stop us marking nytimes.com as insecure.

> Fixing this for example will stop us marking nytimes.com as insecure. But it is insecure, right? If the page is http://, a network attacker can inject script that will read the contents of the password field just before submitting it. (Or is this considered out of scope in terms of the threat model for this feature?)

Asking for a password over HTTP is insecure regardless of whether or not you submit that password over HTTPS. Submitting over HTTP is horrible, as any eavesdropper can read your password in cleartext. Asking for a password over HTTP means any MITM can steal the password by: * injecting javascript that will read the contents of the password field, log keystrokes, etc. * changing the <form action= to submit the password to their server. I've got to update the Learn More link https://support.mozilla.org/en-US/kb/insecure-password-warning-firefox?as=u&utm_source=inproduct as part of https://bugzilla.mozilla.org/show_bug.cgi?id=1217165 to make this clear. Marking this bug as invalid.

Hmm, I am still struggling to understand the thread model here. If comment 1 is the threat model then that attack is possible on all HTTP pages -- all you have to do is to inject an <input type=password> before doing what David described. Why does the pre-existing <input type=password> tip the scale either way?

(In reply to :Ehsan Akhgari from comment #3) > Hmm, I am still struggling to understand the thread model here. If comment > 1 is the threat model then that attack is possible on all HTTP pages -- all > you have to do is to inject an <input type=password> before doing what David > described. Why does the pre-existing <input type=password> tip the scale > either way? Yes, an attacker can inject <input type=password> on any page. But this might be odd if the user isn't trying to login. If the user explicitly clicks login and is redirected to real login page, they are more likely to enter their username and password. If the login page they are redirected to is over HTTP, then a man-in-the-middle attacker can change the form action from https://login.mysite.com to https://login.attacker.com without the user having any idea. Moreover, the attacker could then redirect the credentials back go to login.mysite.com and the user could be logged into their site without knowing that their credentials were also sent to attacker.com and their login is compromised. It is true that <input type=password> could be added to any page in an attempt to convince a user to share credentials. But that doesn't take away from the threat mentioned above. Moreover, with the new UI, if an attacker did inject a password field into an HTTP page, the page would show the broken lock. Ehsan, does that make sense? Or would you like to chat more about this.

I guess it makes sense. In my ideal world, we'd mark all HTTP pages as insecure. But I guess we need to take baby steps. :-)

(And thanks for the explanation!)

Re-opening as we should consider an HTTPS form submitting to HTTP as insecure at least for the purposes of the contextual feedback and therefore autofill. It may be a bit confusing for the identity block indicator but consistency would be good. To reduce confusion we may want to have an additional string for this case in Control Center to explain that even though the page is https, the/a login form is still not secure. We already have similar confusing messages with our current implementation as the identity block indicator changes for subframes which is also not relevant to the URL bar scheme. We already handle the form action for the Web Console warnings but not for the UI. We should unify the logic (putting it all in InsecurePasswordUtils.jsm) so it's consistent.

Telemetry shows this is very rare.