Bug 1524850 - Documented password guidelines do not match implementation

Bugzilla.pm

@@ -217,41 +217,6 @@ sub github_token {
   return $cache->{github_token};
 }
 
-sub passwdqc {
-  my ($class) = @_;
-  require Data::Password::passwdqc;
-
-  my $cache  = request_cache;
-  my $params = $class->params;
-
-  return $cache->{passwdqc} if $cache->{passwdqc};
-
-  my @min = map { $_ eq 'undef' ? undef : $_ }
-    split(/\s*,\s*/, $params->{passwdqc_min});
-
-  return $cache->{passwdqc} = Data::Password::passwdqc->new(
-    min              => \@min,
-    max              => $params->{passwdqc_max},
-    passphrase_words => $params->{passwdqc_passphrase_words},
-    match_length     => $params->{passwdqc_match_length},
-    random_bits      => $params->{passwdqc_random_bits},
-  );
-}
-
-sub assert_password_is_secure {
-  my ($class, $password1) = @_;
-
-  my $pwqc = $class->passwdqc;
-  ThrowUserError('password_insecure', {reason => $pwqc->reason})
-    unless $pwqc->validate_password($password1);
-}
-
-sub assert_passwords_match {
-  my ($class, $password1, $password2) = @_;
-
-  ThrowUserError('password_mismatch') if $password1 ne $password2;
-}
-
 sub login {
   my ($class, $type) = @_;
 

Bugzilla/Auth/Verify/DB.pm

@@ -18,6 +18,8 @@ use Bugzilla::Token;
 use Bugzilla::Util;
 use Bugzilla::User;
 
+use Try::Tiny;
+
 sub check_credentials {
   my ($self, $login_data) = @_;
   my $dbh = Bugzilla->dbh;
@@ -71,16 +73,18 @@ sub check_credentials {
   if ( Bugzilla->usage_mode == USAGE_MODE_BROWSER
     && Bugzilla->params->{password_check_on_login})
   {
-    my $pwqc = Bugzilla->passwdqc;
-    unless ($pwqc->validate_password($password)) {
-      my $reason     = $pwqc->reason;
+    try {
+      assert_valid_password($password);
+    }
+    catch {
       my $old_reason = $user->password_change_reason;
 
       Bugzilla->audit(sprintf "%s logged in with a weak password (reason: %s)",
-        $user->login, $reason);
+        $user->login, $_);
       $user->set_password_change_required(1);
       $user->set_password_change_reason(
-        "You must change your password for the following reason: $reason");
+        "You must change your password in order to meet the minimum requirements"
+      );
 
       # Remove disabled text if user was previously disabled due to inactivity
       if ($old_reason eq 'Inactive Account') {
@@ -89,7 +93,18 @@ sub check_credentials {
       }
 
       $user->update();
-    }
+    };
+  }
+
+  # Remove disabled text if user was previously disabled due to inactivity
+  if ( $user->password_change_reason
+    && $user->password_change_reason eq 'Inactive Account')
+  {
+    $user->set_disabledtext('');
+    $user->set_disable_mail(0);
+    $user->set_password_change_required(0);
+    $user->set_password_change_reason('');
+    $user->update();
   }
 
   # The user's credentials are okay, so delete any outstanding

Bugzilla/Config/Auth.pm

@@ -12,8 +12,6 @@ use strict;
 use warnings;
 
 use Bugzilla::Config::Common;
-use Types::Standard qw(Tuple Maybe);
-use Types::Common::Numeric qw(PositiveInt);
 
 our $sortkey = 300;
 
@@ -95,47 +93,6 @@ sub get_param_list {
 
     {name => 'password_check_on_login', type => 'b', default => '1'},
 
-    {
-      name    => 'passwdqc_min',
-      type    => 't',
-      default => 'undef, 24, 11, 8, 7',
-      checker => \&_check_passwdqc_min,
-    },
-
-    {
-      name    => 'passwdqc_max',
-      type    => 't',
-      default => '40',
-      checker => \&_check_passwdqc_max,
-    },
-
-    {
-      name    => 'passwdqc_passphrase_words',
-      type    => 't',
-      default => '3',
-      checker => \&check_numeric,
-    },
-
-    {
-      name    => 'passwdqc_match_length',
-      type    => 't',
-      default => '4',
-      checker => \&check_numeric,
-    },
-
-    {
-      name    => 'passwdqc_random_bits',
-      type    => 't',
-      default => '47',
-      checker => \&_check_passwdqc_random_bits,
-    },
-
-    {
-      name    => 'passwdqc_desc',
-      type    => 'l',
-      default => 'The password must be complex.',
-    },
-
     {name => 'duo_host', type => 't', default => '',},
     {name => 'duo_akey', type => 't', default => '',},
     {name => 'duo_ikey', type => 't', default => '',},
@@ -159,52 +116,4 @@ sub get_param_list {
   return @param_list;
 }
 
-my $passwdqc_min = Tuple [
-  Maybe [PositiveInt],
-  Maybe [PositiveInt],
-  Maybe [PositiveInt],
-  Maybe [PositiveInt],
-  Maybe [PositiveInt],
-];
-
-sub _check_passwdqc_min {
-  my ($value) = @_;
-  my @values = map { $_ eq 'undef' ? undef : $_ } split(/\s*,\s*/, $value);
-
-  unless ($passwdqc_min->check(\@values)) {
-    return "must be list of five values, that are either integers > 0 or undef";
-  }
-
-  my ($max, $max_pos);
-  my $pos = 0;
-  foreach my $value (@values) {
-    if (defined $max && defined $value) {
-      if ($value > $max) {
-        return "Int$pos is larger than Int$max_pos ($max)";
-      }
-    }
-    elsif (defined $value) {
-      $max     = $value;
-      $max_pos = $pos;
-    }
-    $pos++;
-  }
-  return "";
-}
-
-sub _check_passwdqc_max {
-  my ($value) = @_;
-  return "must be a positive integer" unless PositiveInt->check($value);
-  return "must be greater than 8" unless $value > 8;
-  return "";
-}
-
-sub _check_passwdqc_random_bits {
-  my ($value) = @_;
-  return "must be a positive integer" unless PositiveInt->check($value);
-  return "must be between 24 and 85 inclusive"
-    unless $value >= 24 && $value <= 85;
-  return "";
-}
-
 1;

Bugzilla/Constants.pm

@@ -283,8 +283,8 @@ use constant AUTH_NO_SUCH_USER => 5;
 use constant AUTH_LOCKOUT      => 6;
 
 # The minimum and maximum length a password will have.
-# BMO uses 8 characters.
-use constant USER_PASSWORD_MIN_LENGTH => 8;
+# BMO uses 12 characters.
+use constant USER_PASSWORD_MIN_LENGTH => 12;
 use constant USER_PASSWORD_MAX_LENGTH => 4096;
 
 use constant LOGIN_OPTIONAL => 0;

Bugzilla/Install.pm

@@ -30,6 +30,8 @@ use Bugzilla::User::Setting;
 use Bugzilla::Util qw(get_text);
 use Bugzilla::Version;
 
+use Try::Tiny;
+
 use constant STATUS_WORKFLOW => (
   [undef,         'UNCONFIRMED'],
   [undef,         'CONFIRMED'],
@@ -514,16 +516,13 @@ sub _prompt_for_password {
     print "\n", get_text('install_confirm_password'), ' ';
     my $pass2 = <STDIN>;
     chomp $pass2;
-    my $pwqc = Bugzilla->passwdqc;
-    my $ok   = $pwqc->validate_password($password);
-    if (!$ok) {
-      print "\n", $pwqc->reason, "\n";
-      undef $password;
+    try {
+      assert_valid_password($password, $pass2);
     }
-    elsif ($password ne $pass2) {
-      print "\n", "passwords do not match\n";
+    catch {
+      print "\n$_\n";
       undef $password;
-    }
+    };
     system("stty", "echo") unless ON_WINDOWS;
   }
   return $password;

Bugzilla/User.pm

@@ -37,7 +37,7 @@ use base qw(Bugzilla::Object Exporter);
 with 'Bugzilla::Role::Storable', 'Bugzilla::Role::FlattenToHash';
 
 @Bugzilla::User::EXPORT = qw(is_available_username
-  login_to_id user_id_to_login
+  login_to_id user_id_to_login assert_valid_password
   USER_MATCH_MULTIPLE USER_MATCH_FAILED USER_MATCH_SUCCESS
   MATCH_SKIP_CONFIRM
 );
@@ -365,7 +365,7 @@ sub _check_password {
   # authentication.
   return $pass if $pass eq '*';
 
-  Bugzilla->assert_password_is_secure($pass);
+  assert_valid_password($pass);
   my $cryptpassword = bz_crypt($pass);
   return $cryptpassword;
 }
@@ -2875,6 +2875,42 @@ sub user_id_to_login {
   return $login || '';
 }
 
+sub assert_valid_password {
+  my ($password, $matchpassword) = @_;
+
+  if (length($password) < USER_PASSWORD_MIN_LENGTH) {
+    ThrowUserError('password_too_short');
+  }
+  elsif ((defined $matchpassword) && ($password ne $matchpassword)) {
+    ThrowUserError('password_mismatch');
+  }
+
+  if (Bugzilla->params->{password_complexity} eq 'bmo') {
+
+    # A password can be at least four unique words of three characters or longer
+    my %seen;
+    my $good_words = 0;
+    my @words = split /\s+/, $password;
+    foreach my $word (@words) {
+      next if $seen{$word};
+      $good_words++ if length($word) >= 3;
+      $seen{$word} = 1;
+    }
+    return if $good_words >= 4;
+
+    # Or have at least 3 of the following classes of characters.
+    my $features = 0;
+    $features++ if $password =~ /[[:lower:]]/;
+    $features++ if $password =~ /[[:upper:]]/;
+    $features++ if $password =~ /[[:digit:]]/;
+    $features++ if $password =~ /[[:punct:]]/;
+    $features++ if length($password) > USER_PASSWORD_MIN_LENGTH;
+    return if $features >= 3;
+
+    ThrowUserError('password_not_complex');
+  }
+}
+
 1;
 
 __END__
@@ -3501,6 +3537,15 @@ Returns the login name of the user account for the given user ID. If no
 valid user ID is given or the user has no entry in the profiles table,
 we return an empty string.
 
+=item C<assert_valid_password($passwd1, $passwd2)>
+
+Returns true if a password is valid (i.e. meets Bugzilla's
+requirements for length and content), else throws an error.
+Untaints C<$passwd1> if successful.
+
+If a second password is passed in, this function also verifies that
+the two passwords match.
+
 =item C<match_field($data, $fields, $behavior)>
 
 =over

Bugzilla/Util.pm

@@ -733,12 +733,12 @@ sub bz_crypt {
 
 # If you want to understand the security of strings generated by this
 # function, here's a quick formula that will help you estimate:
-# We pick from 62 characters, which is close to 64, which is 2^6.
+# We pick from 70 characters, which is close to 64, which is 2^6.
 # So 8 characters is (2^6)^8 == 2^48 combinations. Just multiply 6
 # by the number of characters you generate, and that gets you the equivalent
 # strength of the string in bits.
 sub generate_random_password {
-  my $size = shift || 10;    # default to 10 chars if nothing specified
+  my $size = shift || USER_PASSWORD_MIN_LENGTH;    # default to USER_PASSWORD_MIN_LENGTH if nothing specified
   return
     join("", map { ('0' .. '9', 'a' .. 'z', 'A' .. 'Z')[irand 62] } (1 .. $size));
 }

Bugzilla/WebService/Constants.pm

@@ -166,7 +166,8 @@ use constant WS_ERROR_CODE => {
   auth_invalid_email           => 302,
   extern_id_conflict           => -303,
   auth_failure                 => 304,
-  password_insecure            => 305,
+  password_too_short           => 305,
+  password_not_complex         => 305,
   api_key_not_valid            => 306,
   api_key_revoked              => 306,
   auth_invalid_token           => 307,
@@ -182,8 +183,8 @@ use constant WS_ERROR_CODE => {
   auth_cant_create_account    => 501,
   account_creation_disabled   => 501,
   account_creation_restricted => 501,
+  password_too_short          => 502,
 
-  # Error 502 password_too_short no longer exists.
   # Error 503 password_too_long no longer exists.
   invalid_username     => 504,
   iam_illegal_username => 504,

Makefile.PL

@@ -52,7 +52,6 @@ my %requires = (
   'DBIx::Connector'                     => 0,
   'DBIx::Class'                         => 0,
   'DBIx::Class::Helpers'                => '== 2.034002',
-  'Data::Password::passwdqc'            => '0.08',
   'Date::Format'                        => '2.23',
   'Date::Parse'                         => '2.31',
   'DateTime'                            => '0.75',

README.rst

@@ -57,7 +57,7 @@ The procedure should be similar for other browsers.
 .. _`Firefox Connection Settings`: https://support.mozilla.org/en-US/kb/connection-settings-firefox
 
 After that, you should be able to visit http://bmo.test/ from your browser.
-You can login as [email protected] with the password "password01!" (without
+You can login as [email protected] with the password "password012!" (without
 quotes).
 
 If you want to update the code running in the web container, you do not need to restart everything.

conf/checksetup_answers.txt

@@ -1,6 +1,6 @@
 $answer{'ADMIN_EMAIL'}          = '[email protected]';
 $answer{'ADMIN_OK'}             = 'Y';
-$answer{'ADMIN_PASSWORD'}       = 'password01!';
+$answer{'ADMIN_PASSWORD'}       = 'password012!';
 $answer{'ADMIN_REALNAME'}       = 'Admin User';
 $answer{'bugzilla_version'}     = '1';
 $answer{'create_htaccess'}      = '1';
@@ -16,7 +16,6 @@ $answer{'insidergroup'}         = 'admin';
 $answer{'interdiffbin'}         = '/usr/bin/interdiff';
 $answer{'mail_delivery_method'} = 'Test';
 $answer{'NO_PAUSE'}             = 1;
-$answer{'passwdqc_min'}         = '8, 8, 8, 8, 8';
 $answer{'password_complexity'}  = 'bmo';
 $answer{'size_limit'}           = 750000;
 $answer{'skin'}                 = 'Mozilla';