[SOLVED] Is this script insecure?

[Bricktop]

Hi there,

 

I have a basic download script which counts downloads every time a file is rewusted, you specifiy a download directory and a counter directory, call the download file via this script (i.e. download.php?file=download_file.zip) and the file is downloaded.

 

However, I've been told this is a huge security risk and any file can be downloaded using this, is this the case?

 

<?php

$downloads = 'downloads';

$counters = 'counters';

$path = $downloads.'/'.$HTTP_GET_VARS['file']; 

if(file_exists($path))
{
$file = fopen($counters.'/'.$HTTP_GET_VARS['file'].'.txt','r+');

$count = fread($file,100);

fclose($file);

$count++;

$file = fopen($counters.'/'.$HTTP_GET_VARS['file'].'.txt','w');

fwrite($file, $count);

fclose($file);

$size = filesize($path);

header('Content-Type: application/octet-stream');
header('Content-Disposition: attachment; filename='.$HTTP_GET_VARS['file']);
header('Content-Length: '.$size);

readfile($path);

}else{
echo "File not found";
}
?>	

[phporcaffeine]

Well lets take a look at what is and what isn't about your snippet

 

$path = $downloads.'/'.$HTTP_GET_VARS['file']; 

 

You use the $path var to get the filesize but that is the only place, when you send the file to the header you don't specify it so what is the point of $path?

 

Additionally, despite what all your code is doing at the vary end you put $HTTP_GET_VARS['file'] to the header ... but you have not added, validated or subtracted anything from it.

 

So, I could very well (as a user) put mydomain.com/download.php?file=clownFart.txt and it would send clownFart.txt in the header.

 

I wouldn't use user namespace (the GET stack) for something like this regardless

[Bricktop]

Thanks, I have replaced $HTTP_GET_VARS['file'] with $_GET['file'] throughout.

 

I have also stopped sending the filesize to the header and removed the code completely.

 

However I need someway to ensure that a user can only download the file specified in the $downloads = 'downloads'; variable.  For example you were able to type download.php?file=../index.php and this would drop down one level from the value specified in the $downloads variable and allow the user to download the file.

 

I have added a clean function to remove ../ from $_GET['file'] before it's handled by the script but obviously this is very basic and wonder how I can ensure only files in the $download directory can be downloaded.

 

new code below:

 

<?php

$downloads = 'downloads';

$counters = 'counters';

$cleanFile = clean($_GET['file']);

$path = $downloads.'/'.$cleanFile; 

if(file_exists($path))
{
$file = fopen($counters.'/'.$cleanFile.'.txt','r+');

$count = fread($file,100);

fclose($file);

$count++;

$file = fopen($counters.'/'.$cleanFile.'.txt','w');

fwrite($file, $count);

fclose($file);

header('Content-Type: application/octet-stream');
header('Content-Disposition: attachment; filename='.$cleanFile);

readfile($path);

}else{

	echo "File not Found";
}

function clean($string) {
$string = str_replace('../', '', $string);
return strtolower($string);
}
?>	
,/CODE]

[Bricktop]

Secured it myself, thanks for the pointers.