From c1951ffd2e678a378b77032f5afe0fc586893cb0 Mon Sep 17 00:00:00 2001 From: Srikanth Dhondi Date: Wed, 13 Feb 2013 14:40:02 +1300 Subject: [PATCH] Bug 9611 - Changing the hashing algorithm from MD5 to Bcrypt Content-Type: text/plain; charset="utf-8" http://koha-community.org What this patch aims to accomplish? * All new passwords are stored as Bcrypt-hashes * For password verification: - If the user was created before this patch was applied then use MD5 to hash the entered password <-- backwards compatibility - If the user was created after this patch was applied then use Bcrypt to hash the entered password * Any password change will be automatically Bcrypt-hashed, this applies to old members whose passwords were stored as MD5 hashes previously Test plan: 1) Add new users and check whether their passwords are stored as Bcrypt hashes or not 2) To test that authentication works for both old as well as new members a) Login as an existing user whose password is stored as a MD5 hash b) Login as an existing user whose password is stored as a Bcrypt hash 3) Change the password of an existing member whose password is stored as an MD5 hash a) Check the new password is stored as a Bcrypt-hash in the database b) Try to login with the new password Signed-off-by: Bernardo Gonzalez Kriegel Comment: Work as described. Small tabulation errors fixed in followup. Test with patches 1-3 applied, run updatedatabase 1) Old user can login 2) New user can login 3) User with updated password can login 4) Inspection of DB shows different passwords length Signed-off-by: Mason James --- C4/Auth.pm | 54 +++++++++++++++++++++++++++++++++++++++---- C4/Members.pm | 33 ++++++-------------------- members/member-password.pl | 2 +- 3 files changed, 58 insertions(+), 31 deletions(-) diff --git a/C4/Auth.pm b/C4/Auth.pm index 0c57a3f..e482ae4 100644 --- a/C4/Auth.pm +++ b/C4/Auth.pm @@ -23,6 +23,8 @@ use Digest::MD5 qw(md5_base64); use Storable qw(thaw freeze); use URI::Escape; use CGI::Session; +use Crypt::Eksblowfish::Bcrypt qw(bcrypt en_base64); +use Crypt::Random::Source; require Exporter; use C4::Context; @@ -46,7 +48,7 @@ BEGIN { $debug = $ENV{DEBUG}; @ISA = qw(Exporter); @EXPORT = qw(&checkauth &get_template_and_user &haspermission &get_user_subpermissions); - @EXPORT_OK = qw(&check_api_auth &get_session &check_cookie_auth &checkpw &get_all_subpermissions &get_user_subpermissions); + @EXPORT_OK = qw(&check_api_auth hash_password &get_session &check_cookie_auth &checkpw &get_all_subpermissions &get_user_subpermissions); %EXPORT_TAGS = ( EditPermissions => [qw(get_all_subpermissions get_user_subpermissions)] ); $ldap = C4::Context->config('useldapserver') || 0; $cas = C4::Context->preference('casAuthentication'); @@ -1429,6 +1431,31 @@ sub get_session { return $session; } +# Using Bcrypt method for hashing. This can be changed to something else in future, if needed. +sub hash_password { + my $password = shift; + + # Generate a salt if one is not passed + my $settings = shift; + unless( defined $settings ){ # if there are no settings, we need to create a salt and append settings + # Set the cost to 8 and append a NULL + $settings = '$2a$08$'.en_base64(generate_salt('strong')); + } + # Encrypt it + return bcrypt($password, $settings); +} + +# Return a random salt +sub generate_salt { + my ($type) = @_; + + if ($type eq 'strong') { + return Crypt::Random::Source::get_strong(16); + } else { + return Crypt::Random::Source::get_weak(16); + } +} + sub checkpw { my ( $dbh, $userid, $password, $query ) = @_; @@ -1454,10 +1481,18 @@ sub checkpw { ); $sth->execute($userid); if ( $sth->rows ) { - my ( $md5password, $cardnumber, $borrowernumber, $userid, $firstname, + my ( $stored_hash, $cardnumber, $borrowernumber, $userid, $firstname, $surname, $branchcode, $flags ) = $sth->fetchrow; - if ( md5_base64($password) eq $md5password and $md5password ne "!") { + + # check what encryption algorithm was implemented: Bcrypt - if the hash starts with '$2' it is Bcrypt else md5 + my $hash; + if ( substr($stored_hash,0,2) eq '$2') { + $hash = hash_password($password, $stored_hash); + } else { + $hash = md5_base64($password); + } + if ( $hash eq $stored_hash and $stored_hash ne "!") { C4::Context->set_userenv( "$borrowernumber", $userid, $cardnumber, $firstname, $surname, $branchcode, $flags ); @@ -1470,10 +1505,19 @@ sub checkpw { ); $sth->execute($userid); if ( $sth->rows ) { - my ( $md5password, $cardnumber, $borrowernumber, $userid, $firstname, + my ( $stored_hash, $cardnumber, $borrowernumber, $userid, $firstname, $surname, $branchcode, $flags ) = $sth->fetchrow; - if ( md5_base64($password) eq $md5password ) { + + # check what encryption algorithm was implemented: Bcrypt - if the hash starts with '$2' it is Bcrypt else md5 + my $hash; + if ( substr($stored_hash,0,2) eq '$2') { + $hash = hash_password($password, $stored_hash); + } else { + $hash = md5_base64($password); + } + + if ( $hash eq $stored_hash ) { C4::Context->set_userenv( $borrowernumber, $userid, $cardnumber, $firstname, $surname, $branchcode, $flags ); diff --git a/C4/Members.pm b/C4/Members.pm index 5c857f6..c672962 100644 --- a/C4/Members.pm +++ b/C4/Members.pm @@ -24,7 +24,6 @@ use strict; #use warnings; FIXME - Bug 2505 use C4::Context; use C4::Dates qw(format_date_in_iso format_date); -use Digest::MD5 qw(md5_base64); use String::Random qw( random_string ); use Date::Calc qw/Today Add_Delta_YM check_date Date_to_Days/; use C4::Log; # logaction @@ -40,6 +39,7 @@ use DateTime; use DateTime::Format::DateParse; use Koha::DateUtils; use Text::Unaccent qw( unac_string ); +use C4::Auth qw(hash_password); our ($VERSION,@ISA,@EXPORT,@EXPORT_OK,$debug); @@ -719,7 +719,7 @@ sub ModMember { if ($data{password} eq '****' or $data{password} eq '') { delete $data{password}; } else { - $data{password} = md5_base64($data{password}); + $data{password} = hash_password($data{password}); } } my $execute_success=UpdateInTable("borrowers",\%data); @@ -768,10 +768,15 @@ sub AddMember { } # create a disabled account if no password provided - $data{'password'} = ($data{'password'})? md5_base64($data{'password'}) : '!'; + $data{'password'} = ($data{'password'})? hash_password($data{'password'}) : '!'; $data{'borrowernumber'}=InsertInTable("borrowers",\%data); + # generate a proper login if none provided + $data{'userid'} = Generate_Userid($data{'borrowernumber'}, $data{'firstname'}, $data{'surname'}) if $data{'userid'} eq ''; + # create a disabled account if no password provided + $data{'password'} = ($data{'password'})? hash_password($data{'password'}) : '!'; + $data{'borrowernumber'}=InsertInTable("borrowers",\%data); # mysql_insertid is probably bad. not necessarily accurate and mysql-specific at best. logaction("MEMBERS", "CREATE", $data{'borrowernumber'}, "") if C4::Context->preference("BorrowersLog"); @@ -1386,28 +1391,6 @@ sub GetExpiryDate { } } -=head2 checkuserpassword (OUEST-PROVENCE) - -check for the password and login are not used -return the number of record -0=> NOT USED 1=> USED - -=cut - -sub checkuserpassword { - my ( $borrowernumber, $userid, $password ) = @_; - $password = md5_base64($password); - my $dbh = C4::Context->dbh; - my $sth = - $dbh->prepare( -"Select count(*) from borrowers where borrowernumber !=? and userid =? and password=? " - ); - $sth->execute( $borrowernumber, $userid, $password ); - my $number_rows = $sth->fetchrow; - return $number_rows; - -} - =head2 GetborCatFromCatType ($codes_arrayref, $labels_hashref) = &GetborCatFromCatType(); diff --git a/members/member-password.pl b/members/member-password.pl index fe24df3..c423600 100755 --- a/members/member-password.pl +++ b/members/member-password.pl @@ -55,7 +55,7 @@ my $minpw = C4::Context->preference('minPasswordLength'); push(@errors,'SHORTPASSWORD') if( $newpassword && $minpw && (length($newpassword) < $minpw ) ); if ( $newpassword && !scalar(@errors) ) { - my $digest=md5_base64($input->param('newpassword')); + my $digest=C4::Auth::hash_password($input->param('newpassword')); my $uid = $input->param('newuserid'); my $dbh=C4::Context->dbh; if (changepassword($uid,$member,$digest)) { -- 1.7.2.5