download file security help

[daneth1712]

Hi guys,

 

I have written a script that checks if a file is stored on the database and then forces the download to start.

 

the script is below;

 

<? 
//start the session
session_start();
//check to make sure the session username variable is registered
if(isset($_SESSION['login']) && isset ($_SESSION['user'])){
$fid1=$_SESSION['login'];
$nid2=$_SESSION['user'];
}
else{
//the session variable isn't registered, send them back to the login page
header( "Location: login.php" );
exit;
}
//get file info
if (isset($_GET["file"])){
$f=$_GET["file"];
//connect to the database and check if file exists
include('inc/includes/config.php');
$db = mysql_connect("host", "user", "pass") or die ("Error connecting to database.");
mysql_select_db("database", $db) or die ("Couldn't select the database.");
$result=mysql_query("SELECT ID,type,count,path FROM table WHERE filename='$f'", $db);
$row=mysql_num_rows($result);
//if exactly 1 file found on database get current hit count, file type and path to the file
if($row == 1){
while($row = mysql_fetch_array($result)){
$id=$row['ID'];
$type=$row['type'];
$count=$row['count'];
$path=$row['path'];
//check if file type is only a doc, pdf, ppt, or xls
if($type=="application/pdf" || $type=="application/vnd.ms-excel" || $type=="application/msword" || $type=="application/vnd.ms-powerpoint"){
//update the database and add count to file download amount
$newcount=$count +1;
$update=mysql_query("UPDATE table SET count='$newcount' WHERE filename='$f' AND ID='$id'", $db);
//if there are any spaces in the filename, replace with an underscore 
$fn = preg_replace("/[^a-zA-Z0-9s.]/", "_", $f);
//download file
header("Content-type: $type");
header("Content-Disposition: attachment;filename=$fn");
header("Content-Transfer-Encoding: binary");
header('Pragma: no-cache');
header('Expires: 0');
set_time_limit(0);
readfile($path);
	}
}
}
else{
// wrong file type, probably someone messing about.
echo "I dont think so.....";	
}
}
else{
header ("Location: ../../../index.php");
}
?>

 

The script works fine, what I would like to know is if anyone can offer some tips on how to make this whole process more secure? Am I going about this the wrong way? and are there any security holes in the script?

 

Appreciate any help and advice.

[mikesta707]

$f=$_GET["file"];

 

you should escape that with mysql_real_escape_quote()

$f=mysql_real_escape_quote($_GET["file"]);

 

beyond that I would need more information. can anybody download anything? is there a user system? can certain users only download certain files? etc.

 

[daneth1712]

Hi

 

Sorry, more info probably would have been useful.

 

There is a login system, this is checked by the following part:

 

//start the session
session_start();
//check to make sure the session username variable is registered
if(isset($_SESSION['login']) && isset ($_SESSION['user'])){
$fid1=$_SESSION['login'];
$nid2=$_SESSION['user'];
}
else{
//the session variable isn't registered, send them back to the login page
header( "Location: login.php" );
exit;
}

 

Users will be able to view the files, but only those logged in will be able to download them.

 

I am adding ?file=$filename to the link on the previous page, which I was using $f=$_GET["file"]; for, but I will use yours.

 

I am then checking the database for a row which is equal to the $_GET value, which should be correct as the links are beng added from the database. I thought it best doing it this way so there was no option to download other files within my root files. Just incase I missed anything there, I only allowed certain file tyes to be downloaded.

 

I am then adding a hit rate for each download and then allowing the download.

 

I am worried I have missed stuff on the headers, not too sure there.

 

btw, I couldnt find mysql_real_escape_quote in the php manual, did you mean mysql_real_escape_string?

[mikesta707]

oh yeah sorry, brain fart. Kudos for checking the manual, thats rare around here

 

but yeah, once you sanitize your inputs, I don't see much of a problem. Its a very simple script, so that doesn't leave much room for vulnerabilities. And your headers look fine to me

 

[daneth1712]

hey,

 

when I use

 

$f=mysql_real_escape_string($_GET["file"]);

 

I get this...

Warning: mysql_real_escape_string() [function.mysql-real-escape-string]: Access denied for user 'SYSTEM'@'localhost' (using password: NO) in path to file on line 16

 

Warning: mysql_real_escape_string() [function.mysql-real-escape-string]: A link to the server could not be established in path to file on line 16

I dont think so.....

 

Not sure what I am doing wrong  :-\

 

Is there an extension I need to change or use?

[mikesta707]

Did you connect to the database before you tried that. There needs to be an active mysql connect resource before you use it

[daneth1712]

ok thanks, I actually cant believe I missed that. 

 

Its a very simple script, so that doesn't leave much room for vulnerabilities.

 

I always thought the simple scripts were the ones that left yourself open for attacks, I have only been learning php for a few months, so to me this is a complicated script 

[mikesta707]

Well think about it. Generally, the more code you have, the more chances you have to open something up for attack. If you have a simple hello world program

echo "Hello World";

 

There isn't really anything a hacker could do to hack into that script (and then into your server)

 

Simple scripts are just that, simple. Simple to make, and simple to secure. Its much harder to make a really complicated program secure then a simple program. Look at an operating system. Thats really complicated, but its also really hard to make secure