1

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');
}
?>
4
  • 3
    Don't try to make your own SQL cleaner, use the one provided to you. mysql_real_escape_string. Commented Mar 11, 2013 at 19:44
  • 1
    What was the PDO code you tried? Commented Mar 11, 2013 at 19:45
  • mysql_fetch_array gives you all the fields, you don't need to use mysql_result also. Commented Mar 11, 2013 at 19:49
  • Thank you for the tips Rocket, I'll try dig out my previous code when I'm back on laptop Commented Mar 11, 2013 at 21:08

2 Answers 2

2

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.

Sign up to request clarification or add additional context in comments.

3 Comments

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.
@YourCommonSense, post your own answer, voting down someone else's because of a tangential design issue is petty and rude.
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
-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.

Comments

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.