1

I am learning about SQLi (SQL injection) and I know that the solution to avoid them is prepared statements. However, does this mean that without them we are sure that we can get hacked? Are prepared statements the only solution? I was thinking about that and I have decided to write a piece of code that I think is safe without using prepared statements. The code is the following.

$username = escapeSimple($_POST['username']);
$password = $_POST['password'];
$sql= "SELECT user_id, username, password
            FROM users WHERE username='".$username."'";
$result = mysql_query($sql);
$row = mysql_fetch_array($result);
if ($username == escapeSimple($row["username"])) {
   if (md5($password) == escapeSimple($row["password"])) {
         //Log in
   }
}

As you can see in users table I save the passwords using md5.

I think that this is safe. Am I missing something? Are there some vulnerabilities that I do not see?

Conor Mancone
  • 29,899
  • 13
  • 91
  • 96
Walker18
  • 21
  • 3
  • 1
    What does `escapeSimple` do? – gowenfawr Apr 16 '21 at 21:09
  • 2
    I found it here https://pear.php.net/manual/en/package.database.db.db-common.escapesimple.php – Walker18 Apr 16 '21 at 21:11
  • 2
    just a note that it's still possible to perform an injection attack when prepared statements are used... but much less likely on that particular statement. (impossible?) It's still important to filter any user input especially if it's stored in the DB. Whether "escapeSimple" is good enough for that I don't know, but why wouldn't you use prepared statements to ensure that the user input is always treated as data and never as pure code? – pcalkins Apr 16 '21 at 21:20
  • 1
    Thank you fro the answer. I was just curious if I can prevent sql injection without prepared statements. Can you see any vulnerabilities in this code? – Walker18 Apr 16 '21 at 21:24
  • 1
    I think it depends on the server... try injecting via unicode. Some will do an automatic translation which allows you to insert a quote. See this post: https://security.stackexchange.com/questions/54734/sql-injection-via-unicode – pcalkins Apr 16 '21 at 21:28
  • 1
    one of your checks here assumes that the query performed is a SELECT. If an injection is successful it may not be, or may include a SELECT along with others. The result could just be a count of records affected. – pcalkins Apr 16 '21 at 21:42
  • 1
    What do you mean? – Walker18 Apr 16 '21 at 21:46
  • 1
    I mean that if an injection gets through you have no idea what it did... any check you make after that can't possibly account for any possible code injection. So "if ($username == escapeSimple($row["username"]))" is very limited. If an injection did get through you can't even know that any code after that would run. – pcalkins Apr 16 '21 at 22:40
  • 1
    Note that you can't use parameter substitution (the important part of prepared statements) for everything. In particular, parameter substitution does not allow you to add clauses (such as `JOIN`) or references (such as table or column names). These are the realm of dynamic SQL (only rarely needed), and you have to be much more careful. Usually the idea is build a prepared statement without user-specified elements, using parameter substitution as normal. – Clockwork-Muse Apr 17 '21 at 06:33
  • Why do you not want to use prepared statements? –  Apr 18 '21 at 18:17

3 Answers3

5

Not sure what escapeSimple does exactly, but if it behaves like mysqli_real_escape_string then it's ok, as long as you are not using some weird character encoding or forget to set it correctly (UTF8 should be ok by default, as far as I know).

Remember that using MD5 or any other simple hashing function is considered bad practice today, and you should use better functions which take more resources to compute (and with salts), for example bcrypt, argon2, etc.

I also don't like that == in the comparison, you should always use === unless you have a good reason to want type juggling. Otherwise you are going to get into trouble and something like this might happen: https://stackoverflow.com/questions/22140204/why-md5240610708-is-equal-to-md5qnkcdzo By the way, there might also be timing attacks to consider (see Alexander O'Mara's comment below).

That said, your question was about avoiding SQL injection without prepared statements. Yes, it's definitely possible to avoid it, if you are very careful and make sure every statement is correctly constructed and its parts correctly escaped. However in some cases it might be tricky, and to avoid any mistakes of course prepared statements are the way to go.

reed
  • 15,398
  • 6
  • 43
  • 64
2

If escapeSimple behaves like mysqli_real_escape_string then there is one important caveat to be aware of:

mysqli::real_escape_string -- mysqli_real_escape_string — Escapes special characters in a string for use in an SQL statement, taking into account the current charset of the connection

(emphasis is mine)

So you need an active connection for this charset-aware function. But I don't see in your code how escapeSimple relates to an existing, open DB connection. So does it really behave the way you would expect ?

This type of coding is not advisable for several reasons, one being the lack of portability. Software nowadays is often designed to run on different DBMSes (eg Mysql or SQLite), that is why we have abstraction classes like PDO etc.

But this code is problematic for other reasons: it uses mysql_fetch_array, which is deprecated (PHP5.5.0). In fact what you are showing is the plain old mysql functions.

At the very least it means your environment is not up to date. Your PHP install may carry a number of security vulnerabilities that won't be fixed as it is EOL. Thus, the real vulnerability could come from something else than this particular code.

Kate
  • 6,967
  • 20
  • 23
1

tl/dr: The ability to properly escape user input once does not mean your company will do it every time. You can't. Don't even try.

You have 2 good answers for the specific question you asked. However, I wanted to touch on some broader issues that are very important here:

Prepared statements aren't a foolproof solution to SQLi

Prepared statements are a strong safeguard to SQLi, but they aren't the solution to SQLi because prepared statements can't solve all SQLi issues. You can't user prepared statements when the table name or column name comes from user input, nor can you use them to specify pagination parameters on the query. As a result, they are a key security measure, but you can still have SQLi vulnerabilities when using prepared statements.

Not using prepared statements guarantees SQLi vulnerabilities!

While it is certainly possible to use proper escaping methods to properly escape and build a single query, like in your example, this is not a successful long-term strategy. I know, because I've been personally involved in cleaning up breaches that happened because, years ago, a team decided, "We don't need prepared statements, we'll just use escaping!" That works fine as long as you can guarantee that every single developer who ever writes a query with user input understands perfectly how to escape user input in every injection point. This will never happen. Can you spot the SQLi vulnerability in this simple example?:

$result = mysql_query("SELECT * FROM users WHERE id=" . escapeSimple($_POST['user_id']))

The ability to properly escape user input once does not mean your company will do it every time. You can't. Don't even try.

Other Gotchas

Some more points:

  1. md5 for passwords died 15 years ago. PHP has password_hash. Use it.
  2. Never directly compare two hashes. This introduces potential side-channel attacks. Use password_verify.
  3. The mysql_query line of functions were deprecated a long time ago. Switch to mysqli or PDO
  4. Why are you explicitly checking if the username matches when you already did that in the query? Just exit if the query returned no results.
Conor Mancone
  • 29,899
  • 13
  • 91
  • 96