Bugzilla – Attachment 20252 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), 10.91 KB, created by
Galen Charlton
on 2013-08-09 22:55:50 UTC
(
hide
)
Description:
Bug 9611: Change the password hashing algorithm from MD5 to Bcrypt
Filename:
MIME Type:
Creator:
Galen Charlton
Created:
2013-08-09 22:55:50 UTC
Size:
10.91 KB
patch
obsolete
>From fc44931ba0f6285d1b316b14dccbc2bf5ea8ee51 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 | 54 ++++++++++++++++++++++++++++++++++++++++---- > C4/Members.pm | 35 +++++----------------------- > members/member-password.pl | 2 +- > opac/opac-passwd.pl | 13 ++++++++--- > 4 files changed, 66 insertions(+), 38 deletions(-) > >diff --git a/C4/Auth.pm b/C4/Auth.pm >index 1e17e59..d0b17fd 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 Crypt::Random::Source; > > 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,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 ) = @_; >@@ -1489,10 +1516,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 +1540,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 6d8d23a..2d31a1a 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 >@@ -718,10 +718,10 @@ 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); >+ 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 >@@ -767,10 +767,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"); > >@@ -1439,28 +1438,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.7.10.4
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