0

First of all please forgive me for what must seem such a primitive question, I am very new to development and well we all have to start somewhere!

Apparently my code is open to SQL injection, the thing is I have tested following online SQL Injection tutorials, I have been trying to drop the table sub_category but it doesn't work so I think my code is safe?

This is what I put in the URL:

&scatid=1059' OR DROP TABLE sub_category //

Here is the code:

$scatid = $_GET['scatid'];    
mysql_select_db($database_yappee, $yappee);
                $query_products = "SELECT * FROM products WHERE sub_category='$scatid' AND active = 'Y' ORDER BY id DESC";
                $products = mysql_query($query_products, $yappee) or die(mysql_error());
                $row_products = mysql_fetch_assoc($products);
                $totalRows_products = mysql_num_rows($products);

Forgive me if this seems a silly question, I am just trying to get my head around things!

gccoda
  • 31
  • 5
  • Q: You've just answered your own question, haven't you? ALSO: Do *NOT* use [mysql_query](http://php.net/manual/en/function.mysql-query.php) in any new code. *ALWAYS* use either [mysqli](http://php.net/manual/en/book.mysqli.php) or [PDO MySQL](http://php.net/manual/en/ref.pdo-mysql.php) APIs instead. *ALWAYS* prefer [prepared statements](http://php.net/manual/en/mysqli.quickstart.prepared-statements.php) over "raw SQL". – FoggyDay Jan 11 '15 at 08:42
  • You cannot see how it can be injected?! Your code is the "Hello World" example of SQL injection. – Sverri M. Olsen Jan 11 '15 at 08:44
  • Sorry please I am new to this I am sure you were once! - please give examples or not bother posting. – gccoda Jan 11 '15 at 08:46
  • @gccoda: does the examples in the answer below help? (Aside: if you find someone's contribution unhelpful here, it is often best not to reply to it). – halfer Jan 11 '15 at 10:44
  • Bottom line: If you build executable SQL code with untrusted data, you are open to SQL injection. – Andy Lester Jan 20 '15 at 16:26

1 Answers1

3

(My implementation of MySQL uses # to denote comments, not //).

Here is how a user could get all of your products, active or not:

&scatid=1059' UNION ALL SELECT * from products #

Or an even easier way:

&scatid=1059' OR 1=1 #

For each example, imagine what the query would then look like:

SELECT * FROM products
WHERE sub_category='1059' OR 1=1 #
AND active = 'Y' ORDER BY id DESC

The comment device prevents the rest of the query getting in the way, so we essentially have:

SELECT * FROM products
WHERE sub_category='1059' OR 1=1

The 1=1 will always be true, hence all filtering has been subverted. This is of particular concern where security is involved, e.g. user login systems.

However, you have been trying this:

&scatid=1059' OR DROP TABLE sub_category //

That will result in a SELECT statement containing a DROP statement, which is not valid SQL. This is why your injection attempt did not work - the database would have returned an error, but your app did not report it.

halfer
  • 19,824
  • 17
  • 99
  • 186
chiliNUT
  • 18,989
  • 14
  • 66
  • 106
  • I've added another example to this, hope that's OK. I guess both of them would need a `#` at the end too? – halfer Jan 11 '15 at 10:40
  • @halfer For such a large scale edit I would have been bothered if you just did it without the extra comment, but since you asked, its all good! Thanks for adding the `#` I forgot as well. – chiliNUT Jan 11 '15 at 10:50
  • No probs. Substantial edits are part of the Stack Overflow ethic, I think, as long as they improve an answer. (You are still free to rollback or edit again, as it is your original authorship). – halfer Jan 11 '15 at 10:54
  • I'm not against it, but I think I've just mainly seen edits for formatting, grammer, and clarity. This seems more like an Orwellian rewrite of history, but as you say, I am of course free to rollback and future readers see the edited flag. Its interesting though – chiliNUT Jan 11 '15 at 11:02
  • That would be rather against the spirit of Stack Overflow, I'm afraid. Grammar and formatting are improved all the time, and it is an abuse of Orwell to invoke him in such a way. As the guidelines say, "if you are unhappy with your posts being edited, Stack Overflow may not be for you". If someone makes a change to your post and it is an obvious improvement, it is not worth rolling back unless there is a good reason. After all, we all want the best content on our community site, right? – halfer Jan 11 '15 at 11:03