4

Recently, I had an audit run on some of my sites by a client. One of the things they came back with was that I could be sanitizing the input data a little better as people could still cause potential harm to the database.

The function below is what I am currently using (a leftover from the old developer) but I cannot see where the potential issue may lie.

The string that gets passed through to the database will be displayed via XML which in turn is read by a Flash application.

Could anyone tell me what I might be missing? Thanks

function secure_string($string)
{   
    return (strip_tags(addslashes(mysql_real_escape_string(
                      stripslashes($string)))));
}
Drew
  • 3,194
  • 8
  • 40
  • 62
  • 1
    Show what you need that string for, not the current function. Protection depends on what you want to guard against. – scragar Oct 29 '09 at 10:01
  • @scragar, MySQL injection. At least he says that the client said: "potential harm to the database". – Frankie Oct 29 '09 at 10:07
  • 7
    I don't do php (anymore), but, honestly, calling `addslashes` AND `stripslashes` in one chain certainly brought a tear to my eye. – shylent Oct 29 '09 at 10:44

6 Answers6

5

It looks like there's too much going on in that function. mysql_real_escape_string() already escapes everything you need to escape, so there's no need to run addslashes() on that. In fact, it could do more harm than good by escaping the backslashes mysql_real_escape_string() creates.

Kaivosukeltaja
  • 15,541
  • 4
  • 40
  • 70
  • Just for hint, there exist a function like mysql_real_escape_string() but for postgresql? pg_escape_string or pg_escape_bytea dont seem to works against injection – Strae Oct 29 '09 at 13:30
  • How doesn't `pg_escape_string` work? It should work just fine. `pg_query_params` is a better way to run queries though. – Lukáš Lalinský Oct 29 '09 at 14:45
5

Better use the new PHP function filter_var() for cleaning input. New and better.

Elzo Valugi
  • 27,240
  • 15
  • 95
  • 114
2

mysql_real_escape_string is the last step, you shouldn't use it your application logic. It's sole purpose is to pass strings to the database, so use it only when constructing queries. You can pass anything to mysql_real_escape_string and it will make sure you can safely store it in the database.

For the rest, it depends what do you want. If you want to strip tags, use strip_tags, etc.

Lukáš Lalinský
  • 40,587
  • 6
  • 104
  • 126
  • The string gets outputted to an xml file which in turn is read by a flash application. Thanks – Drew Oct 29 '09 at 10:10
  • If you are adding it to an XML file, you should escape it for XML *when* adding it to an XML file. Here you are dealing with adding the string to a database, so you escape it for the database. You need to use different validation/escaping functions for different purposes. – Lukáš Lalinský Oct 29 '09 at 10:13
  • thanks man! i'm not to worried about sanity when i extract! :) – Drew Oct 29 '09 at 14:14
1

Depends on where the "secured" string will be used. If it's going to be used in the database, you only need mysql_real_escape_string(), nothing more. If it's going to be displayed in html, you only need htmlentities(), nothing more. In short: your code is doing way too much, which could even be harmful.

If you want to store it in the database for displaying it in html lateron (like a comment, for example), you should be using mysql_real_escape_string() when storing the string and htmlentities() when displaying it.

soulmerge
  • 73,842
  • 19
  • 118
  • 155
1

If your server uses php 5.2 or better, you should use filter_var for the XML part.

$output = filter_var($input, FILTER_SANITIZE_STRING);

To store something into your database, use PDO and parameterized queries.

Arkh
  • 8,416
  • 40
  • 45
1

It's a misnomer to try and fix the problem at input time, since the problem happens at output time. See my answer over here:

What’s the best method for sanitizing user input with PHP?

Community
  • 1
  • 1
troelskn
  • 115,121
  • 27
  • 131
  • 155