Welcome to WebmasterWorld Guest from 54.144.77.26

Forum Moderators: coopster & jatar k

Message Too Old, No Replies

Can a downloadable file with a .zip file extension be a problem?

Security of files with ,zip files for upload

     

kiwibrit

9:16 pm on Sep 16, 2011 (gmt 0)

10+ Year Member



I'm preparing an upload page where zipped files only should be uploaded. Visitors will go to a download page with a user name and password, and expect to download zipped files. The files will not be opened on the web host server. Whilst access to the upload page will be restricted, I want to make sure I am offering reasonable security.

Can a file with a .zip extension have malicious script which can be activated in such circumstances? I am hoping not, and that I can merely check if the files have the .zip extension with something like:
$filename = 'afile.zip';
$ext = pathinfo($filename, PATHINFO_EXTENSION);

and rely on that.

Would that be sufficient? If not, what would you recommend?

Readie

9:33 pm on Sep 16, 2011 (gmt 0)

WebmasterWorld Senior Member 5+ Year Member



If you simply do the "send download headers, get raw file data, print" method, you should be fine - nothing the user uploaded will be executed by the server.

The following code should do what you want to do:

<?php

$file_path = '/path/to/file/from/server/root.zip';

if(headers_sent()) {
die('Headers Sent');
}
if(ini_get('zlib.output_compression')) {
ini_set('zlib.output_compression', 'Off');
}
if(file_exists($file_path)) {
header('Pragma: public');
header('Expires: 0');
header('Cache-Control: must-revalidate, post-check=0, pre-check=0');
header('Cache-Control: private', false);
header('Content-Type: application/force-download');
header('Content-Disposition: attachment; filename="' . basename($file_path) . '";');
header('Content-Transfer-Encoding: binary');
header('Content-Length: ' . filesize($file_path));
ob_clean();
flush();
readfile($fullPath);
} else {
die('File Not Found');
}

?>

kiwibrit

11:10 pm on Sep 16, 2011 (gmt 0)

10+ Year Member



I'll go over that again tomorrow morning when I am less tired to make sure I really understand it - but at first look I get the gist - and it certainly looks very neat. Many thanks.

kiwibrit

10:20 am on Sep 17, 2011 (gmt 0)

10+ Year Member



It seems to work well - thanks. This is the first time I have had to code the handling of files uploaded over the web, so I've taken a good look at the code. Could you explain:

if(headers_sent()) {
die('Headers Sent');
}

For the rest, I have adapted my server-side uploadify file as shown below. I'd be grateful if you could see if what I have done looks reasonable - and also advise if the comments I have added are correct.


<?php
//This page is for handling zip files only.
if (!empty($_FILES)) {
$tempFile = $_FILES['Filedata']['tmp_name'];
$targetPath = $_SERVER['DOCUMENT_ROOT'] . $_REQUEST['folder'] . '/';
$targetFile = str_replace('//','/',$targetPath) . $_FILES['Filedata']['name'];
//not understood
if(headers_sent()) {
die('Headers Sent');
}
//make sure zlib.output_compression is off to allow output handlers
if(ini_get('zlib.output_compression')) {
ini_set('zlib.output_compression', 'Off');
}
// allow cache
header('Pragma: public');
//make permanent
header('Expires: 0');
header('Cache-Control: must-revalidate, post-check=0, pre-check=0');
// allow cache through proxy
header('Cache-Control: private', false);
header('Content-Type: application/force-download');
header('Content-Disposition: attachment; filename="' . basename($tempFile) . '";');
header('Content-Transfer-Encoding: binary');
header('Content-Length: ' . filesize($tempFile));
ob_clean();
flush();
$fileTypes = str_replace('*.','',$_REQUEST['fileext']);
$fileTypes = str_replace(';','|',$fileTypes);
$typesArray = split('\|',$fileTypes);
$fileParts = pathinfo($_FILES['Filedata']['name']);
if (in_array($fileParts['extension'],$typesArray)) {
move_uploaded_file($tempFile,$targetFile);
echo str_replace($_SERVER['DOCUMENT_ROOT'],'',$targetFile);
} else {
echo 'Invalid file type.';
}
}
?>


PS how did you get that box-effect around your code? It does make for easier reading.

Readie

11:00 am on Sep 17, 2011 (gmt 0)

WebmasterWorld Senior Member 5+ Year Member



if(headers_sent()) {
die('Headers Sent');
}

If you've already echoed anything to the browser, using the header() function will fail and issue a warning (can't remember off the top of my head what level of warning) - this will stop the script before it reaches the header functions.

how did you get that box-effect around your code? It does make for easier reading.

[quote][pre]{code tags - break if I put them in here}[/pre][/quote]

kiwibrit

2:06 pm on Sep 17, 2011 (gmt 0)

10+ Year Member



Got it - I like to understand code when it's passed on. I take it the rest of the code looks good? Thanks for your help.

Readie

8:46 pm on Sep 17, 2011 (gmt 0)

WebmasterWorld Senior Member 5+ Year Member



Well, you seem to be assuming that several variables created from user defined input will be available, without validating their existance or data type.

The isset() function should be used to make sure that they are set, and a commonly overlooked part of validation when dealing with user defined input data is to make sure you are dealing with a string. is_string() is your friend :) prevent errors being triggered, even if display_errors is set to off in the ini file.

rocknbil

4:25 pm on Sep 19, 2011 (gmt 0)

WebmasterWorld Senior Member rocknbil is a WebmasterWorld Top Contributor of All Time 10+ Year Member



I'm preparing an upload page where zipped files only should be uploaded.


This is your point of danger, checking the file extension is not enough. you need to contrive a way of reading the file safely to see if it is indeed a zip file - I have no examples handy but there should be plenty out there.

Second, just a FYI, the "force download" method above is handy for cases where the browser would otherwise display the object in the browser (like a JPG or PDF, for example.) You only need to link to a zip file, as the default method of handling those files is to download. :-)