PHP download counter script with htaccess

Question

I'd like to show you my script and would like to hear your security hints. Maybe the code is also useful to someone else.

What I want to archive:

1. Someone clicks on a link on my web page
2. a htaccess file looks for some file-extensions and if the requested file is one of these, it will be counted.
3. After the count (done with PDO prepare), the file is proceed to the user.

Because I want to get maximum security and not offering someone other files than from the upload folder I ask before doing it wrong.

.htaccess:

RewriteEngine on
RewriteRule ^(.*).(rar|zip|pdf|tar|apk|rpm|exe)$ /download.php?file=$1.$2 [L,NC]

download.php

<?php

// Allowed extensions
$filetypes = array("rar", "zip", "pdf", "tar", "apk", "rpm", "exe");

// Details of requested file
$file_path  = $_REQUEST['file'];

// NullByte cleaner
$file_path = str_replace(chr(0), '', $file_path);

// Split details
$path_parts = pathinfo($file_path);
$file_name  = $path_parts['filename'];
$file_ext   = $path_parts['extension'];

// CleanUp
$file_name  = preg_replace('/[^\0-9_a-zA-ZäöüÄÖÜ]+/', '', $file_name);
$file_ext  = preg_replace('/[^\0-9a-zA-Z]+/', '', $file_ext);

// Create path
$dir_path = $_SERVER['DOCUMENT_ROOT']."/";
$finalpath  = $dir_path . $file_name . "." . $file_ext;

// Allowed extension?
if ( in_array($file_ext, $filetypes) ) {

    // Does the file exist?
    if ( is_file($finalpath) ) {

        // Final security check
        setlocale(LC_ALL,'en_US.UTF-8');
        $local_ext = pathinfo($finalpath, PATHINFO_EXTENSION);

        // Allowed local extension?
        if ( in_array($local_ext, $filetypes) ) {

            // Logging
            // ..........

            // Download
            header('Content-Description: File Transfer');
            header('Content-Type: application/octet-stream');
            header('Content-Disposition: attachment; filename=' . basename($finalpath));
            header('Content-Transfer-Encoding: binary');
            header('Expires: 0');
            header('Cache-Control: must-revalidate, post-check=0, pre-check=0');
            header('Pragma: public');
            header('Content-Length: ' . filesize($finalpath));
            ob_clean();
            flush();
            readfile($finalpath);
            exit;
        }

    }

}

// Something went wrong
echo "Error...";
exit;

Answer 1

You are doing a lot of things right. You are using a whitelist instead of a blacklist for extensions, you are using pathinfo to get the extension, you clean nullbytes (optional, but still good practice), you use basename in the filename (also optional), you have a strict whitelist for filenames (theoretically), you use a good content type, and so on.

There is one issue, and two small suggestions:

• Your filename whitelist allows all characters between ascii 0 and ascii 57, eg NUL, TAB, !, ", <, etc. Given the rest of your code, I'm assuming that this is a bug and that you only wanted to allow alphanum + umlaute. The same is true for the extension filter.
• There is pretty much never a good reason to use REQUEST. You probably know the HTTP method you are using, so you should use that. This is somewhat of an issue if the method is POST, as an attacker can downgrade that to GET, which can make exploitation of some issues easier.
• You may want to consider checking if the resolved $finalpath is inside the download directory. This isn't necessary right now, as your pathinfo approach as well as your whitelist both on their own are defense enough against directory traversal. But it is best-practice, and it may be useful if the code is reused in a different context, or if your whitelist (and pathinfo) approach need to be changed for some reason.

Regarding non-security aspects:

• Your comments do not add all that much and should probably be removed. Some can be used to create better variable names (filetypes might be allowedExtensions), others are just repeating the code, and some are somewhat unclear (what is a local extension?).
• Your ifs are a bit too nested for my taste. If you reverse the ifs they will be easier to read. If you return/die on the else case, you would also get rid of the nesting.