PDO code still vulnerable to SQL Injection

Question

I would like to know why my code is vulnerable to SQL Injection, despite me using PDO prepare and execute?

Heres my code:

$conn = new PDO('mysql:host=localhost;dbname=SQLHack', $username, $password);
$conn->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_SILENT);

$id = $_GET['id'];

$query = "SELECT * FROM users WHERE username ='$id'";
$data = $conn->prepare($query) or die("ERROR: " . implode(":", $conn->errorInfo()));

$data->execute(array(':username'));
$data->setFetchMode(PDO::FETCH_BOTH);

while ($r = $data->fetch()) {
    echo "<br />\n";
    print_r("ID: " . $r['id'] . "  Username: " . $r['username']);
}

The line that this is vulnerable to is this one, but if its vulnerable to this it is vulnerable to many other.

' or 1=1 union select 1,2,3'

If that is entered it reveals information when it really shouldn't.

You're not using bound parameters, is why. You're passing `$_GET['id']` right into your query. — andrewsi, Dec 16 '13 at 19:28
Have a look at [**this Q&A on SO**](http://stackoverflow.com/q/5741187/1415724) – There's a mention about PDO injection on that page. Whoever said PDO was perfect, is (probably) not working in this town again. — Funk Forty Niner, Dec 16 '13 at 19:29
@andrewsi I thought the way PDO does this is with the prepare? I assumed that was the alternative to `mysqli_real_escape_string()`? — user2157179, Dec 16 '13 at 19:30
simply switching mysql -> pdo/mysqli will **NEVER** magically cure sql injection problems. You have to fundamentally alter how you write queries, and you're still writing as if prepared statement **PLACEHOLDERS** didn't exist. — Marc B, Dec 16 '13 at 19:30
@user2157179 - You need to have a placeholder in your SQL for the bound parameter to replace. Try replacing `'$id'` with `:username`. — andrewsi, Dec 16 '13 at 19:31
"Use these parameters to bind any user-input, do not include the user-input directly in the query." from http://www.php.net/manual/en/pdo.prepare.php — geedubb, Dec 16 '13 at 19:32
You're not using a placeholder, instead you're directly passing the variable inside the query. — Ali, Dec 16 '13 at 19:34

score 4 · Answer 1 · answered Dec 16 '13 at 19:36

You're directly inserting external data into a query. Consider these two variations of a simple query:

SELECT foo FROM bar WHERE baz=$_GET[id]
SELECT foo FROM bar WHERE baz=:id

The first one is "classic" traditional PHP. "Golly gee willikers... we live in a perfect world where no one would EVER attack a server. Let's just directly stuff user-provided data into this SQL query and go sing Kumbaya while it executes. Nothing could ever possibly go wrong".

The second is classical defensive programming using a placeholder. In both cases, the value of this id thingie will eventually get stuffed into the baz field in your table. But how it gets there is totally different.

Consider a malicious user passing in some bad data: $_GET['id'] = '0 or 1=1' With the first one, the SQL server simply sees:

SELECT foo FROM bar WHERE baz=0 or 1=1

It has no ABSOLUTELY NO WAY of knowing that the 0 or 1=1 came from some external source - all of that metainformation is LOST when you create the query string.

If you use a prepared statement, then the DB knows that :id is a place where "external" data is going to be used, and it can keep that external data totally separate from the query itself, never allowing the contents of the external data to influence the meaning of the query. How? Because it's a placeholder - it's a direct "beat the DB server over the head with a clue-bat" datum that says "this is where outside data goes. put on your biohazard suit and treat it carefully".

BaBL86 · Answer 2 · 2013-12-16T19:41:18.323

-1

that's because you don't really use PDO.

$query = "SELECT * FROM users WHERE username = :username";
$data = $conn->prepare($query);
$data->bindValue(':username', $id, PDO::PARAM_STR);
$data->execute();
$data->setFetchMode(PDO::FETCH_BOTH);

Vulnerability in your code is because of you put into PDO::prepare() this string:

SELECT * FROM users WHERE username ='' or 1=1 union select 1,2,3

and PDO correctly prepare it. Your $query already include this injection code, before PDO works.

edited Dec 16 '13 at 19:41

answered Dec 16 '13 at 19:31

BaBL86

2,602
1
14
13

This doesn't really explain the vulnerability. – Dec 16 '13 at 19:32
Vulnerability in his code, because of he puts into PDO::prepare this string: "SELECT * FROM users WHERE username ='' or 1=1 union select 1,2,3" and PDO correctly prepare it. His $query already include this bad code, before PDO works. – BaBL86 Dec 16 '13 at 19:36
2

I understand that, but the OP clearly doesn't. For this to make a good answer you need to include that explanation in the body of the answer itself. – Dec 16 '13 at 19:39

score -1 · Answer 3 · answered Dec 16 '13 at 19:40

-1

I managed to solve the issue, I misread the PDO man page and didn't fully understand the execute properly and thought it was supposed to have my column name instead of the form value, which is what it actually needs. So here are the lines I have changed:

$query = "SELECT * FROM users WHERE username ='$id'";

To this

$query = "SELECT * FROM users WHERE username =?";

And secondly

$data->execute(array(':username'));

To this

$data->execute(array($_GET['id']));

This has solved the error if, tell me if the site is still insecure

answered Dec 16 '13 at 19:40

user2157179

238
2
4
19

This security hole is plugged. Note that it is still possible to place text into your database that represents a threat if subsequent code treats it as safe. This is called a second-order injection. Make sure that all use of user data is similarly protected, wherever it comes from. – Dec 16 '13 at 19:43

PDO code still vulnerable to SQL Injection

3 Answers3