3
\$\begingroup\$

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';
}
\$\endgroup\$
  • 1
    \$\begingroup\$ What's a "special symbol"? Does that mean anything that's not ASCII printable, anything that's not a Unicode number or letter, or some other definition? \$\endgroup\$ – Toby Speight Aug 24 '17 at 10:11
  • \$\begingroup\$ @TobySpeight anything that is not a unicode number or letter. I suppose I could also edit this in the database table? \$\endgroup\$ – Cartman Aug 24 '17 at 10:28
3
\$\begingroup\$

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…

\$\endgroup\$
1
\$\begingroup\$

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,}.

\$\endgroup\$

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.