Question

This is my first time creating a PHP form that will run a MySQL query using INSERT INTO to store data in a production DB. Will this pass for "secure" or is it over-kill?

$orderText = $mysqli->real_escape_string(stripslashes(htmlentities((!isset($_POST["order_text"])?"undefined":$_POST["order_text"]))));
$stmt = $mysqli->prepare("INSERT INTO testtable (order_text) VALUES (?)");
$stmt->bind_param('s',$orderText);
$stmt->execute();

I'm not sure how the lack of a SELECT * affects the amount of risk I'm opening myself up to, but it seems like a script that only uses INSERT is safer. True?

Was it helpful?

Solution

There is a great amount of false assumptions in your question.

  1. It is certainly an overkill.
    Let's examine your extremely-hard-to-read zillion-nested-operator statement:

    • storing word 'undefined' makes no sense. A database has a special mark for the undefined fields - a NULL value. Or simply an empty string would be enough.
    • unconditional stripslashes adds nothing to security but may spoil the data.
    • htmlentities has nothing to do with SQL security, may help with site security in other aspects and may spoil the data as well.
    • escaping adds nothing to security and will spoil the data.
  2. You are taking the problem from the wrong end.
    Your primary goal is to format your query properly. Not to defend from imaginary "attackers" but to privent malfunction with most honest data. While properly formatted query will be invulnerable to various attacks just as a side effect.
    Say, real_escape_string has nothing to do with security. It is used merely to format strings. There are no strings (data enclosed in quotes) in your query - thus this function is utterly useless (and even harmful).

  3. In fact, an injection via INSERT is no less disastrous than via SELECT.

Finally, the right code would be

$stmt = $mysqli->prepare("INSERT INTO testtable (order_text) VALUES (?)"); 
$stmt->bind_param('s',$_POST["order_text"]); 
$stmt->execute(); 

and when printing the order text back to the site, use htmlspecialchars()

that's all.

OTHER TIPS

Variable binding, which you do on line 3, is sufficient to prevent injection attacks in general. Binding is a good idea and, in my opinion, should always be done. It has not only security advantages, but can yield a performance boost as well.

I would argue that performing the extra parsing at line 1 is actually a disadvantage: It increases complexity, and some attacks take advantage of known data transformations, though using binding mitigates those as well.

I will recommend to treat everything that comes from a client, your visitors, as threat. Don't relax and only focus on some sql queries. Practicing a good habit has no limitation.

I agree with Charles, by binding the param you are already properly escaping the variable removing the chance of a SQL injection attack, and the complexity of line 1 is overkill. This will be come evident when you make the jump to PDO, because there is no specific $dbo->escape() call, as the escaping work is already completed with the prepare() / bind() calls.

Licensed under: CC-BY-SA with attribution
Not affiliated with StackOverflow
scroll top