Is this a secure way to include a file with a variable name?

[leebow]

Hi, can you tell me if this is secure:

$loadPage = filter_input(INPUT_GET, 'l', FILTER_SANITIZE_STRING);


if (file_exists('pages/' . $loadPage . '.php')) {

include("pages/$loadPage.php");

}else{
die();
}

Thank you!

[robzilla]

If I pass "../otherpage" to $_GET['l'] then you would include ./otherpage.php rather than ./pages/otherpage.php (on Linux, "/dir/../" equals "/"). Whether that's "insecure" is hard to say, but it's not ideal. I would probably use preg_match() to make sure the variable contains only valid characters, e.g. #^[0-9a-z-_]$#i, depending on how you name your PHP files in that directory. Alternatively, you could read the PHP files contained in the pages directory with scandir() and check $_GET['l'] against that, but that may be a tiny bit slower, especially if you have many files in that folder, unless you cache the array of pages (and have a way of clearing that cache when the files change) or store a list of pages somewhere else (e.g. MySQL, text file).

[leebow]

Oh that's not too good is it :(

I've tried your example - and it does what you say - for example: ?l=../admin/config.php

Does load the config.php file - but it doesn't seem to be run though the "include" the URL seems to change to go directly to that config file.

I've tried ?l=../.htaccess but that doesn't work. It's just an error page.

Because it's a small site - I could make a list of acceptable include files - it's not ideal.

Would it be secure if I checked the $_get to see if the first few characters are "../" and if they are then die() is there anything else the variable could start with that could be insecure?

Thanks for the help.

[bhukkel]

Just check if the $_get var only contains a..z characters and length 1-10.

if (preg_match('/^[a-z]{1,10}$/i', $get[''])) { ....}

[robzilla]

Why 1-10?

Would it be secure if I checked the $_get to see if the first few characters are "../" and if they are then die() is there anything else the variable could start with that could be insecure?

There are simpler and more inherently secure methods, like this:

if(preg_match($_GET['l'],'#^[a-z0-9-_]+$#i') && file_exists('pages/' . $_GET['l'] . '.php')) {

 include('pages/'.$_GET['l'].'.php');

} else {

 die();

}

[leebow]

Thank you so much! That is perfect. I'll update my site now :)

Thanks again for the help.

edit:

Not really understanding regular expressions, I found out that the pattern and the string were the wrong way round in your example - and I had to change the pattern to this:

if(preg_match('/[0-9a-z-]/i',$loadPage) && file_exists('pages/' . $loadPage . '.php')) {

Thanks again for the help! I'm hoping this has made my site that little bit more secure.

[robzilla]

Oops, sorry! I always mix those up.

Your new code isn't secure, though, as your regex only checks to see if any of those characters are contained within the $loadPage string, so something like "../config" will still match because there's a "c" in it (and other letters from a-z). As in my example, you need the ^ in front, indicating the start of the string, and the $ at the back for the end of the string. You also need a quantifier (+ for 1 or more occurrences or * for 0 or more occurrences). That way, you're saying $loadPage may contain only the characters a-z, 0-9 and - from the start of the string to the end, with no exceptions.

if(preg_match('/^[0-9a-z-]+$/i',$loadPage) && file_exists('pages/' . $loadPage . '.php')) {

It's good to read up on regular expressions. It doesn't take much time to learn the basics (Google "regexone" for a tutorial I used some years ago) and it's a powerful thing to have in your toolbox. A regex tester like RegexBuddy (there are lots of online alternatives, as well) is also helpful.