Header field from _GET

Discussions of secure PHP coding. Security in software is important, so don't be afraid to ask. And when answering: be anal. Nitpick. No security vulnerability is too small.

Moderator: General Moderators

Post Reply
GloriousEremite
Forum Newbie
Posts: 13
Joined: Tue Aug 14, 2007 1:00 pm

Header field from _GET

Post by GloriousEremite »

I wanted to allow visitors of a page to click on an image to download that image, but a simple link only redirected their browser rather than displaying the download dialog, so I came up with this script:

Code: Select all

<?php
$file = $_GET['file'];
$userfilename = $_GET['saveas'];
$type = $_GET['type'];

header('Content-Type: '.$type);
header('Content-Disposition: attachment; filename="'.$userfilename.'"');
readfile($file);
?>
Not being very experienced with php, I thought it best to ask if there are any security concerns here?
User avatar
stereofrog
Forum Contributor
Posts: 386
Joined: Mon Dec 04, 2006 6:10 am

Post by stereofrog »

Yes, this code enables HTTP response splitting

http://en.wikipedia.org/wiki/HTTP_response_splitting

and, even worse, allows the attacker to read arbitrary files from your server.
User avatar
Mordred
DevNet Resident
Posts: 1579
Joined: Sun Sep 03, 2006 5:19 am
Location: Sofia, Bulgaria

Post by Mordred »

Also it can be used to turn your script into a web proxy.
http://php.net/manual/en/features.remote-files.php
GloriousEremite
Forum Newbie
Posts: 13
Joined: Tue Aug 14, 2007 1:00 pm

Post by GloriousEremite »

>Yes, this code enables HTTP response splitting

That Wikipedia page says, "It is worth noting that although this is not a PHP specific problem, the PHP interpreter contains protection against this attack since version 4.4.2 and 5.1.2 [1]."

That makes it sound like I'm automagically protected...or do I still have to sanitize the string?

>and, even worse, allows the attacker to read arbitrary files from your server.
>Also it can be used to turn your script into a web proxy.

What if I check that the file starts with "./tmp" (the folder the files should be stored in) and contains only a single '.' character?
miro_igov
Forum Contributor
Posts: 485
Joined: Fri Mar 31, 2006 5:06 am
Location: Bulgaria

Post by miro_igov »

Mordred wrote:Also it can be used to turn your script into a web proxy.
http://php.net/manual/en/features.remote-files.php
Only if allow_url_fopen is On


If you check for ./tmp then is ok but make sure it will not pass ./tmp/../config.php :)
User avatar
feyd
Neighborhood Spidermoddy
Posts: 31559
Joined: Mon Mar 29, 2004 3:24 pm
Location: Bothell, Washington, USA

Post by feyd »

GloriousEremite wrote:That makes it sound like I'm automagically protected...or do I still have to sanitize the string?
I wouldn't expect PHP to magically protect you.
GloriousEremite
Forum Newbie
Posts: 13
Joined: Tue Aug 14, 2007 1:00 pm

Post by GloriousEremite »

"From now on, all new PHP versions will no longer support multiple headers in the header() call and therefore all vulnerable applications will only be exploitable on hosts with old PHP versions." (php-security.org)

"This function [header()] now prevents more than one header to be sent at once as a protection against header injection attacks." (php.net)

Just so I get this straight, is there anything to worry about if an attacker can't send multiple headers by inserting a CRLF? (That is, if I'm running the latest version of PHP or remove them manually)

Here's my updated version:

Code: Select all

<?php 
$file = $_GET['file']; 
$userfilename = $_GET['saveas']; 
$type = $_GET['type']; 

//prevent HTTP response splitting (header() shouldn't allow it in 5.1.2/4.4.2, but just in case)
if(count($c=preg_grep('/(\r)|(\n)/',$_GET))!==0)
  exit("Invalid String!");

//only allow files in ./tmp directory (and only extensionless files)
if (strpos($file,"./tmp")!==0||strpos($file,'.',1))
  exit("Invalid File!");



header('Content-Type: '.$type); 
header('Content-Disposition: attachment; filename="'.$userfilename.'"'); 
readfile($file); 
?>
timvw
DevNet Master
Posts: 4897
Joined: Mon Jan 19, 2004 11:11 pm
Location: Leuven, Belgium

Post by timvw »

- The code for verifying where the file exists is flawed... Use the realpath function to determine where the file *really* lives...
- The filename in the header should not contain the path..
GloriousEremite
Forum Newbie
Posts: 13
Joined: Tue Aug 14, 2007 1:00 pm

Post by GloriousEremite »

How exactly is it flawed? I could do something like this:

Code: Select all

if (dirname(realpath($file))!=realpath("./tmp"))
  exit("Invalid File!");
Is that what you meant? It just seems like realpath will have to do uneccessary processing.

And the filename in the header doesn't contain the path. It's just the name that is displayed in the download dialog.
timvw
DevNet Master
Posts: 4897
Joined: Mon Jan 19, 2004 11:11 pm
Location: Leuven, Belgium

Post by timvw »

Simply request page = /etc/motd\x00 and enjoy ;)
GloriousEremite
Forum Newbie
Posts: 13
Joined: Tue Aug 14, 2007 1:00 pm

Post by GloriousEremite »

I think I must be incredibly dense, but I don't see the problem. Right now I'm testing on a windows box, so that's not a possibility, but anyways it shouldn't pass validation using my code since /etc/motd\x00 doesn't start with './tmp'...
timvw
DevNet Master
Posts: 4897
Joined: Mon Jan 19, 2004 11:11 pm
Location: Leuven, Belgium

Post by timvw »

I thought i had editted my post to: "Search the web for 'php null byte' and see how it can be exploited".

(On second thought: it seems this bug has been solved for a while now)
Post Reply