How secure is this php code against SQL injection?

Question

The code given below is a portion of a simple login script code..

if (isset ($_POST['login'])) { 
                $username = $_POST['username']; 
                $password = $_POST['password'];
                $password_hash = sha1($password);

                $query = "SELECT id,username FROM users WHERE username = '".mysqli_real_escape_string($link,$username)."' AND password ='".$password_hash."'";

Here is some of my doubts:

1. Is the above code vulnerable to any kinds (error,blind,union based) of SQL injection?
2. If the above code is vulnerable to SQL injection attacks, how and why can the attacker launch the attack?
3. If the above code is safe, why do people still go for Prepared Statements etc...?

Answer 1

Is the above code vulnerable to any kinds (error,blind,union based) of SQL injection?

No, but only because sha1() happens by luck to produce output that never contains any SQL metacharacters.

Your query-creating code should not need to know about that implementation detail. It should be escaping the $password_hash as well, so that the line of code works on its own, and can be verified as correct and safe without having to look at the wider context. Otherwise, when you change how that $password_hash is calculated (say, to store a more secure salted hash), you may find that you've just created a vulnerability in that query code. Creating unnecessary coupling between disparate bits of code, potentially in different files, is in general a bad idea.

If the above code is safe, why do people still go for Prepared Statements etc...?

Because prepared statements are typically easier to read, understand and verify; it is harder to accidentally omit an escape. Compare:

$query = "SELECT id,username FROM users WHERE username = '"
         .mysqli_real_escape_string($link, $username)
         ."' AND password ='"
         .mysqli_real_escape_string($link, $password_hash)
         ."'";

with:

$query = $mysqli->prepare('SELECT id,username FROM users WHERE username=? AND password=?')
$query->bind_param('ss', $username, $password_hash);

Although I'm not a huge fan of mysqli's particular brand of parameterised queries (especially binding to variables instead of values), the latter still seems simpler and more maintainable to me.

Answer 2

You are using SHA-1 for your password hash without a salt even. This is bad bad bad. See How to securely hash passwords? for a much better way to hash your passwords.

Secondly, see How to prevent SQL injection in PHP for an explanation of mysqli_real_escape_string vs prepared statements.

You might forget to escape strings. Using prepared statements for your entire application makes it much less likely that you will be vulnerable to SQL injection attacks.

Plus that escape string syntax is plain ugly.

Thirdly, why are you using the WHERE clause to perform authentication instead of retrieving both the username and password hash from your database and performing a comparison?