PHP: readfile() corrupted download when using & (Ampersand)

Question

I want to make a pdf down-loadable which can't be directy accessed by an url.

This is how the link would look like

<a href="getFile.php?file=myfile.pdf" >Myfile</a>

and that is getFile.php

<?php

  $file=$_GET['file'];

  // Check if file really exists:
  $files = array_diff(scandir('/www/secretPlace/'), array('.', '..'));
  $show = (in_array($file,$files)) ? true : false;
  if(!$show) exit();

  header('Content-Type: application/pdf');
  header('Content-Disposition: attachment; filename="' . $file . '"');
  readfile( '/www/secretPlace/' . $file);

This works fine, as long as the pdf name does not include the & symbol. If I try

<a href="getFile.php?file=myfile & other stuff.pdf" >My Special File</a>

I would be able to download a file with the name, but I cant open it with a pdf reader. All programs tell me that the file is corrupted. How can I get open pdfs with readfile that have a & symbol in their name?

Never pass user input in function; If you do so user can download any file from your server; — itzmukeshy7, Dec 07 '16 at 11:46
@itzmukeshy7 I have added a check if the file is actually in the directory. Do you think its save now? Otherwise I am thinking about using http://stackoverflow.com/a/4205278/2311074. — Adam, Dec 07 '16 at 13:49
No need to add `foreach` loop simply use `in_array()` to check; — itzmukeshy7, Dec 08 '16 at 07:01

Rápli András · Answer 1 · 2016-12-07T20:08:17.980

2

Your code is vulnerable to Path Traversal attacks. I highly suggest you to specifically tell which files a user can download.

To your question (updated),

I'd suggest that you store the filenames (encoding is not neccessary) in a database, then each row would have a separate ID. You would make a page, let's say /download/1234 for ID 1234 , so you would automatically know which file it belongs to, not having to mess with special characters breaking the URI string. The URL structure can be configured in .htaccess, /download/[1-9][0-9]* pointing to that PHP page you included in your question, where you should handle all this. Then you would fetch the filename from the database, do a glob() check to see whether it exists, and start user download with Content-Disposition.

If you don't prefer the database solution, you need to encode special characters in the URI string (especially & as you mentioned), or write a function that parses the URI string by hand and not rely on $_GET, which splits the text at & and = while rendering.

To the extended question on security side:

I can provide filenames for which this turns true even if they shouldn't match.

if($pdf == $file) $show = true;

If you use three equal signs, it's way better, but still might be vulnerable to C null byte injection. So the suggestion here, as always, is to whitelist input instead of blacklisting (in this case ..)

edited Dec 07 '16 at 20:08

answered Dec 07 '16 at 11:58

Rápli András

3,556
1
29
52

Do you suggest that I should make a page for each individual pdf? Because I am looking for a dynamical solution. I also added a check to my **getFile.php** to make sure that the file actually exists in the folder (just 1 sec before I saw your answer), so I think its not vulnerable to **Path Traversal** anymore, right? – Adam Dec 07 '16 at 12:03
This is a dynamic enough solution, if you do it with `.htaccess` the right way. You have to make one single PHP page. – Rápli András Dec 07 '16 at 12:05
It is still vulnerable to specific injections (CR, LF, null-byte). (As an advice, you never blacklist bad input, you always whitelist good). If you want to make a specific download directory available, just use `glob` to read filenames and whitelist them. – Rápli András Dec 07 '16 at 12:08
I don't see how my approach is vulnerable to a null byte injection, because I am doing the whitelist approach. – Adam Dec 07 '16 at 12:16
`$pdf = "abc.pdf"; $file="abc.pdf\0../../../../etc/passwd";` They might equal. – Rápli András Dec 07 '16 at 12:19
Just please believe me, this is my job :) – Rápli András Dec 07 '16 at 12:19
Okay. Thanks for your effort. But I still have 2 questions: 1) I don't completely understand your answer to my main question - are you basicly saying, that I should avoid the function `readfile` and save the content of the pdf in some php page that I somehow have to dynamically create? And then with .htacces I have to map a pdf filename to this dynamicaly created php that cointains the pdf in a variable? – Adam Dec 07 '16 at 12:30
2) You said `I highly suggest you to specifically tell which files a user can download.` and when I iterate through all possible filenames in the folder and compare them with the userinput `$file` then you say this is still not enough. How should I do it then? I mean even if I had a handwritten array with allowed filenames, in the end I need to compare it with the userinput, or nor? I am not questioning your competence, I just want to understand what I have to do. Sorry for the confusion. – Adam Dec 07 '16 at 12:33
Sorry about that, I didn't update the answer since you updated yours, but here it is. I proposed a more suitable solution. – Rápli András Dec 07 '16 at 20:02

score 1 · Answer 2 · edited May 23 '17 at 11:45

It did not work because when using

<a href="getFile.php?file=myfile & other stuff.pdf" >My Special File</a>

then

$file=$_GET['file'];

has only the value myfile. One needs to decode the url like this:

<a href="getFile.php?file=<?php echo urlencode('myfile & other stuff.pdf'); ?>" >My Special File</a>

The security white-list approach could be replaced by the maybe more secure method explained in https://stackoverflow.com/a/4205278/2311074

PHP: readfile() corrupted download when using & (Ampersand)

2 Answers2