1
\$\begingroup\$

I'm working on a game project (with Unity) that shall handle user credentials and other data. The game connects send requests to a php script and this script send requests to the database. After spending some days learning about sql injections and password handling, I came up with this approach. I am obviously still very naive with all this, but I would like to know if it is good or not. The first one is the script for creating a new user, and the second one is for login.

<?php

$servername = "localhost";
$username = "whatever";
$password = "whatever";
$dbname = "whatever";

try {

    $db = new PDO("mysql:host=$servername;dbname=$dbname", $username, $password);


    $db->setAttribute(PDO::ATTR_EMULATE_PREPARES, false);
    $db->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);


    $salt = bin2hex(random_bytes(strlen($_POST["providedpass"])));
    $hashpsw = hash('sha256', $_POST["providedpass"].$salt);

    $name = $_POST["usrnm"];


    $stmt = $db->prepare("INSERT INTO users (username, password, salt, level) VALUES (?,?,?,?)");

    if ($stmt->execute(array($name,$hashpsw, $salt, 1))) {

        echo "USER CREATED";
    }


    $db = null;

} catch (PDOException $e){

    echo "Error: " . $e->getMessage();
}


?>

-

<?php

$servername = "localhost";
$username = "whatever";
$password = "whatever";
$dbname = "whatever";

try {


    $db = new PDO("mysql:host=$servername;dbname=$dbname", $username, $password);



    $db->setAttribute(PDO::ATTR_EMULATE_PREPARES, false);
    $db->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);


    $stmt = $db->prepare("SELECT * FROM users WHERE username = ?");



    $providedname= $_POST["usrnm"];

    $stmt->execute(array($providedname));

    while($row = $stmt->fetch(PDO::FETCH_OBJ)){

            echo "FOUND USER";

            $hashpsw = hash('sha256', $_POST["providedpass"].$row->salt);

            if($hashpsw == $row->password) {

                        echo "CORRECT CREDENTIALS";

            }





    }

    $db = null;

} catch (PDOException $e){

    echo "Error: " . $e->getMessage();

}
?>
\$\endgroup\$

1 Answer 1

1
\$\begingroup\$

Nope, it is not safe. You can refer to the following article thet explains why you shouldn't use SHA-family hashing and required to use password_hash() instead. Beside other things, it means that salt field should be removed, and password field should be varchar(255).

To soften the pill, I've got the reference code to check the hashed passsword.

There is one more issue which is connected to security: wherever an error occurs, you are echoing it right in the hands of a potential hacker, revealing a lot of sensitive information about your software, database structure, etc. You should never ever be doing things like echo "Error: " . $e->getMessage();! For more info refer to my article on the basics of PHP error reporting

Everything else is rather good, save for peculiar code formatting and unnecessary WHILE statement. When you expect just a single row, there is no need to run a while loop. Just fetch your row right away.

Also, it does no good to duplicate the database connection code. Store it in a file and just include wherever needed.

So the refactored version would be like

db.php:

<?php

$servername = "localhost";
$username = "whatever";
$password = "whatever";
$dbname = "whatever";
$charset = 'utf8mb4';

$options = [
    PDO::ATTR_ERRMODE            => PDO::ERRMODE_EXCEPTION,
    PDO::ATTR_DEFAULT_FETCH_MODE => PDO::FETCH_ASSOC,
    PDO::ATTR_EMULATE_PREPARES   => false,
];
$dsn = "mysql:host=$servername;dbname=$dbname;charset=$charset";
try {
     $db = new PDO($dsn, $username, $password, $options);
} catch (\PDOException $e) {
     throw new \PDOException($e->getMessage(), (int)$e->getCode());
}

register.php

require 'db.php';
$hashpsw = password_hash($_POST["providedpass"],);

$stmt = $db->prepare("INSERT INTO users (username, password, level) VALUES (?,?,?)");
$stmt->execute(array($_POST["usrnm"], $hashpsw, 1));
echo "USER CREATED";

auth.php:

require 'db.php';
$stmt = $db->prepare("SELECT * FROM users WHERE username = ?");
$stmt->execute([$_POST['usrnm']]);
$user = $stmt->fetch();

if ($user && password_verify($_POST['providedpass'], $user['password']))
{
    echo "FOUND USER";
}
\$\endgroup\$
2
  • \$\begingroup\$ Thank you so much, I'll follow your advices. \$\endgroup\$
    – PM U
    Feb 18, 2019 at 15:48
  • \$\begingroup\$ Would it be secure to store the username and the hashed password in the session variables? \$\endgroup\$
    – PM U
    Feb 19, 2019 at 12:48

Your Answer

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

Not the answer you're looking for? Browse other questions tagged or ask your own question.