748193 - Highlight Password Form Fields on http pages or with http submits

Status: RESOLVED FIXED

(Firefox :: Security, defect)

Description

[Tanvi Vyas[:tanvi]]

Phase 1 of https://wiki.mozilla.org/Security/HighlightCleartextPasswords

Outline the password field in red in the following 3 cases:

* A user is asked to login on an http page. The login form submits to an http destination. Users password is sent in cleartext.

* A user is asked to login on an https page. The login form submits to an http destination. Users password is sent in cleartext.
  
* A user is asked to login on an http page. The login form submits to an https destination. An attacker can mitm the first request to the login page and replace the form with one that submits the password to the attackers webpage instead.

Comment 1

[Mardeg]

Would the description of each case appear when hovered via the title attribute? Otherwise I foresee many bugs filed about the red outline being buggy page rendering, especially in that third case.

Comment 2

[Tanvi Vyas[:tanvi]]

Justin - I am considering putting my code in nsLoginManager.js and wanted to see what you think about that.  In some ways, the features are similar (deal with passwords, go through the page looking for forms, etc).  And in some way they are different (my feature is not limited to 3 passwords on a page, can't be turned off with a preference, etc).

I've started hacking this together, adding code to _webProgressListener's onStateChange that calls _hightlightCleartextPasswords().  This is a new internal method in LoginManager.  Before I continue any further, I wanted to know if this is the right place.  If it is, I can make some changes to the _getPasswordFields so that we only have to go through the html once when looking for password elements.  Also, I'd like to see if we can prevent autofill of passwords on these insecure pages, and instead present the user with the multi-user experience (where they first select the username, and then the password is filled in).

I will attach my pseudocode (that compiles, but doesn't really work yet).  Thanks!

Comment 3

[Tanvi Vyas[:tanvi]]

Attachment: First hack

Beginning of patch.

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 617745 [details] [diff] [review]
First hack

Review of attachment 617745 [details] [diff] [review]:
-----------------------------------------------------------------

I think we should include an about:config pref for this. It wouldn't be hard to, and it would be there for users who run in to issues with it.

Can we reuse some of the code for HTML5 form validation? IIRC, that code puts an outline around elements that aren't passing validation. We'll probably also need a way for the browser to explain why there is an outline or what is happening.

::: toolkit/components/passwordmgr/nsLoginManager.js
@@ +277,5 @@
>              // STATE_START is too early, doc is still the old page.
>              if (!(aStateFlags & Ci.nsIWebProgressListener.STATE_TRANSFERRING))
>                  return;
> +	          var pwClear = false;
> +	          pwClear = _highlightCleartextPasswords(domDoc);

nit: please use |let| instead of |var|

@@ +1357,5 @@
> +        var pwClear = false;
> +
> +        if(documet.location.protocol != "https:") {
> +          http=true;
> +        }

let isHttp = document.location.protocol != "https:";

@@ +1359,5 @@
> +        if(documet.location.protocol != "https:") {
> +          http=true;
> +        }
> +
> +        if(!forms || forms.length==0) {

nit: space after if

@@ +1362,5 @@
> +
> +        if(!forms || forms.length==0) {
> +          return;
> +        } 
> +        else {

} else {

@@ +1363,5 @@
> +        if(!forms || forms.length==0) {
> +          return;
> +        } 
> +        else {
> +          for(var i=0; i<forms.length; i++) {

for each (let form in forms)

@@ +1398,5 @@
> +             pwFields[i].value="DANGER";
> +             pwFields[i].placeholder="DANGER2";
> +             pwFields[i].title="DANGER3";
> +             //need to change outline to red
> +             //need to add constraint validation style custom note

How can we make sure that the site cannot override this warning? If we are worried about a malicious attack, what would stop the attacker from including a style that disables our warning?

@@ +1408,5 @@
> +     * _getAllPasswordFields
> +     *
> +     */
> +     _getAllPasswordFields : function(form) {
> +         for (var i = 0; i < form.elements.length; i++) {

return form.elements.filter(function(element) {
  return (element instanceof Ci.nsIDOMHTMLInputElement && element.type == "password");
});

Comment 5

[Ian Melven :imelven]

(In reply to Jared Wein [:jaws] from comment #4)
> Comment on attachment 617745 [details] [diff] [review]
>
> I think we should include an about:config pref for this. It wouldn't be hard
> to, and it would be there for users who run in to issues with it.

+1 for a pref for this - we could start with it disabled and let people try it out, a la click to play.

Comment 6

[Tanvi Vyas[:tanvi]]

(In reply to Mardeg from comment #1)
> Would the description of each case appear when hovered via the title
> attribute? Otherwise I foresee many bugs filed about the red outline being
> buggy page rendering, especially in that third case.

Yes, I'm still working out what this will look like (placeholders, html5 custom constraint validation, etc).

> I think we should include an about:config pref for this. It wouldn't be hard
> to, and it would be there for users who run in to issues with it.
> 

> +1 for a pref for this - we could start with it disabled and let people try
> it out, a la click to play.
>


We can also add an about:config pref to turn this off.  I'm not sure I agree with having it off by default and then turning it on for a later version of firefox, but we'll see how it goes since we are still in the very early stages.

> @@ +1398,5 @@
> > +             pwFields[i].value="DANGER";
> > +             pwFields[i].placeholder="DANGER2";
> > +             pwFields[i].title="DANGER3";
> > +             //need to change outline to red
> > +             //need to add constraint validation style custom note
> 
> How can we make sure that the site cannot override this warning? If we are
> worried about a malicious attack, what would stop the attacker from
> including a style that disables our warning?

I haven't worked out how this is going to be implemented yet.  I was talking to David Keeler and he said we might need to use overlays like you do with click to play.  I need to investigate more here.

Thanks for the feedback Jared!

Comment 7

[Justin Dolske [:Dolske]]

A few general concerns:

1) I'm a little dubious about the feature in general... It's not something the user can really fix or change, and the end result is likely to be the browser scaring them over something they'll just have to end up ignoring. [Perhaps there's value in shipping it disabled by default, for the minority of users who understand what's going on.]

2) Password manager isn't quiiiite the right place for this, but I guess it's close enough.

3) Performance concerns. It's not really great to be running script that trudges through every page that loads (unavoidable for password manager, but it tries hard to avoid doing work it doesn't need to. If you code only ran for a small set of sites that's probably ok. Otherwise we should find something better.

The good news on #3 is that we've been talking about ways that Gecko's content code could drive password manager, instead of how it works now. The nutshell idea is that whenever content creates a password field, pwmgr gets some notification of that (and otherwise does nothing). That limits the overhead of pwmgr to only pages that have password fields. That would likely work for what you're doing here too. [Perhaps, depending on desired UX, it could also be a CSS pseudoclass. That would be cheaper still.] That said, the perf impact is of more concern for an actual landing, and continuing to prototype/experiment in pwmgr is fine.

Comment 8

[Devdatta Akhawe [:devd]]

I share @dolske's concerns re 1. But instead of making it default off, how about Mozilla ship with a list of sites that do have HTTPS support and redirect automatically for those sites? For the remaining sites, the feature can stay default off. This is similar to how Chrome ships with a list of HSTS sites preloaded (and Mozilla plans to do the same)

Comment 9

[Devdatta Akhawe [:devd]]

(I meant, list of sites with login pages that support HTTPS)

Comment 10

[Tanvi Vyas[:tanvi]]

There is a similar proposal in bug https://bugzilla.mozilla.org/show_bug.cgi?id=983326

Comment 11

[Tanvi Vyas[:tanvi]]

Looks like these are from HTMLMediaElement.  Perhaps another null check was missed?
https://crash-stats.mozilla.com/report/index/ca189039-a252-40c7-aa9a-43aac2141105

Comment 12

[Tanvi Vyas[:tanvi]]

(In reply to Tanvi Vyas [:tanvi] from comment #11)
> Looks like these are from HTMLMediaElement.  Perhaps another null check was
> missed?
> https://crash-stats.mozilla.com/report/index/ca189039-a252-40c7-aa9a-
> 43aac2141105

Sorry :(  Wrong bug.

Comment 13

[Tanvi Vyas[:tanvi]]

Comment on attachment 617745 [details] [diff] [review]
First hack

This bug is currently on hold since I'm unsure of what the right UI is for this and I'm unsure if we can actually ship this.

Comment 14

[Tanvi Vyas[:tanvi]]

Attachment: poc - insecurepass-04-21-15.patch

This is just a proof of concept patch.  Here is a screenshot of the UI it produces:
http://people.mozilla.org/~tvyas/insecurePasswordDoorhanger.png

There is still much more work to be done here:
* Add about:config pref to turn this on/off
* Make the "try secure version" button work
* Add telemetry probes.
* Fix up the text.  Add links to Learn More pages
* Write the Learn More pages.

Potentially: 
* Integrate with existing door hanger so we don't get two doorhangers.  For some cases (ex: fill) this makes sense.  For others (ex: capture) it might not.

* Do a preflight for the https version before offering the user an option to try the secure version

Open Questions:
* When does the doorhanger pop open?  Likely on focus of the username or password field.

* What happens when password is autofilled already?  Does the doorhanger pop up automatically?

* What should the mainAction be?  "Got it" or "try secure version".  If we have data suggesting that the secure version exists (ex: password manager record for https page), then "try secure version" could be the first option.  We need to be careful to weigh the code complexity here with the benefit of switching around mainActions.

Comment 15

[Dave Garrett]

Comment on attachment 8595442 [details] [diff] [review]
poc - insecurepass-04-21-15.patch

>diff --git a/toolkit/locales/en-US/chrome/passwordmgr/passwordmgr.properties b/toolkit/locales/en-US/chrome/passwordmgr/passwordmgr.properties
>--- a/toolkit/locales/en-US/chrome/passwordmgr/passwordmgr.properties
>+++ b/toolkit/locales/en-US/chrome/passwordmgr/passwordmgr.properties
>@@ -43,3 +43,8 @@
> removeAllPasswordsTitle=Remove all passwords
> loginsSpielAll=Passwords for the following sites are stored on your computer:
> loginsSpielFiltered=The following passwords match your search:
>+insecurePasswordMsg=Passwords entered on HTTP pages are easily exposed to attackers.
>+insecurePasswordMsgOKButtonText=Got it
>+insecurePasswordMsgOKAccessKey=g
>+trySecurePageButtonText=Try https version of the page
>+trySecurePageButtonAccessKey=t

I'm aware that this is only at the POC stage, but I have to point out that this current text will not be understood by most people. (especially because people usually assume "attackers" mean active, not passive) It needs to be way more blunt. e.g.:

"This page attempts no security. Passwords entered on HTTP pages are easily stolen by third parties."

Comment 16

[Tanvi Vyas[:tanvi]]

(In reply to Dave Garrett from comment #15)
> I'm aware that this is only at the POC stage, but I have to point out that
> this current text will not be understood by most people. (especially because
> people usually assume "attackers" mean active, not passive) It needs to be
> way more blunt. e.g.:
> 
> "This page attempts no security. Passwords entered on HTTP pages are easily
> stolen by third parties."

Yes; the text is only a placeholder right now.  It needs lots of changes.  Thank you for your suggestion!

Comment 17

[Ash Grigas]

Tanvi - can Javaun's team take the UX piece of this - we want to give feedback on the password field itself since its more actionable than notifying before a user interacts with a password field. The same content would exist however we're not sure we can reliably offer a secure version if we don't know if one exists. Don't want to do duplicate work and with the new control center design, the password icon will be integrated into the control center most of the time.

Comment 18

[Ian Macdonald]

There is a saying, that a good worker never uses a spanner as a hammer. 

SSL is not the right tool for protecting passwords from misappropriation. Hashing is the correct approach. Like hitting a nail with a spanner it may prove partially effective, but encouraging the improper use of tools is almost always a bad idea. 

As a Web app coder, I see a problem here in that if browsers keep telling users that a site is insecure because the app has been installed on a non-SSL host, then that damages our reputation. Because, the site visitor will most likely not appreciate the reason for this situation and will badmouth the app itself, not the person who deployed it. 

A simple alternative might be to check for unscripted password submission -in other words a plain Submit button and no scripting in the form action URL- since that will almost always imply that the password is not being hashed. Of course, scripting of the submit action does not necessarily imply password hashing, but at least this check will trap the worst cases.

Comment 19

[Tim Taubert [:ttaubert] (inactive)]

Yes, TLS is the right tool to protect users when they enter a password into a form on some web page. Hashing the password before sending it to the server won't do anything good if the submission page is HTTP and can simply be MITM'ed by anyone between the user and the server. As soon as the user enters the password an attacker can simply inject a script that record key strokes or do something similar. That's why we not only have to ensure it is submitted *to* a secure site but also submitted *by* a secure page.

Comment 20

[Ash Grigas]

Attachment: mvp v1.png

MVP release version with new icon (pending final visual design) and control center panel feedback if password field exists on an http page.

Comment 22

[Mark Doliner]

[e-mail]: Minor bug in mvp v1.png: "You're login could be compromised" should be "Your ..."

Comment 23

[miketosh]

If you resolve a bug (bug 261294) marked as [ Importance: P1 enhancement || Rank: 4 ] as a duplicate of another (this), is it standard practice at Mozilla to carry that importance and rank over?

Comment 24

[Ian Macdonald]

Tim, I don't really understand the heavy emphasis on MITM attacks. How prevalent are they? Do you have any stats for that? 

The security reports I read show a depressingly repeating pattern of SQL code injections, cross-site scripting, buffer overflow exploits etc. These are all situations where failure to protect the password in disk or RAM storage is the key concern. TLS is of no help in such cases, because it automatically decrypts the password on arrival. 

The other issue I see with forcing the use of https on general-purpose sites (as opposed to banking sites etc where it should always be used) is that forums and the like will typically want to host third-party adverts for revenue. Since these ads are not https (and are not from the same domain covered by the certificate anyway) you will have mixed-content insecurity warnings popping-up if https is used. How does the webmaster work around that one?

Comment 25

[Tanvi Vyas[:tanvi]]

(In reply to Ian Macdonald from comment #24)
> The other issue I see with forcing the use of https on general-purpose sites
> (as opposed to banking sites etc where it should always be used) is that
> forums and the like will typically want to host third-party adverts for
> revenue. Since these ads are not https (and are not from the same domain
> covered by the certificate anyway) you will have mixed-content insecurity
> warnings popping-up if https is used. How does the webmaster work around
> that one?

They should at least host their login form on an https site that either doesn't have ads or uses SSL ads.  Even if logging into that site doesn't really leak any personal information, many users use the same username and password for their email, banking, social networks, etc.  So exposing credentials on one site over HTTP, exposes credentials for all sites.

cross-site scripting, buffer overflows, etc are all security issues that need to be resolved.  That doesn't mean we shouldn't protect against cleartext passwords flowing through the internet for anyone to come by and read.  TLS is to ensure transport security and protect against this specific threat.

Comment 26

[Ian Macdonald]

"..many users use the same username and password for their email, banking, social networks, etc." Which is exactly the reason that a salted hash should be used. With only SSL, the password can be read in plaintext from the server, and freely used on other sites. It is extremely unlikely that a salted hash of a password will work on other sites, even if the same plaintext password was used. 

In any event the key question is, how prevalent are MITM attacks in relation to the other classes of vuln? There is plenty of verifiable evidence that SQL code injections are extremely prevalent. Do MITM attacks constitute a similar level of threat, or not? I think we need some sort of concrete figures on this one. Without such figures we may be simply chasing a straw man and ignoring the real issue.

Comment 27

[soft]

Related but not duplicate: bug 1185145 Firefox should warn if using HTTP basic auth without TLS

Comment 28

[Marco Mucci [:MarcoM]]

Tracking in [fxprivacy]

Comment 29

[Tanvi Vyas[:tanvi]]

https://bugzilla.mozilla.org/show_bug.cgi?id=1179961 has landed to use degraded security UI when a password field is present on an HTTP page.

https://bugzilla.mozilla.org/show_bug.cgi?id=1217162 has been filed to implement contextual feedback to password fields (as designed in https://bugzilla.mozilla.org/show_bug.cgi?id=1217150 / https://invis.io/8A4NGNKQK.

In this bug, I was using the key doorhanger for password manager.  If we are going to degrade the security UI and give contextual info in the password field dropdown, then we don't need to use the key icon at all.

Hence, this might now be a duplicate of https://bugzilla.mozilla.org/show_bug.cgi?id=1217162.  But not marking it yet, want to make sure 1217162 is the way we are planning to go forward here.

Comment 30

[Kaartic Sivaraam]

I guess the message shown has a small issue as described in bug 1342414

Comment 31

[Jonathan Kingston [:jkt] he/him]

This to me looks like it has been resolved by Bug 1217142 dependencies. Please reopen if this isn't the case.