Update of Classes for user registration and authentication

Question

Old Post: Classes for user registration and authentication

This post is an update of the old post of mine. This is the code after suggestions were implemented in the review. Do you think I could make this even better?

Please criticize as thoroughly as possible for the smallest thing possible as it will be very useful for me.

<?php
namespace Users;

use mysqli;

class UserService {
    public function handleUserLogin($username, $password) {
        if($userMapper->checkLogin($username, $password) === true) {
            $userMapper->fetch($username);
            $userMapper->createSession($username);
            return true;
        } else {
            return false;
        }
    }
    public function handleUserCreation($username, $password, $rank, $name) {
        if($userMapper->newUserCreation($username, $password, $rank, $name) === true) {
            return true;
        } else {
            $errors['count'] = count($errors);
            return $errors;
        }
    }
}

class User {
    public $id;
    public $username;
    public $password;
    public $rank;
    public $name;

    public function __construct($id, $username, $password, $rank, $name) {
        $this->id       = $id;
        $this->username = $username;
        $this->password = $password;
        $this->rank     = $rank;
        $this->name     = $name;
    }
}

class UserMapper {
    private $db;
    public function __construct(DB $db) {
        $this->db = $db;
    }

    public function checkLogin($username, $password) {
        $query = $this->db->prepare("SELECT username, password FROM users WHERE username=? AND password=? LIMIT 1");
        $password = $this->hash($password);
        $query->bind_param('ss', $username, $password);
        $query->execute();
        $query->bind_result($username, $password);
        $query->store_result();
        $query->num_rows == 1 ? true : false;
        $query->free_result();
        $query->close();
    }

    public function fetch($username) {
        $query = $this->db->prepare("SELECT id, username, password, rank, name FROM users WHERE username=? LIMIT 1");
        $query->bind_param('s', $username);
        $query->execute();
        $query->bind_result($id, $username, $password, $rank, $name);
        $query->store_result();
        while($query->fetch()) {
            $user = new User($id, $username, $password, $rank, $name);
        }
        $query->free_result();
        $query->close();
    }

    public function createSession($username) {
        $_SESSION['logged_in'] = 1;
        $_SESSION['username']  = $username;
        header("Location: dashboard.php");
    }

    public function newUserCreation($username, $password, $rank, $name) {
        $errors = array();
        $query = $this->db->prepare("INSERT INTO users VALUES('', $username, $password, $rank, $name);");
        $password = $this->hash($password);
        $query->bind_param('ssss', $username, $password, $rank, $name);

        if(strlen($username) < 3 || strlen($username) > 15) {
            $errors[] = "The length of the username can only range from 3 to 15 characters.";
        }
        if(strlen($password) < 3 || strlen($password) > 15) {
            $errors[] = "The length of the password can only range from 3 to 15 characters.";
        }
        if(strlen($name) < 3 || strlen($name) > 15) {
            $errors[] = "The length of the name can only range from 3 to 15 characters.";
        }

        if(count($errors) > 0) {
            return $errors;
            exit();
        } else {
            $query->execute();
            $query->close();
            return true;
        }
    }

    public function hash($string) {
        $fSalt = "{z6Q9&2eHn69,9/cXa=<,Nm[?^^Tw-";
        $sSalt = ":L+;fwS<q%)3f?N_d)(Ta_*fN&ax}+";

        $sting = md5(md5($fSalt) . md5($string) . md5($sSalt));

        for($i=0; $i < 3; $i++) {
            $string = md5($string);
            $string = sha1($string);
            $string = bin2hex($string);
            $string = md5($string);
        }

        return $string;
    }
}

class DB {
    public $db;
    public function __construct() {
        $this->db = new mysqli('localhost', 'root', 'php123', 'sms');
    }
}

$userService = new UserService();
$db          = new DB();
$userMapper  = new UserMapper($db);
?>

Answer 1

Right now, you're using the same salt for every user. This pretty much defeats the purpose of using salt in the first place.

The point of using salt is to ensure that even if two users choose the same password, the hashed value will still be unique for each. So, to be useful, you compute the salt as a random string for each user, then store that random string in the user's record in the database. Although you don't particularly try to make it public, it's not necessary that you take any particular pains to keep it private either.

I believe it's been commented on already, but I'll be a little more direct: I would eliminate the check for maximum password length, or at least increase it to some "ridiculous" length (e.g., 4 kilobytes). If somebody wants to use a 20- or even 50-character password, let them. The only reason to limit the length is to prevent wasting a lot of time reading a bunch of data that clearly wasn't ever intended to be a password at all.

So, when you're going to create a new user, you want to compute a random string as the salt for this particular user. Then you store their user name and the hash of the salted password in the database (you don't store their raw password anywhere, at all).

Better still is to have most of this done on the client machine--the user enters their password, and code (usually in JavaScript) on their machine computes a salt and a hash. That is then transmitted to the server. The server stores the user name, salt and hash, but never sees the raw password at all.

When you're going to authenticate a user, the server should not plan on receiving the password in the clear either. What you typically want to do is use a challenge/response protocol. The server generates a (cryptographically strong) random number. It transmits that random number (and optionally that user's salt) to the user as the challenge.

The user enters their password, and code on their machine (typically in JavaScript) puts the password together with the salt, and hashes the result. They then use that to encrypt the random number using some agreed-upon algorithm (e.g., AES) and transmit the result back to the server.

The server does roughly the same thing: uses the stored value of the hashed/salted password to encrypt the random number. When the encrypted random number arrives back from the user, it compares those to see if the user entered the same password. Alternatively (and equivalently) it can decrypt the response from the user with their hashed/salted password, and see if the result equals the random number they originally sent out.

Why you want to do this:

1. It means even if all the data on your servers is made public, you haven't revealed a single user's password.
2. Since each user has their own unique salt, a dictionary attack on one user's password reveals nothing about any other user's password.
3. A user's password is never transmitted over the network.
4. The data for each login is unique, so if somebody snoops on a user logging in, they won't be able to play that back to log in as that user.

Answer 2

Security

Focusing on only the password hashing for a second, let's talk security.

public function hash($string) {
    $fSalt = "{z6Q9&2eHn69,9/cXa=<,Nm[?^^Tw-";
    $sSalt = ":L+;fwS<q%)3f?N_d)(Ta_*fN&ax}+";

    $sting = md5(md5($fSalt) . md5($string) . md5($sSalt));

    for($i=0; $i < 3; $i++) {
        $string = md5($string);
        $string = sha1($string);
        $string = bin2hex($string);
        $string = md5($string);
    }

    return $string;
}

First, the md5 of the fSalt and sSalt is poor-performing. Consider this alternative:

private static $fSalt = md5("{z6Q9&2eHn69,9/cXa=<,Nm[?^^Tw-");
private static $sSalt = md5(":L+;fwS<q%)3f?N_d)(Ta_*fN&ax}+");

public function hash($string) {

    $sting = md5(UserMapper::$fSalt . md5($string) . UserMapper::$sSalt);

    ....

    return $string;
}

Then, statistically, there is no value in doing the md5 on the password before the concatenation. It actually may make things worse because it makes the input value to the outer md5 a predictable length....

I would just simplify things as:

    $sting = md5(UserMapper::$fSalt . $string . UserMapper::$sSalt);

At this point, the security 'level' is not changed from your original code.

Unfortunately, if someone knows your code, and knows all the hashed passwords, they can easily rebuild this function, and then just start running dictionary values through the same algorithm, and if there are any hits, in any of the password hashes in your 'thousands' of users, they will gain access to your system.

The probabilities of getting lucky are proportional to the number of users you have.

What you have called a 'salt', is not actually a salt, it is just a way to make the attack slightly harder.

A real salt is unique for each user.

When the salt is unique, the potential attacker has to be able to know the salt value, and then guess the password too. The odds of getting these values are not dependant on the number of users you have the password hashes of.

The trick for the salt is to create a random value for each user, and store the salt in the database with that user.