PDO implementation, MySQL, SQL Injection

Question

I've been working on a project for a video website. It pulls information from the database and inserts the details where needed.

Everything so far is working perfectly, but I've just done a SQL injection test and everything is completely open. I've been looking around for answers to close it up and make things a bit more secure.

I've tried to implement the PDO statements but I can't get my head around it. I've only been working on php/sql for this month so I'm extremely new.

any help or other solutions would be amazing, the code below is my main connection point for the page which I believe is also the most vulnerable part

<?php
$username="********"; 
$password="*******";
$database="*******";

$id = $_GET['id']; 
$badchars = array("\"", "\\", "/", "*", "'", "=", "-", "#", ";", "<", ">", "+", "%");
$myid = str_replace($badchars, "", $id); 
mysql_connect('localhost',$username,$password);
@mysql_select_db($database) or die( "Unable to select database");
$result = mysql_query("SELECT * FROM Videos WHERE id='$id'");
while($row = mysql_fetch_array($result)) {
    $title=mysql_result($result,0,"title");
    $url=mysql_result($result,0,"url");
    $id=mysql_result($result,0,"id");
    $description=mysql_result($result,0,"description");
    $source=mysql_result($result,0,"source");
    $type=mysql_result($result,0,'type');
}
?>

***Don't*** try to make your own SQL cleaner, use the one provided to you. `mysql_real_escape_string`. — gen_Eric, Mar 11 '13 at 19:44
`mysql_fetch_array` gives you all the fields, you don't need to use `mysql_result` *also*. — gen_Eric, Mar 11 '13 at 19:49
Thank you for the tips Rocket, I'll try dig out my previous code when I'm back on laptop — Stewart, Mar 11 '13 at 21:08

Bill Karwin · Accepted Answer · 2013-03-11T20:49:06.037

Here's your example rewritten to use PDO, with explanations.

<?php
$username="********"; 
$password="*******";
$database="*******";

try {
  $pdo = new PDO("mysql:host=localhost;dbname=$database", $username, $password);
} catch (PDOException $e) {
  error_log("PDO connection error: " . $e->getMessage());
  header("Location: http://www.example.com/error.php");
  exit;
}

You can cast the GET parameter to int, which will just use the numeric part and strip off anything else.

$id = (int) $_GET['id'];

Leave a placeholder in the query where you want to substitute the dynamic value. You can use either positional parameters with the ? symbol, or named parameters with the colon-prefix syntax.

$sql = "SELECT * FROM Videos WHERE id = :id";

It's important to test for error after every call to prepare() or execute().

It's best if errors don't simply die(), leaving the browser with a white-screen, but they should recover if possible, or else at least display a friendly "whoops!" page so the user can continue to use your site.

$stmt = $pdo->prepare($sql);
if ($stmt === false) {
  $err = $pdo->errorInfo();
  error_log("PDO prepare error: " . $err[2]);
  header("Location: http://www.example.com/error.php");
  exit;
}

Pass an array of parameter values to substitute as an argument to execute(). In the case of named parameters, it's an associative array. In the case of positional parameters, use a simple ordinal array.

if ($stmt->execute(array(":id"=>$id)) === false) {
  $err = $stmt->errorInfo();
  error_log("PDO execute error: " . $err[2]);
  header("Location: http://www.example.com/error.php");
  exit;
}

Then you can fetch an associative array per row from the statement's result set:

while ($row = $stmt->fetch(PDO::FETCH_ASSOC)) {
  extract($row);
}

Note I showed the use of extract(), which is a PHP builtin function that creates variables $title, $url, etc. based on the keys of the associative array $row. But I did this only to match your code; ordinarily I would just reference these fields as $row["title"] and so on.

A note for the OP: all the `try..catch` and `$stmt->errorInfo()` blocks here are useless, making code bloated and less useful for no reason. When using this code for the real project, get rid of them all. — Your Common Sense, Mar 11 '13 at 20:58
@YourCommonSense, post your own answer, voting down someone else's because of a tangential design issue is petty and rude. — Bill Karwin, Mar 11 '13 at 21:00
Thanks for taking the time answer and give a very detailed explanation of each section, Just what I needed I'll try it out when I'm back with laptop and hopefully I can get my head around it. Sorry but, Sadly I can't up-vote this because I need +15 reputation in order to do so — Stewart, Mar 11 '13 at 21:10

Nathan · Answer 2 · 2013-03-11T19:51:07.533

-1

$result = mysql_query("SELECT * FROM Videos WHERE id='" . mysql_real_escape_string($_GET['id']) . "'");

... you might also want to consider using mysql_fetch_assoc() as opposed to mysql_fetch_array() which will greatly simplify the result values. Those calls to mysql_result() aren't needed at all.

edited Mar 11 '13 at 19:51

answered Mar 11 '13 at 19:45

Nathan

1,700
12
15

PDO implementation, MySQL, SQL Injection

2 Answers2