5
\$\begingroup\$

At the moment is my code secure for SQL injections and so forth? I still need to hash passwords and make sure fields are valid and so forth.

<?php

try{
    $handler = new PDO('mysql:host=localhost;dbname=s','root', '*');
    $handler->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
}

catch(PDOException $e){
    echo $e->getMessage();
    die();
}

$name = $_POST['name']; 
$username = $_POST['username']; 
$email = $_POST['email'];   
$password = $_POST['password']; 

$sql = "INSERT INTO userinfo (name ,username, email, password) VALUES (:name,:username,:email,:password)";
$query = $handler->prepare($sql);

$query->execute(array(      
    ':name' => $name,
    ':username' => $username,
    ':email' => $email,
    ':password' => $password
));

?>
\$\endgroup\$
  • 2
    \$\begingroup\$ SQL attack has been over asked both here and on stackoverflow: stackoverflow.com/questions/60174/…, and stackoverflow.com/questions/60174/… both articles are applicable to what you are asking. \$\endgroup\$ – azngunit81 Apr 25 '14 at 5:09
  • 1
    \$\begingroup\$ possible duplicate of Is this code safe from SQL injection? \$\endgroup\$ – azngunit81 Apr 25 '14 at 5:11
  • 2
    \$\begingroup\$ @azngunit81 Same topic, different code. I don't see how this is a duplicate. \$\endgroup\$ – 200_success Apr 25 '14 at 6:14
  • \$\begingroup\$ @200_success same PDO setup with similar code. Its an insert with PDO setup so the same response will be yield: the post needs to be sanitize, prepare statement is already use so that is fine but everything that is said in the articles mentioned is a repetition of similar response that would be drafted up - hence duplication of question. \$\endgroup\$ – azngunit81 Apr 25 '14 at 6:56
  • 3
    \$\begingroup\$ @azngunit81 Remember that all aspects of the code are reviewable, not just the SQL injection issue that the OP raised. Questions submitted by two different users are almost never duplicates of each other. \$\endgroup\$ – 200_success Apr 25 '14 at 7:03
5
\$\begingroup\$

It is safe.

You can improve your code like this:

  • no need to use closing ?> in case that you are not outputting any HTML / or something else after your PHP code
  • no need to use "" to wrap strings in case that you don't have any variables inside a string, you can use '' instead, PHP interpreter does not need to check in that case whether there are any variables in the string or not
  • you can replace echo $e->getMessage; die() by simplier exit($e->getMessage());
  • I added salt generation to your code $salt = md5(uniqid(null, true));
  • I added password hashing to your code by $password = hash('sha256', $password . $salt);

The whole code hereunder:

<?php

try {
    $handler = new PDO('mysql:host=localhost;dbname=s','root', '*');
    $handler->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
} catch (PDOException $e){
    exit($e->getMessage());
}

$name = $_POST['name'];
$username = $_POST['username'];
$email = $_POST['email'];
$password = $_POST['password'];

$salt = md5(uniqid(null, true));
$password = hash('sha256', $password . $salt);

$sql = '
    INSERT INTO userinfo 
        (name ,username, email, password, salt) 
    VALUES 
        (:name,:username,:email,:password, :salt)
';

$query = $handler->prepare($sql);

$query->execute(array(
    ':name' => $name,
    ':username' => $username,
    ':email' => $email,
    ':password' => $password,
    ':salt' => $salt
));
\$\endgroup\$
  • 1
    \$\begingroup\$ Excellent observation, that salting and hashing are essential, you have made. \$\endgroup\$ – 200_success Apr 25 '14 at 16:31
  • \$\begingroup\$ Excellent answer. But isn't password_hash safer? \$\endgroup\$ – user2981256 Apr 25 '14 at 19:21
  • \$\begingroup\$ @user2981256 You can use password_hash as well, safety depends on used hashing algorithm. You can choose these values in both methods so the safety is about the same. \$\endgroup\$ – user40964 Apr 25 '14 at 19:34
  • \$\begingroup\$ Don't use sha256 for passwords. It's much too fast and therefore easy to attack. Use bcrypt, scrypt or PBKDF2. \$\endgroup\$ – bdsl Feb 22 '15 at 16:17
0
\$\begingroup\$

Yoda has some good points, and I agree with it being safe. I noticed you mentioned validating, but I figured I'd add my 2 cents for future readers that may come across this question. A few things jump out at me from the following:

$name = $_POST['name'];
$username = $_POST['username'];
$email = $_POST['email'];
$password = $_POST['password'];

You should check that all fields have been posted, and stop processing if there is information missing. An example of this would be:

$name = isset($_POST['name']) ? $_POST['name'] : null;
if($name==null) { 
    exit(); 
}

This information is being entered into the database without any checks, which can lead to bad data being stored. I'm assuming that there may be checks client side (browser), but those checks are by no means fool-proof. Javascript could be disabled, rending those checks useless. There is required for HTML5, and some browsers have implemented that functionality. However, not all have, and many users will inevitably be using the older versions that haven't. What if the name and username are blank, or the name is 31&^5. Checking the length and content should be done both on the client and the server.

+1 for prepared statements!

\$\endgroup\$

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.