0

First of all, yes, I've seen this Are PDO prepared statements sufficient to prevent SQL injection?, but it's not enough to answer my question, as I'm doing it differently.

Let's say I have code like this:

$userid = $_SESSION['data']['id'];
$query = "SELECT * FROM table WHERE userid='$userid'";
$action = $db->prepare($query);
$action->execute();

It's technically safe, as user can't affect to $userid, right? Say if I'm wrong on this.

But what if the code looks like this:

$userid = $_GET['id'];
$query = "SELECT * FROM table WHERE userid='$userid'";
$action = $db->prepare($query);
$action->execute();

Is it safe anymore? I'm unable to find any documentation about this that gives me black on white.

Community
  • 1
  • 1
  • 2
    [How prepared statements can protect from SQL injection attacks?](http://stackoverflow.com/questions/8263371/how-prepared-statements-can-protect-from-sql-injection-attacks/8265319#8265319) <-- here is that documentation you were asking for – Your Common Sense Mar 10 '13 at 11:44
  • 3
    No, it is not. PDO is too often portrayed as magic solution. But it's "*prepared statements*" which make it secure. Which you are not using. – mario Mar 10 '13 at 11:45
  • Please don't downvote a question without reason. Upvoting. – Nate Higgins Mar 10 '13 at 11:47
  • So, basically, I should `$db->prepare("SELECT * FROM users where id=?");` and then give the data at the execution? –  Mar 10 '13 at 11:48
  • yes. and also http://stackoverflow.com/tags/pdo/info to cover all the complex cases you will face in the future – Your Common Sense Mar 10 '13 at 11:50
  • @YourCommonSense So, if I'm using this on a live site, anyone could simply type `;DROP TABLE users` and good bye table users? –  Mar 10 '13 at 11:52
  • actually not, as it won't let a second query. it's just a saturated example. but there could be other injection, no less disastrous – Your Common Sense Mar 10 '13 at 11:55
  • @YourCommonSense Can you give me an example of a possible injection? –  Mar 10 '13 at 11:57

3 Answers3

2

even if you have used PDO, your code is still vulnerable with SQL INjection because you have not parameterized the query, query must be parameterized in order for the values to be cleaned.

$userid = $_GET['id'];
$query = "SELECT * FROM table WHERE userid=?";
$db->setAttribute( PDO::ATTR_EMULATE_PREPARES, false );
$action = $db->prepare($query);
$action->bindParam(1, $userid);
$action->execute();
John Woo
  • 258,903
  • 69
  • 498
  • 492
  • Could I skip the parameter binding and enter the data on execution point? –  Mar 10 '13 at 11:49
  • Don't forget to use real prepared statements with PDO using `$db->setAttribute( PDO::ATTR_EMULATE_PREPARES, false );`. – Marcel Korpel Mar 10 '13 at 11:53
1

The second statement isn't safe.

Instead, you should do something like

$stmt = $db->prepare('SELECT * FROM table WHERE userid=:id');
$stmt->bindParam(':id', $userid);
$stmt->execute();

Source

Nate Higgins
  • 2,104
  • 16
  • 21
0

It's technically safe, as user can't affect to $userid, right? Say if I'm wrong on this.

You are wrong with that. Session data is outside data and must be treated with caution. This is because of:

  • SessionID and SessionName are given with the request directly. These values can be easily manipulated so that some different data is being put into the memory of your application.
  • Persistence. Session data can be modified in the persistence layer so it qualifies as input data always (!).

You are likely expecting an integer value, so make it one:

$userid = (int) $_SESSION['data']['id'];

Especially as you directly substitute the variable into your SQL query.

In the future don't think if it is safe. Consider to do things in a safe manner so that even if you missed something in another layer (like input through session) don't breaks the data-flow in your application.

$pdo->setAttribute(PDO::ATTR_EMULATE_PREPARES, false);

...

$userid = (int) $_SESSION['data']['id'];

...

$query = "SELECT column FROM table WHERE userid = ?";

$stmt = $pdo->prepare($query);
$stmt->bindParam(1, $userid);
$stmt->execute();
hakre
  • 193,403
  • 52
  • 435
  • 836
  • Even if it were safe: if `$userid` could correctly contain a `'`, things will go wrong. – Marcel Korpel Mar 10 '13 at 11:59
  • Yes it's not safe in PHP because the process writing the data into session is not the same that is reading it from session. Therefore there is no control about what is in there. That actually is because of persistence. – hakre Mar 10 '13 at 12:01
  • So let me get this straigt. If I have a query like this : `SELECT * FROM table WHERE something=? AND somethingelse=? AND something=?` I would **safely** execute it with `$action->execute($var1,$var2,$var3)`? –  Mar 10 '13 at 12:17
  • 1
    @Christian: Please take a look at this short video that explains it pretty well: [Programming With Anthony - Prepared Statements](https://www.youtube.com/watch?v=nLinqtCfhKY) – hakre Mar 10 '13 at 12:32