[aur-dev] [PATCH] Use SHA-512 to hash passwords

24 Feb 2017

Replace the default hash function used for storing passwords by SHA-256.
A new field in the database indicates whether the password of a user is
already stored using the new algorithm or whether it is still hashed
using MD5. Legacy MD5 hashes are still supported but they are
immediately converted using the new function when the user logs in.

Since big parts of the authentication system needed to be rewritten in
this context, this patch also includes some simplification and
refactoring of all code related to password checking and resetting.

Fixes FS#52297.

Signed-off-by: Lukas Fleischer <lfleischer@archlinux.org>
---
This turned out to be more invasive than I initially expected. It would
be great to have some reviews.

 schema/aur-schema.sql     |   3 +-
 upgrading/4.5.0.txt       |  13 +++++
 web/html/passreset.php    |   5 +-
 web/lib/acctfuncs.inc.php | 145 ++++++++++++++++++++++------------------------
 web/lib/aur.inc.php       |  46 +++------------
 5 files changed, 93 insertions(+), 119 deletions(-)

diff --git a/schema/aur-schema.sql b/schema/aur-schema.sql
index 99f9083..44c2401 100644
--- a/schema/aur-schema.sql
+++ b/schema/aur-schema.sql
@@ -27,7 +27,8 @@ CREATE TABLE Users (
 	Username VARCHAR(32) NOT NULL,
 	Email VARCHAR(254) NOT NULL,
 	HideEmail TINYINT UNSIGNED NOT NULL DEFAULT 0,
-	Passwd CHAR(32) NOT NULL,
+	Passwd VARCHAR(128) NOT NULL,
+	PasswdVersion INTEGER UNSIGNED NOT NULL DEFAULT 0,
 	Salt CHAR(32) NOT NULL DEFAULT '',
 	ResetKey CHAR(32) NOT NULL DEFAULT '',
 	RealName VARCHAR(64) NOT NULL DEFAULT '',
diff --git a/upgrading/4.5.0.txt b/upgrading/4.5.0.txt
index fb0a299..8bb08cd 100644
--- a/upgrading/4.5.0.txt
+++ b/upgrading/4.5.0.txt
@@ -18,3 +18,16 @@ ALTER TABLE Users
 ----
 ALTER TABLE Bans MODIFY IPAddress VARCHAR(45) NULL DEFAULT NULL;
 ----
+
+4. Add PasswdVersion column to the Users table:
+
+---
+ALTER TABLE Users ADD COLUMN PasswdVersion INTEGER UNSIGNED NOT NULL DEFAULT 0;
+UPDATE Users SET PasswdVersion = 1 WHERE Passwd <> '';
+---
+
+5. Resize the Passwd column of the Users table:
+
+---
+ALTER TABLE Users MODIFY Passwd VARCHAR(128) NOT NULL;
+---
diff --git a/web/html/passreset.php b/web/html/passreset.php
index cb2f6bc..e89967d 100644
--- a/web/html/passreset.php
+++ b/web/html/passreset.php
@@ -34,10 +34,7 @@ if (isset($_GET['resetkey'], $_POST['email'], $_POST['password'], $_POST['confir
 	}
 
 	if (empty($error)) {
-		$salt = generate_salt();
-		$hash = salted_hash($password, $salt);
-
-		$error = password_reset($hash, $salt, $resetkey, $email);
+		$error = password_reset($password, $resetkey, $email);
 	}
 } elseif (isset($_POST['email'])) {
 	$email = $_POST['email'];
diff --git a/web/lib/acctfuncs.inc.php b/web/lib/acctfuncs.inc.php
index b3cf612..0e33f42 100644
--- a/web/lib/acctfuncs.inc.php
+++ b/web/lib/acctfuncs.inc.php
@@ -278,7 +278,7 @@ function process_account_form($TYPE,$A,$U="",$T="",$S="",$E="",$H="",$P="",$C=""
 			$email = $E;
 		} else {
 			$send_resetkey = false;
-			$P = salted_hash($P, $salt);
+			$P = salted_hash($P, $salt, passwd_current_version());
 		}
 		$U = $dbh->quote($U);
 		$E = $dbh->quote($E);
@@ -351,7 +351,7 @@ function process_account_form($TYPE,$A,$U="",$T="",$S="",$E="",$H="",$P="",$C=""
 		}
 		if ($P) {
 			$salt = generate_salt();
-			$hash = salted_hash($P, $salt);
+			$hash = salted_hash($P, $salt, passwd_current_version());
 			$q .= ", Passwd = '$hash', Salt = '$salt'";
 		}
 		$q.= ", RealName = " . $dbh->quote($R);
@@ -528,19 +528,24 @@ function try_login() {
 	if (user_suspended($userID)) {
 		$login_error = __('Account suspended');
 		return array('SID' => '', 'error' => $login_error);
-	} elseif (passwd_is_empty($userID)) {
-		$login_error = __('Your password has been reset. ' .
-			'If you just created a new account, please ' .
-			'use the link from the confirmation email ' .
-			'to set an initial password. Otherwise, ' .
-			'please request a reset key on the %s' .
-			'Password Reset%s page.', '<a href="' .
-			htmlspecialchars(get_uri('/passreset')) . '">',
-			'</a>');
-		return array('SID' => '', 'error' => $login_error);
-	} elseif (!valid_passwd($userID, $_REQUEST['passwd'])) {
-		$login_error = __("Bad username or password.");
-		return array('SID' => '', 'error' => $login_error);
+	}
+
+	switch (check_passwd($userID, $_REQUEST['passwd'])) {
+		case -1:
+			$login_error = __('Your password has been reset. ' .
+				'If you just created a new account, please ' .
+				'use the link from the confirmation email ' .
+				'to set an initial password. Otherwise, ' .
+				'please request a reset key on the %s' .
+				'Password Reset%s page.', '<a href="' .
+				htmlspecialchars(get_uri('/passreset')) . '">',
+				'</a>');
+			return array('SID' => '', 'error' => $login_error);
+		case 0:
+			$login_error = __("Bad username or password.");
+			return array('SID' => '', 'error' => $login_error);
+		case 1:
+			break;
 	}
 
 	$logged_in = 0;
@@ -736,18 +741,22 @@ function send_resetkey($email, $welcome=false) {
 /**
  * Change a user's password in the database if reset key and e-mail are correct
  *
- * @param string $hash New MD5 hash of a user's password
- * @param string $salt New salt for the user's password
+ * @param string $password The new password
  * @param string $resetkey Code e-mailed to a user to reset a password
  * @param string $email E-mail address of the user resetting their password
  *
  * @return string|void Redirect page if successful, otherwise return error message
  */
-function password_reset($hash, $salt, $resetkey, $email) {
+function password_reset($password, $resetkey, $email) {
+	$ver = passwd_current_version();
+	$salt = generate_salt();
+	$hash = salted_hash($password, $salt, $ver);
+
 	$dbh = DB::connect();
 	$q = "UPDATE Users ";
-	$q.= "SET Passwd = '$hash', ";
-	$q.= "Salt = '$salt', ";
+	$q.= "SET PasswdVersion = " . intval($ver) . ", ";
+	$q.= "Passwd = '" . $dbh->quote($hash) . "', ";
+	$q.= "Salt = '" . $dbh->quote($salt) . "', ";
 	$q.= "ResetKey = '' ";
 	$q.= "WHERE ResetKey != '' ";
 	$q.= "AND ResetKey = " . $dbh->quote($resetkey) . " ";
@@ -778,75 +787,59 @@ function good_passwd($passwd) {
 /**
  * Determine if the password is correct and salt it if it hasn't been already
  *
- * @param string $userID The user ID to check the password against
+ * @param int $user_id The user ID to check the password against
  * @param string $passwd The password the visitor sent
  *
- * @return bool True if password was correct and properly salted, otherwise false
+ * @return int Positive if password is correct, negative if password is unset
  */
-function valid_passwd($userID, $passwd) {
+function check_passwd($user_id, $passwd) {
 	$dbh = DB::connect();
-	if ($passwd == "") {
-		return false;
-	}
 
-	/* Get salt for this user. */
-	$salt = get_salt($userID);
-	if ($salt) {
-		$q = "SELECT ID FROM Users ";
-		$q.= "WHERE ID = " . $userID . " ";
-		$q.= "AND Passwd = " . $dbh->quote(salted_hash($passwd, $salt));
-		$result = $dbh->query($q);
-		if (!$result) {
-			return false;
-		}
-
-		$row = $result->fetch(PDO::FETCH_NUM);
-		return ($row[0] > 0);
-	} else {
-		/* Check password without using salt. */
-		$q = "SELECT ID FROM Users ";
-		$q.= "WHERE ID = " . $userID . " ";
-		$q.= "AND Passwd = " . $dbh->quote(md5($passwd));
-		$result = $dbh->query($q);
-		if (!$result) {
-			return false;
-		}
-
-		$row = $result->fetch(PDO::FETCH_NUM);
-		if (!$row[0]) {
-			return false;
-		}
+	/* Get password version, hash, as well as salt and authenticate. */
+	$q = "SELECT PasswdVersion, Passwd, Salt FROM Users ";
+	$q.= "WHERE ID = " . intval($user_id);
+	$result = $dbh->query($q);
+	if (!$result) {
+		return 0;
+	}
+	$row = $result->fetch(PDO::FETCH_ASSOC);
+	if (!$row) {
+		return 0;
+	}
+	$ver = $row['PasswdVersion'];
+	$hash = $row['Passwd'];
+	$salt = $row['Salt'];
+	if ($ver == 0 || !$hash) {
+		return -1;
+	}
+	if (!$passwd || $hash != salted_hash($passwd, $salt, $ver)) {
+		return 0;
+	}
 
-		/* Password correct, but salt it first! */
-		if (!save_salt($userID, $passwd)) {
-			trigger_error("Unable to salt user's password;" .
-				" ID " . $userID, E_USER_WARNING);
-			return false;
-		}
+	/* Password correct, migrate the hash or add salt if necessary. */
+	if (!$salt || $ver != passwd_current_version()) {
+		$ver = passwd_current_version();
+		$salt = generate_salt();
+		$hash = salted_hash($passwd, $salt, $ver);
 
-		return true;
+		$q = "UPDATE Users SET ";
+		$q.= "PasswdVersion = " . intval($ver) . ", ";
+		$q.= "Passwd = " . $dbh->quote($hash) . ", ";
+		$q.= "Salt = " . $dbh->quote($salt) . " ";
+		$q.= "WHERE ID = " . intval($user_id);
+		$dbh->query($q);
 	}
+
+	return 1;
 }
 
 /**
- * Determine if a user's password is empty
- *
- * @param string $uid The user ID to check for an empty password
+ * Determine the current password version
  *
- * @return bool True if the user's password is empty, otherwise false
+ * @return int The maximum version currently supported
  */
-function passwd_is_empty($uid) {
-	$dbh = DB::connect();
-
-	$q = "SELECT * FROM Users WHERE ID = " . $dbh->quote($uid) . " ";
-	$q .= "AND Passwd = " . $dbh->quote('');
-	$result = $dbh->query($q);
-
-	if ($result->fetchColumn()) {
-		return true;
-	} else {
-		return false;
-	}
+function passwd_current_version() {
+	return 2;
 }
 
 /**
diff --git a/web/lib/aur.inc.php b/web/lib/aur.inc.php
index 94a3849..d81328e 100644
--- a/web/lib/aur.inc.php
+++ b/web/lib/aur.inc.php
@@ -538,39 +538,6 @@ function mkurl($append) {
 }
 
 /**
- * Determine a user's salt from the database
- *
- * @param string $user_id The user ID of the user trying to log in
- *
- * @return string|void Return the salt for the requested user, otherwise void
- */
-function get_salt($user_id) {
-	$dbh = DB::connect();
-	$q = "SELECT Salt FROM Users WHERE ID = " . $user_id;
-	$result = $dbh->query($q);
-	if ($result) {
-		$row = $result->fetch(PDO::FETCH_NUM);
-		return $row[0];
-	}
-	return;
-}
-
-/**
- * Save a user's salted password in the database
- *
- * @param string $user_id The user ID of the user who is salting their password
- * @param string $passwd The password of the user logging in
- */
-function save_salt($user_id, $passwd) {
-	$dbh = DB::connect();
-	$salt = generate_salt();
-	$hash = salted_hash($passwd, $salt);
-	$q = "UPDATE Users SET Salt = " . $dbh->quote($salt) . ", ";
-	$q.= "Passwd = " . $dbh->quote($hash) . " WHERE ID = " . $user_id;
-	return $dbh->exec($q);
-}
-
-/**
  * Generate a string to be used for salting passwords
  *
  * @return string MD5 hash of concatenated random number and current time
@@ -584,14 +551,17 @@ function generate_salt() {
  *
  * @param string $passwd User plaintext password
  * @param string $salt MD5 hash to be used as user salt
+ * @param init $ver The version of the hash method to use
  *
- * @return string The MD5 hash of the concatenated salt and user password
+ * @return string The hash of the concatenated salt and user password
  */
-function salted_hash($passwd, $salt) {
-	if (strlen($salt) != 32) {
-		trigger_error('Salt does not look like an md5 hash', E_USER_WARNING);
+function salted_hash($passwd, $salt, $ver) {
+	switch ($ver) {
+		case 1:
+			return md5($salt . $passwd);
+		case 2:
+			return hash('sha512', $salt . $passwd);
 	}
-	return md5($salt . $passwd);
 }
 
 /**
-- 
2.11.1

    