Download script

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
User avatar
andym01480
Forum Contributor
Posts: 390
Joined: Wed Apr 19, 2006 5:01 pm

Download script

Post by andym01480 »

The following grabs a file requested and serves it with headers forcing a choice of open or save and works well.
Is it secure though?

Code: Select all

 
<?php
 
include("../includes/config.inc.php");
if(!isset($_REQUEST['file'])) header("Location:../index.php?pageid=Sermons");
 
$file=$_REQUEST['file'];
$ext=substr($file,-3);
 
if(file_exists("../sermons/$file")){
 
if($ext=='mp3') $q='mp3';
if($ext=='pdf') $q='pdf';
$sql="UPDATE sermons SET {$q}downloads={$q}downloads+1 WHERE `$q`='".mysql_real_escape_string($file)."'";
$result=mysql_query($sql)oR DIE (mysql_error());
header ("Cache-Control: must-revalidate, post-check=0, pre-check=0");
header("Content-Type: application/$q");
header("Content-Disposition: attachment; filename=$file");
header("Content-Transfer-Encoding: binary");
readfile("../sermons/$file");
}
header("Location:../index.php?pageid=Sermons");
exit();
?>
 
User avatar
Mordred
DevNet Resident
Posts: 1579
Joined: Sun Sep 03, 2006 5:19 am
Location: Sofia, Bulgaria

Re: Download script

Post by Mordred »

No, one can use it to read any file (that PHP has access to).
Start with realpath() and the related functions.

Other problems:
- SQL injection with register globals on and $q
- after the first header redirect there is no die/exit, so one can cause error conditions (useful for the bad guy if error reporting is set to write to the output)
- even with error_reporting off, the mysql statement can be broken and the die() will print

Edit:
- with older versions of PHP, there is HTTP response splitting with $file
User avatar
andym01480
Forum Contributor
Posts: 390
Joined: Wed Apr 19, 2006 5:01 pm

Re: Download script

Post by andym01480 »

Oh my goodness - rule no1 never trust user inputted or clicked on data!

I've rewritten using 2 GET variables the 'id' of table row where the filenames are stored which can be checked as an integer and a 'type' which would be mp3 or pdf - everything else rejected. A db call for the filenames from that row and another rejection if mysql_num_rows=0. Seems a safer way than trying to validate a file name!

I feel a security audit of all my code on that site coming on using that filtering and escaping cheat sheet.... :roll:
Thanks for your help.
User avatar
Mordred
DevNet Resident
Posts: 1579
Joined: Sun Sep 03, 2006 5:19 am
Location: Sofia, Bulgaria

Re: Download script

Post by Mordred »

You check the filetype in a wrong way, post your new code so we can see if it's still the case.
User avatar
andym01480
Forum Contributor
Posts: 390
Joined: Wed Apr 19, 2006 5:01 pm

Re: Download script

Post by andym01480 »

Code: Select all

 
<?
/* Version 2 completely rewritten for security!*/
//define variables
$type='';
$id='';
$sql=array();
$file=array();
//db connect
include("../includes/config.inc.php");
 
//check file is set, validate as integer and escape
if(!isset($_REQUEST['file'])) header("Location:../index.php?page=Sermons");
$id=$_REQUEST['file'];
if(!ctype_digit($id)) header("Location:../index.php?page=Sermons");
$sql['id']=mysql_real_escape_string($id);
//check type is mp3 or pdf 
$type=$_REQUEST['type'];
if($type!='mp3' && $type!='pdf') header("Location:../index.php?page=Sermons");
$sql['type']=mysql_real_escape_string($type);
//grab filename
$filesql="SELECT {$sql['type']} AS file FROM sermons WHERE sermon_id='{$sql['id']}'";
$result=mysql_query($filesql);
//reject if not in db
if(mysql_num_rows($result)=='0') header("Location:../index.php?page=Sermons");
$file=mysql_fetch_assoc($result);
//if file exists then output and update download count
if(file_exists("../sermons/{$file['file']}")){
$sql="UPDATE sermons SET {$sql['type']}downloads={$sql['type']}downloads+1 WHERE `sermon_id`='{$sql['id']}";
$result=mysql_query($sql);
//headers to force open or save
header ("Cache-Control: must-revalidate, post-check=0, pre-check=0");
header("Content-Type: application/{$sql['type']}");
header("Content-Disposition: attachment; filename={$file['file']}");
header("Content-Transfer-Encoding: binary");
readfile("../sermons/{$file['file']}");
}
?>
 
User avatar
kaisellgren
DevNet Resident
Posts: 1675
Joined: Sat Jan 07, 2006 5:52 am
Location: Lahti, Finland.

Re: Download script

Post by kaisellgren »

andym01480 wrote:

Code: Select all

 
<?
/* Version 2 completely rewritten for security!*/
//define variables
$type='';
$id='';
$sql=array();
$file=array();
//db connect
include("../includes/config.inc.php");
 
//check file is set, validate as integer and escape
if(!isset($_REQUEST['file'])) header("Location:../index.php?page=Sermons");
$id=$_REQUEST['file'];
if(!ctype_digit($id)) header("Location:../index.php?page=Sermons");
$sql['id']=mysql_real_escape_string($id);
//check type is mp3 or pdf 
$type=$_REQUEST['type'];
if($type!='mp3' && $type!='pdf') header("Location:../index.php?page=Sermons");
$sql['type']=mysql_real_escape_string($type);
//grab filename
$filesql="SELECT {$sql['type']} AS file FROM sermons WHERE sermon_id='{$sql['id']}'";
$result=mysql_query($filesql);
//reject if not in db
if(mysql_num_rows($result)=='0') header("Location:../index.php?page=Sermons");
$file=mysql_fetch_assoc($result);
//if file exists then output and update download count
if(file_exists("../sermons/{$file['file']}")){
$sql="UPDATE sermons SET {$sql['type']}downloads={$sql['type']}downloads+1 WHERE `sermon_id`='{$sql['id']}";
$result=mysql_query($sql);
//headers to force open or save
header ("Cache-Control: must-revalidate, post-check=0, pre-check=0");
header("Content-Type: application/{$sql['type']}");
header("Content-Disposition: attachment; filename={$file['file']}");
header("Content-Transfer-Encoding: binary");
readfile("../sermons/{$file['file']}");
}
?>
 
Do not use $_REQUEST, either use $_POST or $_GET. Using $_REQUEST may lead into security holes or other problems in general.

Anyone can just put ?type=pdf to pass your type check...

You are vulnerable to SQL injections. I can insert arbitrary code like ?type=injectable_code because you do not stop the execution after using header().

Are you the one who controls files? If anyone can manipulate those filenames in your database, you could be potentially vulnerable to delayed SQL injections.

Because you are not closing the process after header(), you are vulnerable to XSS attacks. I could make you to download a .html file with XSS on it, because the type is inserted in to the download headers. In IE, I can force IE to load the .html page in the same website so my XSS would have an access to e.g. cookies.

This is the last time I say this: stop the execution whenever you have used header().

I think that's it.
User avatar
andym01480
Forum Contributor
Posts: 390
Joined: Wed Apr 19, 2006 5:01 pm

Re: Download script

Post by andym01480 »

Code: Select all

if($type!='mp3' && $type!='pdf') header("Location:../index.php?page=Sermons");
How can you inject code? Does the script still keep going even though there has been a header redirect call?
$type is then escaped anyway.
Would appreciate further explanation....
In the meantime I will change it to

Code: Select all

if($type!='mp3' && $type!='pdf') 
{
header("Location:../index.php?page=Sermons");
exit();
 
EDIT: You have edited!

I'm safe from delayed sql - as only a superadmin (ie me) can upload files which are given a filename based on a timestamp anyway.
User avatar
kaisellgren
DevNet Resident
Posts: 1675
Joined: Sat Jan 07, 2006 5:52 am
Location: Lahti, Finland.

Re: Download script

Post by kaisellgren »

andym01480 wrote:Does the script still keep going even though there has been a header redirect call?
Yea.
andym01480 wrote:$type is then escaped anyway.
Yea, but you pass it directly to file-download-headers, which causes problems (e.g. XSS) IF you do not put exit() after header()s.
User avatar
andym01480
Forum Contributor
Posts: 390
Joined: Wed Apr 19, 2006 5:01 pm

Re: Download script

Post by andym01480 »

Good call. Most of the time I'd exit'd after headers without knowing why. Now I do!

Security is a nightmare - thanks for your teaching. Need to get on with my day job (Church leader) helping sinners repent and do more useful things with their lives! Haven't found any hackers that need converting yet.... :roll:
User avatar
kaisellgren
DevNet Resident
Posts: 1675
Joined: Sat Jan 07, 2006 5:52 am
Location: Lahti, Finland.

Re: Download script

Post by kaisellgren »

andym01480 wrote:Good call. Most of the time I'd exit'd after headers without knowing why. Now I do!

Security is a nightmare - thanks for your teaching. Need to get on with my day job (Church leader) helping sinners repent and do more useful things with their lives! Haven't found any hackers that need converting yet.... :roll:
Well... security is just like other topics. E.g. if you want to learn chemistry, go ahead - it's not gonna be an easy task at least if you want to be aware of everything related to it. Of course science evolves, so does web attacks and protections against them.

Your wife/mom probably knows better how to cook. You can of course cook by yourself if you are alone, but the result is most likely not the best possible. The same thing applies to real world. Companies are not alone, they have a family of experts. Someone is expert in security, and will take care of it, and so on. If you are alone, no family, no team of coders, etc, then you just have to try to do your best.
Post Reply