User ID & password constraints checker

Question

Please take a look at my code below and tell me if I'm doing the right thing here. My wishes:

Username

• The username must be at least 6 characters long
• The username can't contain any special symbols

Password:

• The password must be at least 8 characters
• The password must contain at least 1 upper- and lowercase letter
• The password must contain at least 1 number
• The password can't contain any special symbols

As you can see I am also using a hash function for the password. It is supposed to be hashed like this for the purpose I'm using it for. Does this affect my code at all?

//password hash function
function l2j_hash($password) {
    return base64_encode(pack("H*", sha1(utf8_encode($password))));
}

$pass = mysqli_real_escape_string($db_link, $_POST['password']);
$repass = mysqli_real_escape_string($db_link, $_POST['repeat_password']);
$user = mysqli_real_escape_string($db_link, $_POST['name']);

$uppercase = preg_match('@[A-Z]@', $pass);
$lowercase = preg_match('@[a-z]@', $pass);
$number    = preg_match('@[0-9]@', $pass);

if(!$uppercase || !$lowercase || !$number || !ctype_alnum($password) || strlen($password) < 8) {

    echo "The password must contain at least 1 lowercase letter, 1 uppercase letter and 1 number.";
    echo "The password can't contain special symbols.";

} else if ($pass != $repass) {
    echo "Passwords do not match";
} else if ($user == '' && strlen($user) < 6 && !ctype_alnum($user)) {
    echo "The User ID must be at least 6 characters long and can't contain special symbols.";
} else {
    $pass_hash =  l2j_hash($pass);
    $db_add = mysqli_query( "INSERT INTO `accounts` VALUES ('$user', '$pass_hash', '0', '0', '')" ) or die( 'Error: '.mysqli_error() );  
    echo 'Account created';
}

Answer 1

In no particular order:

• That hashing method is terrible and insecure. Hashes must be costly (slow) most of all, and they must contain a unique salt. Use password_hash, don't invent your own.

  • If that is the required hashing method: move away from it ASAP. But at the very least, utf8_encode is entirely superfluous, since you're only allowing ASCII characters to begin with and it won't do anything in that case.

• Don't validate strings you have already altered (here: after mysqli_real_escape_string).

• You're already using mysqli, use prepared statements rather than tedious and error prone escaping.
• Disallowing "special characters" in passwords makes them weaker, not stronger. Unless you have strong business reasons for this restriction (which legitimately may exist), don't limit the allowed character set.
• Use more functions to make your code more readable.
• Name your SQL columns, don't rely on the implicit order.
• Use DEFAULT values in your database table definitions instead of passing default values through the query, if possible.
• Check whether your SQL query succeeded (and/or use mysqli's exception error mode); presumably you have a UNIQUE constraint on the username, so the query may legitimately fail, and your code doesn't even know it.

function validateUsername($name) {
    return ctype_alnum($name) && strlen($name) >= 6;
}

function validatePassword($str) {
    return ctype_alnum($str) 
        && strlen($str) >= 8
        && preg_match('/[A-Z]/', $str)
        && preg_match('/[a-z]/', $str)
        && preg_match('/[0-9]/', $str);
}

function createUser(mysqli $db, $name, $password) {
    $stmt = $db->prepare('INSERT INTO `accounts` (`name`, `password`) VALUES (?, ?)');
    $stmt->bind_param('ss', $name, password_hash($password, PASSWORD_DEFAULT));
    return $stmt->execute();
}

if (!validateUsername($_POST['name'])) {
    echo 'Invalid name';
} else if (!validatePassword($_POST['password'])) {
    echo 'Invalid password';
} else if ($_POST['password'] !== $_POST['repeat_password']) {
    echo "Passwords don't match";
} else if (!createUser($db_link, $_POST['name'], $_POST['password'])) {
    echo 'Something went wrong';  // add better error handling here
} else {
    echo 'Account created';
}

Of course, you'll probably want to collect all the errors and output them next to the actual <input> elements when you inform the user about errors, instead of just failing on the first error that is produced. That's a bit too broad to tackle here though. And this could all be further improved with OOP or other larger architectural choices of course…

Answer 2

As an extension of deceze's excellent critique, I would like to advise that you validate the password with just one preg_match() call rather than five function calls including three separate preg_match() calls. The cost to this may mean reduced code comprehension depending on your understanding of regex, but it will yield more concise code and perform more efficiently.

function validatePassword($pass){
    // permitted characters throughout string ------------------------↓↓↓↓↓↓↓↓
    return preg_match('/^(?=[^A-Z]*[A-Z])(?=[^a-z]*[a-z])(?=[^\d]*\d)[a-zA-Z\d]{8,}$/',$pass)?true:false;
    // required characters----------↑↑↑-------------↑↑↑-----------↑↑            ↑-minimum length (no max)
}

Here is a PHP demo.

Regex Breakdown:

^                 # match from start of string
(?=[^A-Z]*[A-Z])  # lookahead for one uppercase letter (without advancing)
(?=[^a-z]*[a-z])  # lookahead for one lowercase letter (without advancing) 
(?=[^\d]*\d)      # lookahead for one digit (without advancing)
[a-zA-Z\d]{8,}    # only match if string is comprised of 8 or more of these characters
$                 # match until end of string

To relax the valid characters range, you might like to alter the character class just before $ to use .{8,} or specifically declare additional valid characters with [a-zA-Z\d!@#$%^&*()]{8,}.