current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 5 down vote favorite

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
));

?>
share|improve this question
2  
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. –  azngunit81 Apr 25 at 5:09
1  
possible duplicate of Is this code safe from SQL injection? –  azngunit81 Apr 25 at 5:11
2  
@azngunit81 Same topic, different code. I don't see how this is a duplicate. –  200_success♦ Apr 25 at 6:14
    
@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. –  azngunit81 Apr 25 at 6:56
3  
@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. –  200_success♦ Apr 25 at 7:03
add comment

2 Answers

up vote 5 down vote accepted

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
));
share|improve this answer
1  
Excellent observation, that salting and hashing are essential, you have made. –  200_success♦ Apr 25 at 16:31
    
Excellent answer. But isn't password_hash safer? –  user2981256 Apr 25 at 19:21
    
@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. –  Yoda Apr 25 at 19:34
add comment
up vote 0 down vote

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!

share|improve this answer
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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