2

I'm no PHP/SQL expert, and I've juste discovered that i had to apply mysql_real_escape_string to secure my SQL INSERTS.

I made a function using several advice found on the net, here it is:

function secure($string)
{
if(is_numeric($string)) 
    { $string = intval($string); }
    elseif (is_array($string)) 
    {
        foreach ($string as $key => $value) {
            $string[$key] = secure($value);
        }
    } 
    else if ($string === null) 
    {
        $string = 'NULL';
    }
    elseif (is_bool($string)) 
    {
        $string = $string ? 1 : 0;
    } 
    else 
    {
        if (get_magic_quotes_gpc()) { $value = stripslashes($string); } 
        $string = mysql_real_escape_string($string);
        $string = addcslashes($string, '%_');
    }
    return $string;
}

Thing is, when I have a look at my tables content, it contains backslashes. And then logically, when I retrieve data I have to apply stripslashes to it to remove these backslashes.

Magic Quotes are off.

QUESTION 1) Now I think that even though I use mysql_real_escape_string to secure my data before SQL insertion, backslashes should not appear in my content ? Can you confirm this ?

QUESTION 2) If not normal, why are these backslashes appearing in my phpMyAdmin content and retrievals ? What did I did wrong ?

QUESTION 3) A guess I have is that mysql_real_escape_string could be applied twice, isn't it ? If so, what could be a function to prevent mysql_real_escape_string being applied many times to a same string, leading to many \\ to a same escapable character ?

Thanks a lot by advance for your inputs guys !

Baptiste
  • 323
  • 5
  • 16
  • Re 3: Try to do escaping just before the string is put into a query and don't reuse the escaped variable for anything else. – Mchl Feb 12 '11 at 19:38
  • 1
    You are escaping strings twice. The second addcslashes is potentially harmful. It can re-escape backslashes which were used to escape UTF-8 single quotes, but itself overlook malformed UTF-8 sequences. – mario Feb 12 '11 at 19:47

4 Answers4

1

Your stripslashed $string is stored to the wrong variable $value instead of $string:

if (get_magic_quotes_gpc()) { $value = stripslashes($string); }

should be

if (get_magic_quotes_gpc()) { $string = stripslashes($string); }
Floern
  • 33,559
  • 24
  • 104
  • 119
  • 1
    @Baptiste to catch these silly errors in the future, add this line to your common config file, `error_reporting(E_ALL);` - it will tell PHP to watch all strange variables like this `$value` and report them – Your Common Sense Feb 12 '11 at 20:07
1

oh, what a senseless function. I know it's not your fault but ones who wrote it in their stupid articles and answers.

Get rid of it and use only mysql_real_escape_string to escape strings.

you have mixed up everything.

  • first, no magic quotes stuff should be present in the database escaping function.
    if you want to get rid of magic quotes, do it centralized, at the very top of ALL your scripts, no matter if they deal with the database or not.

  • most of checks in this function are useless. is_bool for example. PHP will convert it the same way, no need to write any code for this.

  • LIKE related escaping is TOTALLY distinct matter, and has nothing to do with safety.

  • is numeric check is completely useless, as it will help nothing.

Also note that escaping strings has nothing to do with security.
I's just a syntax rule - all strings should be escaped. No matter of it's origin or any other stuff. Just a strict rule: every time you place a string into query, it should be quoted and escaped. (And of course, if you only escape it but not quote, it will help nothing)

And only when we talk of the other parts of query, it comes to the SQL injection issue. To learn complete guide on this matter, refer to my earlier answer: In PHP when submitting strings to the database should I take care of illegal characters using htmlspecialchars() or use a regular expression?

Community
  • 1
  • 1
Your Common Sense
  • 156,878
  • 40
  • 214
  • 345
  • thanks a lot ! Not sure to understand why LIKE escaping should be dealed with somewhere else, but i take your word for it :) – Baptiste Feb 12 '11 at 20:01
  • @Baptiste this function is called "secure" but LIKE escaping has nothing to do with security. It's just wildcards, whe way LIKE works. With no wildcards there is no sense in using LIKE. – Your Common Sense Feb 12 '11 at 20:05
  • @Baptiste but most important part is described by mario: it's LIKE escaping and should be applied to the strings used in LIKE statement only, not for ALL strings, right? – Your Common Sense Feb 12 '11 at 20:10
0

Are you sure you aren't calling mysql_real_escape_string more than once, each time you call it with escapable characters you will end up adding more and more slashes. You want to call it only once. Also, why are you also calling addcslashes? mysql_real_escape_string should be enough. If you call it only once, you should never have to call stripslashes on the data after retrieving it from the database.

You can't really tell if mysql_real_escape_string is applied more than once, I'd suggest going back and re-reading your code carefully, try debug printing the values just before they are inserted into the db to see if they are look 'over-slashed'.

Btw, if you are using prepared statements (e.g. via mysqli) you dont need to escape your strings, the DB engine does this for you, this could be the problem too.

Jesse Cohen
  • 4,010
  • 22
  • 25
  • From what I understood, addcslashes adds slashes to _ and % in case of LIKE or GRANT, which mysql_real_escape_string doesn't. Isn't it right ? – Baptiste Feb 12 '11 at 19:47
  • @Baptiste LIKE is going to be **completely useless** with these chars escaped. Go figure. I do understand that no one can learn everything at once. But such a mindless copy-paste makes me sick. – Your Common Sense Feb 12 '11 at 19:54
  • Well since everyone on the internet talk like they hold the truth, not easy to distinguish good advice from bad advice, particularly when you lack experience like me. I thought it wasn't harmfull to try and deal with these LIKE issues. Being wrong is how I learn though. – Baptiste Feb 12 '11 at 20:04
0

Remove addslashes completely from all of your code. This is the leading cause for slashes being inserted into database.

function escape($string) {
    if (get_magic_quotes_gpc()) {
        $string = stripslashes($string);    
    }
    return mysql_real_escape_string($string); 
}

Always check if magic_quotes_gpc is enabled, if it is perform stripslashes and escape the data.

Escaped = "don\'t use addslashes"

When it goes into database the '\' is removed.

BGPHiJACK
  • 1,277
  • 1
  • 8
  • 16
  • that's widely used but completely wrong approach. magic quotes stuff has nothing to do with DB escaping. – Your Common Sense Feb 12 '11 at 19:40
  • uh oh what a great opinion. Why not get some experience first, dude? What if you gonna store your POST data in the cookie, not database? stripslashes it again? **magic quotes stuff has nothing to do here** – Your Common Sense Feb 12 '11 at 19:48
  • 1
    @tj: Your code is indeed working. But it's not a good practice and approach to do this per-variable. If the config setting cannot be fixed, then magic_quotes should be undone centrally and just **once** with e.g. [`$_POST = array_map("stripslashes", $_POST)`](http://www.php.net/manual/en/security.magicquotes.disabling.php#91585) – mario Feb 12 '11 at 19:53
  • You misunderstood the question that he/she had asked originally. They had slashing issues with their data in regards to MySQL not cookies, sessions, posts, gets, and environment. So don't come in here trolling answers because you think yours is the best, it's disrespectful. @mario Yes, that would be ideal to map it versus seperate. – BGPHiJACK Feb 12 '11 at 19:56
  • Thanks guys for this mini debate, now I understand why magic quotes stuff should be unrelated to this. I'm a "he" btw :) – Baptiste Feb 12 '11 at 19:58