Search code examples
phpsql-injection

Is this code anti mysql injection?


I got this example from one website. And I am about to upgrade the security of my code. So May I have your opinion if this kinda code strong enough to prevent the injection?

$sql = sprintf(
    "
    INSERT INTO `members`
    (`id`, `username`, `password`, `first_name`, `last_name`)
    VALUES
    ('', '%s', '%s', '%s', '%s')
    ",
    mysql_real_escape_string($con,$_POST['username']),   // %s #1
    mysql_real_escape_string($con,$_POST['password']),   // %s #2
    mysql_real_escape_string($con,$_POST['first_name']), // %s #3
    mysql_real_escape_string($con,$_POST['last_name'])   // %s #4
);

$con is from the mysql connection


Solution

  • No, that code is not strong enough to prevent injection. There are a number of things wrong with it. Firstly, mysql_* functions are deprecated since PHP 5.5.0. Always use the mysqli_ extension's equivalent functions. I find it peculiar you knew to escape the strings, but you didn't know to hash() your password before inserting it into the table. Never store users' passwords in plain text in your database. You want to use Prepared Statements for the most effective way of preventing SQL injection. Try something like this.

    $q = "INSERT INTO `members` (`id`, `username`, `password`, `first_name`, `last_name`) VALUES ('', ?, ?, ?, ?)";
    $stmt = mysqli_prepare($con, $q);
    mysqli_stmt_bind_param($stmt, "ssss", $p_user, $p_pass, $p_first, $p_last);
    
    $p_user = $_POST['username'];
    $p_pass = hash("sha256", $_POST['password']);
    $p_first = $_POST['first_name'];
    $p_last = $_POST['last_name'];
    
    mysqli_stmt_execute($stmt);
    mysqli_stmt_close($stmt);
    

    See, in the initial query string, you want to use VALUES ('', ?, ?, ?, ?). The question marks are placeholders for the parameters, which you bind using variables.

    Have a look at the PHP.net manual on mysqli::prepare to get a feel for how prepared statements work. These things are very important for securing your website, and protecting users' information. It is not necessary to use mysqli_real_escape_string() when using prepared statements, as the parameters are already escaped.

    And a side note: when you hash your passwords, don't fall back on the commonly-used md5(), as this is very insecure and easily-crackable by today's standards. Use a more secure algorithm, like sha256.