Closed Bug 1548389 Opened 5 years ago Closed 5 years ago

Add an API to allow unmasking password fields

Categories

(Core :: DOM: Editor, enhancement, P1)

Product:
Core
Component:
DOM: Editor
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla70
Tracking Flags:
Tracking Status
firefox70 --- fixed

People

(Reporter: MattN, Assigned: masayuki)

References

(Blocks 1 open bug)

Details

(Keywords: parity-chrome, Whiteboard: [passwords:generation] [skyline])

User Story

* Handle bfcache to not persist the unhiding after navigation since the `value` isn't persisted in bfcache currently.

Attachments

(11 files, 2 obsolete files)

Bug 1548389 - part 0: Wrap modifying text node in editor with particular methods
47 bytes, text/x-phabricator-request
Details | Review
Bug 1548389 - part 1: Implement API to get/set unmask-range of password editor
47 bytes, text/x-phabricator-request
Details | Review
Bug 1548389 - part 2: Make `TextEditRules` use the new editor API to mask/unmask characters in password editor
47 bytes, text/x-phabricator-request
Details | Review
Bug 1548389 - part 3: Make editor mark text node in password field as "maybe masked"
47 bytes, text/x-phabricator-request
Details | Review
Bug 1548389 - part 4: Implement `nsContentUtils::GetTextEditorFromAnonymousNodeWithoutCreation()` to retrieve `TextEditor` instance from an anonymous node in `HTMLInputElement` or `HTMLTextAreaElement`
47 bytes, text/x-phabricator-request
Details | Review
Bug 1548389 - part 5: Make `nsTextFrame` and related code treat masking password characters as text-transform
47 bytes, text/x-phabricator-request
Details | Review
Bug 1548389 - part 6: Add automated tests for new API and rendering of password fields
47 bytes, text/x-phabricator-request
Details | Review
Bug 1548389 - part 7: Drop supporting dragging text from `<input type="password">`
47 bytes, text/x-phabricator-request
Details | Review
Bug 1548389 - part 8: Make `nsTextFrame::PeekOffsetWord()` scan masked text
47 bytes, text/x-phabricator-request
Details | Review
Bug 1548389 - part 9: Make ContentEventHandler return masked text
47 bytes, text/x-phabricator-request
Details | Review
Bug 1548389 - part 10: Make `TextEditor::SetUnmaskRangeInternal()` expand the range if specified offset is middle of a surrogate pair
47 bytes, text/x-phabricator-request
Details | Review

If we generate a password for the user, it may need editing in order to meet the website requirements so we should make it readable to the user to do so.

Relevant code in nsTextControlFrame::UpdateValueDisplay

Flags: qe-verify+

Cam and/or Masayuki, do either of you have a preference of how to implement this feature, I have some potential paths:

Flags: needinfo?(masayuki)
Flags: needinfo?(cam)

A reason against using input-security is that it would mean the page can observe that the computed value of the property has changed, when really this should be a non-observable side effect of using the password generation.

AddManuallyManagedStates would be preferred if we want to write some UA style sheet rules that match against those state bits. But it doesn't look like that is the case here.

So my preference is a ChromeOnly DOM API, which is probably simplest anyway.

Flags: needinfo?(cam)

Thanks Cameron!

(In reply to Cameron McCormack (:heycam) from comment #2)

A reason against using input-security is that it would mean the page can observe that the computed value of the property has changed, when really this should be a non-observable side effect of using the password generation.

Would that be true even if it's chrome-only? Would chrome-only properties appear in getComputedStyle? I'm not even sure that making this observable to the web is that bad.

AddManuallyManagedStates would be preferred if we want to write some UA style sheet rules that match against those state bits. But it doesn't look like that is the case here.

I gave this option in case we didn't want to tie the password field presentation more to the DOM and thus make it harder to implement input-security in the future.

So my preference is a ChromeOnly DOM API, which is probably simplest anyway.

OK, I'll try that approach unless Masayuki disagrees before I get to it.

Sorry for the delay to reply.

From the point of view of editor, it needs to update the text in anonymous <div> element in <input type="password">. So, I'm not sure how layout or style notifies editor of changing the style. On the other hand, if seems that web apps or extensions can get passwords with capturing the screenshot when it's shown. I'm not sure whether this is new security risk or not, though.

Flags: needinfo?(masayuki)

(In reply to Matthew N. [:MattN] (PM me if requests are blocking you) from comment #3)

Would that be true even if it's chrome-only? Would chrome-only properties appear in getComputedStyle? I'm not even sure that making this observable to the web is that bad.

If it's chrome-only then it wouldn't, but in a future where we do want to ship input-security, then the changes to the computed value of that property would be observable to the page. I didn't check the spec, but I don't think it would be expected that UA style rules could affect the value of the property at arbitrary times.

(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Might be slow response) from comment #4)

Sorry for the delay to reply.

From the point of view of editor, it needs to update the text in anonymous <div> element in <input type="password">. So, I'm not sure how layout or style notifies editor of changing the style.

Is this a comment on the first bullet from comment 1?

On the other hand, if seems that web apps or extensions can get passwords with capturing the screenshot when it's shown. I'm not sure whether this is new security risk or not, though.

Which APIs do you have in mind for web app access? Are you talking about webRTC screen sharing? Or canvas' drawWindow (which I believe wouldn't work from a web app, only extensions)?

Other than this potential leak, are you fine with the DOM API approach?

(In reply to Cameron McCormack (:heycam) from comment #5)

(In reply to Matthew N. [:MattN] (PM me if requests are blocking you) from comment #3)

Would that be true even if it's chrome-only? Would chrome-only properties appear in getComputedStyle? I'm not even sure that making this observable to the web is that bad.

If it's chrome-only then it wouldn't, but in a future where we do want to ship input-security, then the changes to the computed value of that property would be observable to the page. I didn't check the spec, but I don't think it would be expected that UA style rules could affect the value of the property at arbitrary times.

I'm not sure how that's so different than :-moz-autofill setting the filter property… in my quick test it's exposed to the web via getComputedStyle.

Assignee: nobody → MattN+bmo
Status: NEW → ASSIGNED

(In reply to Matthew N. [:MattN] (PM me if requests are blocking you) from comment #6)

(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Might be slow response) from comment #4)

Sorry for the delay to reply.

From the point of view of editor, it needs to update the text in anonymous <div> element in <input type="password">. So, I'm not sure how layout or style notifies editor of changing the style.

Is this a comment on the first bullet from comment 1?

Yes, it is, but I don't have any idea for which API is better to manage the state.

On the other hand, if seems that web apps or extensions can get passwords with capturing the screenshot when it's shown. I'm not sure whether this is new security risk or not, though.

Which APIs do you have in mind for web app access? Are you talking about webRTC screen sharing? Or canvas' drawWindow (which I believe wouldn't work from a web app, only extensions)?

Ah, I was thinking about drawWindow. I've not known that it's not available on web apps. I don't know about webRTC but I think that it does not matter since nobody would input passwords when the screen is shared.

Other than this potential leak, are you fine with the DOM API approach?

Yeah, from point of view of editor module, any API is okay. Finally, TextEditor should have it as public method.

Currently, I'm thinking that we should get rid of TextEditRules and HTMLEditRules. When do you want to fix this bug? At 69?

Flags: needinfo?(MattN+bmo)

As I was using Fennec on my phone just now, I realised that when I fill in a password from my password manager (Bitwarden), that it does show the password in the clear when it gets filled in, despite it being filled in to an <input type=password> field. Maybe there is an existing mechanism that you can re-use?

Yes, there is. On Android, we need to show the last character for a while, but it'll be hidden by a timer. However, currently, there is no way to enable it temporarily on desktop.

(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Might be slow response) from comment #7)

Currently, I'm thinking that we should get rid of TextEditRules and HTMLEditRules. When do you want to fix this bug? At 69?

Yes, I want to fix this bug for 69. I was planning to work on it starting today.

(In reply to Cameron McCormack (:heycam) from comment #8)

As I was using Fennec on my phone just now, I realised that when I fill in a password from my password manager (Bitwarden), that it does show the password in the clear when it gets filled in, despite it being filled in to an <input type=password> field. Maybe there is an existing mechanism that you can re-use?

Yeah, I already saw that code before requesting your feedback but it didn't seem useful as we want to reveal the whole password and not have a timer. It will give code hints of where I may need to change though.

Flags: needinfo?(MattN+bmo)

(In reply to Matthew N. [:MattN] (PM me if requests are blocking you) from comment #10)

(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Might be slow response) from comment #7)

Currently, I'm thinking that we should get rid of TextEditRules and HTMLEditRules. When do you want to fix this bug? At 69?

Yes, I want to fix this bug for 69. I was planning to work on it starting today.

Sure. I'll wait to request reviews of big changes in *EditRules after your fix.

This is a minimal approach that works in the common case but there are still cases to consider:

  • Is this direction fine?
  • Add a mochitest
  • Check interaction with IMEs
  • Check interaction with a11y, specifically screen readers
  • Is there any easy way to clear the flag upon navigation (to not leave fields unmasked when restored from bfcache)?

Follow-up bugs:

  • Support cut and copy from the unmasked pw field.

I have an idea right now. Currently, editor puts masked characters into the text node in anonymous <div> element in <input type="password">. However, this causes some problems like bug 1345229, and not working properly with IME composition. Additionally, when editor newly masks visible character(s), that causes DOM mutation internally. This is expensive. So, I'm currently thinking that it must be better if possible: editor always sets the text node as is even in <input type="password"> and nsTextFrame (or gfxTextRun renders each cluster boundary with a mask character), and the unmasked range can be managed in nsTextFrame or editor, must make this kind of changes easier.

I guess that Jonathan Kew is familiar with current nsTextFrame. Do you have any idea/suggestion?

Flags: needinfo?(jfkthame)
Depends on: 1560029

Comment on attachment 9070113 [details]
Bug 1548389 - WIP: Check if a password field should be masked before masking. r=masayuki

Revision D33878 was moved to bug 1560029. Setting attachment 9070113 [details] to obsolete.

Attachment #9070113 - Attachment is obsolete: true

As far as I've investigated, nsTextFrame does not have any space to store new flag since all bits are already used. However, looks like that we can store it in CharacterData node.

So, perhaps, we can do:

  • Make editor set "maskable text" flag to the node.
  • Make nsTextFrame check it and when it's set, make it retrieve unmasking range from editor.
  • Make gfxTextRun initialized with masked characters.
  • Make editor reset nsTextFrame when password field is changed.
  • Make nsCSSFrameConstructor always creates nsTextFrame when the flag is set since password may be only whitespaces.

I still need to investigate what would be changed from point of view of a11y module with this change.

Okay, looks like that we can use nsCaseTransformTextRunFactory to teat as text-transforming to mask character.

And perhaps, new API is independent method from the flags because we need to manage masked range for mobile. So, anyway, editor needs to keep having nsITimer. If you'd like to making text automatically with specified time, I can add a parameter to timeout.

Flags: needinfo?(jfkthame)

Comment on attachment 9070114 [details]
Bug 1548389 - Make password field values visible when focused after a generated password is filled.

Revision D33879 was moved to bug 1560029. Setting attachment 9070114 [details] to obsolete.

Attachment #9070114 - Attachment is obsolete: true
Attachment #9070113 - Attachment is obsolete: false

Since there was more DOM/Editor discussion in this bug I will make this one the editor one instead.

Assignee: MattN+bmo → masayuki
Blocks: 1560029
No longer blocks: 376674
Component: Password Manager → Editor
Depends on: 1560032
No longer depends on: 1560029
Priority: P2 → P1
Product: Toolkit → Core
Summary: Make password field values visible when focused after a generated password is filled → Add an API to allow unmasking password fields

(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Might be slow response) from comment #17)

Okay, looks like that we can use nsCaseTransformTextRunFactory to teat as text-transforming to mask character.

Yes, this sounds like a possible approach: we could create a new (internal-only) text-transform value that masks all characters by simply mapping them to U+2022.

We also need to consider how to apply this to a range of the field, though; for the mobile case where the last character is initially displayed, and then masked after a timeout, we can't just apply this new text-transform to the whole element.

I don't think we have any simple API to ask nsTextFrame to apply a transform to a specific range of its text. But maybe we could invent an internal pseudo-element like ::-moz-recently-typed? This would resolve to the most recently typed character, and expire after a short timeout. Then the styling for password inputs could use the masked text-transform by default, but remove it for the recently-typed pseudo.

Here are WIP patches:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=618125c0ca96c718d235c734d7cb68c5b83e2e8e
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ed9393f23b8ab93a280b454aa5cc3777b5a45acc

Currently, these builds work well at least visually. However, there are some pending issues:

One is here:
https://searchfox.org/mozilla-central/rev/8a990595ce6d5ed07ace2d4d5d86cc69aec90bde/accessible/tests/mochitest/elm/test_HTMLSpec.html#828-830
This gets Accessible object for the <input type="password"> and refers name of its child. I guess that this refers the anonymous text node in the <input type="password">.

The other is here:
https://searchfox.org/mozilla-central/rev/8a990595ce6d5ed07ace2d4d5d86cc69aec90bde/accessible/tests/mochitest/text.js#76-80
This gets Accessible object for the <input type="password"> and checks the getText() result.

The a11y test failures are just my patch's bug. Currently, I'm writing rendering tests of new password field and behavior tests of the API.

Here is newer build:
https://treeherder.mozilla.org/#/jobs?repo=try&selectedJob=255459796&revision=7469ed87feb55a79ee46876294dc933254fe61fc

I'm still working on moving caret per word, selecting per word with keyboard and deleting per word.

Regarding a11y, as well as ensuring that the password still remains masked when it is masked visually (I just tested the latest build and that still works), we also need to make sure the password is unmasked for a11y when it is unmasked visually. That is, a11y should reflect the visual state in both cases.

(In reply to James Teh [:Jamie] from comment #24)

Regarding a11y, as well as ensuring that the password still remains masked when it is masked visually (I just tested the latest build and that still works),

Thank you for the check!

we also need to make sure the password is unmasked for a11y when it is unmasked visually. That is, a11y should reflect the visual state in both cases.

Indeed. It must work with the latest build since a11y accesses nsTextFrame::GetRenderedText() and that is computed with the unmasked range. But I'll add mochitests for unmasked cases.

Whiteboard: [passwords:generation] [skyline]
status-firefox70: --- → affected

In the next patch, we need to update unmasked range when anonymous text node
in <input type="password"> is modified. This patch makes editor manage it
easier to update the range.

Note that the next patch also handles text node creation and destruction.

This patch creates editor API to get/set unmask-range of password field.
Its design is similar to setSelectionRange() and selectionStart/
selectionEnd attributes. The unmasked range is automatically
masked if aTimeout of unmask() is set to non-zero.
Otherwise, unmasked range won't be masked automatically even after user
or web apps modifies the editor, and inserting new character expands
unmasking range.

The following patch makes TextEditRules use these API to implement
delayed masking of password.

Note that editor has never supported dynamic eEditorPasswordMask change.
E.g., if you have typed some characters into an editor and toggle the flag,
the characters are not unmasked nor masked. Then, if you type new characters,
only they are correctly masked or unmasked. This patch puts MOZ_ASSERT()
to reject this feature completely on debug build for making the unmasking
implementation simpler.

Now, TextEditRules should use the new editor API to mask/unmask characters
if it's for a password editor. With this change, it does not need to manage
masked characters and unmasked characters separately since the anonymous
text node always have unmasked characters and nsTextFrame will mask the
characters at painting time.

With the previous patches, editor has stopped masking characters in password
field. Instead, layout code needs to handle it. However, layout code
especially around nsTextFrame is performance critical area. Therefore,
layout code requires a quick way to check whether a text node in password
field or not.

This patch creates new flag for CharacterData node and marks all text nodes
whose characters should be masked with the flag when EditorBase or
nsTextControlFrame creates them.

This method makes the following patches simpler and avoid duplicating same
code.

The utility method retrieves TextEditor from HTMLInputElement and
HTMLTextAreaElement. However, this won't cause creating corresponding
editor because when it calls from TextEditor, editor shouldn't be created
at that time due to requiring another reflow. Additionally, it's not
necessary for nsTextFrame because without TextEditor, all characters
in password field should be masked.

Anonymous text node in password field has NS_MAYBE_MASKED flag and
TextEditor managing the node has range of unmasking. This patch makes
BuildTextRunsScanner::BuildTextRunForFrames() treat text frame in
password field as text-transform applied. For recording the unmask range,
this patch creates nsTransformedCharStyle::mMaskPassword and treats unmask
range start/end edges as different style boundary. Therefore,
nsCaseTransformTextRunFactory::TransformString() can replace each character
with a password mask character with the information.

Note that we need to kill bidi algorithm in password fields with forms.css
because caret position will tell everybody whether the typing character is
an RTL or an LTR character.

Now, the anonymous text node has raw value of password so that we shouldn't
allow users to drag the text since another user may have typed the password
and left from the device.

Note that we've supported the operation, however, it does not make sense
since the dragging data is masked text.

Additionally, Chrome also doesn't support dragging text in password fields.
So, we have no reason to support dragging raw value from password fields.

Double click, long tap, moving selection with keyboard and deleting text
scan word boundary. With these changes, the text node has raw password
value even if it's masked visually. For making safer,
nsTextFrame::PeekOffsetWord() should search word boundary in masked text.
Then, we can hide word boundary at masked range, but keep allowing to look
for word boundary only in unmasked range.

Do we have tests ensuring that changing type="password" to type="text" ends up showing the right text, or from text to password etc. ?

(In reply to Olli Pettay [:smaug] from comment #35)

Do we have tests ensuring that changing type="password" to type="text" ends up showing the right text, or from text to password etc. ?

Yes, part 5 has them: https://phabricator.services.mozilla.com/D38009

Attachment #9070113 - Attachment is obsolete: true

(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Still struggling with the pain, but becoming better) from comment #33)

Now, the anonymous text node has raw value of password so that we shouldn't
allow users to drag the text since another user may have typed the password
and left from the device.

Unfortunately, it seems there are still other ways the user can easily "spy" on such a password; see comment at https://phabricator.services.mozilla.com/D38013#1146706.

The security holes are fixed in my local build. However, for making this fix faster and safer, I give up to use a mask for a non-BMP character since it causes one of the security hole (but using 2 masked characters for a surrogate pair is also a minor security issue though).

Flags: qe-verify+ → qe-verify-

In some cases, other tools may show selected content in their UI. E.g.,
character palette of macOS. Therefore, we shouldn't allow them to show
masked password.

Unmasking is an optional style of showing password. Therefore, if callers of
nsIEditor::Unmask() specify middle of surrogate pair(s), it may mean that
they want to expand the unmask range from shorter range which does not include
the high and/or low surrogate. Therefore, one of the surrogates is in unmasked
range, we unmask the surrogate pair. However, we handle this in a lot of
places, i..e., we have duplicated code. This can get rid of these duplicates
with making nsIEditor::Unmask() expand the range automatically.

Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/743175390781
part 0: Wrap modifying text node in editor with particular methods r=m_kato
https://hg.mozilla.org/integration/autoland/rev/f1d32f0d5b87
part 1: Implement API to get/set unmask-range of password editor r=m_kato
https://hg.mozilla.org/integration/autoland/rev/32c7c4840b1e
part 2: Make `TextEditRules` use the new editor API to mask/unmask characters in password editor r=m_kato
https://hg.mozilla.org/integration/autoland/rev/eebd87d55700
part 3: Make editor mark text node in password field as "maybe masked" r=smaug
https://hg.mozilla.org/integration/autoland/rev/bbd901cd1d96
part 4: Implement `nsContentUtils::GetTextEditorFromAnonymousNodeWithoutCreation()` to retrieve `TextEditor` instance from an anonymous node in `HTMLInputElement` or `HTMLTextAreaElement` r=smaug
https://hg.mozilla.org/integration/autoland/rev/389b013a1cea
part 5: Make `nsTextFrame` and related code treat masking password characters as text-transform r=jfkthame
https://hg.mozilla.org/integration/autoland/rev/2a77d3e78e0b
part 6: Add automated tests for new API and rendering of password fields r=m_kato,Jamie
https://hg.mozilla.org/integration/autoland/rev/2dddbc45b53b
part 7: Drop supporting dragging text from `<input type="password">` r=smaug
https://hg.mozilla.org/integration/autoland/rev/7558df23d9e8
part 8: Make `nsTextFrame::PeekOffsetWord()` scan masked text r=jfkthame
https://hg.mozilla.org/integration/autoland/rev/d2d0127a3710
part 9: Make ContentEventHandler return masked text r=m_kato
https://hg.mozilla.org/integration/autoland/rev/81e094372c41
part 10: Make `TextEditor::SetUnmaskRangeInternal()` expand the range if specified offset is middle of a surrogate pair r=m_kato
Depends on: 1568753
Regressions: 1571719
Depends on: 1571719
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: