1

Possible Duplicate:
How to prevent SQL injection?

I am setting up a comment system on my site and I wanted to know if this is save. I use PHP and MySQL. - Do not use code below, it's horribly insecure -

Creating a new comment:

  1. User writes $comment, submits it
  2. $comment = addslashes($comment);
  3. insert $comment into MySQL database

Reading a comment:

  1. User requests a comment, database delivers $comment
  2. $comment = htmlspecialchars(stripslashes($comment));
  3. echo $comment;

The system should be secure against HTML manipulations and MySQL injections. And all other nasty stuff I am not aware of. Am I doing it right?

Bonus question: What collation should I use for $comment in my MySQL table?

Edit: wow I didn't think my question could cause this huge discussion. Thank you for all your answers :)

Community
  • 1
  • 1
Jonas Kaufmann
  • 1,797
  • 3
  • 22
  • 43
  • 1
    addslashes is not as good as mysql_real_escape_string. [See here](http://stackoverflow.com/questions/91216/what-is-the-difference-between-mysql-real-escape-string-and-addslashes) – John V. Dec 23 '12 at 01:22
  • 9
    It looks like you're still learning PHP. Now would be a great time to [switch to PDO](http://php.net/book.pdo) or [mysqli](http://php.net/book.mysqli) instead of using the horrible old deprecated `mysql_` family of functions. Using [prepared statements with parameterized queries](http://en.wikipedia.org/wiki/Prepared_statement) beats the pants off of doing `mysql_real_escape_string` every time. – Charles Dec 23 '12 at 01:23
  • 1
    @AlexLunix So I should use mysql_real_escape_string() when inserting. What do I need to use for reading then? – Jonas Kaufmann Dec 23 '12 at 01:23
  • Don't use any of these functions, just use prepared statements with bound variables. – jeroen Dec 23 '12 at 01:24
  • @JonasKaufmann, if you're getting backslashes when pulling out of the database, check that your host doesn't have [magic quotes](http://php.net/security.magicquotes) enabled. If they do, **run** away from them, as they are *horrible*. – Charles Dec 23 '12 at 01:24
  • 3
    [SQL injection that gets around mysql_real_escape_string()](http://stackoverflow.com/questions/5741187/sql-injection-that-gets-around-mysql-real-escape-string) – John Woo Dec 23 '12 at 01:30

2 Answers2

10

Consider switching to prepared statements right from the start :-)

They may seem a bit overheaded now, but you safe so much time worrying about escaping each and every parameter that it pays back.

Here is a good Tutorial: http://www.kitebird.com/articles/php-pdo.html.

When printing out user defined content, you still need to use htmlspecialchars to account for XSS invulnerabilities.

Phil Rykoff
  • 11,999
  • 3
  • 39
  • 63
  • 4
    Seriously. I wish I'd had prepared statements when I was starting out. Learn it once, learn it right. – Ian Atkin Dec 23 '12 at 01:33
  • To be honest, the site is already up and running and I've coded a lot. I am also planning to use Amazon's RDS very soon. Does Amazon RDS support PDO / MYSQLI? Why should I choose PDO over MYSQLI? Also, I am very interested in database caching. Any recommandation in which direction I should head...? – Jonas Kaufmann Dec 23 '12 at 01:34
  • If you setup a MySQL instance on your Amazon RDS box, then it will support mysqli and pdo. – Phil Rykoff Dec 23 '12 at 01:39
  • @Phil Rykoff Great! Another stupid question of mine: why can't I use htmlspecialchars() before inserting it into my DB? I could save computing and coding time this way. – Jonas Kaufmann Dec 23 '12 at 01:42
  • For why you should make use of pdo, prepared statements: it's safer. Check the link JW provided above, http://stackoverflow.com/questions/5741187/sql-injection-that-gets-around-mysql-real-escape-string. – Phil Rykoff Dec 23 '12 at 01:43
  • Depends on your use case. If you want your users to be able to edit the original source, you have to do it while printing out. Otherwise, you can do it while inserting it into the DB imho. – Phil Rykoff Dec 23 '12 at 01:44
  • @JonasKaufmann If the content is only going to be printed (not used for anything else) then you *could* use `htmlspecialchars` before inserting into the database. If the content was used for anything else, you would have to decode it before using it. (Generally, it is a good idea to only use `htmlspecialchars` when outputting data.) – uınbɐɥs Dec 23 '12 at 02:51
-5

Use mysql_real_escape_string (or whatever escaping function comes with your library) instead of addslashes.

Don't use stripslashes, because they have already been stripped by having been parsed by MySQL, and you run the risk of erasing legitimate backslashes in the input.

For the collation, use whatever you like. I prefer latin_general_ci, but if you want unicode you should use a UTF8-compatible collation.

Niet the Dark Absol
  • 320,036
  • 81
  • 464
  • 592
  • 6
    Say to switch to PDO ;) – Gabriel Santos Dec 23 '12 at 01:24
  • 2
    Please don't use `mysql_real_escape_string` or any of the `mysql_*` functions. If you're just starting (like the OP), go for PDO or mysqli directly... – jeroen Dec 23 '12 at 01:25
  • Why do that when I stick with `mysql`? I know about injection, and I'd rather just remember to escape strings than have to use complex hacks just to do things like selecting a variable column name. – Niet the Dark Absol Dec 23 '12 at 01:26
  • 2
    As a sidenote, though what they are speking here are all string, `mysql_real_escape_string` doesn't guarantee to protect injection from numbers. [SQL injection that gets around mysql_real_escape_string()](http://stackoverflow.com/questions/5741187/sql-injection-that-gets-around-mysql-real-escape-string) – John Woo Dec 23 '12 at 01:27
  • 1
    @Kolink http://news.php.net/php.internals/53799 – Gabriel Santos Dec 23 '12 at 01:29
  • 4
    `mysql_real_escape_string` is [deprecated from 5.5, and will be removed in a future release](http://php.net/manual/en/function.mysql-real-escape-string.php). – Ian Atkin Dec 23 '12 at 01:33
  • To be honest, the site is already up and running and I've coded a lot. I am also planning to use Amazon's RDS very soon. Does Amazon RDS support PDO / MYSQLI? Why should I choose PDO over MYSQLI? Also, I am very interested in database caching. Any recommandation in which direction I should head...? – Jonas Kaufmann Dec 23 '12 at 01:39
  • @JW. I am aware of that. That's part of knowing how to use the tools. – Niet the Dark Absol Dec 23 '12 at 01:44
  • 2
    Prepared statements are really helpful. Ircmaxell did a nice video that simply explains why: https://www.youtube.com/watch?v=nLinqtCfhKY&list=PLM-218uGSX3DQ3KsB5NJnuOqPqc5CW2kW&index=4 – hakre Dec 23 '12 at 02:40
  • 2
    @Kolink Object-oriented programming isn't a 'complex hack'. – uınbɐɥs Dec 23 '12 at 02:53
  • @ShaquinTrifonoff How would you go about doing this in PDO? mysql_query("SELECT `".$fieldname."` FROM `".$tablename."` WHERE ".$some_preconstructed_condition);` What about `$arr = Array(); /* build up $arr where each item is a row to insert */ mysql_query("INSERT INTO `table` VALUES ".implode(",",$arr)); – Niet the Dark Absol Dec 23 '12 at 03:03
  • @Kolink Why would you need to select from an arbitrary column? You would be better off creating a whitelist, and checking if the column is in the whitelist. – uınbɐɥs Dec 23 '12 at 03:06
  • The table is a lookup table of affinities of Pokémon attacks against Pokémon types. The `$fieldname` is the type of the attack. The condition would be the type(s) of the target. I frequently use such two-dimensional tables, and PDO would drastically slow that. – Niet the Dark Absol Dec 23 '12 at 03:14
  • 4
    @Kolink: ``$link->query("SELECT `$fieldname` FROM `$tablename` WHERE $some_preconstructed_condition");`` is how I'd do it. PDO doesn't *restrict* you from doing anything you could with `mysql_`, but it does help a lot with most common queries. – Ry- Dec 23 '12 at 16:22
  • @minitech That's how I would do it too, if there was no user input in the variables. – uınbɐɥs Dec 23 '12 at 17:53