2
\$\begingroup\$

I am writing an application for work. This application will sit on a closed network but I still have to be concerned about security. There will be a maximum of three types of user for this application: User (read only), admin (data edits only) and super admin (access all areas). But at the moment I am trying to get a feel for security on a basic level.

Just so I am clear about the user options: I meant that only 3 people will be permitted to use the application. So, there will only ever be ONE basic "User" and not a number of basic users.

Would one of you kind people please look at the following code to review its efficiency and security with the possibility of offering some tips on improvement.

function loginUser($username, $password)
{

    include('assets/con.php');

    /* Set the salt. */
    $salt = 'JwethYGkuuygTYFIYTFrfkuyGUTf75kUY4uyk6';

    /* Apply SHA512 password and salt encryption. */
    $password = hash('SHA512', $password);
    $salt = hash('SHA512', $salt);

    $query = 'SELECT username FROM users WHERE username = ? AND password = ? AND pass_salt = ?';
    if( $statement = $con->prepare($query) )
    {
        $statement -> bind_param( 'sss', $username, $password, $salt );
        $statement -> execute();
        $statement -> store_result();
        $num_row = $statement -> num_rows;
        $statement -> bind_result($username);
        $statement -> fetch();
        $statement -> close();
    } 

    else die( "Failed to prepare query." );

    if( $num_row === 1 )
    {
        $_SESSION['userName'] = $username;
        return true;
    }
    return false;
}
\$\endgroup\$
2
  • \$\begingroup\$ I have rolled back the last edit. Please see What to do when someone answers. \$\endgroup\$ – Mast Jan 16 '16 at 12:21
  • \$\begingroup\$ @Mast. Apologies for that. \$\endgroup\$ – NOJ75 Jan 16 '16 at 12:22
2
\$\begingroup\$

Hashing

Let's start with the correct solution for hashing passwords: use bcrypt; PHP offers it with password_hash.

It's secure, and it's really easy to use (it's just a simple function call, and it manages salts etc for you).

What is wrong with your approach:

  • simple sha512 is way too fast; it's still better than plaintext, but it's really not the correct approach.
  • you don't have a per-user salt.
  • you don't even have a per-site salt. pass_salt doesn't serve any purpose, the check will always match as you are using it wrong and are thus still open to rainbow attacks etc (it should be something like $password = hash('SHA512', $password . $salt);, but again, site-salts are not enough).

Misc

  • your comment talks about encryption, but you are never encrypting, only hashing.
  • putting spaces around -> is unusual and leads to harder to read code.
  • don't die in functions, it makes it impossible for the calling code to recover. Just throw an exception.
  • pass the connection to the function instead of including it to make your function easier to reuse and test.

Apart from these minor issues and the very weak hashing, your code looks good to me.

\$\endgroup\$
5
  • \$\begingroup\$ Thank you for your time Tim. I think you may have been answering whilst I made the edit. If you read the edit you will see that there will not be numerous users on this script. Does that make a difference? Also, could you point me in the right direction on how to pass the connection to the function? Many thanks. \$\endgroup\$ – NOJ75 Jan 16 '16 at 11:09
  • \$\begingroup\$ @NOJ75 not really, you never know if requirements will change, so a per-user salt would be good. But anyways, you don't have ANY salts, and password_hash gives you per-user salts for free, so it doesn't really matter (definitely don't use sha512). As to passing the connection: just add it as argument: function loginUser(mysqli $connection, $username, $password) {...} include('assets/con.php'); loginUser($con, ...) \$\endgroup\$ – tim Jan 16 '16 at 11:17
  • \$\begingroup\$ Thank you again Tim. I have posted a "reviewed code" rather than posting another question. If you would be so kind as to take a look it would be very appreciated. \$\endgroup\$ – NOJ75 Jan 16 '16 at 12:17
  • \$\begingroup\$ @NOJ75 Yes, that's a lot better. The password hashing is now as it should be. You could combine the two if into one with an &&, and you still die, but otherwise it looks fine. I still rolled back your edit, please see What you may and may not do after receiving answers \$\endgroup\$ – tim Jan 16 '16 at 12:22
  • \$\begingroup\$ Thank you very much for your continued support on this. Your time and support has been most helpful. \$\endgroup\$ – NOJ75 Jan 16 '16 at 12:24
1
\$\begingroup\$

IMHO it's a pretty good code except one thing. Currently you have a fixed salt, for every user the same. You should consider to change it. I would recommend to generate a random salt for every user, you already have a salt field in the user table.

You could also consider the "new" way with http://php.net/manual/de/function.password-hash.php

Here is what we've done in the old fashioned way.

Generate a unique salted password for each user with this.

$randomSalt = hash('SHA512', uniqid(null, true));
$passwordSalt = hash('SHA512', $password . $randomSalt);

So that a possible attacker need to brute force the passwords seperatly for each user.

In the login check first get the salt for the entered username, something like this: SELECT salt FROM users WHERE username = ?.

With this salt you verify the password with something like this.

$passwordSalt = hash('SHA512', $password . $saltFromUserTable);
$query = "SELECT salt FROM users WHERE username = ? && password = ?yoursalt";

Hope this give you some idea of improving password logins. For sure this is not a "Just do it so" guide with the top-best security available but for most of applications this should be good. Security always also depends on the application and use-case.

\$\endgroup\$
1
  • \$\begingroup\$ Thank you for your answer. The reason I have a set salt is because there will only ever be ONE basic User. Not a number of them. I have updated my question to reflect this. \$\endgroup\$ – NOJ75 Jan 16 '16 at 11:02

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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