Page 1 of 2

Another Include...

Posted: Fri Sep 16, 2005 3:42 am
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!

Posted: Fri Sep 16, 2005 5:28 am
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?? ');
}

Posted: Fri Sep 16, 2005 6:06 am
by grkpfl
hey, thats very nice :roll:
THANKS!

Posted: Tue Sep 20, 2005 5:17 am
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 :)

Posted: Tue Sep 20, 2005 5:47 am
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?? ');
} 
?>

Posted: Tue Sep 20, 2005 6:25 am
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:

Posted: Tue Sep 20, 2005 6:41 am
by timvw
I would say that your code smells...

Posted: Tue Sep 20, 2005 7:20 am
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".

Posted: Tue Sep 20, 2005 7:33 am
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:

Posted: Tue Sep 20, 2005 12:18 pm
by John Cartwright
Any hacker would not reveal his true ip :twisted:

Posted: Tue Sep 20, 2005 1:12 pm
by Jenk
And any hacker would only be spurred on by any comment such as what has been suggested :p

Posted: Tue Sep 20, 2005 1:47 pm
by timvw
I don't read the output my "hack scripts" recieve.. I only test if they contain what i'm looking for ;)

Posted: Wed Sep 21, 2005 2:35 am
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.

Posted: Wed Sep 21, 2005 4:25 am
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....

Posted: Wed Sep 21, 2005 4:58 am
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. :)