Page 1 of 1

Safe include practices

Posted: Wed Sep 20, 2006 8:40 pm
by klarinetking
Hi,

Just a quick question. Would you consider this method of including files safe? If not, what would you suggest?

Code: Select all

<?php
$pageIncPath = './inc/pages/';
$rootPath = './';

if ( isset($_GET['p']) && !empty($_GET['p']) )
{
	if ( is_file($pageIncPath . $_GET['p'] . '.php') )
	{
		include ($pageIncPath . $_GET['p'] . '.php');
	}
	else
	{
		include ($pageIncPath . 'index.php');
	}
}
else
{
	include ($pageIncPath . 'index.php');
}
?>
Thanks for any and all comments.

klarinetking

EDIT: How much safer would it be if I were to use defines?

Code: Select all

<?php
define('PAGE_INC_PATH', './inc/pages/');
$rootPath = './';

if ( isset($_GET['p']) && !empty($_GET['p']) )
{
	if ( is_file(PAGE_INC_PATH . $_GET['p'] . '.php') )
	{
		include (PAGE_INC_PATH . $_GET['p'] . '.php');
	}
	else
	{
		include (PAGE_INC_PATH . 'index.php');
	}
}
else
{
	include (PAGE_INC_PATH . 'index.php');
}
?>

Posted: Wed Sep 20, 2006 9:00 pm
by neophyte
Answer with a question: What might happen if someone includes a file that exists on your server but wasn't the one you had in mind. Would that cause problems to the rest or your system? Would it introduce new security risks?

Re: Safe include practices

Posted: Wed Sep 20, 2006 9:00 pm
by alex.barylski
Depends on how you define "safe"???

Anytime you use dynamic strings inside a include or require your asking for security problems, such as people somehow including files not meant to be included, which could:

- Expose passwords
- Over write globals

AFAICT your code doesn't perform any kind of directory verification...

Code: Select all

$pageIncPath = './inc/pages/'; 

        if ( is_file($pageIncPath . $_GET['p'] . '.php') ) 
        { 
                include ($pageIncPath . $_GET['p'] . '.php'); 
        } 
        else 
        { 
                include ($pageIncPath . 'index.php'); 
        }
You check if it's a file your including which is great, but what if someone passes a $_GET['p'] with the value:

Code: Select all

$p = '/../../../path/to/htacess/or/htpasswd';
There is alot that could wrong now, such as potentially executing code (if it's inlined and is why it's best to pack as much as possible inside functions of classes) or gaining access to your htaccess or htpasswd files...

You need to epxand the path's using realpath() or similar and start checking it's base directory ensuring it's allowed...

My CMS: http://www.sourceforge.net/projects/texocms/ has functions which check just this, you could use that code as an example...

Cheers :)

Posted: Wed Sep 20, 2006 9:12 pm
by klarinetking
Hmm... thanks for the great replies. I'll fool around with this some more, and post any revised versions.

Thanks,

klarinetking

Posted: Wed Sep 20, 2006 9:31 pm
by klarinetking
Ok, I think I have a better system now. Here's the new code. If you can spot any issues, please let me know. I'm still looking at some other functions; see if they'll be useful.

Code: Select all

<?php
define('pageIncPath', './inc/pages/');
$rootPath = './';

if ( isset($_GET['p']) && !empty($_GET['p']) )
{
	$file = str_replace('.', '', $_GET['p']);
	$file = str_replace('/', '', $file);
	
	if ( dirname(pageIncPath . $file . '.php') != './inc/pages/' )
	{
		include (pageIncPath . 'index.php');
	}
	
	if ( is_file(pageIncPath . $file . '.php') )
	{		
		include (pageIncPath . $file . '.php');
	}
	else
	{
		include (pageIncPath . 'index.php');
	}
}
else
{
	include (pageIncPath . 'index.php');
}
?>
Thanks again,

klarinetking

Posted: Wed Sep 20, 2006 10:32 pm
by alex.barylski
str_reaplce is only handling . and /

From what I can tell by quickly glancing at your code it's more secure but still not ideal IMHO

Code: Select all

$file = str_replace('.', '', $_GET['p']); 
$file = str_replace('/', '', $file);
Test: $p = 'mytest/test.php';

Output:

mytesttestphp which is now a broken file path and will choke your script when file_exists() is called, so yes it's secure in that regard...but it still is not ideal.

Also consider that someone may be able to escape the file path delimiters like '.' or '/' in fact, I'm not sure but a user could possibly use '\' instead of '/' and acheive the same results on Windows servers at least. As a path seperators can be either forward or backward slashes...

And then, there is the issue of specially encoding characters, I'm not sure if this is possible, but like SQL injection attacks use mysql_escape_quotes instead just escaping the quotes...it's possible a similar attack could be carried out on your string for file paths...

Perhaps the resident multi-language expert AC could shine some light on how PHP deals with international strings as I'm sketchy myself...

But if it's possible to encode a period or slash as an HTML entity or similar, that would easy bypass your security checks if PHP knows or can be tricked into handling international strings (not sure by I'm guessing setlocale() it changes how PHP handles strings???)

for instance lets pretend the internatinal code for period or slash in Farsi is as such: &7366; and the user can then somehow set the PHP locale to Farsi

your scripts assume ASCII period and slashes, and completely ignore the multi-byte coded periods/slashes, so imaginary data input as follows:

&7364; = Slash
&83476; = Period
Test: $p = 'mytest&7364;test&83476;php';

Would reach the PHP include function, but I *think* if the user can also set the locale to Farsi, then PHP include would then be capable of including again, whatever the attacker wanted to inject

Test: $p = 'mytest&7364;&83476;&83476;&7364;&83476;&83476;&7364;test&83476;php';

Ignored by your script, completely legitimate attack - I think...this is atleast something worth investigating as a truth because it's I belive the gist of the idea behind SQL injection attacks, using international codes instead of English ASCII codes...

I don't think I considered that in my own code either...I'll have to look into this :P

Cheers :)

Posted: Wed Sep 20, 2006 10:51 pm
by klarinetking
Hmm... all right. Looks like I have a bit more research to do. Also in regards to your statement
Test: $p = 'mytest/test.php';

Output:

mytesttestphp which is now a broken file path and will choke your script when file_exists() is called, so yes it's secure in that regard...but it still is not ideal.
The way I've designed my system is that the URI param is only the basename of the file. So, the only types of requests that should be accepted are things like (?p=index, edit, etc)

But thank you for your insight on character encoding. I'll look into it and see what I can come up with.

Thanks again,

klarinetking

Posted: Wed Sep 20, 2006 10:58 pm
by alex.barylski
klarinetking wrote:Hmm... all right. Looks like I have a bit more research to do. Also in regards to your statement
Test: $p = 'mytest/test.php';

Output:

mytesttestphp which is now a broken file path and will choke your script when file_exists() is called, so yes it's secure in that regard...but it still is not ideal.
The way I've designed my system is that the URI param is only the basename of the file. So, the only types of requests that should be accepted are things like (?p=index, edit, etc)

But thank you for your insight on character encoding. I'll look into it and see what I can come up with.

Thanks again,

klarinetking
Indeed you should investigate and get back to me...not sure if what I have described is possible it just kinda sprang to mind :P

Not sure it would be a security risk either, as the user would first need the ability to set the locale.

URL encoding characters such as %20 though, you should see what happens when you pass in a SLASH or PERIOD that way too, cuz I don't think that would require any locale settings.

Ideally, if all you absolutely ever need or want is a single file name you could do something like:

Code: Select all

$name = basename(realpath($_GET['p']));
That will expand *any* relative or canonical paths into absolute goodness and drops everything except the filename and it's extension, which could easily be stripped off leaving you with *nothing* but what you want...

The considerations I mentioned above are more targeted towards advanced file operations needed in my CMS, in which case many checks were nedded, you shoudl be able to get away with what I've shown just now ;)

HTH

Cheers :)

Posted: Thu Sep 21, 2006 3:43 pm
by feyd
You know, I seem to remember a thread about this some time ago... hmm where was it?

Oh, that's right, it was linked from Useful Posts. :?