CODEWITHSUNDEEP
phpsecurityget
Nikolay
Nikolay

Reputation: 2214

Handle $_GET safely PHP

I have a code like this:

$myvar=$_GET['var'];

// a bunch of code without any connection to DB where $myvar is used like this:

$local_directory=dirname(__FILE__).'/images/'.$myvar;
if ($myvar && $handle = opendir($local_directory)) {
    $i=0; 
    while (false !== ($entry = readdir($handle))) {
        if(strstr($entry, 'sample_'.$language.'-'.$type)) {
            $result[$i]=$entry;
            $i++;
        }
    } 
    closedir($handle);
} else {
    echo 'error';
}

I'm a little confused with a number of stripping and escaping functions, so the question is, what do i need to do with $myvar for this code to be safe? In my case i don't make any database connections.

Upvotes: 1

Views: 1493

Answers (4)

Ilmari Karonen
Ilmari Karonen

Reputation: 50378

Let me just note for completeness that, if you can be sure your code will only be run on Unixish systems (such as Linux), the only things you need to ensure are that:

  1. $myvar does not contain any slash ("/", U+002F) or null ("\0", U+0000) characters, and that

  2. $myvar is not empty or equal to "." (or, equivalently, that ".$myvar" is not equal to "." or "..").

That's because, on a Unix filesystem, the only directory separator character (and one of the two characters not allowed in filenames, the other being the null character "\0") is the slash, and the only special directory entries pointing upwards in the directory tree are "." and "..".

However, if your code might someday be run on Windows, then you'll need to disallow more characters (at least the backslash, "\\", and probably others too). I'm not familiar enough with Windows filesystem conventions to say exactly which characters you'd need to disallow there, but the safe approach is to do as Rich Bradshaw suggests and only allow characters that you know are safe.

Upvotes: 2

maraspin
maraspin

Reputation: 2393

It's really, really dangerous to do something like: opendir($local_directory) where $local_directory is a value which could come from the outside.

What if someone passes in something like ../../../../../../../../../etc ...or something like that? You risk of compromising security of your host.

You can take a glance here, to start: http://php.net/manual/en/book.filter.php

IMHO, if you don't create anything on the fly, you should have something like:

$allowed_dirs = array('dir1','dir2', 'dir3');
if (!in_array($myvar, $allowed_dirs)) {
    // throw an error and log what has happened
}

You can do this right after you receive your input from "outside". If it's impractical for you to do this because the number of image dirs can vary with time and you're afraid of missing the sync with your codebase, you could also populate the array of valid values making a scan of subdirectories you have into the image folders first.

So, at the end, you could have something like:

$allowed_dirs = array();
if ($handle = opendir(dirname(__FILE__) . '/images')) {
    while (false !== ($entry = readdir($handle))) {
            $allowed_dirs[] = $entry;
    } 
    closedir($handle);
}
$myvar=$_GET['var'];

// you can deny access to dirs you want to protect like this
unset($allowed_dirs['private_stuff']);

// rest of code
$local_directory = dirname(__FILE__) . "/images/.$myvar";
if (in_array(".$myvar", $allowed_dirs) && $handle = opendir($local_directory)) {
    $i=0; 
    while (false !== ($entry = readdir($handle))) {
        if(strstr($entry, 'sample_'.$language.'-'.$type)) {
            $result[$i]=$entry;
            $i++;
        }
    } 
    closedir($handle);
} else {
    echo 'error';
}

Code above is NOT optimized. But let's avoid premature optimization in this case (stating this to avoid another "nice" downvote); snippet is just to get you the idea of explicitly allowing values VS alternate approach of allowing everything unless matching a certain pattern. I think the former is more secure.

Upvotes: 3

Gumbo
Gumbo

Reputation: 655755

As with every data that comes from an untrusted source: Validate it before use and encode it properly when passing it to another context.

As for the former, you first need to specify what properties the data must have to be considered valid. This primarily depends on the purpose of its use.

In your case, the value of $myvar should probably be at least a valid directory name but it could also be a valid relative path composed of directory names, depending on your requirements. At this point, you are supposed to specify these requirements.

Upvotes: 1

Rich Bradshaw
Rich Bradshaw

Reputation: 73045

You are trying to prevent directory traversal attacks, so you don't want the person putting in ./../../../ or something, hoping to read out files or filenames, depending on what you are doing.

I often using something like this:

$myvar =  preg_replace("/[^a-zA-Z0-9-]/","",$_GET['var']);

This replaces anything that isn't a-zA-Z0-9- with a blank, so if the variable contains say, *, this code would delete that.

I then change the a-zA-Z0-9- to match which characters I want to be allowed in the string. I can then lock it down to only containing numbers or whatever I need.

Upvotes: 5

Related Questions