PHP Sql injection vulnerable

Question

I wrote the below script as my very first ever php mysql application. I am self taught and the code works as intended. My host thinks it may be vulnerable to sql injection attacks but cannot tell me why or what I should change to make it better. I'm sure it's not as clean as it could be but if anyone has any suggestions or insight I would certainly appreciate it.

<form  method="post" action="search.php?go"  id="searchform">
      <?php
      $db=mysql_connect ("server",  "*", "*") or die ('I cannot connect  to the database because: ' . mysql_error());
  $mydb=mysql_select_db("*");
$category_sql="SELECT distinct category FROM Members";
$category_Options="";
$category_result=mysql_query($category_sql) or die ('Error: '.mysql_error ());
while ($row=mysql_fetch_array($category_result)) {

    $category=$row["category"];
    $category_Options.="<OPTION VALUE=\"$category\">".$category.'</option>';
}
?>
   <p>
          <SELECT NAME="category"><OPTION VALUE=0>Choose<?=$category_Options?></SELECT>

   </p>
<input name="submit" "id="submit" type="submit" value="submit" />
    </form>


<?php
  if(isset($_POST['submit'])){
  if(isset($_GET['go'])){
  $category=$_POST['category'];
  $category=mysql_real_escape_string($category);
  $sql="SELECT category, company, address, city, state, zip, phone, web, addescription, image
  FROM Members
  WHERE category LIKE '$category'";
$result=mysql_query($sql);
  while($row=mysql_fetch_array($result)){
        $category2=$row["category"];
        $company=$row["company"];
        $address=$row["address"];
        $city=$row["city"];
        $state=$row["state"];
        $zip=$row["zip"];
        $phone=$row["phone"];
        $web = $row["web"];
        $addescription = $row["addescription"];
        $image = $row["image"];
  echo "<blockquote>";
  if(@file_get_contents($image))
{
  echo "<img src='".$image ."' class='image'/>\n";
}
else
{
}
  echo "<p>\n";
  echo "</br>".$category2 . "\n";
  echo "</br><b>".$company . "</b>\n";
  echo "</br>".$address . "\n";
  echo "</br>".$city . ", ".$state. " ".$zip . "\n";
  echo "</br>".$phone . "\n";
  echo "</br><a href=http://".$web .">".$web ."</a>\n";
  echo "</br>".$addescription . "\n";
  echo "</br><a href=http://www.printfriendly.com style=color:#6D9F00;text-decoration:none; class=printfriendly onclick=window.print();return false; title=Printer Friendly and PDF><img style=border:none; src=http://cdn.printfriendly.com/pf-button.gif alt=Print Friendly and PDF/></a>\n";
  echo "</p>";
  echo "</blockquote>"
  ;
 }


  }
  else{
  echo  "<p>Please select a Category</p>";
  }
  }
mysql_close($db)
?>

Have you checked this [article](http://stackoverflow.com/questions/60174/how-to-prevent-sql-injection-in-php)? It pretty much covers it. — Hristo Valkanov, Jul 26 '13 at 15:13
There are lots of article to prevent SQL injection. Why not google it ;) — tonoslfx, Jul 26 '13 at 15:14
Please note that the `mysql_xxx()` functions are deprecated, and have been considered obsolete for years. If you learnt from a tutorial that used them then you are almost certainly doing things wrong, as a tutorial old enough to recommend using those functions would also be old enough to be using other bad practices that aren't recommended any more. I suggest finding a site with more up-to-date tutorials to learn from. — Spudley, Jul 26 '13 at 15:15
If you are not using some character sets (GBK or BIG-5) you are ok: http://stackoverflow.com/a/12118602/151758 Someone could still make a form (for example using firebug) to replace one of your categories values by "%" which could be a bad thing if you have millions of members. But as you are learning, you should get good habits as soon as possible: the mysql extension is deprecated in php 5.5 and a prefered way to send SQL queries is by using parameterized queries. PDO is a really good way to do it, you should be able to get a lot of infos about it on Stackoverflow or the php doc. — Arkh, Jul 26 '13 at 15:36

score 3 · Answer 1 · answered Jul 26 '13 at 15:18

The MySQL functions are deprecated. Using the MySQLi functions, and prepared statements, are a better way to protect against sql injection attacks.

$stmt = $mysqli->prepare('SELECT category, company, address, city, state, zip, phone, web, addescription, image FROM Members WHERE category LIKE ?');
$stmt->bind_param('s', $category);

score 1 · Answer 2 · answered Jul 26 '13 at 15:26

I'll show the implementation of a PDO connection and how to query with it. Here it goes! First we create the connection variable with your database credentials. We'll store this connection in $db.

$username = "root";
$password = "";
$host = "localhost";
$dbname = "my_database";

$options = array(PDO::MYSQL_ATTR_INIT_COMMAND => 'SET NAMES utf8');

try{
   $db = new PDO("mysql:host={$host};dbname={$dbname};charset=utf8"; $username, $password, $options);
}catch(PDOException $ex){
   die("Failed to connect: ".$ex->getMessage());
}

Now you have a PDO connection stored in $db which you can query through. You may want to account for magic quotes if you're not using PHP 5.4, so keep that in mind.

Otherwise, create your query statement like so..

$query = "SELECT category, company, address, city, state, zip, phone, web, addescription, image FROM Members WHERE category LIKE :category"

Afterwards, you want to bind the value from the $_POST['category'] variable (or $category since you created that) to the parameter :category. Do that like so:

$query_params = array( ':category' => $category);

Finally, now that you have the statement and the parameters, use the previously created $db variable to prepare and execute the statement.

$statement = $db->prepare($query);
$result = $statement->execute($query_params);

Since we're SELECTing data where it could return multiple rows (assuming you have multiple rows within a category), we need to account for that. Grab the rows that the statement returns like so:

$rows = $statement->fetchAll();

And now you could refer to column headers within each $row of the database table by utilizing a foreach statement.

$citiesArray = array();
foreach($rows as $row){
   if(isset($row['city'])){
       $citiesArray[] = $row['city'];
   }
}

Hope that helps out!

score 1 · Answer 3 · answered Jul 26 '13 at 15:27

Just remember the golden rule of never trusting your users. Never take any raw user input and insert it into a database, as there's a chance that you have left yourself wide open for a security issue.

Your code seems fine. However, do note that MySQL is deprecated as of PHP 5.5.0, and instead you should use MySQLi or PDO extension which provide more security.

Maybe that's the reason your host said such thing, but from a quick look on your code it seemed fine to me.

Cheers.

Lucas Kahlert · Answer 4 · 2013-07-29T13:53:37.713

The problem is the follwoing part in your code

$sql = "SELECT category, company, address, city, state, zip,
             phone, web, addescription, image
        FROM Members
        WHERE category LIKE '$category'";
$result=mysql_query($sql);

If the parameter $category is read from the GET or POST parameters, it should be escaped:

$sql = "SELECT category, company, address, city, state, zip,
             phone, web, addescription, image
        FROM Members
        WHERE category LIKE '" . mysql_real_escape_string($category) . "';";

If you are doing it this way, the variable cannot be used for SQL Injection

By the way (like Matthew Johnson said), the procedural mysql extension is deprecated since PHP 5.5. You should better use Mysqli or PDO.

The OOP way (strongly recommended) would look like:

$pdo = new PDO($dsn, $user, $password, $options);
$statement = $pdo->prepareStatement(
    "SELECT category, company, address,
            city, state, zip, phone, web,
            addescription, image
     FROM Members
     WHERE category LIKE :category;");
$statement->bindParam(':category', $category, PDO::PARAM_STR);
$statement->execute();
$categories = $statement->fetchAll();

This is how PHP was done fifteen years ago and it's about time people stopped doing it this way. — tadman, Jul 26 '13 at 15:35
Like I mentioned in my post: the procedural way is depracted. I have added to usage of PDO. — Lucas Kahlert, Jul 29 '13 at 13:55

PHP Sql injection vulnerable

4 Answers4