[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