Is this code still safe

[cloudll]

Hi guys, I have been using the same code for years now to include my default page and pull content into my layouts.

 

I found the code online and its a bit confusing so was just wondering if its still safe to use, and is it all needed nowadays?

or is there a simpler way i could be doing this?

 

Thanks for any help

<?php

    if (isset($_GET['nav'])) 
{
    if (strpos($_GET['nav'], "/")) 
{
    $direc = substr(str_replace('..', '', $_GET['nav']), 0, strpos($_GET['nav'], "/")) . "/";
    $file = substr(strrchr($_GET['nav'], "/"), 1);
    if (file_exists($direc.$file.".php")) 
{
    require($direc.$file.".php");
} else {
    require("error.php");
}
} else {
    if (file_exists(basename($_GET['nav']).".php")) 
{
    require(basename($_GET['nav']).".php");
} else {
    require("error.php");
}
}
} else {
    require("links.php");
}
?>

[requinix]

It's not safe. With some trial and error and a fair bit of effort, I can construct a nav value that will include any PHP file I want. If I don't want to expend that effort, I can simply pass nav=index (or whatever name) and crash your code because the file will keep including itself until PHP runs out of stack space.

 

I'm sure there's a simpler way. What files do you need to support? Just the current directory? Any directory? Will keeping a list of valid filenames be a problem?

Edited October 19, 2014 by requinix

[cloudll]

Oh wow, didn't realise it was so bad the only files i have been including with it and .html pages or .php pages.

And I have only really been including files from the root directory.

 

I dont have too many pages so a list of valid pages would probably be okay, is that what you would reccomend?

I'm not too sure how I would do it, would it be an array?

 

Thanks

[requinix]

Oh wow, didn't realise it was so bad

It's not so bad. It tries to be good. But there's an (exploitable) bug in how it deals with filenames. And checking that the file doesn't include itself is often overlooked in code like this.

 

I dont have too many pages so a list of valid pages would probably be okay, is that what you would reccomend?

I'm not too sure how I would do it, would it be an array?

Yes. An array would be easiest. Code looks like

<?php

$files = array(
	'file1',
	'file2',
	'dir1/file',
	'dir2/file'
);

if (isset($_GET['nav'])) {
	if (in_array($_GET['nav', $files)) {
		require $_GET['nav'] . '.php';
	} else {
		require 'error.php';
	}
} else {
	require 'links.php';
}

If keeping the list of valid files isn't a problem then this is the best solution: the code is only capable of including files that you allow. Without the array (or something equivalent, like filenames in a database) you have put logic into the code to make sure it only works on the right files. But if there's a bug in the logic then the security goes out the window.

[cloudll]

Ahh I see, thanks for the code

[Psycho]

Also, since you are only including those files as opposed to having a user load them directly passing actual file names is typically a bad idea. It gives a malicious users information about your application that they could try any use to exploit the system. So, I would suggest giving each file an "label" that you would pass in the URL and use that to reference the file. Plus, you could ditch the error condition. If the user doesn't pass a valid value, just direct them to a default page.

 

 

$files = array(
    'home' => 'index.php',
    'contact' => 'contact.php',
    'products' => 'dir1/products.php',
    'services' => 'dir2/services.php'
);
 
if (isset($_GET['nav']))
{
    if (isset($files[$_GET['nav']])) {
        require $files[$_GET['nav']];
    } else {
        require 'error.php';
    }
} else {
    require 'links.php';
}