5
\$\begingroup\$

I've written this simple script to force download some files (jpg, mp3, usually those which are loaded in browser by default). I was wondering whether there's any way this could be improved upon, which means:

  • making it more secure
  • making it use less cpu (filesize(), fopen(), fpassthru() are the usual suspects here)
  • making it simpler maybe

Here it goes:

<?php

if (!isset($_GET['file'])){
   die('no file requested');
}

else{

   if (substr($_GET['file'], 0, 1) == '.'){
      die('trying to leave this directory? :)');
   }

   $path = './'.$_GET['file'];
   if (file_exists($path) && is_readable($path)){
    $size = filesize($path);
    header('Content-Type: application/octet-stream');
    header('Content-Length: '.$size);
    header('Content-Disposition: attachment; filename='.$_GET['file']);
    header('Content-Transfer-Encoding: binary');
    $file = fopen($path, 'rb');
    fpassthru($file);
    exit;
   }

}

?>

Usage: /?file=sample.jpg

\$\endgroup\$
6
  • 1
    \$\begingroup\$ Before you start worrying about the microscopic amount of cpu time that filesize() will consume, you should start panicking about how this script lets a malicious user download ANY file on your server for which they know the path. Your substr test is NOT sufficient \$\endgroup\$
    – Marc B
    Commented Oct 21, 2012 at 15:35
  • \$\begingroup\$ @MarcB: please, do elaborate (why the substr check isn't enough) \$\endgroup\$
    – RiMMER
    Commented Oct 21, 2012 at 15:35
  • 2
    \$\begingroup\$ http://example.com?file=/../../../../../../etc/passwd first char is a /, not ., and file path operations will collapse // into a single /, so you've done nothing to secure things. \$\endgroup\$
    – Marc B
    Commented Oct 21, 2012 at 15:35
  • \$\begingroup\$ @MarcB: you seem to know the game, why not submit an answer? I'll be happy to upvote and accept it! \$\endgroup\$
    – RiMMER
    Commented Oct 21, 2012 at 15:36
  • \$\begingroup\$ Also, I would check for the ':' (for higher portability) and for encoded 'file://' and other schemes \$\endgroup\$
    – Serge
    Commented Oct 21, 2012 at 15:37

6 Answers 6

4
\$\begingroup\$

A small improvement I've spotted, take care to enclose the filename in double quotes to handle the case when file name contains spaces, like so:

header('Content-Disposition: attachment; filename="'.$_GET['file'].'"');
\$\endgroup\$
4
\$\begingroup\$

Always use === in your comparisons.

if (substr($_GET['file'], 0, 1) == '.'){
  die('trying to leave this directory? :)');
}

Should be:

if (substr($_GET['file'], 0, 1) === '.'){
  die('trying to leave this directory? :)');
}

Also look into other ways to sanitize $_GET['file'] properly at the moment I could pass in something like /../ and I could access other files.

From the docs you can save on a fopen() call by using readfile() instead see fpassthru()

A better way to use variables in a string is to use curly syntax so this line

header('Content-Disposition: attachment; filename='.$_GET['file']);

Would be better as

header("Content-Disposition: attachment; filename={$_GET['file']}");

I think that covers the three bullet points :)

\$\endgroup\$
4
  • \$\begingroup\$ There seems to be a problem with my current code anyway. It cannot send big files. Any idea how to resolve this? PHP Fatal error: Allowed memory size of 268435456 bytes exhausted (tried to allocate 594223105 bytes) in index.php on line 21 \$\endgroup\$
    – RiMMER
    Commented Oct 21, 2012 at 16:26
  • \$\begingroup\$ The absolute comparison rule is subjective. If you are comparing for the same type as well as value, then yes, the absolute === is better. But if all you want is to compare for a pattern, then the loose comparison is fine ==. Also, "curly syntax" is a matter of preference. Both methods are perfectly fine and it is up to the author whether they want to use one over the other. There is no difference except in subjective legibility. \$\endgroup\$
    – mseancole
    Commented Oct 23, 2012 at 17:19
  • \$\begingroup\$ Not sure I agree on the === vs == argument. If it was down to me I'd never see == in PHP code there's no need for it. Any code that is relying on == to do the comparison is probably a code smell. \$\endgroup\$ Commented Oct 23, 2012 at 19:28
  • \$\begingroup\$ They do essentially the same thing, but their purposes are as I described. Many share your opinion, many don't. That's why I said it was subjective. Here's a related SO Question/Answer and here's the official documentation \$\endgroup\$
    – mseancole
    Commented Oct 23, 2012 at 19:36
4
\$\begingroup\$

You should never accept a user to send a path, only accept file names as input, only you in the server side will know where directory go to get that file, so use basename() for that:

$_GET['file'] = basename( $_GET['file'] ); //permit only filename
\$\endgroup\$
1
  • \$\begingroup\$ $_GET['file'] is expected to be a filename and never a path, yes. \$\endgroup\$
    – RiMMER
    Commented Oct 21, 2012 at 15:51
4
\$\begingroup\$

It's much better to avoid using PHP and allow your web server to manage this. If you're using Apache, the following lines in your .htaccess file will do the trick quite nicely.

AddType application/octet-stream .jpg 
AddType application/octet-stream .mp3

This is much less server intensive and more secure.

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

what about just puting a link to that file?

for example:

<a href="/file.mp3">download file</a>

as far as i know it depends of the user configs if it will download it or use it in the browser

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

I agree with most of these other answers, there are just a few things I thought I'd point out.

I would never directly use user input. Always sanitize and validate it. You've already validated, you just need to sanitize. An easy way to do this if your PHP version is >= 5.2 is to use filter_input().

$file = filter_input( INPUT_GET, 'file', FILTER_SANITIZE_STRING );

$path = "./$file";
if( file_exists( $path ) && is_readable( $path ) ) {
    //etc...
    header( "Content-Disposition: attachment; filename=$file" );
}

You should use is_file() instead of file_exists(). The difference is that the latter doesn't assume its parameter is an actual file and will also return TRUE for directories. However, neither is actually necessary in this context. is_readable() also checks if a file exists, so this is redundant and unnecessary. Just use is_readable().

if( is_readable( $path ) ) {

Depending on the size of these files, imploding the input into an array and iterating over that will allow you to download multiple files, though at this point you will want to offer links to said files instead of automatically generating a save file dialog.

$filestring = filter_input( INPUT_GET, 'file', FILTER_SANITIZE_STRING );
$files = implode( ',' $filestring );
foreach( $files AS $file ) {
    //this is where having a function, as Jack mentioned, would be handy
}

My final suggestion. Don't use an if/else statement if you can get away with just using the if. Your if statement returns early, therefore the else statement is implied. There is no need to explicitly use an else statement and force yourself to indent your code unnecessarily. This will help you to avoid the Arrow Anti-Pattern.

\$\endgroup\$

Your Answer

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