Welcome to WebmasterWorld Guest from 35.173.48.224

Forum Moderators: coopster & jatar k

Message Too Old, No Replies

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

     
1:15 pm on Oct 8, 2016 (gmt 0)

Junior Member

Top Contributors Of The Month

joined:Nov 29, 2015
posts: 109
votes: 32


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!
2:57 pm on Oct 8, 2016 (gmt 0)

Senior Member

WebmasterWorld Senior Member 10+ Year Member Top Contributors Of The Month

joined:Sept 25, 2005
posts:2091
votes: 370


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).
4:25 pm on Oct 8, 2016 (gmt 0)

Junior Member

Top Contributors Of The Month

joined:Nov 29, 2015
posts: 109
votes: 32


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.
5:07 pm on Oct 8, 2016 (gmt 0)

Full Member

5+ Year Member

joined:Aug 16, 2010
posts:257
votes: 21


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

if (preg_match('/^[a-z]{1,10}$/i', $get[''])) { ....}
6:58 pm on Oct 8, 2016 (gmt 0)

Senior Member

WebmasterWorld Senior Member 10+ Year Member Top Contributors Of The Month

joined:Sept 25, 2005
posts:2091
votes: 370


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();

}
9:53 am on Oct 9, 2016 (gmt 0)

Junior Member

Top Contributors Of The Month

joined:Nov 29, 2015
posts:109
votes: 32


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.
3:37 pm on Oct 9, 2016 (gmt 0)

Senior Member

WebmasterWorld Senior Member 10+ Year Member Top Contributors Of The Month

joined:Sept 25, 2005
posts:2091
votes: 370


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.
 

Join The Conversation

Moderators and Top Contributors

Hot Threads This Week

Featured Threads

Free SEO Tools

Hire Expert Members