Bugzilla – Attachment 15518 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 - Changing the hashing algorithm from MD5 to Bcrypt
Bug-9611---Changing-the-hashing-algorithm-from-MD5.patch (text/plain), 8.77 KB, created by
Mason James
on 2013-02-19 04:36:48 UTC
(
hide
)
Description:
Bug 9611 - Changing the hashing algorithm from MD5 to Bcrypt
Filename:
MIME Type:
Creator:
Mason James
Created:
2013-02-19 04:36:48 UTC
Size:
8.77 KB
patch
obsolete
>From c1951ffd2e678a378b77032f5afe0fc586893cb0 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 - 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 <bgkriegel@gmail.com> > >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 <mtj@kohaaloha.com> >--- > 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
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