CODEWITHSUNDEEP
phpsql-injection
Wilf
Wilf

Reputation: 2315

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

Upvotes: 1

Views: 123

Answers (2)

Bill Karwin
Bill Karwin

Reputation: 562911

You should read SQL injection that gets around mysql_real_escape_string() for some cases where mysql_real_escape_string() is known to fail.

In the specific code you show, assuming you can control the character set as well, you should be safe. But overall, mysql_real_escape_string() is not a reliable or safe solution for SQL injection protection.

mysql_real_escape_string() is also tedious to type. So some developers skip safe coding when they're trying to finish code quickly. "I'm just getting this code working first, I'll add security later." BAD IDEA.

Whereas query parameters do not fail, and they're actually quite easy to use, so once one gets into the habit, it's more likely that one will use them to write secure code from the start.

@TannerBabcock provides a mysqli example, but I prefer PDO, because it's even easier:

$sql = "
    INSERT INTO `members`
    (`username`, `password`, `first_name`, `last_name`)
    VALUES (?, ?, ?, ?)
    ",
$params = [
    $_POST['username'],
    password_hash($_POST['password'], PASSWORD_DEFAULT),
    $_POST['first_name'],
    $_POST['last_name']
];
$stmt = $pdo->prepare($sql);
$stmt->execute($params);

In PDO you can simply pass an array of parameters to execute(), which I think is easier than the mysqli method of passing a variable argument list to mysqli_stmt_bind_param().

PDO also supports named parameters, whereas mysqli does not.

Upvotes: 2

Tanner Babcock
Tanner Babcock

Reputation: 3360

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.

Upvotes: 2

Related Questions