Security of login form

Question

I would like to know how secure is this code I made for a login form... because I'm new in this and have serious doubts.

$sql = "SELECT user_name, user_password "
      ."FROM users "
      ."WHERE user_name = '".mysqli_real_escape_string($db,$_POST['user_name'])."'";

$q = mysqli_query($db, $sql);

if (mysqli_num_rows($q) > 0) {
  $users         = mysqli_fetch_object($q);
  $user_name     = $users->user_name;
  $user_password = $users->user_password;
} else {
  $errors        = 'Please enter a valid username and password.';
}

if ($user_password != crypt($_POST['password'], $user_password)) {
  $errors        = 'Please enter a valid username and password.';
}

Answer 1

I can't see any way to exploit this directly at a glance but I can see a few places where the code "smells" a bit.

The conditional

if (mysqli_num_rows($q) > 0)

should be

if ($q && mysqli_num_rows($q) === 1)

Although you don't use a LIKE clause in your query, if you (or someone) did change the code, a % or _ symbol could cause multiple names to match and therefore would pass that step.

This is just defensive coding. Although you can't see any way for more than one row to be returned, it's safer to restrict the code so that it will fail if somehow more than one row is returned.

Also, if you have a temporary error in the database server or at any time there is a syntax error in your SQL, $q will be NULL. We should test for that first.

Note that mysqli_num_rows() can return a string if the number of rows is greater than the highest allowable int in PHP but that's not likely to happen with this query.

---

Credential enumeration in login forms is often not as important as people think. If you have a signup form, there's no way to prevent credential enumeration. Nonetheless, there are some improvements that can be made if I assume that you create accounts manually and there is no signup form.

You are using the same error message whether the attacker gets the username wrong or the password wrong and this is generally considered good practice. However you have two copies of that error message and that makes it easy for a future coder to change one of them without changing the other. Instead, put the error message in a variable or a constant and reference the error message where the two strings are now.

There's a timing attack with credential enumeration. If a user does not exist, $user_password will be undefined on the final if() clause where the posted password and the stored password are compared. This will cause a PHP error and errors in PHP are very slow. An attacker could utilise this delay to enable credential enumeration.

---

You are using mysqli but you are not using bound parameters. There are fewer mistakes that can be made with bound parameters. For example, mysql_real_escape_string() depends on the character set being used and if you change it after escaping the strings or if you set it in the wrong way ( i.e not using mysqli_set_charset() ) then you may enable SQL injection.

---

It's not clear from this code which hashing algorithm you are using but bcrypt (also known as CRYPT_BLOWFISH and $2a$) is the best choice in PHP.

If you're on PHP > 5.5, there's a password_hash() function which improves on crypt() but is compatible with it.

---

It's also not clear how you are handling salt. The bcrypt option generates salt automatically and stores it as part of the string it returns with the hash so you don't need to worry about it if you are using bcrypt.

---

Some of the hashing algorithms have a cost parameter. This cost should increase over time as attacker's capabilities get better and as your ability to compute the hashes gets better. (Actually, probably just the latter.)

---

Some other questions about your code:

1. What happens on failure? It seems that the only difference between valid username/password and invalid is that the variable $errors is set to a string and is undefined when the password is valid. Furthermore, the second time overwrites the first string. If a later bit of code encounters a different error and overwrites that variable again, there will be no record in the state of the program that the user you are trying to authenticate failed the username or password tests.
2. What about throttling? You don't want to let attackers have as many attempts as they feel like. As you can see from any one of dozens of password dumps, the top few passwords tend to be used by large numbers of users. (One analysis of the RockYou dump showed that the top 5,000 passwords were used by 20% of the users and that the top password was used by 0.9%.) Throttling can be made more difficult by bot-nets but you should not allow a single IP address to try passwords on lots of different accounts. A nice way of handling the slow-down is to increase the delay between allowed authentication attempts by 1 second after every failed attempt. This allows users who have forgotten which one of several passwords they know not to be inconvenienced but causes a brute-force bot an intolerable delay after only a few attempts.

There are likely to be other problem. There are plenty of pitfalls in authentication. Why not use a library like PHPass which neatly avoids many of these pitfalls for you? We've covered it here.

Answer 2

This is not a direct answer to your question. But, I wanted to tell you this. I just visited the page where your registration form is hosted. One thing which bothered me very much was your captcha implementation.

http://shippictures.byethost22.com/_user/register.php

Your captcha challenge always consists of 5 letters which are actually 5 separate images with the filename in the format 'number.jpg'. (Refer to the pic)

A simple script, with not much difficulty can be coded to execute in a loop to send a GET request to register.php to get the page source and parse it to extract the names of the images you use as captcha and then send a POST to register.php to register the new user. Voila! Your database is being flooded now.

If I were you, I'd use libraries like Securimage for implementing captcha challenges.