Need Some Advice

Discussions of secure PHP coding. Security in software is important, so don't be afraid to ask. And when answering: be anal. Nitpick. No security vulnerability is too small.

Moderator: General Moderators

Post Reply
User avatar
myister
Forum Newbie
Posts: 12
Joined: Tue Sep 28, 2010 7:03 am
Location: Ohio
Contact:

Need Some Advice

Post by myister »

Ok so I am creating this project management system for myself and and everything is going good.

I need to know if there is another way to do this or if there is a way to get around this.

I am kinda trying this new simple Idea of a template system using $_GET.

So what I did was made my index page and wrote

Code: Select all

<?php $empty=strlen($_GET['page']); 
if ($empty==0) 
{ 
header ("Location: 404error.php"); 
} 
// Now that the url is clean lets make sure that the file exsists and if not send them to the 404 page........
$page = $_GET['page'];
$urlcheck = $page.".php";
if (!file_exists($urlcheck)) {
    header("Location: 404error.php");
} 

$pagetitle = $page;
?>
Works awesome but for the fact that any of the files that I am including into the index file are unprotected because i can use sessions on the or it will give an error about sessions already being started. My main link is
localhost/administrator/index.php?page=main
When I direct link to the main.php file it will open in the browser since it does not use sessions. So I added in main.php the following

Code: Select all

<?php $empty=strlen($_GET['page']); 
if ($empty==0) 
{ 
header ("Location: 404error.php"); 
} 
// Now that the url is clean lets make sure that the file exsists and if not send them to the 404 page........
$page = $_GET['page'];
$urlcheck = $page.".php";
if (!file_exists($urlcheck)) {
    header("Location: 404error.php");
} ?> 
as far as I know it is working. When I go to the file directly it does do work but when called in the include it see's itself and then displays.

How secure could this be. I imagine it aint very at all but it is working!!!?


EDIT: Ok so it does NOT work. ANYONE have any suggestions on how to secure the included files?
User avatar
myister
Forum Newbie
Posts: 12
Joined: Tue Sep 28, 2010 7:03 am
Location: Ohio
Contact:

Re: Need Some Advice

Post by myister »

Code: Select all

<?phpif(!isset($_SESSION['myusername'])){

// if the User is NOT logged in we will redirect him back to the login page first then back to here after they have entered the correct username and password
header("Location: login.php");die;
}?>
Stopped all direct access and still let the script use read the file. So far so good
User avatar
timWebUK
Forum Contributor
Posts: 239
Joined: Thu Oct 29, 2009 6:48 am
Location: UK

Re: Need Some Advice

Post by timWebUK »

DON'T take page names through GET... ever!

People can use directory traversal, if there's an upload script, execute arbitrary code. Access your servers passwd file, etc. List is endless. DO NOT do it this way.

Use a whitelist array of keywords, and associate those with file names.

Code: Select all

$whitelist = {'main', 'home', 'help'};

if(in_array($_GET['i'])){

switch($_GET['i'])
{
case 'main':

//do this
break;
}

}else
{

// 404

}
User avatar
VladSun
DevNet Master
Posts: 4313
Joined: Wed Jun 27, 2007 9:44 am
Location: Sofia, Bulgaria

Re: Need Some Advice

Post by VladSun »

Always put an exit() call after having a header('Location ... ') call. Otherwise redirecting could be (forced to be) ignored by the client.
There are 10 types of people in this world, those who understand binary and those who don't
User avatar
John Cartwright
Site Admin
Posts: 11470
Joined: Tue Dec 23, 2003 2:10 am
Location: Toronto
Contact:

Re: Need Some Advice

Post by John Cartwright »

@timWebUK: A switch statement is essentially a whitelist already, so there is no use for having an array of accepted values, then have to repeat a case for those values. Then, you can use the default case to catch invalid values.
User avatar
myister
Forum Newbie
Posts: 12
Joined: Tue Sep 28, 2010 7:03 am
Location: Ohio
Contact:

Re: Need Some Advice

Post by myister »

Ok so using $_GET is bad if you leave it open to whatever for input.
I understand the concept to using an array to only let certain titles to pass through.

This is why I did the code below

Code: Select all

<?php 
$page = $_GET['page'];
$urlcheck = $page.".php";
if (!file_exists($urlcheck)) {
    include '404error.php'; // Changed this so that the file is included in the template instead of a whole page!
}
?> 
To my understanding this only checks for a file within the current DIR which would only contain the index file and all the other files to include. (Not The config file) and if not found it would include the 404 Error page instead.

Maybe This is a bad way of doing it and arrays is just has easy to do, but I do not want to add a file name to the array every time a page is created. Could you further explain how this is a security risk if I scan the "page=" before I do anything with it.?

any other suggestions for this without having to use a template system like smarty or something? :banghead:
User avatar
greyhoundcode
Forum Regular
Posts: 613
Joined: Mon Feb 11, 2008 4:22 am

Re: Need Some Advice

Post by greyhoundcode »

File_exists can be used with various wrappers, depending on your setup:
php.net wrote:As of PHP 5.0.0, this function can also be used with some URL wrappers. Refer to List of Supported Protocols/Wrappers for a listing of which wrappers support stat() family of functionality.
So if someone passes http://www.unknowncode.ru/script within $_GET['page'] then the PHP engine may well attempt to include the output from http://www.unknowncode.ru/script.php, if that is allowed within your particular setup.
php.net wrote:If "URL fopen wrappers" are enabled in PHP (which they are in the default configuration), you can specify the file to be included using a URL (via HTTP or other supported wrapper - see List of Supported Protocols/Wrappers for a list of protocols) instead of a local pathname. If the target server interprets the target file as PHP code, variables may be passed to the included file using a URL request string as used with HTTP GET. This is not strictly speaking the same thing as including the file and having it inherit the parent file's variable scope; the script is actually being run on the remote server and the result is then being included into the local script.
User avatar
VladSun
DevNet Master
Posts: 4313
Joined: Wed Jun 27, 2007 9:44 am
Location: Sofia, Bulgaria

Re: Need Some Advice

Post by VladSun »

There are 10 types of people in this world, those who understand binary and those who don't
User avatar
timWebUK
Forum Contributor
Posts: 239
Joined: Thu Oct 29, 2009 6:48 am
Location: UK

Re: Need Some Advice

Post by timWebUK »

John Cartwright wrote:@timWebUK: A switch statement is essentially a whitelist already, so there is no use for having an array of accepted values, then have to repeat a case for those values. Then, you can use the default case to catch invalid values.
Valid point, I guess I'm just unnecessarily using processing power in my example.
Post Reply