file_save_upload() (as committed in #115267: Simplify File Uploads, Centralize File Validation and Quotas, Fix File Previews....) tries to apply the $validators before doing the following:
// Move uploaded files from PHP's upload_tmp_dir to Drupal's temporary directory.
// This overcomes open_basedir restrictions for future file operations.
$file->filepath = $file->destination;
if (!move_uploaded_file($_FILES['files']['tmp_name'][$source], $file->filepath)) {
form_set_error($source, t('File upload error. Could not move uploaded file.'));
watchdog('file', 'Upload error. Could not move uploaded file %file to destination %destination.', array('%file' => $file->filename, '%destination' => $file->filepath));
return 0;
}
The comment proves that an effort was made to consider the open_basedir restriction, but the authors/reviewers didn't realize that is_uploaded_file() and move_uploaded_file() (and inspecting $_FILES) is about all you can do as long as the file is in the /tmp directory.
The file needs to be moved before the $validators can be run on it.
For example, an image upload will fail with something like...
is_file(): open_basedir restriction in effect. File(/tmp/phpP9LR6m) is not within the allowed path(s): (/srv/www/apps/drupal:/srv/www/vhosts/example.com/httpdocs:/tmp/.drupal) in /srv/www/apps/drupal/drupal-6.8/includes/image.inc on line 117.
... (Drupal 6.8) if /tmp is not in the allowed path.
I found this in 6.8, but the code is unchanged in HEAD.
Comment | File | Size | Author |
---|---|---|---|
#11 | file_upload_357469_10_d6.patch | 3 KB | nedjo |
#10 | file_upload_357469_10.patch | 2.8 KB | nedjo |
#4 | 357469-4-axyjo.patch | 1.38 KB | axyjo |
Comments
Comment #1
drewish CreditAttribution: drewish commentedyeah i was discussing this over on #353580: file_validate_image_resolution doesn't allow not resizing if a toolkit is present.. we need to move it into our temp directory so we can validate/modify it and assuming that it passes validation then move it into the final destination.
Comment #2
drewish CreditAttribution: drewish commentedComment #3
axyjo CreditAttribution: axyjo commentedsubscribing.
Comment #4
axyjo CreditAttribution: axyjo commentedCould it really be as simple as this? Am I omitting something significant?
Comment #5
drewish CreditAttribution: drewish commentedaxyjo, it's not that simple. did you read my comments in #1? we don't want to overwrite the file if it fails validation.
Comment #6
axyjo CreditAttribution: axyjo commentedI knew I was missing something. I wasn't quite sure what exactly. However, the issue states that we need to move it before the validators are run. Is there any place in Drupal that acts like /tmp and removes files from there periodically?
Comment #7
drewish CreditAttribution: drewish commentedwe could move it to our temp directory, run the validators, then move it to the final destination.
Comment #8
Andrew Schulman CreditAttribution: Andrew Schulman commentedsubscribing
Comment #9
quicksketchSubscribing.
Comment #10
nedjoPatch following drewish's suggestion. Seems to fix the error, but I hit this error only intermittently, making it hard to test.
Comment #11
nedjoD6 version of the patch.
Comment #13
drewish CreditAttribution: drewish commentedthe failures seem like they might have another cause. flipping the status to get a re-test.
Comment #14
salvisMaybe there is an easier fix. Part of the issue is that the temporary directory set on admin/settings/file-system is ignored. For example, if you set it to /tmp/drupal, then you still get something like
Note that /tmp/drupal is in the allowed paths, but Drupal tried to create a file in /tmp rather than in /tmp/drupal!
Adding /tmp to the allowed paths is a work-around, but it shouldn't be necessary.
(This is for D6.)
Comment #16
danielbeeke2 CreditAttribution: danielbeeke2 commentedFor me this patch works (file_upload_357469_10_d6.patch), but now i get an other error
warning: move_uploaded_file() [function.move-uploaded-file]: open_basedir restriction in effect. File(/tmp/phpz1Xi1Q) is not within the allowed path(s): (/var/www/g30841/projectliberate.com) in /var/www/g30841/projectliberate.com/HTML/includes/file.inc on line 553.
When i submit the content i just made, everything works great
Daniel Beeke
Comment #17
imclean CreditAttribution: imclean commentedsalvis, any luck with this? Looks like you might be onto something, I can't see if/when the file is actually moved to Drupal's temp directory. danielbeeke, this could be the cause of your problem.
Or I should ask, is there any chance of this being fixed in D6?
Comment #18
dijuremo CreditAttribution: dijuremo commentedWe were having this exact problem and it all came down to a bad entry for the variable
upload_tmp_dir
in /etc/php.ini
After properly adjusting this entry and restarting apache, we were able to upload files properly. Note that based on your configuration, you may have that variable commented out in php.ini and are probably setting it elsewhere. If it is neither uncommented/defined in php.ini nor any other location, you will need to set it somewhere.
Comment #19
salvis@dijuremo: Yes, you can make it work by massaging your environment to accommodate Drupal's quirks (and lose some security on the way), but the intent was and should still be to get Drupal to work correctly.
Comment #20
joosts CreditAttribution: joosts commentedHi all,
Any luck on this issue? I have run across the same issue (see http://drupal.org/node/1002048) and am now noticeing that I am not the only one.
Regards,
Joost
Comment #21
nedjoRelated issue: #323110: Remove the open_basedir requirement check.
Comment #22
Damien Tournoud CreditAttribution: Damien Tournoud commentedFrom #16 this seems like a won't fix anyway.
move_uploaded_file() is affected by open_basedir restrictions. That means that you have to make sure that the upload_tmp_dir is in the open_basedir paths for file uploads to work in any PHP application in the first place.
Comment #23
nedjoNot convinced this is won't fix. On my limited testing the patch in #11 resolves the issue without raising the warning reported in #16. More research needed.
Comment #24
Byteme8199 CreditAttribution: Byteme8199 commentedSubscribed...
Comment #25
Caffeine Addict CreditAttribution: Caffeine Addict commentedI'd like to help get this one fixed, but don't know what needs fixing as the old patch above can't be applied anymore to the file.inc, any suggestions on where to start?
Here is the topic i started on this issue and am really interested in getting drupal working smoothly with multi hosting environments:
#1270862: ArchiverZip::extract() needs workaround for open_basedir & stream wrappers
On the bottom of the above topic i did some testing to do with uploading of modules with the open_base_dir restriction in place on the vhost for my drupal site.
Comment #26
salvisCheck whether this issue is still present in D8. If yes, then we need a patch for D8.
Thank you for your help with this!
Comment #27
Caffeine Addict CreditAttribution: Caffeine Addict commentedSo this won't get fixed on D7.X ?
Sure i'll download D8 and test now.
Comment #28
Caffeine Addict CreditAttribution: Caffeine Addict commentedDrupal 8 installs fine on the new version of zpanel with php 5.3.* and the open_base_dir setting enabled, but i need modules to install, plus i think the install module link is missing on the module administration page...
Unable to test...
Comment #29
Caffeine Addict CreditAttribution: Caffeine Addict commentedMy issue is slightly different from this one, got directed from a different post to this one:
#1270862: ArchiverZip::extract() needs workaround for open_basedir & stream wrappers
Going to start some new tests for this bug, already done testing for the other bug on zip file extracts
Comment #30
salvisYes, it will (hopefully) be fixed in D7, and maybe even in D6, but all patches need to go to the latest version first and then be back-ported to older ones if necessary. We can't risk fixing something in D7 and leaving it broken in D8.
It's easier if you install a WAMP or LAMP server on your local computer. This is easier than it may appear — just try it.