0

Simple piece of PHP code:

#login.php

$_SESSION['valid_user_id'] = getUserId($username, $password);


#user_auth_fns.php

function getUserId($username, $password)
{
    $username = addslashes($username);
    $username = mysql_real_escape_string($username);
    $password = addslashes($password);
    $password = mysql_real_escape_string($password);

    $conn = db_connect();

    $result = $conn->query("select id from user where username='$username' and password= sha1('$password')");
    if (!$result) {
        throw new Exception('Could not retrieve your user id.');
    }
    if ($result->num_rows > 0) {
        return $result;
    } else {
        throw new Exception('Could not retrieve your user id.');
    }
}

"return $result" is wrong, however I have no idea what I should put there in order to return the id from a certain user. The PHP manual didn't provide the answer either. I know this function works because replacing

return $result by return "test"

returns the correct value as expected.

8
  • 1
    $username = addslashes($username); is not needed and does not add security anyway. Use $username = htmlentities($username), but only when getting the username out of the database, not when putting it into the database. Commented Oct 13, 2011 at 12:55
  • 3
    The manual for the database wrapper you are using should give examples. (It's not clear from the code which library it is) Commented Oct 13, 2011 at 12:55
  • And replace this: and password= sha1('$password')"); with and password = SHA2(CONCAT(salt,'$password'),512)");, SHA1 is broken and without a salt it will not withstand rainbow tables, allowing someone with one to break your passwords in seconds. Commented Oct 13, 2011 at 12:56
  • @Johan not if it wasn't originally done so, he'll need to have all password rehashed Commented Oct 13, 2011 at 12:57
  • 1
    addslashes() followed by mysql_real_escape_string()? It burns! The goggles do nothing! Commented Oct 13, 2011 at 12:57

6 Answers 6

4
if ($result->num_rows > 0) { 
    $row = mysql_fetch_row($result);
    return $row['id'];
} else { 
    throw new Exception('Could not retrieve your user id.'); 
} 

I would rewrite the whole function like so:

function getUserId($username, $password) 
{ 
    $username = mysql_real_escape_string($username); 
    $password = mysql_real_escape_string($password); 

    $conn = db_connect(); 

    $result = $conn->query("SELECT id FROM user 
                            WHERE username = '$username' 
                              AND password = sha2(CONCAT(user.salt,'$password'),512)"); 
    if (!$result) { 
        throw new Exception('Could not retrieve your user id.'); 
    } 
    if ($result->num_rows > 0) { 
        $row = mysql_fetch_row($result);
        return $row['id'];
    } else { 
        throw new Exception('Could not retrieve your user id.'); 
    } 
} 

To prevent XSS
Before echoing any value from a database sanitize it:

echo htmlentities($row['username']);

Make sure you salt those hashes, or you will not be secure
Note that you will need to add a new field called SALT to your user table.

ALTER TABLE user ADD COLUMN salt INTEGER NULL default NULL;

Because the passwords are hashed, you will need time to translate them, use the following code to insert new entries:

INSERT INTO user (username, salt, password) 
  SELECT '$username', @salt, SHA2(CONCAT(@salt,'$password'),512)
  FROM DUAL CROSS JOIN (SELECT @salt:= FLOOR(RAND()*99999999)) q;

And this code to test for valid passwords:

SELECT id, COALESCE(salt,-1) as salt FROM user 
  WHERE username = '$username' 
    AND CASE WHEN salt IS NULL 
             THEN password = SHA1('$password)
             ELSE password = SHA2(CONCAT(salt,'$password'),512) END;

Update the user table like so when salt turns out be be -1.

 UPDATE user 
   CROSS JOIN (SELECT @salt:= FLOOR(RAND()*99999999)) q
 SET salt = @salt, password = SHA2(CONCAT(@salt,'$username'),512); 
Sign up to request clarification or add additional context in comments.

1 Comment

+1 for the completeness and detail of the explanation, thanks.
2

$result contains only the object of resulting rows. To access the data, you need to fetch the row from result.

With the mysqli library:

$result = $conn->query("select id from user where username='$username' and password= sha1('$password')");
$row = $result->fetch_object(); // or $row = $result->fetch_array();
return $row->id;

With the mysql library using array:

$result = $conn->query("select id from user where username='$username' and password= sha1('$password')");
$row = $result->fetch_assoc();
return $row['id'];

1 Comment

That did it, thank you. Will accept your answer as soon as it becomes available :)
0

From php.net:

// Use result
// Attempting to print $result won't allow access to information in the resource
// One of the mysql result functions must be used
// See also mysql_result(), mysql_fetch_array(), mysql_fetch_row(), etc.
while ($row = mysql_fetch_assoc($result)) {
    echo $row['firstname'];
    echo $row['lastname'];
    echo $row['address'];
    echo $row['age'];
}

Comments

0

Try this:

if ($result->num_rows > 0) {
    $tmp = $result->fetch_assoc();
    $return = $tmp['id'];
    return $return;
} else { ...

Comments

0

I don't know what DB wrapper you are using since you haven't shown us db_connect(), but I bet this will work:

$result = $conn->query(...);
$result = $result->fetch();
return $result['id'];

The value returned from $conn is probably an object representing a result resource, not actual rows. You need to fetch the rows from the resource as there are potentially more than one.

Comments

0

In my opinion, the main problem is that your function is returning the result object, while you actually had to return only the id field. In order to do that you had to fetch the row from the result object as object / array / associative array and return the id field from it. Just to illustrate my idea, in terms of native MySQL functions it would look something like this:

$res = mysql_query("SELECT id FROM usersTable WHERE username = '".mysql_real_escape_string($userName) . "' AND password = '" .mysql_real_escape_string($password) ."' LIMIT 1") or die("Cannot select id: ".mysql_error());
if(mysql_num_rows($res) > 0) {
   $row = mysql_fetch_assoc($res); // fetch the retrived row as associative array, my personal favorite though others may prefer mysql_fetch_row or mysql_fetch_object instead
   return $row['id']; // return the retrived id 
}
else {
   // your exception throwing code goes here
}

As a side note, do not see any sense in addslashes followed by mysql_real_escape_string since they are basically meant to do the same thing and mysql_real_escape_string is the preferred way to escape user input in database queries. So, I think you should just leave mysql_real_escape_string lines and get rid of addslashes stuff.

Comments

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.