1

Im using the following code

$query = "SELECT * FROM raids WHERE RaidNum = '".$_GET["RaidNum"]."'";

which catches from /raiddisplay.php?RaidNum=r75

My question is this entirely safe? can the value be exploited in some way to do something nasty and is there ways in which you can cleanse it. I tried to lookup up example usage of it but most were horribly complex and i really didn't know where to start with it. Basically i want to make sure that somebody doesn't purposely put in a value into the browser address bar that could have nasty adverse effects

Ryan Walkowski
  • 311
  • 10
  • 19
  • 1
    It is infact, the exact opposite of safe. Read [here](http://stackoverflow.com/questions/60174/best-way-to-stop-sql-injection-in-php) for safer ways to accomplish this. – Perception Mar 11 '12 at 18:44
  • No, this is not safe. Read up on sanitizing inputs. Here's a snippet from W3 on [sanitizing inputs](http://www.w3schools.com/php/php_filter.asp). – nategood Mar 11 '12 at 18:45
  • possible duplicate of [Best way to stop SQL Injection in PHP](http://stackoverflow.com/questions/60174/best-way-to-stop-sql-injection-in-php) – dynamic Mar 11 '12 at 18:45
  • @Perception That is why i posted it because i was sure it was unsafe but in attempt to make it safe all the literature i came up with was very complex – Ryan Walkowski Mar 11 '12 at 18:53
  • @yes123 i was not aware this is classified as sql injection? I dont know enough about that practises to really comment – Ryan Walkowski Mar 11 '12 at 18:55
  • @nategood Thanks i did not even realise php have such filter functions. Im reading up on this now – Ryan Walkowski Mar 11 '12 at 18:57

4 Answers4

4

It is entirely not safe. Let's assume I visited /raiddisplay.php?RaidNum=';drop%20table%20raids;--, then I would effectively drop your table instead of reading a record.

The best solution is to use prepared statements. Some may suggest to use mysql_real_escape_string, but even that is old and cumbersome. Although it is safe in itself, you must remember to always apply it. Using prepared statements with parameters or use a library that creates the statements for you, you are always safe.

I think the easiest way to use this feature, is to use PDO, or PHP Data Objects.

GolezTrol
  • 114,394
  • 18
  • 182
  • 210
  • Could you elaborate on what a prepared statement is? – Ryan Walkowski Mar 11 '12 at 18:45
  • 1
    Yes, it is a statement that contains parameters in which you pass the values to query for. The statement is prepared, then the values are entered. In these kind of queries, the values are not parsed by the query parser at all, so you don't need any escaping. Please, take a look at the PDO link I added. – GolezTrol Mar 11 '12 at 18:51
  • 2
    Gosh there is a lot to look through their but thanks for pointing me in the direction i will look into that right away – Ryan Walkowski Mar 11 '12 at 19:04
1

Your best bet is to wrap the $_GET with a mysql_real_escape_string function. When you are more advanced, move on to PDO.

 $query = "SELECT * FROM raids WHERE RaidNum = '".mysql_real_escape_string($_GET["RaidNum"])."'";
yehuda
  • 1,254
  • 2
  • 11
  • 21
  • 1
    So PDO is advanced? It is not more difficult at all. Don't bother with the old `mysql_` functions. Forget them, the sooner the better. – kapa Mar 11 '12 at 19:00
  • I find they are usually associated with OOP which is semi advanced PHP, def not where the asker is holding at the moment! – yehuda Mar 11 '12 at 19:02
  • Yeah i am nowhere near advanced i have to lookup pretty much every piece of code i do to find examples and explanations for it. – Ryan Walkowski Mar 11 '12 at 19:06
  • @RyanWalkowski then stick with my suggestion, you can always upgrade later! Unless you know Object Oriented Programming, you will find it quite hard! – yehuda Mar 11 '12 at 19:09
1

You should be using PDO as best practice, this will allow you to parameterise your query ensuring it is as safe as possible.

First you can do a simple check that RaidNum is an int (I am assuming this is the case here);

$raidNum = (int)$_GET['RaidNum']; //This is pretty good sanitisation in itself if
                                  //you are expecting integers

You can see how to instantiate a PDO object in the docs.

Then you first prepare your query:-

$stmnt = $dbh->prepare('SELECT * FROM raids WHERE RaidNum = :RaidNum');

Then you can execute the statement:-

$stmnt->execute(array(':RaidNum' => $raidNum));

The final step is to fetchAll or fetch the result

$result = $stmnt->fetchAll();

This probably a little more complex than you were expecting, but it is well spent effort. You can always hide this away in an abstraction layer or a function.

vascowhite
  • 18,120
  • 9
  • 61
  • 77
-2

Your instincts were correct. This isn't safe. Anytime you pass sensitive information (like form data or data anyone can view/alter) through MySQL, you want to escape it. Do this:

$raidNum = mysql_real_escape_string($_GET["RaidNum"]);
$query = sprintf("SELECT * FROM raids WHERE RaidNum = '$raidNum'");

http://php.net/manual/en/function.mysql-real-escape-string.php

Norse
  • 5,674
  • 16
  • 50
  • 86
  • This is a good answer. Wonder why it was down voted. Oh well ;) – Norse Mar 11 '12 at 18:51
  • from the phh/mysql manual i read the following: "mysql_real_escape_string() calls MySQL's library function mysql_real_escape_string, which prepends backslashes to the following characters: \x00, \n, \r, \, ', " and \x1a." Aside from these dangerous characters one of which i actually knew from above. Does this method protect it enough. What im trying to get at is there anything else that can be passed that does not involve these special characters – Ryan Walkowski Mar 11 '12 at 18:52
  • @RyanWalkowski I have never encountered any problems using `mysql_real_escape_string` in combination with `sprintf`. As to your question, I do not know exactly. – Norse Mar 11 '12 at 18:54
  • Why that `sprintf` there? It makes no sense. – kapa Mar 11 '12 at 18:57
  • 1
    LOL you were probably downvoted for using `sprintf` for no reason... just a waste of server CPU cycles. – kitti Mar 11 '12 at 18:59