PHP - Smart way to protect $_GET vars from malicious injection

Question

I found this piece of code here: http://php.net/manual/de/reserved.variables.get.php

Want to use it to make my code safer. I use quite a few $_GET var in my project.

Please, if possible I would like you professionals to have a look and see if this piece of code could be enhanced or has any problems.

There is a smart way to protect the $ _GET input from malicious injection and options for inserting default values:

<?php 
// Smart GET function
public function GET($name=NULL, $value=false, $option="default")
{
    $option=false; // Old version depricated part
    $content=(!empty($_GET[$name]) ? trim($_GET[$name]) (!empty($value) && !is_array($value) ? trim($value) : false));
    if(is_numeric($content))
        return preg_replace("@([^0-9])@Ui", "", $content);
    else if(is_bool($content))
        return ($content?true:false);
    else if(is_float($content))
        return preg_replace("@([^0-9\,\.\+\-])@Ui", "", $content);
    else if(is_string($content))
    {
        if(filter_var ($content, FILTER_VALIDATE_URL))
            return $content;
        else if(filter_var ($content, FILTER_VALIDATE_EMAIL))
            return $content;
        else if(filter_var ($content, FILTER_VALIDATE_IP))
            return $content;
        else if(filter_var ($content, FILTER_VALIDATE_FLOAT))
            return $content;
        else
            return preg_replace("@([^a-zA-Z0-9\+\-\_\*\@\$\!\;\.\?\#\:\=\%\/\ ]+)@Ui", "", $content);
    }
    else false;
}

/*
DEFAULT: $_GET['page'];
SMART: GET('page'); // return value or false if is null or bad input
*/
?>

Source : http://php.net/manual/de/reserved.variables.get.php

There is no such thing as a single piece of code that "makes a string safe", because it all depends what you're using the variable *for*: Are you putting into a database, echoing it in HTML, or maybe both, or something else entirely? Escaping should always be done for a specific piece of data, in a specific context where it's being used; it cannot be generically done up front on the entirety of user input. — IMSoP, Jul 19 '15 at 14:20
There are also bugs in this function; for instance, is_bool and is_float will never return true, because everything in $_GET is a string, as is everything returned from trim(); those functions don't look at the contents of strings, just the type of variables. And it will return null, not false, because `else false` doesn't mean anything (I'm surprised it's not a syntax error). — IMSoP, Jul 19 '15 at 14:24
possible duplicate of [How can I prevent SQL-injection in PHP?](http://stackoverflow.com/questions/60174/how-can-i-prevent-sql-injection-in-php) — vascowhite, Jul 19 '15 at 15:22
That function is by no way smart: values in `$_GET` are *always* of the type of either string, array, or null. So the `is_bool` and `is_float` branches are never taken. — Gumbo, Jul 19 '15 at 16:12

arkascha · Answer 1 · 2015-07-19T14:38:02.367

The idea is old and often implemented. I personally prefer a variant where you also specify the expected type of argument. Without that I see no point in the approach, since the function can only react to what it gets.

Take the "is_bool" case for example. That does not make any sense at all the way it is implemented now. php will never provide a boolean here, it will always interpret get argument as strings with the content "false" or "true" if specified or 0 and 1 or even an empty string. So this condition will never met.

Take the "is_numeric" case as another example. Certainly it is possible to convert a string variable to a clean numeric type. But what is the idea behind that regex based replacement? If the content results in true as return value of the is_numeric() call, then there certainly are only numbers in there. Per definition. This assumes that what was actually meant here was is_int(), since otherwise that condition branch would block out the two following ones which does not make any sense at all.

What I miss here is the explicit conversion too. Why all the effort if you then do not convert to a clear variable type?

This would be different if you specify the expected type in the function:

// returns a boolean
$isSomeFlagSet = GET('bool_flag', 'bool'); 
// returns an integer or throws an exception
$numberOfHits = GET('number_of_hits', 'int'); 
// returns a string, maybe trimmed for convenience
$userName = GET('user_name', 'string');

You even an extend that to non builtin types which adds to its value:

// returns a string which is guaranteed to be a valid url
$baseUrl = GET('base_url', 'url'); 
// returns maybe a two char language code (e.g.'en') or throws an exception
$userLang = GET('lang', 'langcode');
// you could add a type 'json' which returns an array from a json get parameter 
$priceMatrix = GET('prices', 'json');

In that case a value validation actually does make sense, since it can clearly decide if a conversion of the provided string does make sense. And it can do an explicit and expected conversion so that you have a guaranteed variable type after calling that function.

The example you give with explicit types is basically what the built-in ext/filter does, although its interface is rather horrible. — IMSoP, Jul 19 '15 at 16:14
The idea is old and misguided. Yes, you are suggesting some improvements to the given code, but even with the improvements, the whole idea behind the code is still flawed. The only proper answer is to not attempt something like this at all. To see *why* this idea is misguided, see the comment on the question starting with `There is no such thing as` — Jasper, Jul 20 '15 at 16:22
@Jasper OK, thanks for your comment. We absolutely agree that such generic validation of input values is not a free ticket that solves all problems. But I fail to see why that should mean that such validation is senseless. This is an invalid conclusion. In my eyes the approach above offers a basic validation of input values, helpful explicit type casts, makes using request arguments much more convenient and does not have negative side effects. — arkascha, Jul 20 '15 at 19:14

PHP - Smart way to protect $_GET vars from malicious injection

1 Answers1

Linked