[SOLVED] PHP: www.domain.com/index.php?page=xxxxx security

Snark1994 · 12-13-2011, 10:59 AM

I was wondering how safe urls of the form "www.domain.com/index.php?page=xxxxx" are. I don't want something which could be exploited by giving values such as 'index.php' or '../../etc/passwd' or whatever. Is what I have below secure?

PHP Code:



<?php 
$pages = array ( 
    'main' => 'main.php', 
    'aboutus' => 'aboutus.php', 
    'foo' => 'bah.php' 
    ); 
if(!isset($_GET['page']) || empty($_GET['page'])){ 
    $page = 'main'; 
} else { 
    $page = filter_var($_GET['page'],FILTER_SANITIZE_URL); 
    if(!array_key_exists($page,$pages)){ 
        $page = 'main'; 
    } 
} 
include('pages/' . $pages[$page]); 
?>

I can't think of any way to exploit that, even if the attacker were aware of the source code, but wanted to check...

Thanks!

Noway2 · 12-13-2011, 02:50 PM

Having spent the last several minutes reading the code, it looks OK to me. Lets break it down:
1) you define a $pages array with the keys: main, aboutus, foo.
2) If the superglobal page does not exist or is null, you set it to main.
3) if it does exist and is not null, you "filter" it, which removes all characters except letters, digits and $-_.+!*'(),{}|\\^~[]`<>#%";/?:@&=.
4) you then match the remaining data against the set of keys. This step is the critical part and you are actually white listing by verifying supplied data against a set of known test cases, which is the correct action. You do not "process" on any supplied data.
5) once you have conclusively set $page, either by matching an array key, you include the appropriate page.

As I said, I think this code will work as intended. I am wondering, though, why go through these convoluted steps instead of just using a case statement that matches main, aboutus, foo, or sets default (main)? and includes the appropriate line?

Snark1994 · 12-13-2011, 03:23 PM

Thanks!

Um... That would work too, and is probably far more readable.

Noway2 · 12-14-2011, 08:45 AM

I was thinking of this thread this morning and I had an epiphany about an advantage to your approach. Your approach will let you pass in an array of sites and keys to a function and using this array, filter the results and return the correct page. If you have a lot of pages, code reuse of a tested algorithm would be better than recreating a hard coded case statement many times. I am not sure if you can pass parameters into a case block or not and I am not envisioning it working....