1188425 - Add a pref to allow insecure text input, thereby avoiding crashes at [ChildView keyDown:] in non-release builds

Status: VERIFIED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Steven Michaud [:smichaud] (Retired)]

Since bug 807893 landed, we've had code in Cocoa Widgets that crashes whenever it detects that the user is entering a password insecurely -- when "A password editor has focus, but not in secure input mode".  This is to allow us to find and fix the bugs that allow this to happen.

We've fixed some of them:

bug 1051842
bug 1124408
bug 1138678

But others remain unfixed, including the following and (probably) others that haven't yet been opened:

bug 1148196

Though the bugs we're trying to fix also happen in release builds, we don't crash there -- that would be too disruptive and annoying for users who aren't also testers.  But these crashes can also be disruptive/annoying for those who regularly trigger them -- for example those who use 3rd party password managers.  So we should add an about:config pref to allow people to disable the tests for insecure input in non-release builds, if they so choose.

Comment 1

[Steven Michaud [:smichaud] (Retired)]

Here's another of these bugs that's still open:

bug 893973

Comment 2

[Steven Michaud [:smichaud] (Retired)]

Attachment: Fix

Comment 3

[Steven Michaud [:smichaud] (Retired)]

Here's yet another open bug, which concerns crashes with the 1password extension:
bug 1163339

Comment 4

[Jamie Phelps]

dougt pinged 1Password about this on Twitter, so I wanted to jump in and see what we can do to make 1Password a better citizen in this regard.

1Password does create some fake key events (created with KeyboardEvent initKeyEvent with 0 for the keyCode and charCode arguments) to massage websites' Javascript validations and whatnot. For instance, some forms only trigger their validations on key events, not on value change or blur. This is something we've been doing for a long time and it's worked pretty well for us. 

The other thing we use generated key event for is autosubmit. We manually dispatch a key event with the keyCode and charCode set to 13 to the password field in order to submit the form. This works fantastically and is even better than what we're able to do in Chrome and Safari due to this longstanding bug: https://bugs.webkit.org/show_bug.cgi?id=16735

Thanks to Doug for bringing this to our attention and to all for your help and guidance.

Comment 5

[Steven Michaud [:smichaud] (Retired)]

Jamie, it's at bug 1163339 that I'll be working on the problem(s) specific to 1Password -- why these crashes happen with it.  You should probably repeat your comment there.  That'll be the best place to continue our discussion.

Comment 6

[Stephen A Pohl [:spohl]]

Comment on attachment 8640140 [details] [diff] [review]
Fix

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

::: modules/libpref/init/all.js
@@ +4858,5 @@
> +#if !defined(RELEASE_BUILD) || defined(DEBUG)
> +// In non-release builds we crash by default on insecure text input (when a
> +// password editor has focus but secure event input isn't enabled).  The
> +// following pref, when turned on, disables this behavior.  See bug 1188425.
> +pref("intl.allow-insecure-text-input", false);

This seems like a temporary pref for developers working with and/or building non-release Firefox builds. Again, I'm not sure this warrants a permanently visible pref in about:config. I'll let you make the call, but I'd be perfectly fine if you removed it from this file.

Comment 7

[Steven Michaud [:smichaud] (Retired)]

I think we want this patch's functionality to be a little more visible than the one for bug 1186158.  Also note that the pref is only permanently visible (in about:config) in non-release builds.

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/da2876c5f5ac

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/da2876c5f5ac

Comment 10

[Steven Michaud [:smichaud] (Retired)]

This bug's patch is now in today's mozilla-central nightly.  I just tested with the STR from bug 1148196 comment #9, with intl.allow-insecure-text-input set to 'true' and 'false'.  With it set to 'false' the crashes still happen.  With it set to 'true' they don't.  I think this counts as verification.

Comment 11

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 8640140 [details] [diff] [review]
Fix

Approval Request Comment
[Feature/regressing bug #]: bug 807893, sort of
[User impact if declined]: Users of password managers may crash consistently in the Dev Edition
[Describe test coverage new/current, TreeHerder]: Manual testing, as per comment #10
[Risks and why]: Very low
[String/UUID change made/needed]: None

Comment 12

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Steven, is this fix only a temporary fix so we don't crash and move on to fix other bugs in this area? Do we plan to keep this bug open so we remove this temporary fix in a few weeks? Thanks!

Comment 13

[Steven Michaud [:smichaud] (Retired)]

This fix won't *prevent* these crashes.  It will just allow those few who consistently see lots of them to turn off the security check that causes them to happen (in non-release builds).  It's meant as a palliative until we can actually fix all cases of the underlying security bug.

It's been a long time since we started working on this issue, and frankly it's taken us too long to resolve it.  I *hope* it won't take too much longer, but I really don't know.  Once the underlying security bug has been fixed, this patch can come out -- in that sense it *is* temporary.  But I don't know how long "temporary" will be :-)

Comment 14

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8640140 [details] [diff] [review]
Fix

Approved. Seems safe as this is only limited to non-release builds.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Needs rebasing for Aurora uplift.

Comment 16

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 8640140 [details] [diff] [review]
Fix

Landed (after rebasing) on mozilla-aurora:
https://hg.mozilla.org/releases/mozilla-aurora/rev/5c14f816e9da