Bug 211006: Make Bugzilla use SHA-256 instead of crypt() to store hashed passwords in the database

Patch By Max Kanat-Alexander <mkanat@bugzilla.org> r=LpSolit, a=LpSolit

4 files changed, 75 insertions, 28 deletions

diff --git a/Bugzilla/Auth/Verify/DB.pm b/Bugzilla/Auth/Verify/DB.pm
index 0f73063d2..695671a31 100644
--- a/Bugzilla/Auth/Verify/DB.pm
+++ b/Bugzilla/Auth/Verify/DB.pm
@@ -64,6 +64,16 @@ sub check_credentials {
     # password tokens they may have generated.
     Bugzilla::Token::DeletePasswordTokens($user_id, "user_logged_in");
 
+    # 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}$/) {
+        my $new_crypted = bz_crypt($password);
+        $dbh->do('UPDATE profiles SET cryptpassword = ? WHERE userid = ?',
+                 undef, $new_crypted, $user_id);
+    }
+
     return $login_data;
 }
 
diff --git a/Bugzilla/Constants.pm b/Bugzilla/Constants.pm
index c08156335..f191f70d4 100644
--- a/Bugzilla/Constants.pm
+++ b/Bugzilla/Constants.pm
@@ -154,6 +154,9 @@ use File::Basename;
     MAX_COMPONENT_SIZE
     MAX_FIELD_VALUE_SIZE
     MAX_FREETEXT_LENGTH
+
+    PASSWORD_DIGEST_ALGORITHM
+    PASSWORD_SALT_LENGTH
 );
 
 @Bugzilla::Constants::EXPORT_OK = qw(contenttypes);
@@ -437,6 +440,15 @@ use constant MAX_FIELD_VALUE_SIZE => 64;
 # Maximum length allowed for free text fields.
 use constant MAX_FREETEXT_LENGTH => 255;
 
+# 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.
+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.
+use constant PASSWORD_SALT_LENGTH => 8;
+
 sub bz_locations {
     # We know that Bugzilla/Constants.pm must be in %INC at this point.
     # So the only question is, what's the name of the directory
diff --git a/Bugzilla/Install/Requirements.pm b/Bugzilla/Install/Requirements.pm
index 5456fc7d4..2630a4104 100644
--- a/Bugzilla/Install/Requirements.pm
+++ b/Bugzilla/Install/Requirements.pm
@@ -65,6 +65,11 @@ sub REQUIRED_MODULES {
         version => (vers_cmp($perl_ver, '5.10') > -1) ? '3.33' : '3.21'
     },
     {
+        package => 'Digest-SHA',
+        module  => 'Digest::SHA',
+        version => 0
+    },
+    {
         package => 'TimeDate',
         module  => 'Date::Format',
         version => '2.21'
diff --git a/Bugzilla/Util.pm b/Bugzilla/Util.pm
index 982e34c93..376bcf6cd 100644
--- a/Bugzilla/Util.pm
+++ b/Bugzilla/Util.pm
@@ -52,6 +52,8 @@ use Date::Parse;
 use Date::Format;
 use DateTime;
 use DateTime::TimeZone;
+use Digest;
+use Scalar::Util qw(tainted);
 use Text::Wrap;
 
 # This is from the perlsec page, slightly modified to remove a warning
@@ -476,37 +478,54 @@ sub file_mod_time {
 sub bz_crypt {
     my ($password, $salt) = @_;
 
+    my $algorithm;
     if (!defined $salt) {
-        # The list of characters that can appear in a salt.  Salts and hashes
-        # are both encoded as a sequence of characters from a set containing
-        # 64 characters, each one of which represents 6 bits of the salt/hash.
-        # The encoding is similar to BASE64, the difference being that the
-        # BASE64 plus sign (+) is replaced with a forward slash (/).
-        my @saltchars = (0..9, 'A'..'Z', 'a'..'z', '.', '/');
-
-        # Generate the salt.  We use an 8 character (48 bit) salt for maximum
-        # security on systems whose crypt uses MD5.  Systems with older
-        # versions of crypt will just use the first two characters of the salt.
-        $salt = '';
-        for ( my $i=0 ; $i < 8 ; ++$i ) {
-            $salt .= $saltchars[rand(64)];
-        }
+        # If you don't use a salt, then people can create tables of
+        # hashes that map to particular passwords, and then break your
+        # hashing very easily if they have a large-enough table of common
+        # (or even uncommon) passwords. So we generate a unique salt for
+        # each password in the database, and then just prepend it to
+        # the hash.
+        $salt = generate_random_password(PASSWORD_SALT_LENGTH);
+        $algorithm = PASSWORD_DIGEST_ALGORITHM;
     }
 
-    # Wide characters cause crypt to die
-    if (Bugzilla->params->{'utf8'}) {
-        utf8::encode($password) if utf8::is_utf8($password);
+    # We append the algorithm used to the string. This is good because then
+    # we can change the algorithm being used, in the future, without 
+    # disrupting the validation of existing passwords. Also, this tells
+    # us if a password is using the old "crypt" method of hashing passwords,
+    # because the algorithm will be missing from the string.
+    if ($salt =~ /{([^}]+)}$/) {
+        $algorithm = $1;
     }
-    
-    # Crypt the password.
-    my $cryptedpassword = crypt($password, $salt);
 
-    # HACK: Perl has bug where returned crypted password is considered tainted
-    # Upstream Bug: http://rt.perl.org/rt3/Public/Bug/Display.html?id=59998
-    trick_taint($cryptedpassword) unless (is_tainted($password) || is_tainted($salt));
+    my $crypted_password;
+    if (!$algorithm) {
+        # Wide characters cause crypt to die
+        if (Bugzilla->params->{'utf8'}) {
+            utf8::encode($password) if utf8::is_utf8($password);
+        }
+    
+        # Crypt the password.
+        $crypted_password = crypt($password, $salt);
+
+        # HACK: Perl has bug where returned crypted password is considered
+        # tainted. See http://rt.perl.org/rt3/Public/Bug/Display.html?id=59998
+        unless(tainted($password) || tainted($salt)) {
+            trick_taint($crypted_password);
+        } 
+    }
+    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);
+        $hasher->add($password, $salt);
+        $crypted_password = $salt . $hasher->b64digest . "{$algorithm}";
+    }
 
     # Return the crypted password.
-    return $cryptedpassword;
+    return $crypted_password;
 }
 
 sub generate_random_password {
@@ -932,11 +951,12 @@ of the "mtime" parameter of the perl "stat" function.
 
 =item C<bz_crypt($password, $salt)>
 
-Takes a string and returns a C<crypt>ed value for it, using a random salt.
-An optional salt string may also be passed in.
+Takes a string and returns a hashed (encrypted) value for it, using a
+random salt. An optional salt string may also be passed in.
 
-Please always use this function instead of the built-in perl "crypt"
-when initially encrypting a password.
+Please always use this function instead of the built-in perl C<crypt>
+function, when checking or setting a password. Bugzilla does not use
+C<crypt>.
 
 =begin undocumented