[2.1][File Upload] RenameUpload filter rewrite w/option to use uploaded 'name' #3259

Merged
merged 2 commits into from Jan 4, 2013

Conversation

Projects
None yet
3 participants
Contributor

cgmartin commented Dec 19, 2012

Rewrite of the Zend\Filter\File\RenameUpload filter to be easier to use, and with an additional feature to use the $_FILES['name'] as the target filename.

Also, some misc docs cleanup and a cornercase fix of Zend\Filter\File\Rename (from my earlier $_FILES-support-in-filters changes).

RenameUpload usage example:

use Zend\Http\PhpEnvironment\Request;

$request = new Request();
$files   = $request->getFiles();
// i.e. $files['my-upload']['name'] === 'myfile.txt'

$filter = \Zend\Filter\File\RenameUpload("./data/uploads/");
$filter->setUseUploadName(true); // Use the original upload filename
echo $filter->filter($files['my-upload']);
// File has been moved to "./data/uploads/myfile.txt"

Docs: https://raw.github.com/cgmartin/zf2-documentation/f32980cd35bdea21b6560d5dee4d878af002313f/docs/languages/en/modules/zend.filter.file.rename-upload.rst

@cgmartin cgmartin commented on the diff Dec 19, 2012

library/Zend/Filter/File/RenameUpload.php
+
+ // Get the target directory
+ if (is_dir($target)) {
+ $targetDir = $target;
+ $last = $target[strlen($target) - 1];
+ if (($last != '/') && ($last != '\\')) {
+ $targetDir .= DIRECTORY_SEPARATOR;
+ }
+ } else {
+ $info = pathinfo($target);
+ $targetDir = $info['dirname'] . DIRECTORY_SEPARATOR;
+ }
+
+ // Get the target filename
+ if ($this->getUseUploadName()) {
+ $targetFile = basename($uploadData['name']);
@cgmartin

cgmartin Dec 19, 2012

Contributor

When using the useUploadName option, should we be doing any sanitizing of the $_FILES['name'] value which will be used as the target filename?

The basename() should help, and I hesitate to change the filename too much if the user is expecting all characters to be supported.

Wanted to flag this in case I'm overlooking a security issue. Always makes me nervous mixing user input and filesystems.

@EvanDotPro

EvanDotPro Jan 3, 2013

Member

Honestly, I've never relied on $_FILES['name'] when storing an uploaded file. I just always naturally assumed it was a bad idea, but I couldn't really justify it with a specific vulnerability I'm aware of. Generally what I've done is stored the file using some knowingly unique identifier such as an auto increment ID from a database or a checksum/hash, then store any user-specified filename in the database for presentation to the user... I can't say I remember reading this practice anywhere specific. It could just be something I stitched together based on other unrelated best practices.

A quick Google leads me to this: http://stackoverflow.com/questions/9923416/php-file-upload-security-keeping-the-original-file-name

I suppose one risk would be if a user was careless enough to let people upload into the doc root and accepted .php files... Something along those lines could lead to trouble. I'm unsure if there's any risks involved in exploiting specific limitations of certain filesystems or anything like that -- I would imagine that's not really a concern.

@cgmartin

cgmartin Jan 3, 2013

Contributor

@EvanDotPro agreed, I was also wary of this. This is a response to several community members requesting the feature.

Like you mentioned, .php files under the docroot seems like the biggest threat. That alone makes me think the default setting for use_upload_name should be false instead of true (I'll make that change). A warning block in the docs to remind about using a safe target directory would probably be good to do as well.

Much thanks for the feedback!

cgmartin referenced this pull request in cgmartin/ZF2FileUploadExamples Dec 27, 2012

Closed

while accessing the examples #4

Owner

weierophinney commented Jan 2, 2013

@padraic @Maks3w @EvanDotPro -- can one of you look at the comments from @cgmartin and weigh in on his security concerns?

weierophinney was assigned Jan 4, 2013

weierophinney merged commit a911251 into zendframework:develop Jan 4, 2013

1 check failed

default The Travis build failed
Details

@weierophinney weierophinney added a commit to zendframework/zend-filter that referenced this pull request May 15, 2015

@weierophinney weierophinney Merge branch 'feature/3259' into develop bff7eb0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment