diff options
| -rw-r--r-- | Bugzilla/Auth/Verify/DB.pm | 10 | ||||
| -rw-r--r-- | Bugzilla/Constants.pm | 12 | ||||
| -rw-r--r-- | Bugzilla/Install/Requirements.pm | 5 | ||||
| -rw-r--r-- | Bugzilla/Util.pm | 76 |
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 |