5
\$\begingroup\$

I have used this guide to implement prepared statements in order to avoid SQL Injection.

I made some tests and no error was shown. However, I would like to ask you if you see any flaw in the code, so that I can improve it.

This is the code for a simple login:

<form action="conexion_PDO.php" method="post">
<label>User:</label>
<input type="text" name="user">
<label>Pass:</label>
<input type="password" name="pass">
<input type="submit" value="Ingresar">
</form>

This is Conexion_PDO.php:

<?php

try {
$gbd = new PDO('mysql:host=localhost;dbname=database', 'root', '');     
$statement = $gbd->prepare("SELECT * FROM user where email = ? AND
password = ?");
if ($statement->execute(array($_GET['user'],md5($_POST['pass'])))) {
while ($rs = $statement->fetch()) {
print_r($rs);
}   

$gbd = null;
} catch (PDOException $e) {
print "¡Error!: " . $e->getMessage() . "<br/>";
die();
}

?>
\$\endgroup\$
  • \$\begingroup\$ Hope you don't mind, but I've gone ahead and thoroughly reviewed your code. I know, criticism may come over as harsh and arrogant, but trust me, I'm only trying to help. And, IMHO, that entails being bluntly honest \$\endgroup\$ – Elias Van Ootegem Jan 4 '14 at 20:08
3
\$\begingroup\$

Your code does not have an SQL injection vulnerability because you are using placeholders instead of interpolating the variables. The way you interface with the database is laudable.

Other aspects of your code can be improved:

  • The variables $gdb, $statement, $rs, and $e are globals, which is not acceptable within a larger system. Enclose your code with a function to make them locals.

    Otherwise, you could accidentally overwrite variables from other parts in your code (e.g. when you include this snippet). Consider what happens if $gdb is already assigned: You try to clean up your usage by assigning null when you are finished, but the other code now suddenly has its contents for $gdb deleted.

  • You do not seem to be using any intendation. Indent your code properly so that it can be understood easily.

  • You use the literal ¡ inside a string, and print it directly to the output. As this is a non-ASCII character, this can get problematic depending on the encoding of your source code and the character encoding in the HTTP header and the HTML document.

  • HTML markup is a kind of code as well – it too wants to be indented :)

But now something important:

Do not use md5 to hash passwords!

  1. MD5 is extremely weak, and an increasing number of attacks are being discovered. Do not use it for anything in new systems.
  2. It is meant for checksums (e.g. when comparing files), not for hashing passwords. Such a checksum is designed that a subtle change in the input changes the output of the hash function drastically. It is not designed to irreversibly scramble a secret.
  3. It is far to easy to calculate the original password from a given hash, e.g. using rainbow tables. You do not even use a salt to defend against this (but even with a salt, an MD5-hashed password can be cracked within reasonable time).

You should be using a key-stretching algorithm like bcrypt. I already wrote a rant about secure password hashing in another answer which I would ask you to consult for a better hashing solution.

\$\endgroup\$
  • \$\begingroup\$ wow, such a nice review, gracias \$\endgroup\$ – user2095819 Dec 28 '13 at 21:49
  • \$\begingroup\$ Good critique of md5 \$\endgroup\$ – kyle k Jan 4 '14 at 22:42
3
\$\begingroup\$

Flaws in terms of vulnerability to injecction: no. Prepared statements are safe when it comes to injection attacks. Querying the user table however... don't know, user is an iffy name, as it is often a reserved keyword or a table that already exists. Sure, not on your database, but I'd avoid the DB name, if I were you, or at least add the escaping backticks around it, as I will be doing throughout my answer...

Now, in adition to amon's answer (I side with almost completely), I'd like to focus a bit more, if I may, on the code itself. As usual, I'll go through the PHP code practically line-by-line

try {
$gbd = new PDO('mysql:host=localhost;dbname=database', 'root', '');
  • Right, as amon said, and I will now "order" indent your code. And don't do so randomly. Try to stick to the most commonly adopted standards, the PHP-FIG standards are as of yet unofficial, but all major players and Framework devs do adopt this coding style. The sooner we all do, the better.
  • Variable names can be short, that's fine but $gdb might not be as obvious a name to your co-workers as it is to you. $pdo is as short, and is pretty descriptive, just use logical, and easy to understand var names throughout your code.
  • Next, you're wrapping your code in a try-catch block, and that's fine. But do you know when PDO will throw exceptions? My guess is you're expecting it to throw whenever it errors. Well, it doesn't. Only PDO::__construct throws an exception when it failed to establish a connection (see man here). The other methods throw exceptions if the PDO::ATTR_ERRMODE was set to PDO::ERRMODE_EXCEPTION. This is done by either passing an array to the constructor, or calling PDO::setAttribute.
  • Finally, you're using localhost in your DSN string. Now this will work, but can slow you down, in some cases significantly. This is because PDO has to use the DNS to translate localhost into 127.0.0.1, so using the IP directly skips that step, doesn't harm readability one bit, and ensures your code will perform well.

So, change this code to:

try {
    $pdo = new PDO(
        'mysql:host=127.0.0.1;dbname=database',
        'root',
        '',
        array(
            PDO::ATTR_ERRMODE  => PDO::ERRMODE_EXCEPTION
        )
    );

Next bit of code we'll look at

$statement = $gbd->prepare("SELECT * FROM user where email = ? AND password = ?");
if ($statement->execute(array($_GET['user'],md5($_POST['pass'])))) {
while ($rs = $statement->fetch()) {
print_r($rs);
}
$gdb = null;
} catch (PDOException $e) {//we're not going into the catch here, but I need it

If we simply indent the code, you'll soon notice a problem:

if ($statement->execute(array($_GET['user'],md5($_POST['pass'])))) {
    while ($rs = $statement->fetch()) {
        print_r($rs);
    }
//WE ARE MISSING A CLOSING } HERE!
$gdb = null;
} catch (PDOException $e) {}

Ah, what a little indentation can do/avoid. Sorry, couldn't help myself here... Anyway, let's continue:

  • Now that we've set the PDO::ATTR_ERRMODE so that an exception is thrown whenever a problem occurs, we don't need to check if the $statement->execute() call was successful, so the if branch is now redundant. Which is a good thing, too, because it was hellish to read, calling a method, and creating an array and hashing the password in one statement...
  • When preparing statements, if you're not going to use variables in the string, I prefer using single quotes, but that's personal. I would, however, advise you to indent the query string in such a way that it is easy to read, comment and alter.
  • You're dealing with a form, but getting the username from $_GET and the password from $_POST? that's not right, I suppose you made a mistake and meant for both to be $_POST. Either way, on $_GET: avoid whenever possible, it's way to vulnerable. POST isn't much safer, but every bit helps
  • From the query I deduce that $_POST['user'] is supposed to be an email address. Why aren't you checking if it is a valid email before bothering the DB? a simple filter_var($_POST['user'], FILTER_VALIDATE_EMAIL) would avoid a pointless query.
  • The while-loop is fine, but I'd specify what the fetch-mode of PDO should be. If you want an assoc array, use PDO::FETCH_ASSOC, for objects use PDO::FETCH_OBJ... check the man pages, there are plenty to choose from. You can also choose a default fetch mode. Apart from that, all I'd say is that, if you do only have 1 statement in the loop's body (and this may be my bad C-coding habits that show here), I'd just inline it, especially if the statements are that short.
  • Setting the $gdb variable to null in a try block is something I'd avoid, if an exception was thrown, you can't really be sure that the $gdb = null; statement was reached and executed, so don't write it there, there are better places for it

Again, this is the code we now end up with:

//either set default here, in this case, I'll fetch objects by default
$pdo->setAttribute(
    PDO::ATTR_DEFAULT_FETCH_MODE,
    PDO::FETCH_OBJ
);
if (!filter_var($_POST['user'], FILTER_VALIDATE_EMAIL)) {
    echo 'Invalid user: ', $_POST['user'], '<br/>';
    exit();//for now...
}
$statement = $pdo->prepare(
    'SELECT * FROM `user`
     WHERE email = ?
       AND password = ?'
);
$statement->execute(
    array(
        $_POST['user'],
        md5($_POST['pass']  //change this to bcrypt or something, 15 cost should do
    )
);
//or specify fetch mode in fetch call, here, I'm fetching assoc arrays
while ($row = $statement->fetch(PDO::FETCH_ASSOC)) {
    print_r($row);
}
//bad practice
while($row = $statement->fetch()) print_r($row);

Now, more on the fetch modes can be found on the man page here
And now, on to the catch block:

} catch (PDOException $e) {
print "¡Error!: " . $e->getMessage() . "<br/>";
die();
}

There's only 1 thing I have to say about this try-catch block in your example code: GET RID OF IT. If all you're going to do in case of an exception is call die(), why bother catching the exception? why not let the app crash, and see the "fatal error uncaught exception..." message and the stack-trace that goes with it? If it's a production environment, you'll have display erros set to 0 anyway, so when developing, don't hide information, and get the full exception, not just the message.

Other niggles I have are:

  • This is where you should ensure the DB connection is closed, so this is where $gdb = null; belongs
  • print can only handle 1 string, so you have to concatenate it. echo is another language construct, to which you can pass any number of strings, comma-separated, so they needn't be concatenated. I've been rather verbose on the matter here

So, if you are going to keep the catch block, this is what it could end up like

} catch (PDOException $e) {
    echo 'Error: ', $e->getMessage(), '<br/>';//comma-separated
    $pdo = null;
    //if the program should stop in case of an exception, then one of these works:
    exit(1);
    throw $e;//silly re-throw
    die();
    //but NOT catching the exception works best
}

Then finally, you're closing the script using the close tag (?>). Don't, as it can result in unintended white-space in the output.

So, what would this code look like if I were to write it? Probably something like this

<?php
if (!filter_var($_POST['user'], FILTER_VALIDATE_EMAIL)) {
    header('Location: error-url');
    exit();
}
$pdo = new PDO(
    'mysql:host=127.0.0.1;dbname=database',
    'root',
    '',
    array(
        PDO::ATTR_ERRMODE             => PDO::ERRMODE_EXCEPTION,
        PDO::ATTR_DEFAULT_FETCH_MODE  => PDO::FETCH_OBJ
    )
);
//I usually use named placeholders, the result is the same, though
$stmt = $pdo->prepare(
    'SELECT * FROM `user`
     WHERE email = :email
       AND password = :pass'
);
$bind = array(
    ':email' => $_POST['user'],
    ':pass'  => password_hash(
        $_POST['pass'].MY_SALT_CONSTANT,
        PASSWORD_BCRYPT,
        array('cost' => 15)
     )
);
$stmt->execute($bind);
while($row = $stmt->fetch()) {
    print_r($row);
}
$pdo = null;

With the exception that I would use an external config file (yaml, xml, ini, whichever takes your facy) that'd hold the DB connection params, options I'm passing to password_hash and what have you...

Anyway, the resulting code isn't that much longer, it certainly is easier to read/maintain (very important), and just looks cleaner, more airy. There is much room for improvement, still, and yes, I would create a function, or class (so I don't have to use either an ugly global, but can have the constructor connect to the DB, or lazy-load the connection...) to which I can pass the form data, and get a boolean in response from.

\$\endgroup\$
  • \$\begingroup\$ That's one hell of a review, great job. \$\endgroup\$ – Dan Jan 4 '14 at 20:14
  • 2
    \$\begingroup\$ @DanAbramov: Thanks, my girlfriend is a little ticked off, as it took me quite some time to type it up. I thought I'd be done in a jiffy, but I really found myself talking about every line of code... Never let it be said I'm not thorough in my reviews =] \$\endgroup\$ – Elias Van Ootegem Jan 4 '14 at 20:17
  • \$\begingroup\$ Thank you Elias, I really appreciate the time you invested. \$\endgroup\$ – user2095819 Jan 6 '14 at 11:55
  • \$\begingroup\$ @user2095819: You're welcome, but do note: thank-you-comments are discouraged (check help center, it says so explicitly). Instead you are asked to up-vote the helpful answer(s), and accept the one you found most useful \$\endgroup\$ – Elias Van Ootegem Jan 6 '14 at 12:54

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.