10

I looked around everywhere and I can't find the answer to my question.

I'm using the latest PHP for server-side scripting and MySQL for my database. The character set is utf8mb4 if that makes a difference.

Until now, I have used prepared statements to protect myself from SQL injection. However, since most of my queries are only executed once, the code is really slow. When I use $conn->query() instead, and $conn->real_escape_string() for all my variables, it's faster.

However, $conn->real_escape_string() is also slow because there's a roundtrip at each call. But I don't understand why there's a need for a roundtrip because I feel like that escaping function can just be programmed in PHP.

From the docs, a function would look like this:

Characters encoded are NUL (ASCII 0), \n, \r, \, ', ", and Control-Z.

function sanitize($str)
{
    str_replace(array('\\', "\0", "\n", "\r", "'", '"', "\x1a"), array('\\\\', '\\0', '\\n', '\\r', "\\'", '\\"', '\\Z'), $str);
}

Assuming I made sure it's in the right encoding and that it's not empty.

This was posted as a comment in one of the docs. By the way, str_replace works with Unicode.

However, I thought of something that makes a little more sense. Looking at this page, I can stretch this a lot further. For a statement like this:

$sql = "UPDATE ipsum SET lorem=$str WHERE id=1337";

Can't I just do (and I'm checking beforehand to make sure it's a UTF-8):

function sanitize($str)
{
    return "'" .str_replace(array("\\", "'"), array("\\\\", "\\'"), $str) . "'";
}

And that will prevent SQL injections? Why not? I can't think of an input for which it will fail.

If not, is there a better PHP function that can protect against SQL injection better?

Thanks in advance! You can test SQL here.


UPDATE: Received the responses I expected. To clarify, I'm not asking for you to give your classic preaches of "prepared statements are the best thing ever do it or die." If I wanted that, I'd ask on Yahoo! Answers and not here.

I'd like to know why my method (the second one) is not secure (or if it is). It'd also be nice to include a discussion of what real_escape_string or prepared statements actually do to sanitize the data. Both MySQL and PHP are open source, and I'm sure someone knows what's happening when these functions are called.

I simply don't understand why a roundtrip is needed to sanitize strings. How can a method like that not be implemented in PHP?


UPDATE 2: I have iterated through all the unicode characters (0x0000 to 0x1F77F, found this on Wikipedia) and noticed that, if real_escape_string() only escapes single characters and not phrases (which according to the docs it does), under utf8mb4 charset, the characters that change are:

unicode 0 => \0

unicode 10 =>\n

unicode 13 =>\r

unicode 26 =>\Z

unicode 34 =>\"

unicode 39 =>\'

unicode 92 =>\\

So even though it's utf8mb4, it isn't different from what the docs say (guess because UTF-8 is standard). So why is this not implementable in PHP?

Here's a PHP script which combines all these unicodes and sanitizes them. Here's a SQL fiddle.

  • 17
    DON'T ROLL YOUR OWN! – Lucas Kauffman Aug 31 '14 at 20:00
  • 3
    @LucasKauffman Show me an example where it fails then. – Shahar Aug 31 '14 at 20:04
  • 4
    And I don't know who downvoted, but I really don't appreciate receiving a downvote for a valid question that I put effort into. Just because I may be an idiot when it comes to security doesn't mean I deserve a downvote. – Shahar Aug 31 '14 at 20:07
  • 9
    "Because I can't see an instance where mine fails" is not a valid reason to try to 'roll your own.' Just because you don't see any problems with your protection of your database doesn't mean they don't exist. Trust the teams that specialize in this. That being said, I agree that this question shouldn't have been downvoted. Using this implementation wouldn't be bright, but you're asking if something works in theory. The question is valid... – KnightOfNi Sep 1 '14 at 2:33
  • 4
    Shahar - the question, answers and comments actually will be of use to others, as evidenced by the upvotes and activity here. Don't be upset at all the downvotes - they aren't necessarily voting on the effort you put in; most are likely to be because this is an area which is known to be (for most people) an area to just avoid. – Rory Alsop Sep 2 '14 at 6:37
13

Regarding your sanitization function - while I can't produce an exploit for the exact example you provided, if we change your example to something like this:

$sql = "UPDATE ipsum SET price=$str WHERE id=1337";

If a value of 10, otherColumn=1234 or maybe 10;-- was passed in for $str you could see problems. If you roll your own sanitization function and begin using it in your code, it seems like it would only be a matter of time before an vulnerability like this popped up. I'll keep playing around to see if I can find an exploit for the exact example you provided in your question.

That said, I wouldn't recommend rolling your own sanitization function. Smarter people than you and I have put considerable effort into testing the effectiveness of the currently accepted industry standard of using prepared statements to prevent SQL Injection so you can be (fairly) certain that it is safe. Rolling your own is almost always a mistake.

Also - I would wager that there are people out there using prepared statements in environments that handle more traffic and require more performance than your current project.

If you are experiencing performance problems accessing your data layer, I would recommend spending some time looking at performance tuning rather than building your own sanitization function.

Performance tuning access to your data layer is an essential and normal part of development. Rolling your own sanitization is not.

  • Do you know what real_escape_string does on UTF-8 text? Surely it must be some function that can be reproduced in PHP. – Shahar Sep 1 '14 at 14:16
  • By the way, if you remove the quotes, it'd still be vulnerable with real_escape_string so I made sure there are always quotes. – Shahar Sep 1 '14 at 15:28
6

Firstly, PHP already has your second function and it's called addslashes().

The docs explicitly say:

To escape database parameters, DBMS specific escape function (e.g. mysqli_real_escape_string() for MySQL or pg_escape_literal(), pg_escape_string() for PostgreSQL) should be used for security reasons.

If your DBMS doesn't have an escape function and the DBMS uses \ to escape special chars, you might be able to use this function only when this escape method is adequate for your database. Please note that use of addslashes() for database parameter escaping can be cause of security issues on most databases.

Your first function is also missing several characters that are significant to MySQL syntax.

Other important reasons why you shouldn't resort to this:

  • Let's say you can get away with some more efficient but less comprehensive sanitization for specific queries. Do you want to have to try and fully consider all potential attack vectors against every single query you write? How long until you make a mistake?

  • A a good reason to use prepared statements over mysql_real_escape_string is that one day you will inevitably forget to escape something. Many SQL injections in well known PHP projects have been caused by someone simply forgetting to escape something, it happens to everyone. At least with prepared statements if you forget to call bindParam() your query doesn't work.

If you want to improve performance implement caching and avoid MySQL queries entirely.

Also, make sure you're using an up to date version of MySQL:

Before 5.1.17, the query cache is not used for prepared statements. http://dev.mysql.com/doc/refman/5.1/en/query-cache.html

Don't roll your own when it comes to security.

  • 1) I see no particular advantage of the DBMS function over my second function. The docs would obviously say it's preferable, but realistically there's no difference to my knowledge. 2) How do you know I don't have my own function in my webapp class that handles the queries (and sanitizes string inputs)? 3) It'd be nice if you show me an instance where my second function fails. – Shahar Aug 31 '14 at 23:20
  • P.S. - I'm familiar with addslashes() but it isn't Unicode-friendly like str_replace. – Shahar Aug 31 '14 at 23:32
5

Can you remove the brakes of your car to save weight? Well, if you only drive on an isolated site and keep the speed at a minimum and ensure that nothing extraordinary will ever happen (like an uninformed person using the car), probably.

Your method “works” as long as you control every single aspect of the system and don't overlook anything. You've already failed to meet the second requirement (you didn't know the NO_BACKSLASH_ESCAPE setting), so what makes you think it's all good now? Are you sure you haven't overlooked another setting? Can you promise that anybody working on the system now and in the future knows the odd constraints and will never make a mistake?

Personally, I wouldn't bet my money on it. When we write applications for the real world, we want them to work under all conditions, even if we don't control every single setting, even if somebody makes a mistake, even if we overlook some detail. In other words, the solution should be robust, because many decades of software development have shown that humans are in fact fallible. Prepared statements are very robust, mysql_real_escape_string() is also fairly robust. Your function is not. At all.

I also wonder how you've concluded that both prepared statements and mysql_real_escape_string() are “too slow”. If you have so many users that you need to worry about a few extra roundtrips, then the last thing you want is some home-made security tool. And if you don't have that many users, then I think you're simply exaggerating.

4

Your code fails:

  • if the default connection charset on the MySQL server is not a strict ASCII superset, for example if it is Shift-JIS (which can smuggle the byte for backslash in the second byte of a multibyte sequence);

  • if the MySQL server is configured to use the no_backslash_escape sql_mode option (as it may be for interoperability because backslash escapes are non-ANSI-standard).

In both these cases there is input that can escape a string literal with potential-security-disaster consequences.

So it might be OK for your present setup, but it's pretty fragile for anyone else to run anywhere else.

  • 1) Default charset is UTF-8, and I check that each string is UTF-8 using mb_string_encoding(). Is that good? 2) Didn't know that's a thing but it's not configured to use that. – Shahar Sep 1 '14 at 14:38
2

Escaping in general has had issues.

Please use prepared statements, instead of escaping anything. It's the only sane way to pass parameters to SQL queries. I'm pretty sure it's also the fastest, since it's just passing the values to the sql server.

  • He quite plainly says that he has used prepared statements up to this point. It's obvious that he knows about prepared statements. – Kenneth K. Aug 4 '15 at 17:26
  • It's difficult to understand why this is not the best answer. Prepared statement would fix everything because the driver has been written (or rewritten) with SQL injection in mind. Plus, changing the DB supplier can require a different escaping. Code is slow because the prepared statements shall be prepared before the beginning of the request, upon DB session initialization. They will be faster (or equally slow) compared to the concatenated query. – user166832 Dec 28 '17 at 20:47
-2

Using own sanitizer is not a good choice when there exists default function for that. secondly for avoiding sql injection you can use application level firewall or filter. for example you can load mod_security in Apache server for preventing sqli attacks.

Your Answer

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

Not the answer you're looking for? Browse other questions tagged or ask your own question.