PHP PDO Security

Question

Im trying to work with PDO for the first time and I'm just wanting to know how secure what I'm doing is, I'm also new to PHP.

I have a query that when a user is passed ot my page, the page takes a variable using GET and then runs.

With PHP I've always used mysql_real_escape to sanitize my variables.

Can anybody see security flaws with this?

// Get USER ID of person
$userID = $_GET['userID'];

// Get persons
$sql = "SELECT * FROM persons WHERE id =$userID";
$q = $conn->query($sql) or die($conn->error());
while($r = $q->fetch(PDO::FETCH_LAZY)){
    echo '<div class="mis-per">';
    echo '<span class="date-submitted">' . $r['date_submitted'] . '</span>';
   // MORE STUF
    echo '</div>';
}

You are injecting a potentially tainted variable into your SQL. Either cast it to `(int)`, or use parameterisation. — halfer, Jul 12 '12 at 10:43
see [link](http://stackoverflow.com/questions/60174/best-way-to-prevent-sql-injection-in-php) — Federkun, Jul 12 '12 at 10:43
Well, you are not really using any advantages of PDO and are injecting user input directly. PDO doesn't automagically clean everything, you need to use prepared statements so PDO can clean the parameters that are passed later on. — N.B., Jul 12 '12 at 10:45
you have no security at all in your query - you're directly inserting user data into the query string, making you vulnerable to SQL injection attacks. Read up about parameterized/placeholder queries — Marc B, Jul 12 '12 at 10:45
For easy conversion from `mysql_*` to PDO, just replace `mysql_real_escape_string($str)` with `$pdo->quote($str)` and make sure to remove the quotes around the string you were quoting. — Tom van der Woerdt, Jul 12 '12 at 10:53
One important note is, PDO do not execute more than 1 query at time. So, Nobody can run another query in your code. @Tom van der Woerdt answer is complementary your question. — Hamidreza, Jul 12 '12 at 10:53
@hamidreza66 That's not true, someone can still `UNION` a second query to your first query. — Tom van der Woerdt, Jul 12 '12 at 10:54

WolvDev · Accepted Answer · 2012-07-12T10:57:20.877

4

Don't use query, use prepare:

http://php.net/manual/de/pdo.prepare.php

$userID = $_GET['userID'];

$sql = "SELECT * FROM persons WHERE id = :userid";

$q = $conn->prepare($sql)
$q->execute(array(':userid' => $userID ));

while($r = $q->fetch(PDO::FETCH_ASSOC)){ 
    echo '<div class="mis-per">'; 
    echo '<span class="date-submitted">' . $r['date_submitted'] . '</span>'; 
   // MORE STUF 
    echo '</div>'; 
}

The SQL statement can contain zero or more named (:name) or question mark (?) parameter markers for which real values will be substituted when the statement is executed.

edited Jul 12 '12 at 10:57

answered Jul 12 '12 at 10:47

WolvDev

3,182
1
17
32

1

Yeah, didn't spotted I used $sql inside the execute method. Was there cause of copy & paste. Sorry. But instead of downvoting or just writing "this is wrong" you could have said more about it :) – WolvDev Jul 12 '12 at 10:52
Ahh sorry, I wrote $userid inside the execute array and you have $userID – WolvDev Jul 12 '12 at 11:01

score 2 · Answer 2 · answered Jul 12 '12 at 10:47

2

With anything you use, it's about how you use it rather than what you use. I'd argue that PDO itself is very safe as long as you use it properly.

$sql = "SELECT * FROM persons WHERE id =$userID";

That's bad *. Better :

$sql = "SELECT * FROM persons WHERE id = " . $conn->quote($userID);

Better :

$q = $conn->prepare('SELECT * FROM persons WHERE id = ?')->execute(array($userID));

* This is bad, and that's because if $userID is "1 OR 1", the query becomes SELECT * FROM persons WHERE id =1 OR 1 which will always return all values in the persons table.

answered Jul 12 '12 at 10:47

Tom van der Woerdt

29,532
7
72
105

Thanks @Tom, this returns... Fatal error: Call to a member function fetch() on a non-object in public_html/people/user-profile.php on line 12 – Liam Jul 12 '12 at 10:50
@Liam That normally means that the query failed to execute, which causes `query()` (or `execute()`) to return `NULL` and you can't call `fetch()` on `NULL`. – Tom van der Woerdt Jul 12 '12 at 10:51
// Get persons $sql = "SELECT * FROM persons WHERE id = " . $conn->quote($userID); $q = $conn->prepare('SELECT * FROM persons WHERE id = ?')->execute(array($userID)); while($r = $q->fetch(PDO::FETCH_LAZY)){ – Liam Jul 12 '12 at 10:58

score 1 · Answer 3 · answered Jul 12 '12 at 10:49

As the comments say: Atm there is no security whatsoever against SQLI. PDO offers you (if the database driver supports it (mysql does)) Prepared Statements. Think of it like a query-template that is compiled/passed to the dbms and later filled with values. here is an example of usage:

$sql = 'SELECT name, colour, calories
  FROM fruit
  WHERE calories < :calories AND colour = :colour';

 //Prepare the Query

$sth = $dbh->prepare($sql);

     //Execute the query with values (so no tainted things can happen)
$sth->execute(array(':calories' => 150, ':colour' => 'red'));
$red = $sth->fetchAll();

A_nto2 · Answer 4 · 2012-07-12T10:59:37.187

Adjust as follows (you can use either :userId or simply ? as Tom van der Woerdt suggests, even if I think the first one gives more clearness, especially when there are more than just one parameter):

$sql = "SELECT * FROM persons WHERE id =:userID";
$q = $conn->prepare( $sql );
    $q->bindValue( ":userID", $userID, PDO::PARAM_INT ); // or PDO::PARAM_STR, it depends
    $q->execute();
    $r = $st->fetch();
...
...

PHP PDO Security

4 Answers4