Am I missing any security loopholes in this php script that reads and outputs directory/file from $_GET variable

Question

Due to a weird server setup on one of my client's websites, I needed to setup a script to load referenced pdf files through a case-insensitive lookup. We originally looked into mod_speling, but it was causing issues with other mod_rewrite declarations. I'm not entirely sure why, but I didn't have much control over those other declarations, since they had other people working on their site as well.

So anyway, they have a bunch of references to their PDF files all over the internet with a bunch of different capitalization. Realizing that getting all of these to switch over to the proper file name was gonna be nearly impossible, I opted to write a script to do the translation for me.

The script forces http://www.somedomain.com/somedirectory/subdirectory/anypdffile.pdf to load through my pdffiles.php file.

The script seems to work perfectly. I'm not too worried about server performance, as I don't think it's gonna be used all that often. I just wanted to get a second pair of eyes to make sure I didn't leave any major security loopholes in here since I'm reading a directory/file from a $_GET variable.

First I added this to the .htaccess file to redirect all pdf files to a php script

RewriteEngine on
RewriteBase /
RewriteCond %{REQUEST_URI} \.pdf$ [NC]
RewriteRule (.*) pdffiles.php?p=$1

Then put the following code in the pdffiles.php file:

<?php

// shorthand for our docroot
$root = $_SERVER['DOCUMENT_ROOT'];

// get our filepath (correct format is "somedir/otherdir/xyz.pdf")
$path= $_GET['p'];


// Rule 1 - Don't allow empty path
if (!empty($path)) {
    // get the directory for our file
    $dir = dirname($root.'/'.$path);

    // get the filename
    $file = basename($path);

    // get our extension
    $ext = pathinfo($file, PATHINFO_EXTENSION);

    // Rule 2 - Don't allow direct access to the script
    if (!strstr($_SERVER['REQUEST_URI'], 'pdffiles.php')) {

        // Rule 3 - Ensure PDF extension
        if (strtolower($ext) == "pdf") {

            // Rule 3 - Make sure document root + path is a real path
            if (($dir = realpath($dir))) {

                // Loop through directory contents
                if ($fh = opendir($dir)) {
                    while (false !== ($tfile = readdir($fh))) {

                        // ignore the default directories
                        if ($tfile != "." && $tfile != "..") {

                            // convert both to all lower case and try to match
                            if (strtolower($tfile) == strtolower($file)) {

                                // if they match then go ahead and output file
                                header("Content-type: application/pdf");
                                readfile($dir.'/'.$tfile);
                                exit; 

                            }
                        }

                    }
                }

            }
        }

    }
}

// we failed one of our checkpoints, redirect to 404
header("HTTP/1.1 301 Moved Permanently"); 
header("Location: /notfound.html");
?>

Answer 1

As Jason stated checking for ".." is a good idea. If there are only a limited number of path where the pdfs are located on the server, I would even prefer matching against this set.

Nevertheless, you might also want to invert your ifs to get rid of the nesting and use guard conditions.

<?php
    // !!!! comment intention, not what your code is doing

    // Don't allow direct access to the script
    if (strstr($_SERVER['REQUEST_URI'], 'pdffiles.php')) notFound();

    // expected format is "somedir/otherdir/xyz.pdf"
    $path= $_GET['p'];
    if (empty($path)) notFound();

    $root = $_SERVER['DOCUMENT_ROOT'];
    $dir = realpath(dirname($root.'/'.$path));
    if ($dir===FALSE) notFound();
    $file = basename($path);
    $ext = pathinfo($file, PATHINFO_EXTENSION);
    if (strtolower($ext) != "pdf") notFound();

    if ($fh = opendir($dir)) {
        while (false !== ($tfile = readdir($fh))) {
           if ($tfile == "." || $tfile == "..") continue;
           if (strtolower($tfile) != strtolower($file)) continue;

           header("Content-type: application/pdf");
           readfile($dir.'/'.$tfile);
           exit; 
        }
    }

    function notFound() {
         // we failed one of our checkpoints, redirect to 404
         header("HTTP/1.1 301 Moved Permanently"); 
         header("Location: /notfound.html");
         exit();
    }
// ? > !!!! omit this. It's not necessary and you will have no empty line in you php files.

Maybe you want to also have a look at the DirectoryIterator