Bug 278792 : Move Crypt() to Bugzilla::Auth

Patch by Max Kanat-Alexander <mkanat@kerio.com>   r=vladd  a=justdave

7 files changed, 70 insertions, 42 deletions

diff --git a/Bugzilla/Auth.pm b/Bugzilla/Auth.pm
index 71b125e45..6071d3abd 100644
--- a/Bugzilla/Auth.pm
+++ b/Bugzilla/Auth.pm
@@ -23,6 +23,8 @@
 package Bugzilla::Auth;
 
 use strict;
+use Exporter qw(import);
+@Bugzilla::Auth::EXPORT = qw(bz_crypt);
 
 use Bugzilla::Config;
 use Bugzilla::Constants;
@@ -42,6 +44,31 @@ BEGIN {
     }
 }
 
+sub bz_crypt ($) {
+    my ($password) = @_;
+
+    # The list of characters that can appear in a salt.  Salts and hashes
+    # are both encoded as a sequence of characters from a set containing
+    # 64 characters, each one of which represents 6 bits of the salt/hash.
+    # The encoding is similar to BASE64, the difference being that the
+    # BASE64 plus sign (+) is replaced with a forward slash (/).
+    my @saltchars = (0..9, 'A'..'Z', 'a'..'z', '.', '/');
+
+    # Generate the salt.  We use an 8 character (48 bit) salt for maximum
+    # security on systems whose crypt uses MD5.  Systems with older
+    # versions of crypt will just use the first two characters of the salt.
+    my $salt = '';
+    for ( my $i=0 ; $i < 8 ; ++$i ) {
+        $salt .= $saltchars[rand(64)];
+    }
+
+    # Crypt the password.
+    my $cryptedpassword = crypt($password, $salt);
+
+    # Return the crypted password.
+    return $cryptedpassword;
+}
+
 # PRIVATE
 
 # A number of features, like password change requests, require the DB
@@ -128,6 +155,11 @@ __END__
 
 Bugzilla::Auth - Authentication handling for Bugzilla users
 
+=head1 SYNOPSIS
+
+  # Class Functions
+  $crypted = bz_crypt($password);
+
 =head1 DESCRIPTION
 
 Handles authentication for Bugzilla users.
@@ -147,6 +179,23 @@ authentication or login modules.
 
 =over 4
 
+=item C<bz_crypt($password)>
+
+Takes a string and returns a C<crypt>ed value for it, using a random salt.
+
+Please always use this function instead of the built-in perl "crypt"
+when initially encrypting a password.
+
+=begin undocumented
+
+Random salts are generated because the alternative is usually
+to use the first two characters of the password itself, and since
+the salt appears in plaintext at the beginning of the encrypted
+password string this has the effect of revealing the first two
+characters of the password to anyone who views the encrypted version.
+
+=end undocumented
+
 =item C<Bugzilla::Auth::get_netaddr($ipaddr)>
 
 Given an ip address, this returns the associated network address, using
diff --git a/Bugzilla/Auth/Verify/DB.pm b/Bugzilla/Auth/Verify/DB.pm
index ec13bacf8..1d5c6850c 100644
--- a/Bugzilla/Auth/Verify/DB.pm
+++ b/Bugzilla/Auth/Verify/DB.pm
@@ -111,7 +111,7 @@ sub check_password {
 sub change_password {
     my ($class, $userid, $password) = @_;
     my $dbh = Bugzilla->dbh;
-    my $cryptpassword = Crypt($password);
+    my $cryptpassword = bz_crypt($password);
     $dbh->do("UPDATE profiles SET cryptpassword = ? WHERE userid = ?", 
              undef, $cryptpassword, $userid);
 }
diff --git a/checksetup.pl b/checksetup.pl
index 6bbedaaea..37f96df0a 100755
--- a/checksetup.pl
+++ b/checksetup.pl
@@ -1464,6 +1464,14 @@ if ($^O !~ /MSWin32/i) {
 # The only use for loading globals.pl is for Crypt(), which should at some
 # point probably be factored out into Bugzilla::Auth::*
 
+# XXX - bug 278792: Crypt has been moved to Bugzilla::Auth::bz_crypt.
+# This section is probably no longer needed, but we need to make sure
+# that things still work if we remove globals.pl. So that's for later.
+
+# It's safe to use Bugzilla::Auth here because parameters have now been
+# defined.
+use Bugzilla::Auth;
+
 # globals.pl clears the PATH, but File::Find uses Cwd::cwd() instead of
 # Cwd::getcwd(), which we need to do because `pwd` isn't in the path - see
 # http://www.xray.mpe.mpg.de/mailing-lists/perl5-porters/2001-09/msg00115.html
@@ -2776,7 +2784,7 @@ if (GetFieldDef('bugs', 'long_desc')) {
                                  "(login_name, cryptpassword," .
                                  " disabledtext) VALUES (" .
                                  $dbh->quote($name) .
-                                 ", " . $dbh->quote(Crypt('okthen')) . ", " . 
+                                 ", " . $dbh->quote(bz_crypt('okthen')) . ", " . 
                                  "'Account created only to maintain database integrity')");
                         $s2 = $dbh->prepare("SELECT LAST_INSERT_ID()");
                         $s2->execute();
@@ -3200,7 +3208,7 @@ ENDTEXT
 
     print "Fixing password #1... ";
     while (my ($userid, $password) = $sth->fetchrow_array()) {
-        my $cryptpassword = $dbh->quote(Crypt($password));
+        my $cryptpassword = $dbh->quote(bz_crypt($password));
         $dbh->do("UPDATE profiles SET cryptpassword = $cryptpassword WHERE userid = $userid");
         ++$i;
         # Let the user know where we are at every 500 records.
@@ -4474,7 +4482,7 @@ if ($sth->rows == 0) {
     }
 
     # Crypt the administrator's password
-    my $cryptedpassword = Crypt($pass1);
+    my $cryptedpassword = bz_crypt($pass1);
 
     if ($^O !~ /MSWin32/i) {
         system("stty","echo"); # re-enable input echoing
diff --git a/editusers.cgi b/editusers.cgi
index a70e3fcf2..a1eccd956 100755
--- a/editusers.cgi
+++ b/editusers.cgi
@@ -38,6 +38,7 @@ require "globals.pl";
 use Bugzilla;
 use Bugzilla::User;
 use Bugzilla::Constants;
+use Bugzilla::Auth;
 
 # Shut up misguided -w warnings about "used only once".  "use vars" just
 # doesn't work for me.
@@ -452,7 +453,7 @@ if ($action eq 'new') {
             "emailflags, disabledtext" .
             " ) VALUES ( " .
             SqlQuote($user) . "," .
-            SqlQuote(Crypt($password)) . "," .
+            SqlQuote(bz_crypt($password)) . "," .
             SqlQuote($realname) . "," .
             SqlQuote(Bugzilla::Constants::DEFAULT_EMAIL_SETTINGS) . "," .
             SqlQuote($disabledtext) . ")" );
@@ -784,7 +785,7 @@ if ($action eq 'update') {
     if ( $editall && $password ) {
         my $passworderror = ValidatePassword($password);
         if ( !$passworderror ) {
-            my $cryptpassword = SqlQuote(Crypt($password));
+            my $cryptpassword = SqlQuote(bz_crypt($password));
             my $loginname = SqlQuote($userold);
             SendSQL("UPDATE  profiles
                      SET     cryptpassword = $cryptpassword
diff --git a/globals.pl b/globals.pl
index 694d02f49..0badac43e 100644
--- a/globals.pl
+++ b/globals.pl
@@ -34,6 +34,7 @@ use Bugzilla::Util;
 # Bring ChmodDataFile in until this is all moved to the module
 use Bugzilla::Config qw(:DEFAULT ChmodDataFile $localconfig $datadir);
 use Bugzilla::BugMail;
+use Bugzilla::Auth;
 
 # Shut up misguided -w warnings about "used only once".  For some reason,
 # "use vars" chokes on me when I try it here.
@@ -414,7 +415,7 @@ sub InsertNewUser {
 
     # Generate a new random password for the user.
     my $password = GenerateRandomPassword();
-    my $cryptpassword = Crypt($password);
+    my $cryptpassword = bz_crypt($password);
 
 
     my $defaultflagstring = SqlQuote(Bugzilla::Constants::DEFAULT_EMAIL_SETTINGS); 
@@ -696,39 +697,6 @@ sub ValidatePassword {
     }
 }
 
-
-sub Crypt {
-    # Crypts a password, generating a random salt to do it.
-    # Random salts are generated because the alternative is usually
-    # to use the first two characters of the password itself, and since
-    # the salt appears in plaintext at the beginning of the crypted
-    # password string this has the effect of revealing the first two
-    # characters of the password to anyone who views the crypted version.
-
-    my ($password) = @_;
-
-    # The list of characters that can appear in a salt.  Salts and hashes
-    # are both encoded as a sequence of characters from a set containing
-    # 64 characters, each one of which represents 6 bits of the salt/hash.
-    # The encoding is similar to BASE64, the difference being that the
-    # BASE64 plus sign (+) is replaced with a forward slash (/).
-    my @saltchars = (0..9, 'A'..'Z', 'a'..'z', '.', '/');
-
-    # Generate the salt.  We use an 8 character (48 bit) salt for maximum
-    # security on systems whose crypt uses MD5.  Systems with older
-    # versions of crypt will just use the first two characters of the salt.
-    my $salt = '';
-    for ( my $i=0 ; $i < 8 ; ++$i ) {
-        $salt .= $saltchars[rand(64)];
-    }
-
-    # Crypt the password.
-    my $cryptedpassword = crypt($password, $salt);
-
-    # Return the crypted password.
-    return $cryptedpassword;
-}
-
 sub DBID_to_real_or_loginname {
     my ($id) = (@_);
     PushGlobalSQLState();
diff --git a/token.cgi b/token.cgi
index 03d0e8b03..8b4636a79 100755
--- a/token.cgi
+++ b/token.cgi
@@ -33,6 +33,7 @@ use vars qw($template $vars);
 
 use Bugzilla;
 use Bugzilla::Constants;
+use Bugzilla::Auth;
 
 my $cgi = Bugzilla->cgi;
 
@@ -192,7 +193,7 @@ sub cancelChangePassword {
 
 sub changePassword {
     # Quote the password and token for inclusion into SQL statements.
-    my $cryptedpassword = Crypt($cgi->param('password'));
+    my $cryptedpassword = bz_crypt($cgi->param('password'));
     my $quotedpassword = SqlQuote($cryptedpassword);
 
     # Get the user's ID from the tokens table.
diff --git a/userprefs.cgi b/userprefs.cgi
index 1c9cf2068..6950fea88 100755
--- a/userprefs.cgi
+++ b/userprefs.cgi
@@ -28,6 +28,7 @@ use lib qw(.);
 use Bugzilla;
 use Bugzilla::Constants;
 use Bugzilla::Search;
+use Bugzilla::Auth;
 
 require "CGI.pl";
 
@@ -94,7 +95,7 @@ sub SaveAccount {
               || ThrowUserError("new_password_missing");
             ValidatePassword($pwd1, $pwd2);
         
-            my $cryptedpassword = SqlQuote(Crypt($pwd1));
+            my $cryptedpassword = SqlQuote(bz_crypt($pwd1));
             SendSQL("UPDATE profiles 
                      SET    cryptpassword = $cryptedpassword 
                      WHERE  userid = $userid");