SQL Injection and php performance checking - revised

Question

This piece of code selects from the left table and will list the content in the right hand table. This is a working code but I would like to see how a professional would protect and make it faster.

Any suggestion (with some code) would be appreciated. Thanks a lot

PS: There is also a little glitch with it: after deleting it lose the selected item on the right list.

<?php include("db_con1.php");?>

<html>
<head>
</head>
<body>
<form method="post" action="test.php">

<div id="left">
<?php
  $queryl = $pdo->prepare('SELECT id, name FROM test1 ORDER BY name ASC');
  $queryl->execute();
?>

<ul>

  <?php foreach ($queryl as $i => $rowl) { ?>

  <li>
   <?php if ($i)?>
  <input name="checkbox_del[]" id="test_<?php echo $i ?>" type="checkbox" value="<? echo $rowl['id']; ?>"/>
  <label for="test_<?php echo $i ?>">
   <a href="test1.php?gid=<?php echo $rowl['id']; ?>"><?php echo $rowl['name']; ?></a>
  </label>
 </li>
  <?php } ?>
 </ul>
</div>

<div id="right">

<?php
  if(isset($_GET['gid'])) {
   $gid=$_GET['gid'];    
   $queryr = $pdo->prepare('SELECT test3.name FROM test1, test2, test3 WHERE test1.id=test2.groupid AND test3.id=test2.peopleid AND test1.id='.$gid.' ORDER BY test3.name ASC');
   $queryr->execute();
  }
?>

<ul>

  <?php foreach ($queryr as $i => $rowr) { ?>

    <li>
      <?php if ($i)?>
      <input name="checkbox_del[]" id="test_<?php echo $i ?>" type="checkbox" value="<? echo $rowr['id']; ?>"/>
      <label for="test_<?php echo $i ?>"><?php echo $rowr['name']; ?></label>
    </li>
  <?php } ?>
</ul>
</div>

<input type="submit" name="del" value="Delete the selected items">
</form>

<?php
if (isset($_POST['del'])) {
echo "Don't delete:)";
  for ($c = 0; $c < count($_POST['checkbox1_del']); $c++){
    $checkbox1_del = $_POST['checkbox1_del'][$c];
    $sql = 'UPDATE test1 SET status=0, log="'.date("Y-m-d").'"WHERE id='.$checkbox1_del;
    echo $sql;
    $query = $pdo->prepare($sql);
    $query->execute();
  }

  for ($c = 0; $c < count($_POST['checkbox2_del']); $c++){
    $checkbox2_del = $_POST['checkbox2_del'][$c];
    $sql = 'UPDATE test2 SET status=0, log="'.date("Y-m-d").'"WHERE id='.$checkbox2_del;
    echo $sql;
    $query = $pdo->prepare($sql);
    $query->execute();
   }

    if($query){
      echo "<meta http-equiv=\"refresh\" content=\"0;URL=test1.php\">";
     }
 }
?>

</body>
</html>

Revision 1: now I have had some feedback so I just would like to ask which is better, would this be better?

<?php
if(is_numeric($_GET['gid'])) {
 $queryr = $pdo->prepare('SELECT test3.name FROM test1, test2, test3 WHERE test1.id=test2.groupid AND test3.id=test2.peopleid AND test1.id=:id ORDER BY test3.name ASC');
 if( $queryr->execute(array(':id' => $_GET['id'])) ) {
    $result = $queryr->fetch();
 }
}
?>

or this?

<?php
if(is_numeric($_GET['gid'])) {
 $gid = $_GET['gid'];    
 $queryr = $pdo->prepare('SELECT test3.name FROM test1, test2, test3 WHERE test1.id = test2.groupid AND test3.id = test2.peopleid AND test1.id = :gid ORDER BY test3.name ASC');
 $queryr->bindParam(':gid', $gid, PDO::PARAM_INT);
 $queryr->execute();
?>

instead of this? (please be polite if I did something wrong as I am a beginner:)

<?php
  if(isset($_GET['gid'])) {
   $gid=$_GET['gid'];    
   $queryr = $pdo->prepare('SELECT test3.name FROM test1, test2, test3 WHERE test1.id=test2.groupid AND test3.id=test2.peopleid AND test1.id='.$gid.' ORDER BY test3.name ASC');
   $queryr->execute();
  }
?>

If you're really willing to learn, go out on the web and read some tutorials. Search for "prepared statements" and "input sanitation" — Rijk, Aug 31 '11 at 07:52
thanks for the comment I've been there and the plenty different opinion about the whole thing. I would try to ask here as well. More opinions can narrows down the answer. — TryHarder, Aug 31 '11 at 07:59
You might want to consider to look at http://codereview.stackexchange.com/ instead of stackoverflow. They're dedicated to reviewing existing code and improving it. — Konerak, Aug 31 '11 at 08:03
I don't know yet as I have to complete this function and I will upload with a larger amount of data to check it. — TryHarder, Aug 31 '11 at 14:09

score 3 · Answer 1 · answered Aug 31 '11 at 07:54

3

Just remember, if there are any variable which does not do escape in your sql, there could be SQL injection hole.

Don't use the variable in your sql, use the placeholder for prepared statement.

answered Aug 31 '11 at 07:54

xdazz

158,678
38
247
274

thanks for your hint. can $string = mysql_real_escape_string($string); wokr with pdo? – TryHarder Aug 31 '11 at 08:50
2

@Andras With pdo, you should use `PDO::quote()`, but you are strongly recommended to use `PDO::prepare()` to prepare SQL statements with bound parameters instead of using `PDO::quote()` to interpolate user input into an SQL statement. – xdazz Aug 31 '11 at 09:00
thanks for the comment could you have a look at my 'homeworked' solution?:) – TryHarder Aug 31 '11 at 13:58
@Andras If your id is int, the second way. By the way, you can also use `bindValue()` to avoid define the `$gid`. – xdazz Aug 31 '11 at 14:05

score 1 · Answer 2 · answered Aug 31 '11 at 07:53

1

SELECT test3.name FROM test1, test2, test3 WHERE test1.id=test2.groupid AND test3.id=test2.peopleid AND test1.id=... is open to SQL injections... http://xkcd.com/327/

the same with UPDATE test1 SET status=0, log="'.date("Y-m-d").'"WHERE id='.$checkbox1_del;..

your page is open to simple SQL attacks.. you should learn about SQL injection and prepared statements.

answered Aug 31 '11 at 07:53

Quamis

10,924
12
50
66

thanks for pointing it out. Would you be so kind to show some best practice how to fix them? – TryHarder Aug 31 '11 at 08:01
i said "you should learn about SQL injection and prepared statements." try googling for that. – Quamis Aug 31 '11 at 09:10
well done, regarding your revision, in the first case you simply execute a statement, noting is returned... so second case is really the only useable one – Quamis Aug 31 '11 at 13:59
thanks Quarmis, I am glad that I have learned something again today:) – TryHarder Aug 31 '11 at 14:02
@Quarmis, I found this: Although bindValue() escapes quotes it does not escape "%" and "_", so be careful when using LIKE. A malicious parameter full of %%% can dump your entire database if you don't escape the parameter yourself. PDO does not provide any other escape method to handle it. – TryHarder Aug 31 '11 at 14:13
I dont know why that works like this. But you now need to learn about input sanitizing. look @ http://stackoverflow.com/questions/129677/whats-the-best-method-for-sanitizing-user-input-with-php – Quamis Aug 31 '11 at 14:28

SQL Injection and php performance checking - revised

2 Answers2