530367 - Password Number and Letter combos flash in clear text when password manager fills in password field

Status: RESOLVED FIXED

(Toolkit :: Password Manager, defect, P1)

Description

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch

+++ This bug was initially created as a clone of Bug #526880 +++

spinning out as a new bug for the password flashing as clear text.

Comment 1

[Mike Beltzner [:beltzner, not reading bugmail]]

Sorry, from reading bug 526880 I can't figure out why this is nominated to block? Can someone explain the issue and product decision?

Comment 2

[Mike Beltzner [:beltzner, not reading bugmail]]

Gavin explained it: the plaintext password shows up for a long enough period as to expose it. Maybe not a problem on Fennec, but definitely a problem on WinCE where the screen is a touch bigger and more computer like.

Comment 3

[Brad Lassey [:blassey] (use needinfo?)]

When you load a page and the password manager fills in your passwords, they are clear text for 6/10s of a second before being replaced by asterisks. So anyone with access to your device can get your passwords from the password manager by loading the pages in the browser, which is not the desired behavior.

Comment 4

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to comment #2)
> Maybe not a problem on Fennec, but definitely a problem on WinCE

still a problem for Fennec since the danger isn't in someone snooping over your shoulder, but instead picking up your device.

Comment 5

[[e-mail]]

(Paraphrased from bug 526880 comment #11)
The code for password manager filling in the password field goes through nsTextControlFrame.cpp which already has to modify the editor flags to deal with various edge cases, so I think it might be better to move the fix there.

Comment 6

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch v.2

Comment 7

[[e-mail]]

Comment on attachment 414449 [details] [diff] [review]
patch v.2

>+, mTimer(nsnull)
nsCOMPtr defaults to nsnull anyway, no?

Comment 8

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to comment #7)
> (From update of attachment 414449 [details] [diff] [review])
> >+, mTimer(nsnull)
> nsCOMPtr defaults to nsnull anyway, no?

I believe so, I guess that change isn't needed.

Comment 9

[Brad Lassey [:blassey] (use needinfo?)]

pushed http://hg.mozilla.org/mozilla-central/rev/2e8553f076f8

Comment 10

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 414449 [details] [diff] [review]
patch v.2

I'm asking for approval even though this is already blocking because this has an interface change.  I propose removing eEditorDontEchoPassword from nsIPlaintextEditor and replacing it with a comment saying 0x1000 is being used to stop password echoing. and just using a hard coded 0x1000 in nsTextEditRules.cpp and nsTextControlFrame.cpp. Its a bit hacky, but avoids having to create a nsIPlaintextEditor_BRANCH_1_9_2.

Comment 11

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

Adding a constant to an interface isn't an incompatible change, so there's no need to worry about it - you can just land as is.

Comment 13

[Brad Lassey [:blassey] (use needinfo?)]

pushed http://hg.mozilla.org/releases/mozilla-1.9.2/rev/3e0eb9af74bd