5
\$\begingroup\$

I'm not that experienced with PHP and I'm trying to make it so when I load a page, instead of directly browsing to the file, it will be "included" instead.

I have the code finished and it works. I just wanted to get an opinion of if it's safe (will people be able to exploit it in anyway?) and if I'm going about this the right way.

<?php
error_reporting(0); // I'm not expecting any errors and don't want users to see any.

    $page = strtolower($_GET['page']); // File names will always be lowercase

    if (isset($page))
    {
        $page .= ".php";
        $page = basename($page); // Prevent users from entering a directory path to access other files.


        if ((file_exists($page)) && ($page != "index.php")) // Check if exists and prevent infinite include loop.
        {
            die(include($page));
        } 
    }

    include("base.php");
?>
\$\endgroup\$
4
  • \$\begingroup\$ what prevents a user to access /directory1/directory2/file2.php - is this code part of a routing code? \$\endgroup\$
    – azngunit81
    Commented Mar 18, 2014 at 16:10
  • \$\begingroup\$ @azngunit81 $page = basename($page); // Prevent users from entering a directory path to access other files. \$\endgroup\$
    – John Smith
    Commented Mar 18, 2014 at 16:11
  • \$\begingroup\$ yes but that doesnt answer the fact that this file is your commanding index.php or not because if this is not a routing file i can go into a directory manually www.example.com/dir1/dir2/test.php - if no routing system is done your code wont be activated unless there is a check. I will obviously bounce if such directory is not present of course \$\endgroup\$
    – azngunit81
    Commented Mar 18, 2014 at 18:11
  • \$\begingroup\$ @azngunit81 It is a routing file. I'm using get_included_files() within the other PHP scripts to ensure that there is no direct access. \$\endgroup\$
    – John Smith
    Commented Mar 18, 2014 at 19:38

2 Answers 2

1
\$\begingroup\$

As it uses the basename function it should be safe enough.

One note though: I would personally always have a lookup table (PHP "array") for whitelisting files unless there is a very specific reason not to do so. That way I will have full control and the result should be the same for the user - as a side effect it is way way more easy to have shorter names and/or aliases for files later on.

\$\endgroup\$
1
\$\begingroup\$

Ok with all that routing question out of the way - I agree with mbanzon with basename being fine to check for internal directory structure to block people out...

If you want extra security you can run a short alpha numeric check to make sure nothing funny is included.

As for the white list - since this is a routing file and a static one a white list can also help speed things up as long as its minimal, one problem though with that is if you file system becomes extensive it maybe best to not have a whitelist at that point (you dont want your lookup to be too large to maintain and add.

Side note: if your looking for interesting ideas symfony2 routing is pretty good (its being use at core with laravel). It is more extensive than your solution of course but still interesting to look at.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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