Bugzilla – Attachment 20578 Details for
Bug 9611
Changing the password hashing algorithm from MD5 to more secure Bcrypt
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 9611: Change the password hashing algorithm from MD5 to Bcrypt
Bug-9611-Change-the-password-hashing-algorithm-fro.patch (text/plain), 12.81 KB, created by
Srdjan Jankovic
on 2013-08-23 04:56:41 UTC
(
hide
)
Description:
Bug 9611: Change the password hashing algorithm from MD5 to Bcrypt
Filename:
MIME Type:
Creator:
Srdjan Jankovic
Created:
2013-08-23 04:56:41 UTC
Size:
12.81 KB
patch
obsolete
>From 93944e7882495a1fda619a135e9e4f1fa548b134 Mon Sep 17 00:00:00 2001 >From: Srikanth Dhondi <srikanth@catalyst.net.nz> >Date: Wed, 13 Feb 2013 14:40:02 +1300 >Subject: [PATCH] Bug 9611: Change the password hashing algorithm from MD5 to > Bcrypt > >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> >--- > C4/Auth.pm | 114 +++++++++++++++++++++++++++++++++++++++++++-- > C4/Members.pm | 35 +++----------- > members/member-password.pl | 2 +- > opac/opac-passwd.pl | 13 ++++-- > 4 files changed, 126 insertions(+), 38 deletions(-) > >diff --git a/C4/Auth.pm b/C4/Auth.pm >index 1e17e59..7674691 100644 >--- a/C4/Auth.pm >+++ b/C4/Auth.pm >@@ -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; >@@ -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; >@@ -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 ) = @_; >@@ -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 ); >@@ -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 ); >diff --git a/C4/Members.pm b/C4/Members.pm >index 7ac69df..f58bb76 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); > >@@ -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(). >@@ -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 >@@ -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 >@@ -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"); > >@@ -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(); >diff --git a/members/member-password.pl b/members/member-password.pl >index bed8fc4..d2a9a31 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)) { >diff --git a/opac/opac-passwd.pl b/opac/opac-passwd.pl >index 19425f7..923a5cf 100755 >--- a/opac/opac-passwd.pl >+++ b/opac/opac-passwd.pl >@@ -29,6 +29,7 @@ use Digest::MD5 qw(md5_base64); > use C4::Circulation; > use C4::Members; > use C4::Output; >+use C4::Auth qw(hash_password); > > my $query = new CGI; > my $dbh = C4::Context->dbh; >@@ -57,7 +58,7 @@ if ( C4::Context->preference("OpacPasswordChange") ) { > 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 ); >@@ -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; } >-- >1.8.1.2
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 9611
:
15311
|
15427
|
15428
|
15429
|
15505
|
15506
|
15507
|
15508
|
15509
|
15510
|
15514
|
15515
|
15516
|
15517
|
15518
|
15519
|
15520
|
15521
|
15522
|
17203
|
17204
|
20252
|
20253
|
20254
|
20255
|
20576
|
20577
|
20578
|
20621
|
20622
|
20623
|
20624
|
21614
|
21615
|
21621
|
21622
|
21623
|
21624
|
21625
|
21626
|
21627
|
21628
|
21767
|
21768
|
21769
|
21770
|
21771
|
21772
|
21775
|
21776
|
21777
|
21789
|
21790
|
21791
|
21792
|
21793
|
21794
|
21795
|
21796