download script

Coding Critique is the place to post source code for peer review by other members of DevNetwork. Any kind of code can be posted. Code posted does not have to be limited to PHP. All members are invited to contribute constructive criticism with the goal of improving the code. Posted code should include some background information about it and what areas you specifically would like help with.

Popular code excerpts may be moved to "Code Snippets" by the moderators.

Moderator: General Moderators

Post Reply
matthijs
DevNet Master
Posts: 3360
Joined: Thu Oct 06, 2005 3:57 pm

download script

Post by matthijs »

A simple download script used to let people download files of type pdf, doc or other. I use it by supplying the script an array of files which are downloadable. And then in the directory with the files I place a htaccess file to disallow access, so that all downloads pass this script.

Code: Select all

<?php

//download files
$files = array("some.pdf",
               "another.pdf",
               "yetanother.pdf",
               "some.doc",
               "another.doc",
               "yetanother.doc"
			   );
				   
if (isset($_GET['download_file']) && in_array($_GET['download_file'], $files)) 
{ 

  $file = basename($_GET['download_file']);
  
  $path = $_SERVER['DOCUMENT_ROOT'] . "/download/"; 
  $fullPath = $path . $file;

  if (!file_exists($fullPath) OR !is_readable($fullPath)) {
    echo "File doesn't exist or is unreadable!";
    exit;
  }
  
  if ($fd = fopen ($fullPath, "r")) {
      $fsize = filesize($fullPath);
      $path_parts = pathinfo($fullPath);
      $ext = strtolower($path_parts["extension"]);
  
      header("Pragma: public");
      header("Expires: 0");
      header("Cache-Control: must-revalidate, post-check=0, pre-check=0");

      switch ($ext) {
          case "pdf":
          header("Content-type: application/pdf"); 
          header("Content-Disposition: attachment; filename=\"".$path_parts["basename"]."\""); 
          break;
          case "doc":
          header("Content-type: application/doc");
          header("Content-Disposition: attachment; filename=\"".$path_parts["basename"]."\"");
          break;
          default;
          header("Content-type: application/octet-stream");
          header("Content-Disposition: filename=\"".$path_parts["basename"]."\"");
      }
      header("Content-length: $fsize");
      header("Cache-Control: private",false);
      while(!feof($fd)) {
          $buffer = fread($fd, 2048);
          echo $buffer;
      }
  }
  fclose ($fd);
   exit;
}
elseif ( isset($_GET['download_file']) && !in_array($_GET['download_file'],$files  )) 
{
   die('Invalid File'); // or change to some more usefull error message ...
}
?>
Part of the reason I show the code here is the new PDF XSS vulnerability which has been discovered. Which makes any pdf file on any site useable as XSS vector. So I tried it with this script, but whatever I try to add to the URL it doesn't seem to be successful.

Any criticque is welcome. Thanks.
User avatar
Kieran Huggins
DevNet Master
Posts: 3635
Joined: Wed Dec 06, 2006 4:14 pm
Location: Toronto, Canada
Contact:

Post by Kieran Huggins »

Not critique on your code (which I really haven't read), but couldn't you just use a mod_rewrite rule to strip the query string from a PDF request?
matthijs
DevNet Master
Posts: 3360
Joined: Thu Oct 06, 2005 3:57 pm

Post by matthijs »

Kieran Huggins wrote:Not critique on your code (which I really haven't read), but couldn't you just use a mod_rewrite rule to strip the query string from a PDF request?
Could you elaborate on what you mean exactly? To strip out everything after the .pdf extension you mean?
User avatar
pickle
Briney Mod
Posts: 6445
Joined: Mon Jan 19, 2004 6:11 pm
Location: 53.01N x 112.48W
Contact:

Post by pickle »

Pretty straightforward, but I noticed a couple things:

You first check that the file exists & is readable, you then do another check on whether fopen() succeeds. Don't they pretty much give you the same thing? You should be able to eliminate the file_exists()/is_readable() check.

Also, to simplify things, rather than using all the different file functions (feof(), fclose(), etc), just use fopen() to see if you can read the file, then use readfile() to dump the contents of the file.

Also, if this will be used to dump more than the 2 document types you've got, I'd be tempted not to use the switch, but rather set up an array of extension=>mime type. Then you won't have to repeat some of that header() code for each type.
Real programmers don't comment their code. If it was hard to write, it should be hard to understand.
matthijs
DevNet Master
Posts: 3360
Joined: Thu Oct 06, 2005 3:57 pm

Post by matthijs »

Thanks pickle, your feedback is appreciated. I will look into those issues.
User avatar
Kieran Huggins
DevNet Master
Posts: 3635
Joined: Wed Dec 06, 2006 4:14 pm
Location: Toronto, Canada
Contact:

Post by Kieran Huggins »

what I meant was use MOD_REWRITE in your .htaccess file change the request URL from something like:

Code: Select all

http://domain.com/documents/some.pdf?insert_XSS_code_here
to simply:

Code: Select all

http://domain.com/documents/some.pdf
Wouldn't that eliminate the possibility of the exploit?
matthijs
DevNet Master
Posts: 3360
Joined: Thu Oct 06, 2005 3:57 pm

Post by matthijs »

Yes you are right. That would be possible.

However, it seems that everything behind the .pdf is stripped by $_GET already (when # is the character used). So when visiting the URL /?download_file=somefile.pdf#anotherstring, #anotherstring is never passed through to the $_GET var. I'm not sure if the XSS vector works with anything else besides #.

I'm searching the manual now to see what is and what is not passed through. The section about $_GET is not too informative. I remember someone asking about this (the # not being available) because he wanted to use that.

And even if it would pass through, it would not pass this:

Code: Select all

<?php
//download files
$files = array("some.pdf",
               "another.pdf",
               "yetanother.pdf",
               "some.doc",
               "another.doc",
               "yetanother.doc"
                           );
                                  
if (isset($_GET['download_file']) && in_array($_GET['download_file'], $files)) ?>
User avatar
Kieran Huggins
DevNet Master
Posts: 3635
Joined: Wed Dec 06, 2006 4:14 pm
Location: Toronto, Canada
Contact:

Post by Kieran Huggins »

mod_rewrite would solve the issue in apache BEFORE php gets involved... in fact, if the issue is with apache serving PDFs then there's no reason for php to be invoked in the first place. Does that make sense?
matthijs
DevNet Master
Posts: 3360
Joined: Thu Oct 06, 2005 3:57 pm

Post by matthijs »

Yes it does.

But to clarify: I didn't write this specific script in response to that vulnerability. I used this script just to make sure people would be offered a download window/prompt, instead of having the pdf opened in the same window the site is on (with all potential troubles of that).

And I (can) use this script to count the amount of downloads for example, so in that case it is useful as well.
User avatar
Kieran Huggins
DevNet Master
Posts: 3635
Joined: Wed Dec 06, 2006 4:14 pm
Location: Toronto, Canada
Contact:

Post by Kieran Huggins »

oh - I missed that ;-)

Good on you then!
Post Reply