-1

How safe is this? 

   if (isset($_GET["var"]) && file_exists("path/".$_GET["var"].".php")) { 
        include("path/".$_GET["var"].".php");
    } else {  
        echo 'File Does Not Exist!';   
    }

I'm wondering if $_GET["var"] needs to be "sanitized" opposed to just letting it run against the file_exists function before trying to include it or not. Is this dangerous?

+++UPDATED+++

Thank you all for your responses! Please see updated below...

function mrClean($var) {
$clean_var = (isset($var) && !empty($var)) ? $var : 'index';
$clean_var = preg_replace('/[^-A-Za-z0-9_]/', '', $clean_var);
return $clean_var;
}

$var = mrClean($_GET["var"]);

if (file_exists("path/".$var.".php")) { 
  include("path/".$var.".php");
} else {  
  echo 'File Does Not Exist!';   
}

When I call on mrClean to replace all, but the following:

- A-Z a-z 0-9 _ via preg_replace

...will this now be considered safe? Is there anything that can be added to make this any safer?

I will implement a whitelist as suggested... but anything else?

Thank you!!

-Andrew

Andrew
  • 148
  • 1
  • 11
  • Actually one thing you _certainly_ have to make sure is to resolve the resulting path to its real location inside the file system, so that no one can trick your script opening files not meant for that. One could try to do that by using a path like `../../../../etc/passwd` or similar... – arkascha Feb 06 '16 at 15:26
  • In general it rarely is a good idea to use client provided data as a means of flow control. Too many risks, since you can never be certain that you thought of everything. – arkascha Feb 06 '16 at 15:27
  • It is **never safe** to trust user inputs. Nothing prevents me from providing a relative path to a different file you actually don't want to serve. Use a whitelist to compare the user input to and only allow these files to be accessed. – Charlotte Dunois Feb 06 '16 at 15:36

2 Answers2

0

You do not want to do this.

let's say i am running index.php in /var/www/test and inside of that is path (so /var/www/test/path exsts)

if i ask this question - file_exists('path/../../../../etc/nginx/sites-available/default.php')

the answer is YES

i.e. a file in /etc/nginx/sites-available/default.php does exist. All I did was pass in $_GET['var'] as "../../../../etc/nginx/sites-available/default"

so now you have asked the webserver to include some random file into your page.

now - we have to note here that depending on permissions, operating system etc. this may not work the same. But in general, you don't want to do this - go the long way and if statement / switch statement stuff into the includes.

You can try to make sure the input is safe by removing characters etc. but in the end, one mistake will cost you your whole file system - not worth it!

IaMaCuP
  • 835
  • 5
  • 19
0

Yes, the regex replace within your question update is SAFE. But be aware of that ANY include is dangerous and if you will allow the user to include some unsafe script.

mvorisek
  • 3,290
  • 2
  • 18
  • 53
  • Thank you Mvorisek, will the user be able to include (inject rather) via the GET variable? or do you mean like, if they type in a file that does exist, and that file is unsafe? – Andrew Feb 07 '16 at 14:46
  • The regex prevent only including other files from other directories. I meant you should be aware of what scripts in the included directory are. – mvorisek Feb 07 '16 at 14:52