Last Comment Bug 1177911 - Determine and implement better password requirements for BMO
: Determine and implement better password requirements for BMO
Status: RESOLVED FIXED
:
Product: bugzilla.mozilla.org
Classification: Other
Component: General (show other bugs)
: Production
: Unspecified Unspecified
P1 normal (vote)
: ---
Assigned To: Dylan Hardison [:dylan]
:
:
Mentors:
: 1181408 (view as bug list)
Depends on:
Blocks: 1193827 1221851
  Show dependency treegraph
 
Reported: 2015-06-26 14:52 PDT by Colby Russell :crussell (no longer Mozilla-ing)
Modified: 2015-11-04 19:11 PST (History)
10 users (show)
See Also:
Due Date:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
terrible.png (157.19 KB, image/png)
2015-06-26 14:52 PDT, Colby Russell :crussell (no longer Mozilla-ing)
no flags Details
1177911_4.patch (19.84 KB, patch)
2015-08-30 19:31 PDT, Dylan Hardison [:dylan]
glob: review-
Details | Diff | Splinter Review
1177911_7.patch (30.90 KB, patch)
2015-10-19 06:25 PDT, Dylan Hardison [:dylan]
dkl: review-
Details | Diff | Splinter Review
1177911_8.patch (35.40 KB, patch)
2015-10-20 12:48 PDT, Dylan Hardison [:dylan]
dkl: review+
Details | Diff | Splinter Review

Description User image Colby Russell :crussell (no longer Mozilla-ing) 2015-06-26 14:52:14 PDT
Created attachment 8626751 [details]
terrible.png

(This may belong in the product for Bugzilla proper and not b.m.o; I'm not sure if this is site-specific configuration problem or not.)

I just tried to file a bug.  I tried to log in, but my password wouldn't work.  I made a password reset request and followed the link from my inbox.  I tried to set a new password.  No go.

The message tells me that now b.m.o passwords require at least one upper case letter, one lowercase letter, and one digit.  This is surprising and disappointing; these restrictions are the kind of cargo cult I would expect from my local bank (unfortunately), but not on bugzilla.

These password restrictions:

1. Make for insecure and hard to remember passwords.
2. Are a pain to type on mobile.
Comment 1 User image Byron Jones ‹:glob› 2015-07-07 20:52:27 PDT
*** Bug 1181408 has been marked as a duplicate of this bug. ***
Comment 2 User image Byron Jones ‹:glob› 2015-07-07 20:53:16 PDT
from bug 1181408:
> Please allow four word (or more) passphrases that contain only lowercase letters in addition to the current password policy
Comment 3 User image Byron Jones ‹:glob› 2015-08-06 21:43:48 PDT
here's what i'm thinking we should do:

minimum requirements:
- at least 8 characters

require at least three of the following:
- lowercase letters
- uppercase letters
- numbers or symbols
- longer than 12 characters

we should also display a password strength meter, so users get immediate in-page feedback if their password doesn't match the requirements.
Comment 4 User image Richard Soderberg [:atoll] 2015-08-06 23:07:18 PDT
(In reply to Byron Jones ‹:glob› from comment #3)
> here's what i'm thinking we should do:
> 
> minimum requirements:
> - at least 8 characters
> 
> require at least three of the following:
> - lowercase letters
> - uppercase letters
> - numbers or symbols
> - longer than 12 characters

These guidelines cover passwords. What are the guidelines for passphrases?
Comment 5 User image Byron Jones ‹:glob› 2015-08-06 23:29:53 PDT
(In reply to Richard Soderberg [:atoll] from comment #4)
> (In reply to Byron Jones ‹:glob› from comment #3)
> > here's what i'm thinking we should do:
> > 
> > minimum requirements:
> > - at least 8 characters
> > 
> > require at least three of the following:
> > - lowercase letters
> > - uppercase letters
> > - numbers or symbols
> > - longer than 12 characters
> 
> These guidelines cover passwords. What are the guidelines for passphrases?

if we count 'space' as a symbol then we'll be fine as most pastphrases would match letters + symbols + >12 chars
Comment 6 User image Wayne Mery (:wsmwk, NI for questions) 2015-08-07 13:01:44 PDT
I'm in favor of better passwords. Here is my recent experience...

(In reply to Colby Russell :crussell (no longer Mozilla-ing) from comment #0)
> Created attachment 8626751 [details]
> terrible.png
> ... I tried to log in, but my password wouldn't work.  

I got this in chrome one one machine, but not in other machines, and not in my other browsers.  Perhaps the cookie in chrome got horked?

> I made a password reset request and followed the link from my inbox. I tried to set a new password.  No go.

For grins I requested rest but the email sez "This email would have contained sensitive information ... You will have to contact bugzilla-admin ...".  Did it decline to provide the reset info in email because I am in the security group?
Comment 7 User image Richard Soderberg [:atoll] 2015-08-07 13:10:55 PDT
Yes, if you're in securemail groups, you need to have previously configured either GPG or SMIME in your Bugzilla SecureMail preferences to reset your own password. If you didn't, it gives you that error. If none of your browsers are still logged in, then you'll need to file a bug in .. bugzilla.mozilla.org :: Administration .. perhaps? to negotiate a way to reset it.
Comment 8 User image Byron Jones ‹:glob› 2015-08-09 23:07:08 PDT
(In reply to Wayne Mery (:wsmwk, use Needinfo for questions) from comment #6)
> I'm in favor of better passwords. Here is my recent experience...
> [snip]

that process isn't related to this bug, but it's one that i'm in discussion with the security team about resolving.
Comment 9 User image Dylan Hardison [:dylan] 2015-08-26 21:05:02 PDT
Alright, here's what works:

1) password complexity is checked both server side and client side. Making client and server
   work the same required not using character classes...
2) there are two new password complexity settings -- bmo and bmo_dev. "bmo" works as glob mentions in comment #5.
   bmo_dev is the same except it requires only two (instead of three) of the features. In addition to those rules,
   we check that the possible entropy of the password is over a threshold (with a lower threshold for bmo_dev).
3) Existing password complexity rules and code has not been removed, in an effort to not make dkl's merge process any more painful. I fully expect to purge the code after the merge is done.
4) UI-side, there is a password complexity meter that is based on the entropy of the password (which is, of course, a *guess* of the entropy, but a fairly good one). In addition to that, the list of rules is presented below the password dialog. Once the rules are satisfied, they're replaced by the password confirmation input. I went ahead and check that client side as well, because why not?
5) I gave up using anything fancy to display the meter, it's just background color (5 shades, should be safe for most colorblind people).

Exceptional cases, such as a password that doesn't match enough rules "forces" the password meter to be 0, but otherwise it is a visual indication of how many bits of entropy the password might have.


I had added a toggle-view for the password fields (like the disable account button), as originally I was adding more visual complexity to the account pref tab. This is a worthwhile task but unfortunately will need to be done another time.

The remaining difficulty is splitting out the js so that it can be used by the reset_password.cgi page.
Comment 10 User image Byron Jones ‹:glob› 2015-08-26 21:13:40 PDT
(In reply to Dylan William Hardison [:dylan] from comment #9)
> 2) there are two new password complexity settings -- bmo and bmo_dev. "bmo"
> works as glob mentions in comment #5.

ah, for dev we need zero complexity rules.
change this to enabled/disabled and remove all other options from the params list.
Comment 11 User image Dylan Hardison [:dylan] 2015-08-30 19:31:44 PDT
Created attachment 8654692 [details] [diff] [review]
1177911_4.patch

I'm finally at the point where I've stopped finding fault with this. I think this is a drastic improvement to usability as well as security.
Comment 12 User image Byron Jones ‹:glob› 2015-08-30 20:17:43 PDT
Comment on attachment 8654692 [details] [diff] [review]
1177911_4.patch

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

this mostly looks good, although i haven't tested it yet.

::: Bugzilla/User.pm
@@ +2459,5 @@
> +        $features++ if length($password) > 12;
> +
> +        my $entropy = floor(log($charset) * (length($password) / log(2)));
> +        return 'password_not_complex' if $entropy  < $min_entropy;
> +        return 'password_not_complex' if $features < $min_features;

imposing server-side entropy checks is a problem, because they are hard to explain.  the ui meter should drive the password strength, but we cannot enforce entropy server-side.

remove this check.

::: js/account.js
@@ +64,5 @@
> +            $("#password-msg")
> +                .text("Too short")
> +                .attr("class", "password-bad");
> +        }
> +        else if (entropy < 50) {

same here - remove entropy as a minimal requirement

::: template/en/default/admin/params/auth.html.tmpl
@@ +141,5 @@
> +    "<ul><li>uppercase letters</li>" _
> +   "<li>lowercase letters</li>" _
> +   "<li>numbers</li>" _
> +   "<li>symbols</li>" _
> +   "<li>longer than 12 characters</li></ul></ul>"

indentation is wrong here
Comment 13 User image Byron Jones ‹:glob› 2015-08-30 21:15:48 PDT
Comment on attachment 8654692 [details] [diff] [review]
1177911_4.patch

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

after some quick testing:
- add checksetup code to fix now-invalid password_complexity values (map all not-disabled values to 'bmo')
- the password meter should appear to the right of the 1st password, to prevent the page content below it from shifting down as it changes
- the 'Does not match' message should appear to the right of the 2nd password, to prevent the page content below it from shifting down when it appears
- account/email/confirm-new.html.tmpl doesn't contain the meter or proper checking
- account/password/set-forgotten-password.html.tmpl hasn't been updated at all
- global/user-error.html.tmpl's password_not_complex error needs to be updated
- "does not match" message does not disappear when password1 is updated to match password2

::: js/account.js
@@ +18,5 @@
> +
> +        $("#password-meter").show();
> +
> +        if (password.match(/[A-Z]/)) {
> +            delete missing_features["upper"];

use dot notation (this applies multiple times):
  delete missing_features.upper;

@@ +56,5 @@
> +        $password.data("is_complex", false);
> +
> +        if (features.length < min_features) {
> +            $("#password-msg")
> +                .text("Matches " + features.length + " requirements")

if features.length is 1 this should say "Matches 1 requirement" not "Matches 1 requirements"

@@ +100,5 @@
> +        else {
> +            $password.get(0).setCustomValidity("");
> +        }
> +
> +        if (score == 0) {

use === to compare with 0

@@ +101,5 @@
> +            $password.get(0).setCustomValidity("");
> +        }
> +
> +        if (score == 0) {
> +            score = .01;

for clarity write this as 0.01

::: skins/standard/admin.css
@@ +268,5 @@
> +#password-features li.feature-ok:before {
> +    font-size: normal;
> +    content: "\2611";
> +    margin-left: -1em;
> +    margin-right: .100em;

use pixels instead of fractional ems

@@ +279,5 @@
> +#password-features li:before {
> +    font-size: normal;
> +    content: "\2610";
> +    margin-left: -1em;
> +    margin-right: .100em;

use pixels instead of fractional ems

::: template/en/default/account/prefs/account.html.tmpl
@@ +28,5 @@
>  [% Hook.process('start') %]
> +<script>
> +  $(function () {
> +    $("#new_password1").data('password_complexity', '[% Param('password_complexity') FILTER js %]');
> +  });

this isn't guarenteed to execute after the init block in account.js, so complexity checking doesn't always happen (on my system it never happened).
Comment 14 User image Jesse Ruderman 2015-08-31 16:11:08 PDT
Naive password meters based on character classes, like the one in the attached patch, tend to accept bad leetspeak passwords while rejecting good passphrases.

Please use zxcvbn.js instead. https://github.com/dropbox/zxcvbn
Comment 15 User image Dylan Hardison [:dylan] 2015-09-02 17:45:21 PDT
(In reply to Jesse Ruderman from comment #14)
> Naive password meters based on character classes, like the one in the
> attached patch, tend to accept bad leetspeak passwords while rejecting good
> passphrases.
> 
> Please use zxcvbn.js instead. https://github.com/dropbox/zxcvbn

Our password rules are *already* based on character classes,
this patch simplifies those rules and makes their application interactive.
It also makes it possible to use a passphrase (as noted by glob in comment #5.
In fact, a passphrase with three or more average-length words will be considered "very strong" (based on the shannon entropy alone).
Passwords generated by password managers will in general be fine too.

I have looked into zxcvbn during the research phase of this bug, and I would like to make use of it.
However, there are technical hurdles to this at the moment -- the least of which will be porting (probably the python implementation) to perl. (The alternative -- doing the complexity check solely client-side, is unacceptable).

I would like to make use of the "top 1000 used passwords" bloomfilter than Sai Prashanth spoke about a short time ago, as well.

As an aside, I am aware that using the Shannon entropy alone would be a relatively poor measure of password strength.
In the next patch, we're using it solely for displaying the strength meter.
Comment 16 User image Byron Jones ‹:glob› 2015-09-02 20:19:06 PDT
(In reply to Dylan William Hardison [:dylan] from comment #15)
> Our password rules are *already* based on character classes,
> this patch simplifies those rules and makes their application interactive.
> It also makes it possible to use a passphrase (as noted by glob in comment
> #5.
> In fact, a passphrase with three or more average-length words will be
> considered "very strong" (based on the shannon entropy alone).
> Passwords generated by password managers will in general be fine too.

to clarify and correct dylan slightly -- because bmo is open to registration by anyone, we will not be enforcing complex passwords server-side.  we don't want to discourage people from opening accounts to report bugs because their password isn't complex enough.

the password complexity meter is there to _encourage_ strong passwords, not _enforce_.  using zxcvbn instead of the character and entropy to generate the meter would work, but i doubt the work required (and its heavy weight) would be worth the improvement over the current design.

instead of enforcing complex passwords (in js or server-side) we'll be using 2fa as a mechanism to secure sensitive accounts.
Comment 17 User image Byron Jones ‹:glob› 2015-09-02 22:44:45 PDT
(In reply to Byron Jones ‹:glob› from comment #16)
> instead of enforcing complex passwords (in js or server-side) we'll be using
> 2fa as a mechanism to secure sensitive accounts.

by "enforcing complex passwords" i mean beyond what we do with character class based complexity rules, not a complete absence of server-side complexity enforcement.
Comment 18 User image Dylan Hardison [:dylan] 2015-10-19 06:25:37 PDT
Created attachment 8675668 [details] [diff] [review]
1177911_7.patch

A significant refactor of the complexity code -- now available whereever passwords are changed.
Comment 19 User image David Lawrence [:dkl] 2015-10-19 14:15:52 PDT
Comment on attachment 8675668 [details] [diff] [review]
1177911_7.patch

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

Missing account/password/set-forgotten-password.html.tmpl. Doesn't show the strength meter on the right and also when submitting, it just displays "The password must contain at least one: " with no additional information.

./checksetup.pl also displays a broken error message same as above when using --reset-password=<login>.

::: Bugzilla/Config/Auth.pm
@@ +132,5 @@
>    {
>     name => 'password_complexity',
>     type => 's',
> +   choices => [ 'no_constraints', 'bmo' ],
> +   default => 'bmo',

nit; I would be happier if we left the others and just added 'bmo' to the end. If not 'bmo' we fall back to default behavior.

::: Bugzilla/User.pm
@@ +60,4 @@
>  use Storable qw(dclone);
>  use URI;
>  use URI::QueryParam;
> +use POSIX qw(floor);

Not used anywhere from what I can tell.

@@ +2471,5 @@
>  
>      my $complexity_level = Bugzilla->params->{password_complexity};
> +    if ($complexity_level eq 'bmo') {
> +        my $features = 0;
> +        my $charset = 0;

From what I can tell, $charset is incremented but then not used. Seems like this was copied from the Javascript code portion incompletely?

@@ +2472,5 @@
>      my $complexity_level = Bugzilla->params->{password_complexity};
> +    if ($complexity_level eq 'bmo') {
> +        my $features = 0;
> +        my $charset = 0;
> +        my $min_features = 3;

Should this be moved to a constant for easy discovery?

::: template/en/default/account/email/confirm-new.html.tmpl
@@ +16,5 @@
>  [%# INTERFACE:
> +   # token: string. The token to be used in the user account creation.
> +   # email: email address of the user account.
> +   # expiration_ts: expiration date of the token.
> +   #%]

why the extra beginning spaces from here up?

::: template/en/default/account/prefs/account.html.tmpl
@@ +79,2 @@
>            </tr>
> +

nit: remove extra new line
Comment 20 User image Dylan Hardison [:dylan] 2015-10-20 12:48:27 PDT
Created attachment 8676445 [details] [diff] [review]
1177911_8.patch

(In reply to David Lawrence [:dkl] from comment #19)
> Comment on attachment 8675668 [details] [diff] [review]
> 1177911_7.patch
> 
> Review of attachment 8675668 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Missing account/password/set-forgotten-password.html.tmpl. Doesn't show the
> strength meter on the right and also when submitting, it just displays "The
> password must contain at least one: " with no additional information.
Added set-forgotten-password and updated the error (which was weirdly repeated?)
The checksetup.pl error is now understandable although the formatting is weird.


> 
> ::: Bugzilla/Config/Auth.pm
> @@ +132,5 @@
> >    {
> >     name => 'password_complexity',
> >     type => 's',
> > +   choices => [ 'no_constraints', 'bmo' ],
> > +   default => 'bmo',
> 
> nit; I would be happier if we left the others and just added 'bmo' to the
> end. If not 'bmo' we fall back to default behavior.

This was disregarded in the initial patch, it would be somewhat difficult to change it.


> @@ +2471,5 @@
> >  
> >      my $complexity_level = Bugzilla->params->{password_complexity};
> > +    if ($complexity_level eq 'bmo') {
> > +        my $features = 0;
> > +        my $charset = 0;

Removed and simplified this code.

> From what I can tell, $charset is incremented but then not used. Seems like
> this was copied from the Javascript code portion incompletely?
> 
> @@ +2472,5 @@
> >      my $complexity_level = Bugzilla->params->{password_complexity};
> > +    if ($complexity_level eq 'bmo') {
> > +        my $features = 0;
> > +        my $charset = 0;
> > +        my $min_features = 3;
> 
> Should this be moved to a constant for easy discovery?
If we do, I would need the constant in the JS code too -- which would mean injecting more data (using data-) params. I can do that, but I think it's not worth the effort.

Fixed the other nits + js tweaks that weren't in the last patch.
Comment 21 User image David Lawrence [:dkl] 2015-10-21 16:01:20 PDT
Comment on attachment 8676445 [details] [diff] [review]
1177911_8.patch

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

Looks good but my final review will have to wait a couple days while I work on AWS stuff. Just a heads up.
Comment 22 User image David Lawrence [:dkl] 2015-11-04 09:36:54 PST
Comment on attachment 8676445 [details] [diff] [review]
1177911_8.patch

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

Works well and looks good other than some nits that should be fixed on commit. r=dkl

::: js/account.js
@@ +14,5 @@
> +            var features = [],
> +                charset = 0,
> +                score = 0,
> +                min_features = 3,
> +                min_entropy = 50;

min_entropy not used

::: template/en/default/account/email/confirm-new.html.tmpl
@@ +16,5 @@
>  [%# INTERFACE:
> +   # token: string. The token to be used in the user account creation.
> +   # email: email address of the user account.
> +   # expiration_ts: expiration date of the token.
> +#%]

fix indention here and above to top of file

@@ +22,5 @@
>  [% title = BLOCK %]Create a new user account for '[% email FILTER html %]'[% END %]
>  [% PROCESS "global/header.html.tmpl"
> +   title = title
> +   style_urls      = ['skins/standard/admin.css']
> +   javascript_urls = [ "js/account.js" ]

remove extra space after [ and before ] and use ' for consistency

::: template/en/default/account/password/set-forgotten-password.html.tmpl
@@ +20,5 @@
>  
>  [% title = "Change Password" %]
> + [% PROCESS global/header.html.tmpl
> +    style_urls      = ['skins/standard/admin.css']
> +    javascript_urls = [ "js/account.js" ]

removed extra space after [ and before ] and use ' for consistency

::: template/en/default/account/prefs/account.html.tmpl
@@ +79,2 @@
>            </tr>
> +

remove extra newline

::: template/en/default/account/reset-password.html.tmpl
@@ +68,2 @@
>      javascript = inline_js
> +    javascript_urls = [ "js/account.js" ]

same here
Comment 23 User image Dylan Hardison [:dylan] 2015-11-04 14:52:08 PST
To ssh://gitolite3@git.mozilla.org/webtools/bmo/bugzilla.git
   7f43eeb..3238e2d  master -> master
Comment 24 User image Dylan Hardison [:dylan] 2015-11-04 14:57:39 PST
To ssh://gitolite3@git.mozilla.org/webtools/bmo/bugzilla.git
   3238e2d..c7a2cf0  master -> master

Missed the default constraint fix, applied here.

Note You need to log in before you can comment on or make changes to this bug.