1330228 - Expose a chrome-only API indicating whether an <input> was ever type=password

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement, P3)

Description

[Matthew N. [:MattN]]

Since we don't have bug 502258 implemented, sites implement their own password visibility toggles by toggling <input>s between type=password and type=text. By toggling the type, the author loses security/privacy benefits of type=password and this has caused a few types of data leak bugs:
A) Values in type=text fields are written to disk by session restore on an interval. about:accounts/FxA was vulnerable to this in Bug 1280294 (CVE-2016-5260)
B) Values in type=text fields will be saved to form history (which isn't encrypted either)

There are also password manager bugs in dealing with password fields that have switched to non-password fields.

Session store, form history, and password manager could all switch to a (chrome-only) API which indicates whether a field ever was a password field instead of looking only at the current type and it would fix the above three issues.

(Ideally we would just implement bug 502258 and sites would stop toggling the input type in Firefox using some kind of feature or UA detection and the above problems would go away as I don't know other valid/common use cases for toggling from type=password to another type.)

Comment 1

[Andrew Overholt [:overholt]]

Matt, I feel like we're not going to get to this any time soon but I'm open to you convincing me otherwise :) For now, I'm marking this as P3 (aka backlog).

Comment 2

[Andrew Overholt [:overholt]]

IIRC, Nika and Olli discussed adding the chrome-only attribute somewhere near https://searchfox.org/mozilla-central/rev/ce57be88b8aa2ad03ace1b9684cd6c361be5109f/toolkit/components/sessionstore/nsSessionStoreUtils.cpp#58 and then filtering it out ("was-ever-password" or whatever) here: https://searchfox.org/mozilla-central/rev/ce57be88b8aa2ad03ace1b9684cd6c361be5109f/toolkit/modules/sessionstore/FormData.jsm#42

I'm sure they'd be willing to mentor someone who wanted to try this (it shouldn't be too difficult).

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Attachment: input_type_password.diff

this would add the ChromeOnly flag to input elements.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

overholt, could you ask feedback from whoever was asking about this bug earlier today?

Comment 5

[Andrew Overholt [:overholt]]

Ryan, we were talking about this yesterday. Do you know anyone working with you in this area who could see if Olli's patch here works for them?

Comment 6

[Ryan Feeley [:rfeeley]]

I’m not sure what would be required for our team to test this but have NIed Ryan Kelly who is familiar with the original show password session restore bug. Ryan, we have a patch to test! How to proceed?

Comment 7

[Ryan Kelly [:rfkelly]]

Delegating to Shane, I expect he'll have more context on the details than I do.

Comment 8

[Shane Tomlinson [:stomlinson]]

(In reply to Andrew Overholt [:overholt] from comment #5)
> Ryan, we were talking about this yesterday. Do you know anyone working with
> you in this area who could see if Olli's patch here works for them?

I am happy to test, though TBH, I'm not sure how and what to look for. FxA 
has had the autocomplete=off attribute since before the Firefox fix in [1], 
FxA's password field value hasn't been saved into sessionstore.js since that
fix went live.

I could create a test page that has an input[type=password] and toggle that to
[type=text], but since this API is chrome only, the test page won't be able to
access hasBeenTypePassword. I could also hack the idl to remove [ChromeOnly],
but that's starting to tread a fine line.

I'll try to create a test page and look in sessionstore.js, though like I said,
I'm not really sure of what the previous STRs were and what I should be looking 
for other than the password from the field.

Comment 9

[Andrew Overholt [:overholt]]

(In reply to Shane Tomlinson [:stomlinson] from comment #8)
> (In reply to Andrew Overholt [:overholt] from comment #5)
> > Ryan, we were talking about this yesterday. Do you know anyone working with
> > you in this area who could see if Olli's patch here works for them?
> 
> I am happy to test, though TBH, I'm not sure how and what to look for.

I just spoke f2f with Ryan and I think we're just confused :) What Olli's patch does is add an attribute to <input> elements to indicate if it was ever type=password. What we need to do on top of this is:

(In reply to Andrew Overholt [:overholt] from comment #2)
> [filter] it out ("was-ever-password" or whatever) here:
> https://searchfox.org/mozilla-central/rev/
> ce57be88b8aa2ad03ace1b9684cd6c361be5109f/toolkit/modules/sessionstore/
> FormData.jsm#42

in sessionstore. Mike, do you know who could do the latter piece?

Comment 10

[Mike de Boer [:mikedeboer]]

That'd be me or MattN, he's quite familiar with FormData.jsm!

Comment 11

[Shane Tomlinson [:stomlinson]]

(In reply to Andrew Overholt [:overholt] from comment #9)
> (In reply to Shane Tomlinson [:stomlinson] from comment #8)
> > (In reply to Andrew Overholt [:overholt] from comment #5)
> > > Ryan, we were talking about this yesterday. Do you know anyone working with
> > > you in this area who could see if Olli's patch here works for them?
> > 
> > I am happy to test, though TBH, I'm not sure how and what to look for.
> 
> I just spoke f2f with Ryan and I think we're just confused :) What Olli's
> patch does is add an attribute to <input> elements to indicate if it was
> ever type=password. What we need to do on top of this is:
> 

FWIW, I have applied the patch cleanly and been using it for the
past hour to test other development in FxA. I haven't seen the
password show up in in sessionstore-backups/*.

I also applied a modified patch where I removed the [ChromeOnly]
tag on "hasBeenTypePassword" to ensure the attribute is only
set to true when expected. For password fields that start
as password fields, the value is always true. For password
fields that start off as text fields, "hasBeenTypePassword"
starts as false and only goes to true once the element has
been converted to be a text field.

Comment 12

[Mike de Boer [:mikedeboer]]

(In reply to Shane Tomlinson [:stomlinson] from comment #11)
> FWIW, I have applied the patch cleanly and been using it for the
> past hour to test other development in FxA. I haven't seen the
> password show up in in sessionstore-backups/*.

Shane, I'd recommend looking at 'sessionstore.jsonlz4', because that where the most recent state gets written to. It lives in the profile directory's root dir.
(I'd expect the password to still be there, once the sessionstore.jsonlz4 has been flushed to disk.)

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

So, Shane or anyone, do we want to take that patch?

Comment 14

[Shane Tomlinson [:stomlinson]]

(In reply to Olli Pettay [:smaug] from comment #13)
> So, Shane or anyone, do we want to take that patch?

Hi Olli and Mike,

I have put some more testing into this patch and have been unable to get the password written to disk
if the password element starts off as a password element.

Using a password of "testuser", I ran this from the profile's directory


> while true; do
> echo `date`; grep "testuser" *; echo ""; sleep 1
> done

If I modify the page to start with a text element
that is converted to a password element sometime
later, then the password is written to disk
before the element is converted to be of type=password,
but no longer written once that conversion occurs.

Comment 15

[Matthew N. [:MattN]]

Comment on attachment 9012647 [details] [diff] [review]
input_type_password.diff

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

Thanks Olli!

(In reply to Shane Tomlinson [:stomlinson] from comment #14)
> If I modify the page to start with a text element
> that is converted to a password element sometime
> later, then the password is written to disk
> before the element is converted to be of type=password,
> but no longer written once that conversion occurs.

The current patch isn't changing any bfcache or session restore behaviour so testing the patch alone isn't going to be useful.

Yes, I want this patch but we also need bfcache[1], session restore[2], satchel[3], and potentially other places[4] to use it instead of the type=password checks they do. Olli, do you want to make those changes in this bug or a separate one? Fixing [1] and other DOM code here would be my preference. We can file follow-ups to fix the non-DOM components that save form data.

[1] https://dxr.mozilla.org/mozilla-central/rev/c291143e24019097d087f9307e59b49facaf90cb/dom/base/nsDocument.cpp#7900
[2] Like https://dxr.mozilla.org/mozilla-central/rev/c291143e24019097d087f9307e59b49facaf90cb/toolkit/modules/sessionstore/FormData.jsm#164-169
[3] https://dxr.mozilla.org/mozilla-central/rev/c291143e24019097d087f9307e59b49facaf90cb/toolkit/components/satchel/formSubmitListener.js#82
[4] Maybe https://dxr.mozilla.org/mozilla-central/rev/c291143e24019097d087f9307e59b49facaf90cb/dom/html/nsTextEditorState.cpp#1533-1541

::: dom/webidl/HTMLInputElement.webidl
@@ +283,5 @@
>    [ChromeOnly]
>    attribute DOMString previewValue;
> +
> +  [ChromeOnly]
> +  readonly attribute boolean hasBeenTypePassword;

Nit: Could you move this beside `mozIsTextField`[1] higher up in this file since I think many consumers of that API with it's `excludePassword` argument would be interested in this attribute. (I guess you could also move `previewValue` at the same time since I don't think there's a good reason for it to be separate from the other "Mozilla extensions".

[1] https://dxr.mozilla.org/mozilla-central/rev/c291143e24019097d087f9307e59b49facaf90cb/dom/webidl/HTMLInputElement.webidl#182

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

Yeah, I could do [1] + the existing patch. It is unclear to me why we'd want [4].

Comment 17

[Matthew N. [:MattN]]

(In reply to Olli Pettay [:smaug] from comment #16)
> Yeah, I could do [1] + the existing patch.

Great, thanks!

> It is unclear to me why we'd want [4].

Yeah, I was on the fence about that and didn't understand the consequences of "Since changing the control type does a reframe, we don't have to worry about dynamic type changes here.". I was thinking of an edge case where a user could reveal a blanked password field by switching to a type="text" and then undoing. If the comment is implying that the switch will lose the undo history then that's fine.

Comment 19

[Matthew N. [:MattN]]

I'll take this.

Comment 20

[Matthew N. [:MattN]]

Attachment: Bug 1330228 - Expose a chrome-only API indicating whether an <input> was ever type=password r=baku

Cache whether an input was ever type=password so consumers can handle sites which toggle password visibility and don't want to save the value of the field.

Comment 21

[Matthew N. [:MattN]]

Attachment: Bug 1330228 - Use HasBeenTypePassword to skip saving to presentation state. r=baku,smaug

Depends on D15147

Comment 22

[Matthew N. [:MattN]]

Attachment: Bug 1330228 - Use HasBeenTypePassword when sanitizing a document for bfcache. r=baku,smaug

Check whether an input was ever type=password rather than just checking the current type to handle sites which toggle password visibility.

I haven't yet figured out what this code is used for so haven't made a test yet.

Depends on D15148

Comment 23

[Matthew N. [:MattN]]

Attachment: Bug 1330228 - Use hasBeenTypePassword in FormData.jsm to not save former password fields. r=Gijs,felipe

Depends on D15149

Comment 24

[Matthew N. [:MattN]]

Attachment: Bug 1330228 - Use hasBeenTypePassword in formSubmitListener.js to not save former password fields. r=Gijs,felipe

Depends on D15208

Comment 25

[Matthew N. [:MattN]]

Comment on attachment 9012647 [details] [diff] [review]
input_type_password.diff

Replaced by attachment 9032804 [details]

Comment 26

[Matthew N. [:MattN]]

Attachment: Bug 1330228 - Test input.hasBeenTypePassword. r=baku,smaug

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/47b9ca8804e5
Expose a chrome-only API indicating whether an <input> was ever type=password r=baku
https://hg.mozilla.org/integration/autoland/rev/df443391edda
Test input.hasBeenTypePassword. r=baku
https://hg.mozilla.org/integration/autoland/rev/8420eec6f0cc
Use HasBeenTypePassword to skip saving to presentation state. r=baku
https://hg.mozilla.org/integration/autoland/rev/fa89cf35d16f
Use HasBeenTypePassword when sanitizing a document for bfcache. r=baku
https://hg.mozilla.org/integration/autoland/rev/443b0e20be22
Use hasBeenTypePassword in FormData.jsm to not save former password fields. r=Felipe
https://hg.mozilla.org/integration/autoland/rev/3a8931f714f3
Use hasBeenTypePassword in formSubmitListener.js to not save former password fields. r=Felipe

Comment 28

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/47b9ca8804e5
https://hg.mozilla.org/mozilla-central/rev/df443391edda
https://hg.mozilla.org/mozilla-central/rev/8420eec6f0cc
https://hg.mozilla.org/mozilla-central/rev/fa89cf35d16f
https://hg.mozilla.org/mozilla-central/rev/443b0e20be22
https://hg.mozilla.org/mozilla-central/rev/3a8931f714f3