1
\$\begingroup\$

I'm trying to avoid manually updating a whitelist of pages, so I wrote the following that I might use, but I'd like my fellow, more experienced, developers to let me know if this is safe. By safe, I mean that it won't allow the inclusion or execution of arbitrary code. Is it safe from directory traversal and other similar vectors?

<?php
$current_page = 'default_page';
//Get list of acceptable pagenames:
$found_pages = [];
if ($handle = opendir('pages')) {
    while (false !== ($entry = readdir($handle))) {
        if ($entry != "." && $entry != "..") {
            $found_pages[]= $entry;
        }
    }
    closedir($handle);
}

if(isset($_GET)){
  $requested_page=reset($_GET); //just need value of first GET element, regardless of name.
  if(in_array($requested_page.'.php',$found_pages)){
    $current_page=$requested_page;
  }
}

include('pages/' . $current_page . '.php');
\$\endgroup\$
1
\$\begingroup\$

Yes, I believe that will accomplish what you are trying to do. However, I think it can also be streamlined pretty substantially. Some general pointers:

  1. Looping through the directory to get all files on all page requests is going to be pretty inefficient in the long-run. File operations are relatively expensive, because they require disk-seeks. Obviously this is less critical for a low-traffic system with a small number of files.
  2. You are returning the default page if an invalid page is requested. I think it would be better to either show an access-denied error (as such a request might signify someone trying to circumvent restrictions in your system) or at least a 404 (which would be more friendly towards legitimate users). Either way, you should stop executing when an invalid page is found.
  3. You could fix the first issues if you have a database in your system. Instead of directly checking the file path, you could record the list of allowed pages in your database, check against the DB, and then load the page in much the same way you currently are. The advantage of such a solution is that you could separate your public paths from your filenames: the database could record the "url" of the file which you lookup against, and then fetch the actual filename out of the database. Of course if you do that, you pretty much just made a basic routing system.
  4. Is there anything that prevents users from going directly to the pages? From the looks of it your pages are just a sub-directory of your public system. That may suggest room for possible security vulnerabilities.

As an alternate way of accomplishing exactly what you are doing without reading the full directory, you can use realpath. It returns the normalized absolute URL to a file, or nothing if the file doesn't exist. You can then check and see if what is returned by realpath actually lives in your application directory. That will make for fewer disk seeks and will allow your performance to remain (relatively) constant even if the number of files grows. Here is a quick example:

if(isset($_GET)){
    $pages_dir='/path/to/pages/folder/pages/';
    $requested=reset($_GET);
    $path=realpath($pages_dir . $requested);
    if(!$path || substr($path, 0, strlen($pages_dir)) != $pages_dir){
        show_404();
        exit;
    }
}

include($path);

To reiterate, you build the path assuming the file lives in the destination directory. realpath will then resolve the path, accounting for directory traversals. I.e. if $_GET contained ../../../../../../../etc/passwd then you would pass this to realpath: /path/to/pages/dir/pages/../../../../../../../etc.passwd and realpath would return simply /etc/passwd. You then check what realpath returned to see if your results are still in the proper directory. If it is, then you're good.

\$\endgroup\$
  • \$\begingroup\$ Thanks. I had heard of realpath() but hadn't caught on to how it was actually used. It sounds like it was written with this situation directly in mind. I can have default_page.php re-direct or 404 if needed. You're code will fit nicely into my project! \$\endgroup\$ – TecBrat Sep 15 '17 at 17:52
  • \$\begingroup\$ I use it for this purpose on a fairly regular basis, although I don't normally use it to normalize user input: I use it to normalize programmer input. Its easy even for programmers to make mistakes in file paths when they are needed, and a quick realpath can make sure that everything is on the up-an-up, and warn developers of potential bugs before they actually cause trouble. \$\endgroup\$ – Conor Mancone Sep 15 '17 at 17:54

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.