-1

Possible Duplicate:
Best way to prevent SQL Injection in PHP

I'm trying to insert user's inputs into a database.
Is this pdo and hash secure enough: 

$dbh = new PDO("mysql:host=hostName; dbname=dbName", "user", "pass");
if(isset($_POST['Submit']))
{
$user = $_POST['user'];
$pass = $_POST['pass'];
$salt = substr(str_replace('+', '.', base64_encode(sha1(microtime(true), true))), 0, 22);
$hash = crypt($pass, '$2a$12$' . $salt);
$mail = $_POST['mail'];
$confirm_key=md5(uniqid(rand()));

$sth = $dbh->prepare("INSERT INTO members(user, pass, mail, confirm_key)
VALUES ('$user', '$hash', '$mail', '$confirm_key')");
$sth->execute();
Community
  • 1
  • 1
Alegro
  • 7,534
  • 17
  • 53
  • 74
  • No. Don't build SQL by mashing strings together (especially when they include user data, double especially when they include unmodified user data such as user and mail). – Quentin Aug 05 '12 at 12:40
  • 1
    You've *added* problems since the last time you asked [the same question](http://stackoverflow.com/questions/11805986/pdo-to-prevent-sql-injection) about an earlier iteration of this code. – Quentin Aug 05 '12 at 12:42
  • 2
    If you want people to review your code, then please ask on [codereview](http://codereview.stackexchange.com/), not here. – Quentin Aug 05 '12 at 12:42

2 Answers2

4

You seem a bit confused as to what SQL injection is and why we hash passwords.

Password hashing (and salting) is done so that the database contains enough information to validate the password, but not enough to retrieve it. If an attacker gains (offline) access to the database, she still cannot read out any passwords without cracking the secure hash. For this purpose, your code looks reasonable.

SQL injection is an entirely different beast. The exploit works by sending values to your PHP code which, when inserted into a dynamic SQL query, will break it in interesting ways, so that the query now does something different that it was intended to do. For example, the user can provide a value for $user which keeps the INSERT query valid, but breaks out of the quoted string and adds a second query, which may do something harmful such as overwriting every other user's password hash with a known value, after which the attacker can log into any account they want (including administrative ones).

The best way to guard against SQL injection is through parametrized queries, which PDO provides out-of-the-box; you're not using them though, so your code is vulnerable. Read the documentation on PDO's parametrized queries; if you are unsure as to how they work, feel free to come back and ask again.

tdammers
  • 20,353
  • 1
  • 39
  • 56
2

You are abusing the prepared statement, change your code to:

$stmt = $dbh->prepare("INSERT INTO members(user, pass, mail, confirm_key) VALUES (:user, :hash,:mail,:confirm)");
$stmt->bindParam(':user', $user);
$stmt->bindParam(':hash', $hash);
...

See here for more details.

Clive
  • 36,918
  • 8
  • 87
  • 113
Tomer
  • 17,787
  • 15
  • 78
  • 137