1

I want to dynamically generate a downloadable text file with PHP containing only an md5 hash string.

Usage: index.php?filename=myhash.txt&hash=8145c8f6d42fda2c3cfdd1d091bcb331

$filename = isset($_GET["filename"]) ? $_GET["filename"] : "md5.txt";
$hash = (isset($_GET["hash"]) && preg_match('/^[a-f0-9]{32}$/', $_GET["hash"])) ? $_GET["hash"] : "";

header("Content-type: text/plain");
header("Content-Disposition: attachment; filename=$filename");
echo $hash;

My concern is the $filename used in the header which anyone can modify. Is this a security vulnerability?

Elias
  • 134
  • 1
  • 1
  • 6
  • Yes. It's called a file inclusion attack. – Richard Theobald Feb 15 '16 at 16:30
  • 2
    @RichardTheobald — No it isn't. That would only be a problem if the code used the filename provided by the user to access the server's filesystem… which it doesn't. – Quentin Feb 15 '16 at 16:53
  • @Quentin True, it is not technically a file inclusion attack as the file is downloaded rather than included/displayed, but the end result is essentially the same as a LFI attack: one could download a copy of any file that the apache user has access to. http://hakipedia.com/index.php/Local_File_Inclusion – Richard Theobald Feb 15 '16 at 17:02
  • 2
    @RichardTheobald — "one could download a copy of any file that the apache user has access to" — No! It! Can't! The supplied filename is not used to access the server's filesystem! – Quentin Feb 15 '16 at 17:03
  • *My 2 cents:* If you didn't protect that text file through `.htaccess`, anyone could see the contents of it, just by them doing `index.php?filename=myhash.txt`, unless you're using syntax that uses a reference to it outside the public area, but then again that could still be accessible. – Funk Forty Niner Feb 15 '16 at 17:07
  • @Quentin Good point. I didn't read the question as thoroughly as I thought I had. – Richard Theobald Feb 15 '16 at 17:07
  • @Fred-ii- — What text file? The filename is used only in the content-disposition response header. The contents of the file are supplied directly in the URL. – Quentin Feb 15 '16 at 17:07
  • @Quentin `md5.txt` *n'est-ce pas?* and `filename=myhash.txt` or what am I not getting here? – Funk Forty Niner Feb 15 '16 at 17:08
  • @Fred-ii- — That's a file name. It is put in the content-disposition response header. The browser gets the response. The browser creates a file on the user's file system (in the Downloads folder most likely). The *content* comes directly from the URL, not from any file that exists on the server. – Quentin Feb 15 '16 at 17:09
  • Yeah, this is kind of a weird question. The $_GET variable is only being used for the name the downloaded file is saved as; no file is actually ever accessed. I think this is probably okay, but it still reeks of code smell. – Richard Theobald Feb 15 '16 at 17:10

2 Answers2

3

I'm reasonably certain that this doesn't have any security issues, but insert a general disclaimer about how hard it is to prove a negative.

You don't use the filename to access your server's filesystem, so you aren't vulnerable to file inclusion attacks.

You restrict what the content of the file can be, so your users should be safe from being tricked into downloading scripts, but let's take a hypothetical anyway:

Given a programming language in which a program can be written using only the characters which match [a-f0-9] (which means no spaces), and a program can be written in 32 of those characters, then an attacker could craft a URL which appears to be on your site (and therefore trustworthy) which causes a file called something.xyz to be downloaded. (For a value of xyz which is registered as the file extension for scripts written in that language.) This would cause the attacker's program to be downloaded to somewhere that the user might double click on it and execute it.

That's a pretty unlikely case to start with, and modern OSes are quite good at flagging downloaded files as requiring a "Are you sure you want to execute this?" alert, so it is a serious edge case.

To be paranoid, you might want to restrict the filename to ones with .txt or .md5 extensions.

Special characters could be an issue, but I can't think of any that would be a security risk. If, for example, a new line was entered into the file name, then modern versions of PHP would likely throw something along the lines of:

Warning: Header may not contain more than a single header, new line detected in /Users/david/tmp/hfjkljiwe/index.php on line 4

… and prevent the script from outputting the content-disposition header at all.

Do not run old versions of PHP, get your security fixed versions installed!

From the other side, browsers aren't likely to accept a filename that the filesystem they are writing to is going to throw a wobbly over.

That said, paranoia never did anyone any harm when it came to writing secure software (especially when it has to interact with other software — software has bugs!), so adding some restrictions on what characters are allowed in the filename wouldn't be a bad idea.

Community
  • 1
  • 1
Quentin
  • 914,110
  • 126
  • 1,211
  • 1,335
  • Good answer. The only other risk is if an old version of PHP is used where header injection was possible: http://stackoverflow.com/a/11363523/413180 – SilverlightFox Feb 16 '16 at 11:35
  • @SilverlightFox — Thanks for that pointer, I've integrated it into the answer. – Quentin Feb 16 '16 at 11:37
  • Thank you @Quentin for the great reply and SilverlightFox for the warning about older versions of PHP. Header injection was my main concern, so it's good to learn PHP prevents new lines in a header. I've also added additional restrictions to the filename, as you've suggested, just to be extra safe. – Elias Feb 16 '16 at 17:33
0

As far as I can tell, you are theoretically okay to do this; since you never actually access any files on your server, no one can do anything negative to your server. However, one could potentially provide an invalid file name for their OS, and I'm not entirely sure what would happen. Given that it's going to pop open the download dialog either way, it just seems safer to let them enter the file name at that time rather than allow them to enter it beforehand.

Richard Theobald
  • 781
  • 3
  • 9
  • On any system it could be an issue if the file is subsequently processed by a program which does not handle filenames properly (and there are a lot of those) e.g. index.php?filename=afile.txt%3Brm%20-rf%20/ . MSWindows is particularly bad in this regard, but because the shell is so limited, I think the worst onw could do is write to a device. – symcbean Feb 15 '16 at 17:31