4
\$\begingroup\$

I'm just wondering if this code is good against SQL injection attacks. I don't know how to check if is good. Also I would like to know if is good how I'm working or this is just bad practice?

<?php 

if (isset($_POST['register'])) {

$username = $_POST["username"];
$email = $_POST["email"];
$password = $_POST["password"];
$salt = "Not important";
$password_hash = crypt($password, "$2y$". $salt);

if (strlen($username) > 20) {
    echo "Username is too big";
}elseif (strlen($password) > 32) {
    echo "Password is too big";
}elseif (strlen($email) > 100) {
    echo "E-Mail is too big";
}elseif(strlen($username) == 0 || strlen($password) == 0 || strlen($email) == 0){
    echo "Fill every field";
}
else{
    $sth_username = $dbh->prepare("SELECT username FROM user WHERE username = :username");
    $sth_username->bindParam(":username", $username);
    $sth_email = $dbh->prepare("SELECT email FROM user WHERE email = :email");
    $sth_email->bindParam(":email", $email);
    $sth_username->execute();
    $sth_email->execute();
    if ($result = $sth_username->fetch(PDO::FETCH_OBJ)) {
        print $result->username . "The username is already is use";
    }elseif ($result = $sth_email->fetch(PDO::FETCH_OBJ)){
        print $result->email . "That e-mail is already in use";
    }else{
        $sth = $dbh->prepare("INSERT INTO user (username, password, email, salt) VALUES (:username, :password, :email, :salt)");
        $sth->bindParam(":username", $username);
        $sth->bindParam(":password", $password_hash);
        $sth->bindParam(":email", $email);
        $sth->bindParam(":salt", $salt);
        $sth->execute();
        $sth = $dbh->prepare("INSERT INTO stats (strength,agility,intelligence) VALUES (10,10,10)");
        $sth->execute();

        echo "You have been registered";
    }
}
?>
\$\endgroup\$

migrated from stackoverflow.com Jul 10 '13 at 21:02

This question came from our site for professional and enthusiast programmers.

  • 1
    \$\begingroup\$ Looks good to me \$\endgroup\$ – Eric Petroelje Jul 10 '13 at 21:02
  • \$\begingroup\$ Since you're using a Blowfish hash, make sure the format is correct. \$\endgroup\$ – Blender Jul 10 '13 at 21:05
2
\$\begingroup\$

Looks O.K. to me as well, as far as the SQL injection goes.

However, I'd suggest checking the return value of $sth->execute();, so you capture possible errors (i.e., DB being down).

You might also consider doing a case insensitive search for the existing usernames and e-mails, since "person@gmail.com" is the same address as "Person@GMail.com" and users "Person" and "person" are hard to distinguish.

I think empty($string) is more advisable than strlen($string) == 0.

Last, but not least, adding a "retype your password" field would be a good security against user mistyping what he wants for a password and then ending with a useless account (or having to use "I forgot my password" before the very first login).

\$\endgroup\$
  • \$\begingroup\$ The people at UX.SE disagree with you on offering "retype your password" (ux.stackexchange.com/q/20953/16833) \$\endgroup\$ – Brian Jul 12 '13 at 19:54
  • \$\begingroup\$ @Brian I don't agree with you. Yes, the "retyping the password is bad, without a real explanation" answer is the accepted one, and it got the most votes. However, the other answers and the comments throughout that thread are far from consensus, so I don't see this to be "the people at UX.SE" disagreeing (nor agreeing) with me, or among themselves. \$\endgroup\$ – Vedran Šego Jul 12 '13 at 22:39

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.