4

I implemented this sanitization fix:

function san($str, $type="full") {
    switch ($type) {
        case "full":
            $str = preg_replace("/[^a-zA-Z0-9_\-]/i", "", $str);
        break;
        case "mid":
            $str = preg_replace("/</i", "&gt;", $str);
            $str = preg_replace("/>/i", "&lt;", $str);
        break;
        case "low":
            if ($str == "true" || $str == true)
                $str = true;
            else
                $str = false;
        break;
    }

I want to sleep peacefully, but I'm not a security expert. What can pass through that filter? I know how to use regex, but is possible to encode str to bypass this function?

The mid level is intended to protect against XSS and the full level against command injection, such as this:

$str = san($_REQUEST['cmd']);
$cmd = "cmd1 foo | cmd2 -arg=". $str; 
shell_exec($cmd); // is this secure?
  • 6
    Secure against what? – a CVn Mar 27 '16 at 16:13
  • mid - agains XSS full - agains Command injection – user91817 Mar 27 '16 at 16:15
  • I updated my question. – user91817 Mar 27 '16 at 16:20
6

Is a generic input filter enough?

No, input sanitation is not the proper defense against any attacks, so just because you do have some filters is no reason to call it a day.

You do need proper defenses against typical attacks in place, eg prepared statements for SQL injection, encoding variables on output against XSS, etc.

That being said, additional input filtering is highly recommended as defense in depth.

Usability of your function

Usability is important. If no one knows what exactly your function does and how to properly use it, it will provide less security as people - this may include you in the future - will use it incorrectly or change it.

Your function isn't all that usable though. If I see san($x, "full"), I have no idea what is happening. Same goes for mid and low as arguments (and the naming doesn't even seem to make sense; is low supposed to be less secure - against ... something ... - than full or mid?).

A generic "full" also makes it very easy to change what the code does. Maybe you think that you need ; in some other case, so why not add it? It doesn't seem to do any harm, and it still matches the "full" description.

I would suggest some generic Input class with methods such as getInt($name), getAlphaNum($name), getCleanHTML($name), getRaw($name), getFilter($name, $regex), etc. With names like these, you see right away what you get, and if it's appropriate for the input and the situation. You also know not to change the implementation (getInt should always return an integer, getAlphaNum always alphanum).

Defending against Code Execution

Your example should be safe. I can't break out of the current context or add a new command, because I have neither space nor semicolon, nor any other character that may be used in different shells to add new commands.

Still, as suggested above, I would rewrite it. So it might look like this:

$str = Input::getFilter("cmd", [^a-zA-Z0-9_\-]);
$cmd = "cmd1 foo | cmd2 -arg=". escapeshellarg($str); 
shell_exec($cmd); // is this secure?

Now you are using the recommended approach (escapeshellarg), and have an additional filter, where it is quite clear what the filter does currently or might do in the future.

Defending against XSS

First of, your code is bugged. < is $lt (for less than).

Secondly, this will defend against XSS in some cases, but not all. It will not defend against XSS if you are already in a tag context (eg <img src="[USERINPUT]">), or in a JavaScript context (eg <script>var = "[USERINPUT]";</script>).

This is also one of the reasons that input filtering is not enough to prevent XSS (except very strict filtering, eg only accepting integer). You need to know the context to properly defend against it. In most contexts, htmlspecialchars with ENT_QUOTES is good enough though, see this guide for more information.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy