Skip to content

Commit

Permalink
Bug 9611: Change the password hashing algorithm from MD5 to Bcrypt
Browse files Browse the repository at this point in the history 
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 made via the staff interface or the OPAC will
   be automatically Bcrypt-hashed; this applies to old users 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
     users:
       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) In the staff interface, change the password of an existing user
     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
  4) In the OPAC, verify that
    a) Old user with old pass can change password, new format
    b) New user with new pass can change password
    c) Old and new user with self-updated pass can login

Whitespace cleanup was contributed by  Bernardo Gonzalez Kriegel.

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Signed-off-by: Mason James <mtj@kohaaloha.com>
Signed-off-by: Galen Charlton <gmc@esilibrary.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Galen Charlton <gmc@esilibrary.com>
  • Loading branch information
@gmcharlt
Srikanth Dhondi authored and gmcharlt committed Oct 3, 2013
1 parent 88936cb commit f2162a8
Show file tree
Hide file tree
Showing 4 changed files with 126 additions and 38 deletions.
114 changes: 109 additions & 5 deletions C4/Auth.pm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@ use Digest::MD5 qw(md5_base64);
use JSON qw/encode_json decode_json/;
use URI::Escape;
use CGI::Session;
use Crypt::Eksblowfish::Bcrypt qw(bcrypt en_base64);
use Fcntl qw/O_RDONLY/; # O_RDONLY is used in generate_salt

require Exporter;
use C4::Context;
Expand All @@ -47,7 +49,7 @@ BEGIN {
@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
ParseSearchHistoryCookie
ParseSearchHistoryCookie hash_password
);
%EXPORT_TAGS = ( EditPermissions => [qw(get_all_subpermissions get_user_subpermissions)] );
$ldap = C4::Context->config('useldapserver') || 0;
Expand Down Expand Up @@ -1464,6 +1466,91 @@ 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('weak', 16));
}
# Encrypt it
return bcrypt($password, $settings);
}

=head2 generate_salt
use C4::Auth;
my $salt = C4::Auth::generate_salt($strength, $length);
=item strength
For general password salting a C<$strength> of C<weak> is recommend,
For generating a server-salt a C<$strength> of C<strong> is recommended
'strong' uses /dev/random which may block until sufficient entropy is acheived.
'weak' uses /dev/urandom and is non-blocking.
=back
=item length
C<$length> is a positive integer which specifies the desired length of the returned string
=back
=cut


# the implementation of generate_salt is loosely based on Crypt::Random::Provider::File
sub generate_salt {
# strength is 'strong' or 'weak'
# length is number of bytes to read, positive integer
my ($strength, $length) = @_;

my $source;

if( $length < 1 ){
die "non-positive strength of '$strength' passed to C4::Auth::generate_salt\n";
}

if( $strength eq "strong" ){
$source = '/dev/random'; # blocking
} else {
unless( $strength eq 'weak' ){
warn "unsuppored strength of '$strength' passed to C4::Auth::generate_salt, defaulting to 'weak'\n";
}
$source = '/dev/urandom'; # non-blocking
}

sysopen SOURCE, $source, O_RDONLY
or die "failed to open source '$source' in C4::Auth::generate_salt\n";

# $bytes is the bytes just read
# $string is the concatenation of all the bytes read so far
my( $bytes, $string ) = ("", "");

# keep reading until we have $length bytes in $strength
while( length($string) < $length ){
# return the number of bytes read, 0 (EOF), or -1 (ERROR)
my $return = sysread SOURCE, $bytes, $length - length($string);

# if no bytes were read, keep reading (if using /dev/random it is possible there was insufficient entropy so this may block)
next unless $return;
if( $return == -1 ){
die "error while reading from $source in C4::Auth::generate_salt\n";
}

$string .= $bytes;
}

close SOURCE;
return $string;
}


sub checkpw {

my ( $dbh, $userid, $password, $query ) = @_;
Expand All @@ -1489,10 +1576,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 );
Expand All @@ -1505,10 +1600,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 );
Expand Down
35 changes: 6 additions & 29 deletions C4/Members.pm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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);

Expand Down Expand Up @@ -250,7 +250,7 @@ sub Search {
$filter = [ $filter ];
push @$filter, {"borrowernumber"=>$matching_records};
}
}
}
}

# $showallbranches was not used at the time SearchMember() was mainstreamed into Search().
Expand Down Expand Up @@ -283,7 +283,7 @@ sub Search {
}
$searchtype ||= "start_with";

return SearchInTable( "borrowers", $filter, $orderby, $limit, $columns_out, $search_on_fields, $searchtype );
return SearchInTable( "borrowers", $filter, $orderby, $limit, $columns_out, $search_on_fields, $searchtype );
}

=head2 GetMemberDetails
Expand Down Expand Up @@ -750,11 +750,11 @@ 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 $old_categorycode = GetBorrowerCategorycode( $data{borrowernumber} );
my $execute_success=UpdateInTable("borrowers",\%data);
my $execute_success=UpdateInTable("borrowers",\%data);
if ($execute_success) { # only proceed if the update was a success
# ok if its an adult (type) it may have borrowers that depend on it as a guarantor
# so when we update information for an adult we should check for guarantees and update the relevant part
Expand Down Expand Up @@ -805,10 +805,9 @@ 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);


# mysql_insertid is probably bad. not necessarily accurate and mysql-specific at best.
logaction("MEMBERS", "CREATE", $data{'borrowernumber'}, "") if C4::Context->preference("BorrowersLog");

Expand Down Expand Up @@ -1466,28 +1465,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();
Expand Down
2 changes: 1 addition & 1 deletion members/member-password.pl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@
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)) {
Expand Down
13 changes: 10 additions & 3 deletions opac/opac-passwd.pl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
use C4::Circulation;
use C4::Members;
use C4::Output;
use C4::Auth qw(hash_password);

my $query = new CGI;
my $dbh = C4::Context->dbh;
Expand Down Expand Up @@ -57,7 +58,7 @@
if ( $query->param('Newkey') eq $query->param('Confirm')
&& length( $query->param('Confirm') ) >= $minpasslen )
{ # Record password
my $clave = md5_base64( $query->param('Newkey') );
my $clave = hash_password( $query->param('Newkey') );
$sth->execute( $clave, $borrowernumber );
$template->param( 'password_updated' => '1' );
$template->param( 'borrowernumber' => $borrowernumber );
Expand Down Expand Up @@ -113,8 +114,14 @@ sub goodkey {
$dbh->prepare("SELECT password FROM borrowers WHERE borrowernumber=?");
$sth->execute($borrowernumber);
if ( $sth->rows ) {
my ($md5password) = $sth->fetchrow;
if ( md5_base64($key) eq $md5password ) { return 1; }
my $hash;
my ($stored_hash) = $sth->fetchrow;
if ( substr($stored_hash,0,2) eq '$2') {
$hash = hash_password($key, $stored_hash);
} else {
$hash = md5_base64($key);
}
if ( $hash eq $stored_hash ) { return 1; }
else { return 0; }
}
else { return 0; }
Expand Down

0 comments on commit f2162a8

Please sign in to comment.