0

I've been working on a code that escapes your posts if they are strings before you enter them in DB, is it an good idea? Here is the code: (Updated to numeric)

static function securePosts(){
    $posts = array();
    foreach($_POST as $key => $val){
        if(!is_numeric($val)){
            if(is_string($val)){
                if(get_magic_quotes_gpc())
                    $val = stripslashes($val);
                $posts[$key] = mysql_real_escape_string($val);
            }
        }else
            $posts[$key] = $val;
    }   
    return $posts;
}

Then in an other file:

if(isset($_POST)){
    $post = ChangeHandler::securePosts();
    if(isset($post['user'])){
        AddUserToDbOrWhatEver($post['user']);
    }

}

Is this good or will it have bad effects when escaping before even entering it in the function (addtodborwhater)

Kilise
  • 1,051
  • 4
  • 15
  • 35
  • 1
    It's not. We have PDO that does escaping for us, safely and without the need to reinvent the wheel. – N.B. Jan 24 '13 at 12:55
  • possible duplicate of [How to prevent SQL injection in PHP?](http://stackoverflow.com/questions/60174/how-to-prevent-sql-injection-in-php) – Quentin Jan 24 '13 at 12:57
  • Escaping doesn't make your data "safe". – Your Common Sense Jan 24 '13 at 20:14
  • 1
    What do you mean by that? – Kilise Jan 24 '13 at 20:16
  • 1
    @YourCommonSense - The purpose of a function like `mysqli_real_escape_string()` is, to make sure that a value can be safely used with a certain target system, in this case a mysql database. Even when it does not give 100% protection, it definitely should be engouraged, or at least you should provide a reason why not to use an escape function. – martinstoeckli Jan 24 '13 at 20:42
  • Ofcourse it doesn't mean that everything will be safe just by escaping. But it's one thing you need to do. Before the data even get escaped there are parts that wouldn't allow "unsafe" or "not acceptable" data to get through the function securePosts() i made. – Kilise Jan 25 '13 at 09:52

2 Answers2

1

When working with user-input, one should distinguish between validation and escaping.

Validation

There you test the content of the user-input. If you expect a number, you check if this is really a numerical input. Validation can be done as early as possible. If the validation fails, you can reject it immediately and return with an error message.

Escaping

Here you bring the user-input into a form, that can not damage a given target system. Escaping should be done as late as possible and only for the given system. If you want to store the user-input into a database, you would use a function like mysqli_real_escape_string() or a parameterized PDO query. Later if you want to output it on an HTML page you would use htmlspecialchars().

It's not a good idea to preventive escape the user-input, or to escape it for several target systems. Each escaping can corrupt the original value for other target systems, you can loose information this way.

P.S.

As YourCommonSense correctly pointed out, it is not always enough to use escape functions to be safe, but that does not mean that you should not use them. Often the character encoding is a pitfall for security efforts, and it is a good habit to declare the character encoding explicitely. In the case of mysqli this can be done with $db->set_charset('utf8'); and for HTML pages it helps to declare the charset with a meta tag.

martinstoeckli
  • 23,430
  • 6
  • 56
  • 87
0

It is ALWAYS a good idea to escape user input BEFORE inserting anything in database. However, you should also try to convert values, that you expect to be a number to integers (signed or unsigned). Or better - you should use prepared SQL statements. There is a lot of info of the latter here and on PHP docs.

Bud Damyanov
  • 30,171
  • 6
  • 44
  • 52
  • PHP has pretty neat build-in functions for variable type check - avoid using slow and hungry regexp engine, use is_numeric(), is_int(), is_float() etc... – Bud Damyanov Jan 24 '13 at 13:52
  • Yeah i knew that but still used regex don't know why :p changed that now to is_numeric(), so you saying my function would work good right? – Kilise Jan 24 '13 at 13:59
  • One more thing you need to consider - are you going to allow HTML tags in your post content or not? If not - then you need to filter them with strip_tags()... – Bud Damyanov Jan 24 '13 at 14:25
  • what is best? htmlentites() or strip_tags()? – Kilise Jan 24 '13 at 20:05
  • It depends of what you want to achieve. I personally strip all tags in my projects every time I do not need them. – Bud Damyanov Jan 25 '13 at 12:40