Another Include...

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

grkpfl
Forum Newbie
Posts: 6
Joined: Fri Sep 16, 2005 3:32 am

Another Include...

Post by grkpfl »

Hi!
I createt a menu, in this menu there are allways site.php?id=xxx links.
And site.php will include $id.

but..
I did something like this:

Code: Select all

$access = "no";
if($id=="home"){$access="yes";}
elseif($id=="works"){$access="yes";}
elseif($id ... etc. etc. ....

if($access == "yes"){
include $id."php";
}else{
echo "fy";
}
Are there any security holes?

Thanks!
timvw
DevNet Master
Posts: 4897
Joined: Mon Jan 19, 2004 11:11 pm
Location: Leuven, Belgium

Post by timvw »

It seems pretty safe, but I would write that as:

Code: Select all

$allowed_pages = array('home', 'works');

if (isset($_GET['id']) && in_array($_GET['id'], $allowed_pages))
{
  include($_GET['id'] . '.php');
}
else
{
  die('Why?? ');
}
grkpfl
Forum Newbie
Posts: 6
Joined: Fri Sep 16, 2005 3:32 am

Post by grkpfl »

hey, thats very nice :roll:
THANKS!
User avatar
Jenk
DevNet Master
Posts: 3587
Joined: Mon Sep 19, 2005 6:24 am
Location: London

Post by Jenk »

Another good method, which is also secure, is to use page ID's and have a SWITCH statement, such as:

Code: Select all

<?php

switch intval($_GET['pid']) {
    case 1:
        $page = 'home.php';
        break;
    case 2:
        $page = 'works.php';
        break;
    default:
        $page = 'default.php';
        break;
}

?>
This also hides the file names of your includes :)
timvw
DevNet Master
Posts: 4897
Joined: Mon Jan 19, 2004 11:11 pm
Location: Leuven, Belgium

Post by timvw »

Well, here is another version of what i proposed (i really don't like to write long switch cases :p) Notice that it now accepts numbers 0, 1, ...

All solutions have the following in common: You define a public identifier, and then map it to the private/backend script..

Code: Select all

<?php
$allowed_pages = array('home', 'works');

if (isset($_GET['id']) && $_GET['id'] >= 0 && $_GET['id'] < count($allowed_pages))
{
  include($allowed_pages[$_GET['id']] . '.php');
}
else
{
  die('Why?? ');
} 
?>
User avatar
Jenk
DevNet Master
Posts: 3587
Joined: Mon Sep 19, 2005 6:24 am
Location: London

Post by Jenk »

:P

or:

Code: Select all

<?php

$allowed_pages = array('home', 'works');

@include $allowed_pages[$_GET['id']] . '.php' or die('Noooooooo!!');

?>
If you really wanna cut down on code :D


EDIT: Forgot the extension :roll:
timvw
DevNet Master
Posts: 4897
Joined: Mon Jan 19, 2004 11:11 pm
Location: Leuven, Belgium

Post by timvw »

I would say that your code smells...
User avatar
CoderGoblin
DevNet Resident
Posts: 1425
Joined: Tue Mar 16, 2004 10:03 am
Location: Aachen, Germany

Post by CoderGoblin »

If expecting an integer value from outside (for ID etc) I always floor the value. If the user then type in id=admin or somesuch they would then be redirected to the page id=0. In this case this could be your homepage or even a page with "Hacking on this site is not allowed. This has been logged".
User avatar
n00b Saibot
DevNet Resident
Posts: 1452
Joined: Fri Dec 24, 2004 2:59 am
Location: Lucknow, UP, India
Contact:

Post by n00b Saibot »

CoderGoblin wrote:"Hacking on this site is not allowed. This has been logged".
How about :arrow: We understand you potential as a hacker. For futher testing of your skills, we have logged your IP and the FBI is on your way. You have 5 minutes to pack up and leave you country :lol:
User avatar
John Cartwright
Site Admin
Posts: 11470
Joined: Tue Dec 23, 2003 2:10 am
Location: Toronto
Contact:

Post by John Cartwright »

Any hacker would not reveal his true ip :twisted:
User avatar
Jenk
DevNet Master
Posts: 3587
Joined: Mon Sep 19, 2005 6:24 am
Location: London

Post by Jenk »

And any hacker would only be spurred on by any comment such as what has been suggested :p
timvw
DevNet Master
Posts: 4897
Joined: Mon Jan 19, 2004 11:11 pm
Location: Leuven, Belgium

Post by timvw »

I don't read the output my "hack scripts" recieve.. I only test if they contain what i'm looking for ;)
User avatar
CoderGoblin
DevNet Resident
Posts: 1425
Joined: Tue Mar 16, 2004 10:03 am
Location: Aachen, Germany

Post by CoderGoblin »

Only a possible suggestion.. On an online game for instance this could be useful. If someone tries things like passing illegal values you could cancel their account. If you notice my first recommendation was the home page.
dreamline
Forum Contributor
Posts: 158
Joined: Fri May 28, 2004 2:37 am

Post by dreamline »

LOL... and any hacker that has been around knows doesn't get scared by texts like the ones stated, unless it's a GOV site or something like that..... hahahahahh....
User avatar
Jenk
DevNet Master
Posts: 3587
Joined: Mon Sep 19, 2005 6:24 am
Location: London

Post by Jenk »

CoderGoblin wrote:Only a possible suggestion.. On an online game for instance this could be useful. If someone tries things like passing illegal values you could cancel their account. If you notice my first recommendation was the home page.
The problem with this idea, is you need to be able to compensate for those who accidentally pass illegal or invalid values. For instance, if someone tries to open a broken link, the values will appear illegal - in this case you wouldn't want to ban them, just present an error page and redirect to the login page or something :)

IMO, the best practice is to only lock an account when someone fails to login via input boxes/forms. All else, just give a safe error message and redirect. :)
Post Reply