0

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?

nipponese
  • 2,813
  • 6
  • 35
  • 51
  • If i could inject a statement that drops the database, i just need to run it once and you would lose all your data. :) So sql insertion prevention is never an overkill. – iWantSimpleLife Feb 07 '12 at 01:15
  • Wait; I didn't notice that you're escaping it too. That is overkill, and wrong. – SLaks Feb 07 '12 at 03:22

4 Answers4

2

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.

Charles Burns
  • 10,310
  • 7
  • 64
  • 81
  • A simple example of an attack I mentioned, though this specific one doesn't apply in this case is: A cracker learns through experimentation that a website first strips SQL keywords (insert, etc.) from the "name" field, then strips quotation marks. An SQL injection might include a "name" composed of something like "IN'SERT into...". If the parsing is done in the order above, the removal of the tick mark will turn "IN'SERT" into "INSERT", which may then be a valid SQL injection attack. – Charles Burns Feb 07 '12 at 01:05
  • @Col.Shrapnel: It isn't superstition; it's a well-documented hacking technique called "second-order SQL injection." You can read about it in numerous books. For example, look at "Bypassing Filters" on page 267 of the Web Application Hacker's Handbook here: http://www.amazon.com/Web-Application-Hackers-Handbook-Discovering/dp/0470170778 I'd appreciate that whomever down-voted me do a little homework next time. – Charles Burns Feb 07 '12 at 06:24
  • second order has nothing to do here. binding does not "mitigate" an injection. it makes it impossible. – Your Common Sense Feb 07 '12 at 06:58
  • The second paragraph is about second order injection. True, in the simple case of the OP, second-order injection would probably not be a problem, but in many real-world situations, binding can only be used for parts of the query. For example, binding cannot replace column or table names, which occasionally need to be generated based on user-input (though I think it's best to design around this if possible). Binding can also not be used with IN(var1, var2...varN) statements without dynamically generating the query. – Charles Burns Feb 07 '12 at 16:04
  • @Col.Shrapnel: It is true though that in the common cases, queries that use binding and have no dynamically generated parts, I know of no way whatsoever to SQL inject through the parameterized query. I hesitate to use words like "impossible", though, because information security is still a young field and the "impossible" becomes the norm frequently. Remember Oracle's embarrassment around their ad campaign, "Can't break it. Can't break in." Turns out you can... – Charles Burns Feb 07 '12 at 16:07
  • the second paragraph has absolutely nothing to do with second order injections, dynamic identifiers and IN statement. It seems you are just throthing in every term you know, but withiout a real connection to this very case. – Your Common Sense Feb 07 '12 at 18:35
  • @Col.Shrapnel: "some attacks take advantage of known data transformations" has a lot to do with 2O injection. Please see my URI. Is the OP's exact SQL vulnerable to this specific attack? Probably not. That isn't the point. The point is mutating the data can add unneeded complexity, and examples exist where doing so actually creates security holes. This is likely not the most complex SQL he'll ever write. The IN() note was to your point that binding avoids injection. Yes it does. No, it's not always an option. Based on your impressive rep, I assume this is a misunderstanding and not trolling. – Charles Burns Feb 07 '12 at 20:35
  • Well, may be it's my English. It seems I took your second paragraph wrong, as an approval of extra parsing. If so, accept my apologies. However, I still see no point in blaming line 1 for the extra vulnerability (yet no reason to approve it either). It doesn't increase the risk of injection. It's merely a *bungling* and nothing else. To be protected, one have to follow a simple set of protection rules. Once it's applied - no such a mess may interfere with query in either way. I can prove every my word with example. I hope you can do so. – Your Common Sense Feb 07 '12 at 20:59
2

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.

Your Common Sense
  • 156,878
  • 40
  • 214
  • 345
  • Despite disagreements, this is a better post than mine. However, I disagree with, "an injection via INSERT is way more disastrous than via SELECT." An injection to a SELECT can turn into an INSERT. – Charles Burns Feb 07 '12 at 20:48
  • Not with mysql/prepare. But anyway, I won't argue such an empty point. – Your Common Sense Feb 08 '12 at 06:37
  • I think the source of confusion was that I was ansering in terms of general strategy and you were answering in terms of the specific tools and technologies actually used by the original poster. Much of my content doesn't directly apply to his case and much of yours didn't to the general case. No worries. :) – Charles Burns Feb 10 '12 at 17:44
1

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.

the_wizard
  • 477
  • 5
  • 9
1

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.

Mike Purcell
  • 19,847
  • 10
  • 52
  • 89
  • what does it mean, "there is no specific $dbo->escape() call"? – Your Common Sense Feb 07 '12 at 02:44
  • It means there is no real_escape_string counter-part for PDO as it is handled internally by the bind/prepare functions. So there is no need for the complex and convoluted call made in line 1 of the OP. – Mike Purcell Feb 07 '12 at 02:53
  • [PDO::quote](http://php.net/manual/en/pdo.quote.php) it is called. And there is no need for the complex and convoluted call made in line 1 of the OP with mysqli either. I see no point in mentioning PDO here – Your Common Sense Feb 07 '12 at 03:05
  • Did you actually read the link you posted? "you are strongly recommended to use PDO::prepare()". I posted about PDO to make a clear and concise contrast between it and mysqli, where devs have the option to call to real_escape_string or make a call to prepare, but don't need to use both. With PDO there is no confusion, because there is no specific escaping, other than quote, which is obviously not the preferred method of handling strings. – Mike Purcell Feb 07 '12 at 03:12
  • there is NO contrast between PDO and mysqli. devs have the option with either driver. – Your Common Sense Feb 07 '12 at 03:22
  • Again, that's not the point I'm making. Had he opted to use PDO he wouldn't even been asking this question because there is no singular escaping method, outside of prepare, which wraps the escaping internally. – Mike Purcell Feb 07 '12 at 03:23
  • there is. I quoted it before. In fact, mysqli with it's escaping was safer than PDO with it's binding before 5.3.3. And I am sure you will write an vulnerable code using PDO. – Your Common Sense Feb 07 '12 at 03:24
  • Because it does escaping, not binding by default and most devs don't bother to set encoding properly. I don't understand that PDO craze that happens on this site. It is certainly not the tool but knowledge that makes you safe. While PDO looks like more a gospel rather than a tool. "USE PDO and you are safe" won't make your code safe magically. – Your Common Sense Feb 07 '12 at 03:31
  • hard to respond to your comments, frankly they make no sense. m_r_e_s is still not as good a defense against SQL injection attacks as PDO (http://ilia.ws/archives/103-mysql_real_escape_string-versus-Prepared-Statements.html). And your callous allegations make it hard to have a mature discussion. – Mike Purcell Feb 07 '12 at 03:34
  • I fixed a mistake, it should have red "m_r_e_s is still NOT as a good a defense...". – Mike Purcell Feb 07 '12 at 03:37
  • Well I was too hurry in response, I am sorry. Just set up your encoding with mysqli_set_charset() and m_r_e_s won't fail you. this function exists since 5.0.5. While up to 5.3.3 PDO was wulnerable to this very attack then used out of the box. – Your Common Sense Feb 07 '12 at 03:40
  • Setting the connection charset is a valid point, I didn't consider it because we have done this for so long it just becomes automatic. We ever pushed 5.3.3 to production. Normally we wait 3 months after a "stable" PHP release before we start migrating production servers, for the very reason you have described. I recommend updating your response with the connection charset point. – Mike Purcell Feb 07 '12 at 03:47
  • you don't have to wait for the 5.3.3. Just set ATTR_EMULATE_PREPARES to off and PDO will use native prepared statements instead of escaping. It will let you also use statements like `$sth = $dbh->prepare("SELECT id FROM users LIMIT ?,?"); which will cause an error in emulation mode $sth->execute(array(10,10));` – Your Common Sense Feb 07 '12 at 03:56
  • Interesting. TBH I don't like those constructs at all. After about 3 it gets difficult to read, especially because order is important. I prefer using bindValue (http://us2.php.net/manual/en/pdostatement.bindvalue.php) so order of values is taken out of the equasion, plus makes it easier to follow when binding to :apple and :orange, vs ?, ?. – Mike Purcell Feb 07 '12 at 03:59
  • yes, it's not big deal. I am just illustrating the fact that PDO is not a silver bullet and there is no essential difference between mysqli and PDO. **Both** let's you to do either right thing or mistake. And the fact that mindless use of PDO may be harmful as well – Your Common Sense Feb 07 '12 at 04:04