-1

I can't really get my head around all the different methods of cleaning up a GET request so I thought I would ask for some help.

$view=$_GET['view'];

if($view){
$result = mysql_query('SELECT * FROM main WHERE category = "'.$view.'"', $main);
}

Are there any security concerns here? I tried to search for examples to try and inject the string but I couldn't find anything that worked.

I was thinking of using a preg_match with only letters and numbers but wasn't sure if it was even required.

As much as I want to know the answer, is there any background reading that I should look at to help me? There are so many different scenarios and ways to clean a string for those different situations that I'm struggling to get my head around it all.

Funk Forty Niner
  • 74,450
  • 15
  • 68
  • 141
Scott
  • 15
  • 5
  • [The Great Escapism (Or: What You Need To Know To Work With Text Within Text)](http://kunststube.net/escapism/) – deceze Jun 19 '13 at 16:09

3 Answers3

1

Try this:

?view=derp" OR SLEEP(10000000) = "problem?

Enough requests to this and your site is DoS.

Best way to avoid it is with mysql_real_escape_string, or update to something more up-to-date such as PDO.

Niet the Dark Absol
  • 320,036
  • 81
  • 464
  • 592
1

Are there any security concerns here?

Yes, you have an SQL Injection vulnerability.

To secure your query, you should consider using PDO or MySQLi and use a prepared statement. A prepared statement offers much more security over escaping or validating variables. Prepared statements never insert variables into the query, instead what happens is the variables (or parameters) are handled separately by the MySQL server and thus leave no possibilty of SQL Injection.

This is a good PDO tutorial that is aimed at developers coming from a mysql_* background, and it includes some prepared statement examples.

Example of an SQL Injection that would work on your query:

script.php?view='' OR 1

This would fool your code into retrieving all rows from the table because it would cause the WHERE clause to always evaluate to true. This is what the query would become:

SELECT * FROM main WHERE category = '' OR 1

MrCode
  • 63,975
  • 10
  • 90
  • 112
0

You should switch to using prepared statements using mysqli or PDO. That will fix the security issues. Plus, mysql_ functions are deprecated and will be removed.

Jessica
  • 7,075
  • 28
  • 39
  • 2
    Instead of just telling the OP what to do, an *explanation* of what he's doing wrong would be a lot better. Kneejerk *"PDO!"* answers seem to be the new *"jQuery!"*? – deceze Jun 19 '13 at 16:10
  • 2
    @deceze I think that type of question has been asked (and answered) so many times that a RTFM-style answer is perfectly acceptable,. More so if that answer gives the OP the right keywords to look up. – Marcello Romani Jun 19 '13 at 16:25
  • I think he knows what he's doing wrong, because he knew enough to google for SQL injection attempts. He just doesn't know how to fix it. Also PDO *is* the solution to this problem. So... – Jessica Jun 19 '13 at 16:29
  • @Marcello Then a *close as duplicate* is in order. The problem is that other newbs will come across this question, and the only thing they see is the answer "use PDO", which instills the believe that there's no other way to prevent SQL injection (no, it's by far not the only way) and they still don't know what is actually causing the problem or what exactly PDO supposedly does to fix it. Does `$pdo->prepare("SELECT foo FROM bar WHERE baz = '$_GET[baz]'")` solve the problem? – deceze Jun 19 '13 at 16:45
  • @deceze you're right, but Jessica's answer starts with "you should switch to using prepared statements", and PDO are mentioned as a mean through which to implement them. That's quite different from a simple "use PDO", IMHO :-) – Marcello Romani Jun 19 '13 at 17:19
  • @deceze Sorry, wrote "prepared statements" when I was in fact thinking about bind variables... which are not mentioned in Jessica's answer. So you're probably right after all :-P – Marcello Romani Jun 19 '13 at 18:23