[aur-dev] [PATCH] Use bcrypt to hash passwords

Johannes Löthberg johannes at kyriasis.com
Fri Feb 24 21:16:16 UTC 2017


Just for public-record:  Seem to be good from a quick glance, so 
tentative +1 from me!

On 24/02, Lukas Fleischer wrote:
>Replace the default hash function used for storing passwords by
>password_hash() which internally uses bcrypt. Legacy MD5 hashes are
>still supported and are immediately converted to the new format when a
>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 at archlinux.org>
>---
>This replaces the SHA-512 patch sent earlier. Thanks to Johannes for
>suggesting to use bcrypt instead!
>
>Again, it would be great if somebody could review the new patch!
>
> schema/aur-schema.sql     |   2 +-
> upgrading/4.5.0.txt       |   6 ++
> web/html/passreset.php    |   5 +-
> web/lib/acctfuncs.inc.php | 144 +++++++++++++++++++---------------------------
> web/lib/aur.inc.php       |  57 ------------------
> 5 files changed, 67 insertions(+), 147 deletions(-)
>
>diff --git a/schema/aur-schema.sql b/schema/aur-schema.sql
>index 99f9083..b75a257 100644
>--- a/schema/aur-schema.sql
>+++ b/schema/aur-schema.sql
>@@ -27,7 +27,7 @@ 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(255) NOT NULL,
> 	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..37b2b81 100644
>--- a/upgrading/4.5.0.txt
>+++ b/upgrading/4.5.0.txt
>@@ -18,3 +18,9 @@ ALTER TABLE Users
> ----
> ALTER TABLE Bans MODIFY IPAddress VARCHAR(45) NULL DEFAULT NULL;
> ----
>+
>+4. Resize the Passwd column of the Users table:
>+
>+---
>+ALTER TABLE Users MODIFY Passwd VARCHAR(255) 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..fe61f50 100644
>--- a/web/lib/acctfuncs.inc.php
>+++ b/web/lib/acctfuncs.inc.php
>@@ -272,13 +272,12 @@ function process_account_form($TYPE,$A,$U="",$T="",$S="",$E="",$H="",$P="",$C=""
>
> 	if ($TYPE == "new") {
> 		/* Create an unprivileged user. */
>-		$salt = generate_salt();
> 		if (empty($P)) {
> 			$send_resetkey = true;
> 			$email = $E;
> 		} else {
> 			$send_resetkey = false;
>-			$P = salted_hash($P, $salt);
>+			$P = password_hash($P, PASSWORD_DEFAULT);
> 		}
> 		$U = $dbh->quote($U);
> 		$E = $dbh->quote($E);
>@@ -291,9 +290,9 @@ function process_account_form($TYPE,$A,$U="",$T="",$S="",$E="",$H="",$P="",$C=""
> 		$I = $dbh->quote($I);
> 		$K = $dbh->quote(str_replace(" ", "", $K));
> 		$q = "INSERT INTO Users (AccountTypeID, Suspended, ";
>-		$q.= "InactivityTS, Username, Email, Passwd, Salt, ";
>+		$q.= "InactivityTS, Username, Email, Passwd , ";
> 		$q.= "RealName, LangPreference, Timezone, Homepage, IRCNick, PGPKey) ";
>-		$q.= "VALUES (1, 0, 0, $U, $E, $P, $salt, $R, $L, $TZ ";
>+		$q.= "VALUES (1, 0, 0, $U, $E, $P, $R, $L, $TZ ";
> 		$q.= "$HP, $I, $K)";
> 		$result = $dbh->exec($q);
> 		if (!$result) {
>@@ -350,9 +349,8 @@ function process_account_form($TYPE,$A,$U="",$T="",$S="",$E="",$H="",$P="",$C=""
> 			$q.= ", HideEmail = 0";
> 		}
> 		if ($P) {
>-			$salt = generate_salt();
>-			$hash = salted_hash($P, $salt);
>-			$q .= ", Passwd = '$hash', Salt = '$salt'";
>+			$hash = password_hash($P, PASSWORD_DEFAULT);
>+			$q .= ", Passwd = " . $dbh->quote($hash);
> 		}
> 		$q.= ", RealName = " . $dbh->quote($R);
> 		$q.= ", LangPreference = " . $dbh->quote($L);
>@@ -528,19 +526,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 +739,18 @@ 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) {
>+	$hash = password_hash($password, PASSWORD_DEFAULT);
>+
> 	$dbh = DB::connect();
>-	$q = "UPDATE Users ";
>-	$q.= "SET Passwd = '$hash', ";
>-	$q.= "Salt = '$salt', ";
>+	$q = "UPDATE Users SET ";
>+	$q.= "Passwd = " . $dbh->quote($hash) . ", ";
> 	$q.= "ResetKey = '' ";
> 	$q.= "WHERE ResetKey != '' ";
> 	$q.= "AND ResetKey = " . $dbh->quote($resetkey) . " ";
>@@ -778,75 +781,46 @@ 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;
>-		}
>-
>-		/* 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;
>+	/* Get password version, hash, as well as salt and authenticate. */
>+	$q = "SELECT Passwd, Salt FROM Users WHERE ID = " . intval($user_id);
>+	$result = $dbh->query($q);
>+	if (!$result) {
>+		return 0;
>+	}
>+	$row = $result->fetch(PDO::FETCH_ASSOC);
>+	if (!$row) {
>+		return 0;
>+	}
>+	$hash = $row['Passwd'];
>+	$salt = $row['Salt'];
>+	if (!$hash) {
>+		return -1;
>+	}
>+	if (!password_verify($passwd, $hash)) {
>+		/* Invalid password, fall back to MD5. */
>+		if (md5($salt . $passwd) != $hash) {
>+			return 0;
> 		}
>-
>-		return true;
> 	}
>-}
>-
>-/**
>- * Determine if a user's password is empty
>- *
>- * @param string $uid The user ID to check for an empty password
>- *
>- * @return bool True if the user's password is empty, otherwise false
>- */
>-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);
>+	/* Password correct, migrate the hash if necessary. */
>+	if (password_needs_rehash($hash, PASSWORD_DEFAULT)) {
>+		$hash = password_hash($passwd, PASSWORD_DEFAULT);
>
>-	if ($result->fetchColumn()) {
>-		return true;
>-	} else {
>-		return false;
>+		$q = "UPDATE Users SET Passwd = " . $dbh->quote($hash) . " ";
>+		$q.= "WHERE ID = " . intval($user_id);
>+		$dbh->query($q);
> 	}
>+
>+	return 1;
> }
>
> /**
>diff --git a/web/lib/aur.inc.php b/web/lib/aur.inc.php
>index 94a3849..d58df40 100644
>--- a/web/lib/aur.inc.php
>+++ b/web/lib/aur.inc.php
>@@ -538,63 +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
>- */
>-function generate_salt() {
>-	return md5(uniqid(mt_rand(), true));
>-}
>-
>-/**
>- * Combine salt and password to form a hash
>- *
>- * @param string $passwd User plaintext password
>- * @param string $salt MD5 hash to be used as user salt
>- *
>- * @return string The MD5 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);
>-	}
>-	return md5($salt . $passwd);
>-}
>-
>-/**
>  * Get a package comment
>  *
>  * @param  int $comment_id The ID of the comment
>-- 
>2.11.1

-- 
Sincerely,
  Johannes Löthberg
  PGP Key ID: 0x50FB9B273A9D0BB5
  https://theos.kyriasis.com/~kyrias/
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 1796 bytes
Desc: not available
URL: <https://lists.archlinux.org/pipermail/aur-dev/attachments/20170224/c3cd0b60/attachment.asc>


More information about the aur-dev mailing list