2

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...?
Ali Ahmad
  • 4,814
  • 8
  • 35
  • 61
ysj
  • 429
  • 2
  • 7
  • 14
  • 1
    This may help. http://www.codinghorror.com/blog/2005/04/give-me-parameterized-sql-or-give-me-death.html – ponsfonze Jan 27 '13 at 00:11
  • 2
    -1 Akam Omer does not have the correct answer. You should select Bobince's answer and use parametrized quires. – rook Jan 28 '13 at 17:48
  • Asking this question and waiting for an answer is (A) less authoritative, (B) slower and (C) more effort than just parameterizing your SQL. So why didn't you? – Matt Apr 17 '14 at 07:41

2 Answers2

15

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.

bobince
  • 12,534
  • 1
  • 27
  • 42
  • 2
    +1 i am glad someone mentioned parametrized queries. – rook Jan 28 '13 at 17:48
  • Escaping can be done better though: http://pastebin.com/tPaxi3N7. `escape` here additionally inspects the type and wraps in quotes if necessary (not `null` or a number). Just saying though, parametrization ftw. – Esailija Apr 12 '13 at 14:57
8

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?

Community
  • 1
  • 1
    Thanks for reply.. but my focus is not about securing password yet. – ysj Jan 26 '13 at 16:08
  • I don't see anything wrong with my where clause? User enter username and password. I look into my db for records, if 1 records is found i authenticate them? As said, this is just a portion of the login script. Your answers doesn't solve all my doubts. – ysj Jan 27 '13 at 04:51
  • 1
    @shaun - Your method is in principle susceptible to timing attacks; e.g., if the username doesn't exist the SQL query will return quicker than if the username does exist and the password doesn't match. In principle you could also figure out the prefix for the salted hash by a timing attack. (E.g., if the hash in the db starts with `2cf24 ...` then a password with hash starting with `2cfaa` will fail slower than one completely wrong starting with `3ab69`). If you can get the first 4 characters of the sha256 pass; you can filter a list of common passwords. – dr jimbob Jan 28 '13 at 20:56