Can this be SQL injected?

Question

I'm wondering if my code is actually protected from SQL injection. My sites have been injected before and I never really understood how to prevent it. Here is my code for inserting a comment:

if ($_POST['comment']) {
    $comment = strip_tags(nl2br(mysql_real_escape_string($_POST['comment'])));
    $conn = new PDO("mysql:host=$servername;dbname=$dbname", $username, $password);
    // set the PDO error mode to exception
    $conn->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
    $sql = "INSERT INTO posts (comment, authorid)
    VALUES ('$comment', '$uid')";
    // use exec() because no results are returned
    $conn->exec($sql);
    echo '<div style="width: 98%; max-width: 98%; border: 1px solid white; background-color: green; color: white; vertical-align: text-top; text-align: center;">Your comment was added to the wall!</div><br>';
}

yes, it is possible. See this: http://stackoverflow.com/questions/5741187/sql-injection-that-gets-around-mysql-real-escape-string — Harsh, Mar 02 '16 at 23:25
this is the kitchen sink approach, your jurt adding every function you can find in the manual with out understanding any of them — , Mar 02 '16 at 23:26
You'd be better understanding the differences between `strip_tags()`, `nl2br()` and `mysql_real_escape_string()` and what they actually do, and when to use them...... and then switching to using prepared statements/bind variables and only using those functions when appropriate — Mark Baker, Mar 02 '16 at 23:26

score 2 · Accepted Answer · answered Mar 02 '16 at 23:28

2

Yes, it can probably be injected: you don't appear to be protecting your $uid variable. Also, stacking nl2br and strip_tags after escaping is a bad idea - you want to leave mysql_real_escape_string as the last operation to avoid any filter interaction effects.

More generally, you should use prepared statements, not string interpolation, to build SQL queries. It's simpler, more efficient, more secure and requires less code. You can create a prepared statement using $conn->prepare and execute it with arbitrary arguments:

$stmt = $conn->prepare("INSERT INTO posts (comment, authorid) VALUES (?, ?)");
$stmt->execute(array($comment, $uid));

No escaping required.

answered Mar 02 '16 at 23:28

nneonneo

171,345
36
312
383

Prepared statements have the added benefit that a prepared statement is *not allowed* to contain more than one SQL statement, which is necessary for most SQL injection attacks to succeed. – Darwin von Corax Mar 02 '16 at 23:31
1

@DarwinvonCorax: Actually, SQL injection is perfectly feasible with only a single SQL statement in many contexts using *nested queries* - sometimes all you need is to read sensitive data out of the database. – nneonneo Mar 02 '16 at 23:33
Ok. So what about updating? How would I go about that? – Justin Pruett Mar 02 '16 at 23:35
You can use `prepare` with any SQL query, including `SELECT`, `UPDATE`, `INSERT`, `DELETE`, etc. – nneonneo Mar 02 '16 at 23:37

Can this be SQL injected?

1 Answers1