Admin page select function

Question

I am using this script to get the admin pages via this query string ?page= once the admin is logged in.

Just needing a review, opinions

Function:

// GET PAGE FROM QUERY STRING
public function page_select($page) {
 $ext = '.php';
 $file = 'pages/'.$page.$ext;
 if(file_exists($file)) {
 return $file;
 }
 else {
 $file = 'pages/404'.$ext;
 return $file;
 }
}

Index.php

if(isset($_COOKIE["Cckiuas"]) && isset($_SESSION["loggedin"])) {
 if(isset($_GET['page'])) { $page = trim(htmlentities($_GET['page'])); }
 else { $page = 'dashboard'; }
 include($backend->get_base_url().INCLUDES."header.php");
 include($backend->page_select($page));
 include($backend->get_base_url().INCLUDES."footer.php");
}

Answer 1

Let's start with the critical issues:

• You are vulnerable to directory attacks, I can get you to include any file by using the ../ to traverse up the directory structure, revealing details on your system:

  example.com/index.php?page=../../../../../../../etc/passwd%00
  

  The %00 represents the null byte character (See Null Byte Injection ), which will terminate the string (thus ignoring your .php suffix). If I input that, I got all the users and passwords on the server (assuming the PHP user has read access, which it shouldn't, but most likely has).

• The $ext variable is redundant, are you by any chance going to stop using .php extension even though you won't stop using PHP? (Because, you know, the code is PHP)

• htmlentities is pointless if you aren't placing the target string in HTML. (And even if you were, htmlspecialchars is probably better)

• INCLUDES is magic. Immutable global variables (like constants) are still global!

• Consistent spacing and line-breaks - If you started with multiline in functions and control structures, stick with it.

• CodeX: Ok, so it would be better to call / link the page as normal for example /page.php and then pass a query to it, ie. ?action=manage ? The idea behind this btw was to only call header and footer once with a side menu and just include the page content needed, for example data tables
• Madara's Ghost: The best thing is a very specific whitelist. (If the route matches this, it goes there). Don't try to do this part dynamically, at least until you know what you are doing.
• CodeX: Great advice, I'm changing it to a straight link to the page and then I'll use query strings to call specific functions within the page dependant on the action
• CodeX: The above would be similar to how wordpress does it btw

Answer 2

I can't say much on the functionality, but I got a small comment on your if-statements:

Be consistent:

In your "Function" you have the blocks clearly cut out (linebreak & indention), In your "index.php" they are inlined.

I suggest you always do it like in your "Function":

if(isset($_COOKIE["Cckiuas"]) && isset($_SESSION["loggedin")) {
 if(isset($_GET['page'])) {
 $page = trim(htmlentities($_GET['page']));
 }
 else {
 $page = 'dashboard';
 }
 //includes here
}

That btw. also applies for your usage of single and double quotes. Compare:

isset($_COOKIE["Ccuias"])
isset($_GET['page'])

Decide on one

• CodeX: Are there any security concerns doing this?
• Vogel612: @CodeX "I can't say much on the functionality." My PHP is quite bad. in fact I don't actually use PHP anywhere. it's just a few bits I caught here and there. For security you'll have to wait on another review.