0

I have a function that does a security check on my GET parameter (I'm not the author):

function GET($name = NULL, $value = false)
{
    $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;
}

So whenever I'm fetching GET parameter values I call this function. However, if my GET parameter is a string containing special characters like åäö they get replaced. For example, this string Detta är en annons will have the following output: Detta r en annons.

Since I'm sure it's a string variable it's probably the filter_var function that strips my special chars. How should I rewrite the above script to keep my special characters in my string?

Edit

Okay, so above script is thrash. I've been looking at alternatives. If my purpose is to insert the GET parameter value into a database, will filter_input(INPUT_GET,"link",FILTER_SANITIZE_STRING); be sufficient to clean my variable from any malicious code?

Marcus
  • 6,697
  • 11
  • 46
  • 89
  • 5
    that function is just plain dumb. you filter based on what you KNOW should be in there. you don't just throw every filter possible at it and see if anything sticks. – Marc B Jul 22 '15 at 18:04
  • what is the string used for? this is an awful lot of cleaning. there may be a much simpler approach here. – jdu Jul 22 '15 at 18:04
  • You shouldn’t be using this function. Have a look at [Smart way to protect $_GET vars from malicious injection](http://stackoverflow.com/q/31502122/53114). – Gumbo Jul 22 '15 at 18:05
  • I'm open for suggestions @MarcB :-) Please have in mind that I'm a novice programmer :-) – Marcus Jul 22 '15 at 18:06
  • @Gumbo I realize that now. Do you have any alternatives to fetch the GET parameter "safley"? – Marcus Jul 22 '15 at 18:10
  • @Marcus Safely in what manner? – Gumbo Jul 22 '15 at 18:14
  • @Gumbo Safe from malicious code in the GET parameter – Marcus Jul 22 '15 at 18:14
  • @Marcus The maleficence depends on how you use the value. And there is no universal function that can protect against the various injection weaknesses as each language and context in which the value is to be put requires its own context-depending handling. Better look at fixing this at the drain and not at the source. – Gumbo Jul 22 '15 at 18:25

1 Answers1

1

Strictly answering your question, the problem lay in the preg_replace. In the original version, any characters other than the ones explicitly listed are replaced by "", effectively removing them from the input. For example, "2^8" would become "28", because a ^ is not allowed.

To accept any character other than "invisible control characters and unused code points", replace the preg_replace in your function with this:

return preg_replace('@(\p{C})@ui', "", $content);

Working implementation.

Edit

Responding to the OP edit, filter_input is a great way to remove potentially dangerous input and may be what you want in your specific use case. However, please understand that there isn't a magic bullet solution. Have a look at this related SO Q&A.

At any rate, what you would typically want to do is check that the user input conforms to your storage requirements (type, length, etc), then use prepared statements to insert it into your database, then use output escaping to prevent XSS attacks. The pseudo-code goes something like this:

$foo = isset($_GET['foo') ?? false;
if (is_string($foo) && 0 < strlen($foo) && strlen($foo) < 255) {
    $sth = $dbh->prepare('INSERT INTO `table` VALUES (?)');
    $sth->execute(array($foo));
    echo htmlentities($foo);
} else {
    echo 'Error: foo is not valid';
}
Community
  • 1
  • 1
bishop
  • 37,830
  • 11
  • 104
  • 139
  • Thanks for your answer. You did answer the question before the edit so +1 for answering the original question. Could you have a look at the edit aswell? I'll still tick your answer but I'd be happy for a second opinion my alternative approach. – Marcus Jul 23 '15 at 05:21