PHP: is this validity check vulnerable?

Question

I have this code in a page that includes other files via GET request:

$page = strtolower($_GET['page']);

if(!$page or !$allow[$page] or $page == 'home') {
    header("Location: home.php");
}

where $allow is a hardcoded array which contains a list of the allowed strings that are valid files to be included. Am I missing something obvious which would allow some code injection or is this check good enough?

Answer 1

Seems ok. Just add an exit() statement after header() to make sure that even if header() fails, your script terminates.

Answer 2

It's not vulnerable as long as register_globals doesn't allow $allow to be overwritten.

It will throw notices though, and personally I wouldn't have the case-insensitivity, so I'd do it like this:

if (empty($_GET['page']) || empty($allow[$_GET['page']]) || ($_GET['page'] == 'home'))
{
    // Technically a header location should be a complete URL - http://...
    header("Location: home.php");
    exit();
}

Answer 3

this would be enough

 $allow = array('home', 'another_one', 'blah');
 $page = isset($_GET['page']) ? $_GET['page'] : 'home';
 if(in_array($page, $allow, true))
     include "$page.php";

i'm not sure why you're using "header" there

Answer 4

"good enough" is a very wide brush. Can you expand a little? Also, if you know $_GET['page'] should be restricted to a small set of values (say, integers, or identifiers containing only lowercase letters, et cetera) then it never hurts to validate the data with a regex.

Answer 5

it might be better to do a regex on the $page firstly to ensure it meets the sanitised criteria of what can be passed in before checking further, if the contents of $page is not conforming, then do not process further...