1
\$\begingroup\$

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");
}
\$\endgroup\$
3
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ 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 \$\endgroup\$ – CodeX Jul 2 '14 at 9:24
  • \$\begingroup\$ 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. \$\endgroup\$ – Madara's Ghost Jul 2 '14 at 10:55
  • \$\begingroup\$ 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 \$\endgroup\$ – CodeX Jul 2 '14 at 10:58
  • \$\begingroup\$ The above would be similar to how wordpress does it btw \$\endgroup\$ – CodeX Jul 2 '14 at 10:59
1
\$\begingroup\$

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

\$\endgroup\$
  • \$\begingroup\$ Are there any security concerns doing this? \$\endgroup\$ – CodeX Jul 2 '14 at 6:37
  • \$\begingroup\$ @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. \$\endgroup\$ – Vogel612 Jul 2 '14 at 6:39

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.