Page 1 of 1

Stupid question [pleas look at this]

Posted: Wed Jan 04, 2006 9:41 pm
by spamyboy
Here is what I use now

Code: Select all

<?
    $page=$_GET['page'];

    $id = array(
    '' => 'news.php',
    'downloads' => 'downloads.php',
    'news' => 'news.php',
	'about' => 'about.php',
	'lost' => 'lostpw.php',
	'radio' => 'radio.php',
	'lessons' => 'lessons.php',
	'login' => 'login.php',
	'register' => 'register.php',
    'info' => 'data/userinfo.php',
	'logout' => 'data/logout.php',
    );

    if (!isset ($id[$page])){
    echo "ss";
    }
    else {
    @include ($id[$page]);
    }
    ?>
And here is what geve me Maugrim_The_Reaper:

Code: Select all

<?php $page = $_GET['id'];

if(!ctype_alnum($page)) // check whether the ID contains only letters and numbers
{
    trigger_error('Error: Invalid Page ID passed as URL parameter. This may have been a hacking attempt!', E_USER_ERROR); // create a custom error message and exit the script (E_USER_ERROR is a custom fatal error)
}

require('/scripts/' . $page . '.php');
 ?>
Which one is beter and how to use the second e.g. ?
maybe you can ugest somethiing better ! ?

Posted: Wed Jan 04, 2006 10:21 pm
by timvw
differences:
- first script only allows a fixed set of files / doesn't expose real filenames
- first script uses include / second script uses require

I would go for something like this (untested)

Code: Select all

<?php
$default = 'index';
$base_dir = '/var/www/files/';

if (isset($_GET['page']) {
 $path = $base_dir . $_GET['page'] . '.txt';
 // make sure file exists, is readable and that it comes from the directory where i want it to come from
 if (file_exists($path) && is_readable($path) && substr(realpath($path), 0, strlen($base_dir)) == $base_dir) {
  include $path;
 } else {
  include $base_dir . $default . '.txt';
 }
}
?>

Posted: Thu Jan 05, 2006 4:18 am
by Maugrim_The_Reaper
That was me I recall...

I was pointing out to someone who replied that the fixed list was a good idea for small sites, but that an unprotected dynamic version (where $_GET['id'] was not filtered) could be exploited for path traversal or worse, code injection.

Your original version was I recall just fine - no errors or problems. ;)

Posted: Thu Jan 05, 2006 5:17 am
by spamyboy
so this script contains errors ?

Posted: Thu Jan 05, 2006 6:22 am
by Chris Corbyn
spamyboy wrote:so this script contains errors ?
Your version looks fine... I think that's what Maugrim_the_Reaper was saying. Because you're actually checking a predefined list it's secure enough anyway :)

NOTE : Please can you try to name your thread subjects a little better. For example: "Compare these two scripts"

Posted: Thu Jan 05, 2006 7:52 am
by Maugrim_The_Reaper
No errors in your script.

Sorry if I gave that impression - it was directed at someone else completely who replied to your post. They recommended another version which did contain a security exploit - your script contains none since you utilise a predefined list and a comparison against the user input (you don't use the input directly).

Posted: Thu Jan 05, 2006 9:42 am
by spamyboy
NOTE : Please can you try to name your thread subjects a little better. For example: "Compare these two scripts"
Yes, sure and sorry :)