Loui Chang wrote:
> On Mon 19 Apr 2010 15:39 +0200, Linas wrote:
>
>> Loui Chang wrote:
>>
>> Thanks for your support. Here goes an attempt.
>> I have mixed my suggestions with Denis idea of changing the hash algorithm
>> at the same time plus a few bits I found on the way.
>>
>> Actions performed by this patch:
>> salt to be NULL, in which case it is treated as md5 hashed password.
>> *All entries can be automatically updated by a -to be written- script.
>> *Removes add salt on login code, per above.
>> *Salted passwords now use sha512 instead of md5.
>> *Adds requirement on hash extension (usually bundled as static).
>> *try_login() only performs one query to verify user login instead of 5.
>> *generate_salt now uses mt_rand()
>> *Reject passwords given by GET.
>>
> Sorry for the late response.
> Ideally each of these actions should be a separate patch.
>
> That would enable changes that are straightforward to be applied
> quickly, while the other changes might still need review.
> The -to be written- script also needs to be included for the change to
> be complete.
>
> Cheers.
>
Ok. I have splitted it on three patches.
The first patch with the changes to acctfuncs.inc, which speeds up
try_login,
affect the lazy salting, and changes the $_REQUEST to $_POST.
A second patch adding the mt_rand (the one-line change to generate_salt).
A third patch actually using sha512 (salted_hash function, aur-schema.sql
and README) with a format that allows the script update to work.
Should the update script try to perform the alter table itself? If so, assuming
that it has the Salt field (ie. Kobozev change was applied to the db) or not?
diff --git a/web/lib/acctfuncs.inc b/web/lib/acctfuncs.inc
index 9c172bb..d87d5e6 100644
--- a/web/lib/acctfuncs.inc
+++ b/web/lib/acctfuncs.inc
@@ -601,25 +601,19 @@ function display_account_info($U="", $T="", $E="", $R="", $I="") {
* SID of 0 means login failed.
*/
function try_login() {
- $login_error = "";
+ $login_error = "Missing username or password.";
$new_sid = "";
$userID = null;
- if ( isset($_REQUEST['user']) || isset($_REQUEST['passwd']) ) {
+ if ( isset($_REQUEST['user']) && isset($_POST['passwd']) ) {
+ list($userID, $login_error) = valid_user_passwd($_REQUEST['user'], $_REQUEST['passwd']);
- $userID = valid_user($_REQUEST['user']);
-
- if ( user_suspended( $userID ) ) {
- $login_error = "Account Suspended.";
- }
- elseif ( $userID && isset($_REQUEST['passwd'])
- && valid_passwd($userID, $_REQUEST['passwd']) ) {
+ if ( isset($userID) ) {
$logged_in = 0;
$num_tries = 0;
# Account looks good. Generate a SID and store it.
-
$dbh = db_connect();
while (!$logged_in && $num_tries < 5) {
$new_sid = new_sid();
@@ -699,28 +693,6 @@ function valid_username( $user )
return;
}
-/*
- * Checks if the username is valid and if it exists in the database
- * Returns the username ID or nothing
- */
-function valid_user( $user )
-{
- /* if ( $user = valid_username($user) ) { */
- if ( $user ) {
- $dbh = db_connect();
- $q = "SELECT ID FROM Users WHERE Username = '"
- . mysql_real_escape_string($user). "'";
-
- $result = mysql_fetch_row(db_query($q, $dbh));
-
- # Is the username in the database?
- if ($result[0]) {
- return $result[0];
- }
- }
- return;
-}
-
function good_passwd( $passwd )
{
if ( strlen($passwd) >= PASSWD_MIN_LEN ) {
@@ -729,58 +701,33 @@ function good_passwd( $passwd )
return false;
}
-/* Verifies that the password is correct for the userID specified.
- * Returns true or false
+/* Verifies that for the user name specified the user exists, the
+ * password is correct and it is not suspended.
+ * Returns userid and error message
*/
-function valid_passwd( $userID, $passwd )
+function valid_user_passwd( $userName, $passwd )
{
if ( strlen($passwd) > 0 ) {
$dbh = db_connect();
-
- # get salt for this user
- $salt = get_salt($userID);
- if ($salt) {
- # use salt
- $passwd_q = "SELECT ID FROM Users" .
- " WHERE ID = '$userID' AND Passwd = '" .
- salted_hash($passwd, $salt) . "'";
- $passwd_result = mysql_fetch_row(db_query($passwd_q, $dbh));
- if ($passwd_result[0]) {
- return true;
- }
- } else {
- # check without salt
- $nosalt_q = "SELECT ID FROM Users".
- " WHERE ID = '$userID'" .
- " AND Passwd = '" . md5($passwd) . "'";
- $nosalt_result = mysql_fetch_row(db_query($nosalt_q, $dbh));
- if ($nosalt_result[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;
- }
-
- return true;
- }
+
+ $query = "SELECT ID, Suspended, Passwd, Salt FROM Users" .
+ " WHERE Username = '" . mysql_real_escape_string($userName, $dbh) . "'";
+
+ $result = mysql_fetch_row(db_query($query, $dbh));
+ if (!mysql_num_rows($dbh)) {
+ return array(null, "User doesn't exist.");
}
+
+ if ($result[1])
+ return array(null, "Account Suspended.");
+
+ if ( salted_hash($passwd, $result[3]) == $result[2] ) {
+ return array($result[0], "Login successful");
+ }
+ return array(null, "Wrong password.");
}
- return false;
-}
-
-/*
- * Is the user account suspended?
- */
-function user_suspended( $id )
-{
- $dbh = db_connect();
- $q = "SELECT Suspended FROM Users WHERE ID = '$id'";
- $result = mysql_fetch_row(db_query($q, $dbh));
- if ($result[0] == 1 ) {
- return true;
- }
- return false;
+
+ return array(null, "Wrong password.");
}
/*
diff --git a/web/lib/aur.inc b/web/lib/aur.inc
index 8ccce89..f417066 100644
--- a/web/lib/aur.inc
+++ b/web/lib/aur.inc
@@ -456,24 +456,6 @@ function mkurl($append) {
return substr($out, 5);
}
-function get_salt($user_id)
-{
- $dbh = db_connect();
- $salt_q = "SELECT Salt FROM Users WHERE ID = '$user_id'";
- $salt_result = mysql_fetch_row(db_query($salt_q, $dbh));
- return $salt_result[0];
-}
-
-function save_salt($user_id, $passwd)
-{
- $dbh = db_connect();
- $salt = generate_salt();
- $hash = salted_hash($passwd, $salt);
- $salting_q = "UPDATE Users SET Salt = '$salt'" .
- ", Passwd = '$hash' WHERE ID = '$user_id'";
- return db_query($salting_q, $dbh);
-}
-
function generate_salt()
{
return md5(uniqid(rand(), true));
diff --git a/web/lib/aur.inc b/web/lib/aur.inc
index f417066..965a6bd 100644
--- a/web/lib/aur.inc
+++ b/web/lib/aur.inc
@@ -458,7 +458,7 @@ function mkurl($append) {
function generate_salt()
{
- return md5(uniqid(rand(), true));
+ return md5(uniqid(mt_rand(), true));
}
function salted_hash($passwd, $salt)
diff --git a/support/schema/aur-schema.sql b/support/schema/aur-schema.sql
index 250d405..f12c57d 100644
--- a/support/schema/aur-schema.sql
+++ b/support/schema/aur-schema.sql
@@ -25,8 +25,8 @@ CREATE TABLE Users (
Suspended TINYINT UNSIGNED NOT NULL DEFAULT 0,
Username CHAR(32) NOT NULL,
Email CHAR(64) NOT NULL,
- Passwd CHAR(32) NOT NULL,
- Salt CHAR(32) NOT NULL DEFAULT '',
+ Passwd CHAR(128) NOT NULL,
+ Salt CHAR(32),
ResetKey CHAR(32) NOT NULL DEFAULT '',
RealName CHAR(64) NOT NULL DEFAULT '',
LangPreference CHAR(5) NOT NULL DEFAULT 'en',
diff --git a/web/README b/web/README
index 041b521..f3c1f60 100644
--- a/web/README
+++ b/web/README
@@ -44,6 +44,8 @@ Setup on Arch Linux:
If those php extensions are separate packages on your system, install
them.
+ The hash extension is usually statically compiled into php. If it is
+ provided as an extension, add it too.
AUR requires PEAR and the File_Find module.
Installing PEAR will vary depending on the system and may already
diff --git a/web/lib/aur.inc b/web/lib/aur.inc
index 965a6bd..8f8d469 100644
--- a/web/lib/aur.inc
+++ b/web/lib/aur.inc
@@ -463,8 +463,8 @@ function generate_salt()
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);
+ if (!isset($salt))
+ return md5($passwd); /* Legacy */
+
+ return hash('sha512', pack("H*" , $salt) . "##" . pack("H*" , md5($passwd)));
}
