Bug 785283 - Support increased values for PASSWORD_SALT_LENGTH without breaking compat with old hashes

[r=LpSolit a=LpSolit]

4 files changed, 55 insertions, 8 deletions

diff --git a/Bugzilla/Auth/Verify/DB.pm b/Bugzilla/Auth/Verify/DB.pm
index 2ad98874d..82fa662dc 100644
--- a/Bugzilla/Auth/Verify/DB.pm
+++ b/Bugzilla/Auth/Verify/DB.pm
@@ -66,11 +66,22 @@ sub check_credentials {
     Bugzilla::Token::DeletePasswordTokens($user->id, "user_logged_in");
     $user->clear_login_failures();
 
+    my $update_password = 0;
+
     # If their old password was using crypt() or some different hash
     # than we're using now, convert the stored password to using
     # whatever hashing system we're using now.
     my $current_algorithm = PASSWORD_DIGEST_ALGORITHM;
-    if ($real_password_crypted !~ /{\Q$current_algorithm\E}$/) {
+    $update_password = 1 if ($real_password_crypted !~ /{\Q$current_algorithm\E}$/);
+
+    # If their old password was using a different length salt than what
+    # we're using now, update the password to use the new salt length.
+    if ($real_password_crypted =~ /^([^,]+),/) {
+        $update_password = 1 if (length($1) != PASSWORD_SALT_LENGTH);
+    }
+
+    # If needed, update the user's password.
+    if ($update_password) {
         $user->set_password($password);
         $user->update();
     }
diff --git a/Bugzilla/Constants.pm b/Bugzilla/Constants.pm
index 5af171878..8410ae46a 100644
--- a/Bugzilla/Constants.pm
+++ b/Bugzilla/Constants.pm
@@ -567,10 +567,10 @@ use constant MAX_QUIP_LENGTH => 512;
 # This is the name of the algorithm used to hash passwords before storing
 # them in the database. This can be any string that is valid to pass to
 # Perl's "Digest" module. Note that if you change this, it won't take
-# effect until a user changes his password.
+# effect until a user logs in or changes his password.
 use constant PASSWORD_DIGEST_ALGORITHM => 'SHA-256';
-# How long of a salt should we use? Note that if you change this, none
-# of your users will be able to log in until they reset their passwords.
+# How long of a salt should we use? Note that if you change this, it
+# won't take effect until a user logs in or changes his password.
 use constant PASSWORD_SALT_LENGTH => 8;
 
 # Certain scripts redirect to GET even if the form was submitted originally
diff --git a/Bugzilla/Install/DB.pm b/Bugzilla/Install/DB.pm
index abf57ac27..e1a3f3630 100644
--- a/Bugzilla/Install/DB.pm
+++ b/Bugzilla/Install/DB.pm
@@ -24,6 +24,7 @@ use Bugzilla::Field;
 
 use Date::Parse;
 use Date::Format;
+use Digest;
 use IO::File;
 use List::MoreUtils qw(uniq);
 use URI;
@@ -701,6 +702,9 @@ sub update_table_definitions {
     # 2012-08-01 koosha.khajeh@gmail.com - Bug 187753
     _shorten_long_quips();
 
+    # 2012-12-29 reed@reedloden.com - Bug 785283
+    _add_password_salt_separator();
+
     ################################################################
     # New --TABLE-- changes should go *** A B O V E *** this point #
     ################################################################
@@ -3776,6 +3780,39 @@ sub _shorten_long_quips {
     $dbh->bz_alter_column('quips', 'quip', { TYPE => 'varchar(512)', NOTNULL => 1});
 }
 
+sub _add_password_salt_separator {
+    my $dbh = Bugzilla->dbh;
+
+    $dbh->bz_start_transaction();
+
+    my $profiles = $dbh->selectall_arrayref("SELECT userid, cryptpassword FROM profiles WHERE ("
+        . $dbh->sql_regexp("cryptpassword", "'^[^,]+{'") . ")");
+
+    if (@$profiles) {
+        say "Adding salt separator to password hashes...";
+
+        my $query = $dbh->prepare("UPDATE profiles SET cryptpassword = ? WHERE userid = ?");
+        my %algo_sizes;
+
+        foreach my $profile (@$profiles) {
+            my ($userid, $hash) = @$profile;
+            my ($algorithm) = $hash =~ /{([^}]+)}$/;
+
+            $algo_sizes{$algorithm} ||= length(Digest->new($algorithm)->b64digest);
+
+            # Calculate the salt length by taking the stored hash and
+            # subtracting the combined lengths of the hash size, the
+            # algorithm name, and 2 for the {} surrounding the name.
+            my $not_salt_len = $algo_sizes{$algorithm} + length($algorithm) + 2;
+            my $salt_len = length($hash) - $not_salt_len;
+
+            substr($hash, $salt_len, 0, ',');
+            $query->execute($hash, $userid);
+        }
+    }
+    $dbh->bz_commit_transaction();
+}
+
 1;
 
 __END__
diff --git a/Bugzilla/Util.pm b/Bugzilla/Util.pm
index bf072e88d..cee12ee21 100644
--- a/Bugzilla/Util.pm
+++ b/Bugzilla/Util.pm
@@ -591,11 +591,10 @@ sub bz_crypt {
     }
     else {
         my $hasher = Digest->new($algorithm);
-        # We only want to use the first characters of the salt, no
-        # matter how long of a salt we may have been passed.
-        $salt = substr($salt, 0, PASSWORD_SALT_LENGTH);
+        # Newly created salts won't yet have a comma.
+        ($salt) = $salt =~ /^([^,]+),?/;
         $hasher->add($password, $salt);
-        $crypted_password = $salt . $hasher->b64digest . "{$algorithm}";
+        $crypted_password = $salt . ',' . $hasher->b64digest . "{$algorithm}";
     }
 
     # Return the crypted password.