3

This question is not about creating an actual alternative for the proven functions to prevent injections, but about how to argue with people that don't see the flaw in their homebrewed injection-prevention code!

I'm trying to make a point to a colleague but it seems his "solution" to SQL injection seems fairly safe to me.

He clears the query by doing

$query = $_POST['username'];
$look = array('&', '#', '<', '>', '"', '\'', '(', ')', '%');
$safe = array('&amp;', '&#35;', '&lt;', '&gt;', '&quot;', '&#39;', '&#40;', '&#41;', '&#37;');
str_replace($look, $safe, $query);

And then proceeds with the login

"SELECT * FROM users WHERE username = '" . $query . "'
    AND password = '" . md5($_POST['password']) . "'";

I am trying to get him to use PDO or equivalents, but how could you actually breach this protection? I don't have an answer and it's really bugging me because I can't explain him how this is unsafe and why it should not be done this way.

F.P
  • 17,421
  • 34
  • 123
  • 189
  • 1
    Did you even read the question? – F.P Jan 25 '13 at 15:10
  • 1
    check this http://stackoverflow.com/questions/134099/are-pdo-prepared-statements-sufficient-to-prevent-sql-injection/12202218#12202218 – NullPoiиteя Jan 25 '13 at 15:11
  • 1
    It's your decision to use the fiddly workaround if you consider it safe enough. Nobody can greenlight it here however. (And nope, fails charset issues. Apart from this failing to be a context-aware escape; it's HTML conversion, not SQL escaping. ) – mario Jan 25 '13 at 15:11
  • 1
    +1 nice question, waiting for answer) Would like to know too) – Bogdan Burym Jan 25 '13 at 15:12
  • 2
    do you really using md5 for password ?? really ? i think it can be crack easily now a days – NullPoiиteя Jan 25 '13 at 15:13
  • @Wooble I'm not doing anything. It's his so-called "solution" and I want to prove to him that it's not secure in every case... – F.P Jan 25 '13 at 15:15
  • Escaping everything there except the `'` looks like overkill in this case; presumably there are no users in your system with those characters in their usernames and the username isn't being stored to be displayed to other users. It's an ugly design in any case, even if it's secure. – Wooble Jan 25 '13 at 15:17
  • why not **Prepared Statements** ?? – NullPoiиteя Jan 25 '13 at 15:20
  • 2
    `\' OR 1=1--` - Now go and use prepared statements. (Your code thinks it's escaping the quote, it creates an escaped backslash instead, quote closes, query broken) – Leigh Jan 25 '13 at 15:36
  • This would fail on non-string comparisons. If an item of user input is to be treated as a number, you could enter `0; DELETE FROM users; --` and it would inject the second statement. Whether that would succeed depends on what database engine you're using - for example `mysql_query` doesn't support multiple statements. – halfer Jan 25 '13 at 15:38
  • Answer: No, this is not safe. There are a large number of reasons why this is insecure. **PHP has functions built-in** that do what you're trying to do, and do it much better. Why on earth would you write your own rather than using the built-in ones? Especially when you clearly know nothing about security? – SDC Jan 25 '13 at 15:39
  • 1
    It's really interesting how nobody seems to read the question through... – F.P Jan 25 '13 at 18:02

3 Answers3

2

While this is likely to cover most typical SQL injection issues - there are a couple of known issues using multi-byte character sets and certain locale settings on the server.

However, this isn't simply escaping - this is actually changing the data being input to the database. Consider simply adding slashes where appropriate, or using one of the built in escape methods such as mysqli_real_escape_string when inputting data, and htmlentities or the like when re-displaying it to the browser. This ensures that what you store in your database is always what the user actually entered - unless you have a reason not to.

Better yet, when in doubt, use prepared statements and bound parameters. Then you're safe from SQL injection entirely.

Colin M
  • 13,010
  • 3
  • 38
  • 58
  • What exactly are those issues with multibyte/locales? I mean, the single quote is absolutely required to get out of the context of the string in the SQL, isn't it? How could you escape the context if it's replaced, no matter where it sits? – F.P Jan 25 '13 at 15:16
  • 2
    Please don't ask people to use deprecated function. `mysql_*` functions are officially deprecated, and will be removed from PHP at some point in the future. – Leigh Jan 25 '13 at 15:16
  • @Leigh Simple mistake, and it doesn't matter anyway as this question is not about which function to use. – Colin M Jan 25 '13 at 15:17
2

I suggest that the question of "can this approach be breached" is not relevant. The real question to ask is "Why would you use a homegrown ad hoc solution rather than one that has already been written, tested and debugged and is in use by thousands of users already?"

Andy Lester
  • 91,102
  • 13
  • 100
  • 152
  • That's what I've been telling the guy, but well - The system is already nearly done, I don't want to change everything, it's safe enough, yadda yadda... The usual excuses – F.P Jan 25 '13 at 15:17
1

This is actually a terrible way of "escaping" with a lot of pitfalls, and only few of them are

  1. This code is actually changing the data in the assumption that the only output medium would be HTML. From my 15 years experience I would say that it is not true. Altering your data is always a bad thing, it will lead to inconsistency and a lot of pain in the back in the future. One immediate of them is that such a "formatting" would be multiplied on every edit, making &ampampamp out of mere quote.
  2. Injections through numbers.
  3. Injections through identifiers.
  4. Second order injections.

...and much more.
Strict rules of this site prevents me from using proper words in naming such a "protection" but feel free to imagine them yourself.

Your Common Sense
  • 156,878
  • 40
  • 214
  • 345