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.';

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.

  • Thank you for this great explanation. I'm studying PHP since last month and used a borrowed book which is from the year 2004, when raw PHP 5 was being introduced, so my references are somewhat outdated. I use a free server and extensions are not allowed for me, so I stick to basic things. For encrypting the passwords I just use the crypt function that learned in the book. I indeed made my own registration form, which you can see here: shippictures.byethost22.com/_user/register.php – user37769 Feb 16 '14 at 11:48
  • When the user fails authentication, nothing happens on the login form apart from reporting the error on screen. I though already that it should have some limits set to avoid an attack session, but I still don't know how to program that, really... Perhaps it would be useful to add a CAPTCHA to defend against brute force attacks. – user37769 Feb 16 '14 at 12:34

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.


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.

Register Page

  • Wow! You have just wrote what it came to my mind this very night! I indeed felt bad about it, and I was going to start a function to encrypt the pictures' filenames when I came here and read your message. Yes, you're very right. – user37769 Feb 17 '14 at 9:39
  • I fixed it like this: <img src="captcha/crypt/$1$HEXCXw8v$CpwTBNdEQCe8jb5yLNMHa1.jpg" width="20" height="22"/> <img src="captcha/crypt/$1$XBUEFVQN$oFoX.hlccRKzjvUpUnYfE1.jpg" width="20" height="22"/> <img src="captcha/crypt/$1$4zANegle$QCPFJ6Y8cDOHgw.VPgeLM..jpg" width="20" height="22"/> <img src="captcha/crypt/$1$bPwcpvqq$9lORoA.E6BkZzTM3nx90w..jpg" width="20" height="22"/> <img src="captcha/crypt/$1$3XeUz1TP$PffgzzUjNi8CXFAWY8otL0.jpg" width="20" height="22"/> – user37769 Feb 17 '14 at 10:28
  • @SakhalTurkaystan This is better, but still rather ugly. Good programmers should recognise when it's more appropriate to use external libraries and IMHO this is one instance when you should really look into integrating reCATPCHA or similar. – deed02392 Feb 17 '14 at 12:52
  • 1
    @deed02392 I think that the fact of thousands and thousands websites using the same CAPTCHA system is actually a security flaw. The more popular (widespread) a system is the more people will be trying to hack into it, and when it is done, the safety of all those sites is compromised. If each website has its own design, spammers will have to focus only on the most desirable targets and leave alone the rest. – user37769 Feb 20 '14 at 10:09
  • @Sakhal but this does not accomodate for the fact individuals may not be skilled enough to implement good CAPTCHAs. For example the team working on reCAPTCHA being well funded will have tested for things like the character recognition ability of their images. Yours looks very easy to recognise with OCR. Plus I bet I could generate CRCs of all your numbers and use that to determine which it is, regardless of your file naming being hashed - there's another issue for you to go and fix (or use reCAPTCHA). – deed02392 Feb 20 '14 at 10:49

Your Answer

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