# Am I missing any security loopholes in this php script that reads and outputs directory/file from $_GET variable 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);

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
readfile($dir.'/'.$tfile);
exit;

}
}

}
}

}
}

}
}

// we failed one of our checkpoints, redirect to 404
?>

-

## migrated from stackoverflow.comJan 22 '13 at 4:50

This question came from our site for professional and enthusiast programmers.

Ack, sorry, didn't realize there was a code review section. Can I switch it? –  David Stinemetze Jan 21 '13 at 20:50
looks good to me –  cubsink Jan 21 '13 at 20:50
A mod can do it for you. Just flag it and they'll do the rest. –  John Conde Jan 21 '13 at 20:51
K. Thanks @JohnConde. My bad. –  David Stinemetze Jan 21 '13 at 20:51
Not sure if it's an exploitable issue, but what would happen if someone set p to a relative path like ../cgi/example.pdf? I can't forsee any issues though as the pdf extension check will stop you mistakenly serving up internal files. –  Jason Larke Jan 22 '13 at 5:50

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

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
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

-
The concept of a "document directory" is a good one, most sites I've worked on generally store all their documents in a specific folder (with various levels of sub-folders). I'm not sure how the OP's filesystem is set up (and it sounds like he's inherited a bit of a mess), but something like this may be possible. –  Jason Larke Jan 22 '13 at 13:36
@David In addition to that you might want to check if the filenames are unique so you can drop the path completely and create the new "document directory" we recommend. –  mnhg Jan 22 '13 at 15:11
Ideally I would build the document directory, but this is kind of low budget and just a quick fix. The client didn't have the money for the time involved with doing it that way. As far rewriting the code to use guard conditions, does that actually provide any sort of benefit, other than making the code more readable? Don't get me wrong, I'm all about readable code, but if the existing code works as is, is there a point to rewriting it this way? –  David Stinemetze Jan 28 '13 at 16:46
@David The code is your realm. You have to work and live with it. Of course no customer will pay for a refactor/cleanup/test item on the invoice, you just have to include it in your estimation and just do it. Nevertheless even on a short run a cleaner, more readable and structured code will speed up your issue fixing, this means either more time for cleaning up other code or lower cost for your customer. Especially on long term projects you waste a lot of time in understanding your own code after some months. –  mnhg Jan 29 '13 at 4:57
Maybe you want to upvote or accept my answer anyway. –  mnhg Jan 29 '13 at 4:59