Closed Bug 1214333 Opened 4 years ago Closed 4 years ago

Signal to users when they are on an insecure page with a password field

Categories

(Firefox for Android :: General, defect)

ARM
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 44
Tracking Status
firefox44 --- verified

People

(Reporter: liuche, Assigned: liuche)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

This is the mobile corollary to bug 1179961, which adds UX to notify users when they are on a page that has a password field, but is not encrypted (and therefore vulnerable to being man-in-the-middled or having their passwords sniffed).
Blocks: 1214343
It would be really great to try to ship this in the same release as desktop. Chenxia, do you think that it would be possible to land something this week to get this in 44?
Assignee: nobody → liuche
Flags: needinfo?(liuche)
Okay, I'll put a patch together for this.

Anthony, I'll make this the same as our "disabled active mixed content blocking" UI, from bug 1177576.
https://bug1177576.bmoattachments.org/attachment.cgi?id=8631147
Flags: needinfo?(liuche) → needinfo?(alam)
(In reply to Chenxia Liu [:liuche] from comment #2)
> Okay, I'll put a patch together for this.
> 
> Anthony, I'll make this the same as our "disabled active mixed content
> blocking" UI, from bug 1177576.
> https://bug1177576.bmoattachments.org/attachment.cgi?id=8631147

Sure! let me know when you have a screenshot!
Flags: needinfo?(alam) → needinfo?(liuche)
We could add a "Learn more" link too.

Strings look okay? Anything else?

Note, this needs a little bit more work for when it's firing, but that's what the UI I planned on putting in looks like.
Flags: needinfo?(liuche)
Attachment #8679636 - Flags: feedback?(alam)
Comment on attachment 8679636 [details]
Screenshot: Insecure password doorhanger + site identity icon

Looking good! This was definitely the UI I had in mind.

But, is the red "x" missing from the left of the "Insecure connection" here?

Happy to let others chime in about the body copy inside the doorhanger but I wonder if we should say something more akin to "This page has some insecure elements." Or is that too non-specific?

I'm also aware of the fact that we might need to get others like Matej and Legal to review some of the words here.

Thoughts?
Flags: needinfo?(matej)
Attachment #8679636 - Flags: feedback?(alam) → feedback+
(In reply to Anthony Lam (:antlam) from comment #5)
> Comment on attachment 8679636 [details]
> Screenshot: Insecure password doorhanger + site identity icon
> 
> Looking good! This was definitely the UI I had in mind.
> 
> But, is the red "x" missing from the left of the "Insecure connection" here?
> 
> Happy to let others chime in about the body copy inside the doorhanger but I
> wonder if we should say something more akin to "This page has some insecure
> elements." Or is that too non-specific?

I don't think "elements" is relevant in this situation. It's the connection (HTTP vs HTTPS). Desktop uses:

Connection is Not Secure
Your login could be compromised

Looks like Mobile uses "Insecure connection" in other mixed content situations, so it makes sense to use it here too.
(In reply to Mark Finkle (:mfinkle) from comment #6)
> (In reply to Anthony Lam (:antlam) from comment #5)
> > Comment on attachment 8679636 [details]
> > Screenshot: Insecure password doorhanger + site identity icon
> > 
> > Looking good! This was definitely the UI I had in mind.
> > 
> > But, is the red "x" missing from the left of the "Insecure connection" here?
> > 
> > Happy to let others chime in about the body copy inside the doorhanger but I
> > wonder if we should say something more akin to "This page has some insecure
> > elements." Or is that too non-specific?
> 
> I don't think "elements" is relevant in this situation. It's the connection
> (HTTP vs HTTPS). Desktop uses:
> 
> Connection is Not Secure
> Your login could be compromised
> 
> Looks like Mobile uses "Insecure connection" in other mixed content
> situations, so it makes sense to use it here too.

I prefer "not secure" to "insecure" for things like this, but if "insecure" is consistent across mobile, I'm happy to stick with it.

The part that isn't clear to me is "your login could be compromised." Could we specify what could be compromised, like data, password, etc.?
Flags: needinfo?(matej)
(In reply to Matej Novak [:matej] from comment #7)

> The part that isn't clear to me is "your login could be compromised." Could
> we specify what could be compromised, like data, password, etc.?

I believe we are concerned about the password being transmitted in plaintext. From bug 1179961 comment 0:
"HTTP websites have no business asking for passwords.  The passwords can be intercepted by man-in-the-middle attackers and eavesdroppers.  Given that password reuse is very prevalent, when an HTTP website asks for a password, it is compromising the users security on its site and any other site where the user shares the same password."
Desktop has two levels of this in the Control Center:

https://bugzilla.mozilla.org/attachment.cgi?id=8662392 (high level)
https://bugzilla.mozilla.org/attachment.cgi?id=8662393 (detail view)

The detail view does offer a "More Information", which Mobile calls "Learn more"
(In reply to Mark Finkle (:mfinkle) from comment #8)
> (In reply to Matej Novak [:matej] from comment #7)
> 
> > The part that isn't clear to me is "your login could be compromised." Could
> > we specify what could be compromised, like data, password, etc.?
> 
> I believe we are concerned about the password being transmitted in
> plaintext. From bug 1179961 comment 0:
> "HTTP websites have no business asking for passwords.  The passwords can be
> intercepted by man-in-the-middle attackers and eavesdroppers.  Given that
> password reuse is very prevalent, when an HTTP website asks for a password,
> it is compromising the users security on its site and any other site where
> the user shares the same password."

Could we say something like "Your password is vulnerable"? As is, it sounds pretty technical and therefore not as immediately concerning. It would be good to let the user know what's at stake.
> Could we say something like "Your password is vulnerable"? As is, it sounds
> pretty technical and therefore not as immediately concerning. It would be
> good to let the user know what's at stake.

One more piece of information - when the doorhanger is up, it's because the user has clicked on the icon and is summoning it, presumably to check what this warning is. They haven't necessarily entered a password yet, so I want to make sure that they aren't confused (or get worried) which "password" is vulnerable (because they may not have typed it in yet). The idea I'm trying to convey is "Passwords and logins that you submit on this site may be intercepted by people eavesdropping on this network."
Flags: needinfo?(matej)
Bug 1214333 - Signal to users when they are on an insecure page with a password field. r=sebastian
Attachment #8679773 - Flags: review?(s.kaspari)
Sebastian, this approach is a little different from what Desktop does in bug 1179961 because Desktop has access to the state of site identity when it receives the "password insecure" notification, and we don't store that state in Gecko, but pass it along to Java to handle/store. So I'm just adding a listener for the password-insecure notification, and passing that message along to Java.

There is a small race condition that I notice if you go to a password-insecure website right after starting up, but I'd like to get these strings landed in 44. I'll investigate the race tomorrow.

Tanvi's test site (obviously don't use https): people.mozilla.org/~tvyas/password/frame_password.html

(Pending any additional string changes.)

Anthony, I forgot but remembered that the "More information" link on Desktop just opens up the Site Information dialog - we don't really have that, so I'm going to leave that link off.
Status: NEW → ASSIGNED

(In reply to Chenxia Liu [:liuche] from comment #11)
> > Could we say something like "Your password is vulnerable"? As is, it sounds
> > pretty technical and therefore not as immediately concerning. It would be
> > good to let the user know what's at stake.
> 
> One more piece of information - when the doorhanger is up, it's because the
> user has clicked on the icon and is summoning it, presumably to check what
> this warning is. They haven't necessarily entered a password yet, so I want
> to make sure that they aren't confused (or get worried) which "password" is
> vulnerable (because they may not have typed it in yet). The idea I'm trying
> to convey is "Passwords and logins that you submit on this site may be
> intercepted by people eavesdropping on this network."

The problem for me is that I'm not sure what "login" encompasses and what about it is vulnerable. Does it mean that only my username and password can be seen by someone else? Or that the connection isn't secure and anything I share could be collected? I think it would help the user if we specify as much as we can. As it is, it feels so broad that it doesn't actually sound that concerning. It's just a blanket, catchall statement.
Flags: needinfo?(matej)
That's a good point, I think we should try to be explicit to the user, and make our copy apply better to the user's understanding of what's going on.

To answer your questions, the problem is that the connection isn't secure. Therefore, anything you send could be collected. Usually people don't care so much so we just don't display a green lock and people can continue doing whatever they were doing, but for the specific case where people could be submitting login credentials to this page, we want to emphasize "HEY this connection isn't secure". I guess in the short run it's not very actionable, but this will at least put some pressure on websites to use https if they're using logins (because using a non-secure http connection for logins is bad practice).
Comment on attachment 8679773 [details]
MozReview Request: Bug 1214333 - Signal to users when they are on an insecure page with a password field. r=margaret

I forgot sebastian is as Droidcon, so flagging Margaret for review in case she has time :)
Attachment #8679773 - Flags: review?(margaret.leibovic)
Attachment #8679773 - Flags: review?(margaret.leibovic) → review+
Comment on attachment 8679773 [details]
MozReview Request: Bug 1214333 - Signal to users when they are on an insecure page with a password field. r=margaret

https://reviewboard.mozilla.org/r/23531/#review21085

I have a few comments, but this looks fine overall (assuming you get the strings sorted out).

::: mobile/android/base/toolbar/SiteIdentityPopup.java:321
(Diff revision 1)
> +            } else if (siteIdentity.getMixedModeActive() == MixedMode.MIXED_CONTENT_LOADED) {

Just want to double check: is this the same order that desktop prioritizes these icons? That is, do they prioritize showing this login insecure icon over the mixed content icons?

I think it makes sense to me, since a potentially vulnerable password is more worrisome that mixed content.

::: mobile/android/base/toolbar/ToolbarDisplayLayout.java:514
(Diff revision 1)
>              imageLevel = LEVEL_LOCK_DISABLED;

Not related to this patch, but on a page with tracking protection enabled and mixed content disabled, we still show a shield? Is that right? Or should we be showing the LEVEL_LOCK_DISABLED in this case?

::: mobile/android/chrome/content/browser.js:3635
(Diff revision 1)
> +    this.browser.addEventListener("InsecureLoginFormsStateChange", this, true);

Should we be adding this listener for each browser, or can we add this to the deck instead? I see on desktop it was added to `gBrowser`, which is basically the desktop version of our deck, in that contains all the individual browser elements:
https://hg.mozilla.org/mozilla-central/rev/672eab895605#l1.32

Just want to minimize memory overhead where possible. This is actually an issue that probably affects these existing listeners as well.
Matej, to be consistent, I used login instead of password, and I added some context to why this could be vulnerable. With merge moving up to tomorrow, I'll probably land strings today. Any thoughts on this, objections? And if it's really bad, we can always change it :) Thanks!
Attachment #8680281 - Flags: feedback?(matej)
https://reviewboard.mozilla.org/r/23531/#review21105

::: mobile/android/base/toolbar/SiteIdentityPopup.java:321
(Diff revision 1)
> +            } else if (siteIdentity.getMixedModeActive() == MixedMode.MIXED_CONTENT_LOADED) {

This isn't actually a conflict because the insecure password field notification only shows up when we're on http, and the mixed content notifications only show up when we're on https and there is insecure content.

(They're in the same block because in all these cases, the page is "insecure", either because the page is served over http, or because the page is served over https and there is http/insecure content.)

::: mobile/android/base/toolbar/ToolbarDisplayLayout.java:514
(Diff revision 1)
>              imageLevel = LEVEL_LOCK_DISABLED;

Yep, we show the shield icon instead of the crossed out lock icon, although the crossed out lock icon does show up in the doorhanger (tracking protection has a separate doorhanger).

::: mobile/android/chrome/content/browser.js:3635
(Diff revision 1)
> +    this.browser.addEventListener("InsecureLoginFormsStateChange", this, true);

Okay, that's a good point. We can just add the one listener, although I wasn't sure how to pass the browser and tabID in, so in my fix for this, I make a call to BrowserApp.selectedBrowser and BrowserApp.selectedTab. Hopefully that doesn't introduce race conditions :/

And yeah, I just added the listener there because it seemed convenient! A lot of these were probably added there because it just looks like where all the other listeners are...
Comment on attachment 8679773 [details]
MozReview Request: Bug 1214333 - Signal to users when they are on an insecure page with a password field. r=margaret

Bug 1214333 - Signal to users when they are on an insecure page with a password field. r=margaret
Attachment #8679773 - Attachment description: MozReview Request: Bug 1214333 - Signal to users when they are on an insecure page with a password field. r=sebastian → MozReview Request: Bug 1214333 - Signal to users when they are on an insecure page with a password field. r=margaret
Attachment #8679773 - Flags: review?(s.kaspari)
https://hg.mozilla.org/mozilla-central/rev/be3b67884f6e
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 44
(In reply to Chenxia Liu [:liuche] from comment #18)
> Created attachment 8680281 [details]
> Screenshot: Vulnerable login string
> 
> Matej, to be consistent, I used login instead of password, and I added some
> context to why this could be vulnerable. With merge moving up to tomorrow,
> I'll probably land strings today. Any thoughts on this, objections? And if
> it's really bad, we can always change it :) Thanks!

Is the page itself not secure or is it your connection to the page?

I also still find it really vague. I wish we could say something like "Your connection to this page is not secure and information you share may be seen by someone else" — though that's probably too long.

The problem for me is that a "vulnerable login" sounds like a technical problem, like it's a matter of stability or how strong my connection is. It doesn't automatically tell me that my information is at risk.
Attachment #8680281 - Flags: feedback?(matej)
Given that we use the same string (or similar) as Desktop, we could open a new bug to change the string everywhere.
Blocks: 1219828
Verified as fixed in build 44.0a2 2015-11-02;
Device: Motorola Razr (Android 4.4.4).
Depends on: 1220698
Depends on: 1221113
Depends on: 1221711
Depends on: 1272348
You need to log in before you can comment on or make changes to this bug.