Upload.module hard-codes 'view uploaded files' permission check
drewish - April 15, 2008 - 21:09
| Project: | Drupal |
| Version: | 6.x-dev |
| Component: | upload.module |
| Category: | bug report |
| Priority: | critical |
| Assigned: | Unassigned |
| Status: | closed |
Description
The upload module implements hook_file_download() in such a way that once it's enabled for any module to allow download the users must be granted 'view uploaded files' permission. The problem is that the module checks for permissions before checking that it's a file that it controls:
<?php
function upload_file_download($file) {
if (!user_access('view uploaded files')) {
return -1;
}
$file = file_create_path($file);
$result = db_query("SELECT f.* FROM {files} f INNER JOIN {upload} u ON f.fid = u.fid WHERE filepath = '%s'", $file);
if ($file = db_fetch_object($result)) {
return array(
'Content-Type: ' . $file->filemime,
'Content-Length: ' . $file->filesize,
);
}
}
?>The attached patch moves the permissions check down so that it's only enforced once the file is known to be associated with an {upload} record.
To test this you'd need to:
- enable private file transfers
- grant one role 'view uploaded files' permissions
- upload files to a node
- determine the download URL of the uploaded file
- log in as the user with the 'view uploaded files' permissions and ensure that they're able to view the file
- log in as the user without the 'view uploaded files' permissions and ensure that they aren't able to view the file
-
| Attachment | Size | Status | Test result | Operations |
|---|---|---|---|---|
| upload_perms.patch | 1.1 KB | Idle | Failed: Failed to apply patch. | View details |
#1
#2
Big +1 here.
Upload module shouldn't be preventing the downloading of any file it doesn't control. As it is, upload is preventing access to all files (even not in the Edit:
filesupload table).#3
Over on #247780: No links on nodes I got a report from a user that this fixed their problem.
#4
Is this for 7.x-dev only, or would it work on 6.x or 5.x, too?
Thanks!
#5
it will work on D6 as well, hopefully after we get it committed to D7 it can be backported to D6 as a bug fix.
#6
Hi, this fixed the problem for me. I've noticed other iffyness with the upload permissions on another module some time ago but can't remember which one. Thanks drewish for pointing me to this.
#7
Tested out and works great. The code change move is minor but critical for proper handling of files not uploaded through upload.module. This should be backported to 6.x. It probably won't be ported to 5.x though, because upload.module thinks it owns the files table in 5.x (in 6.x, upload.module was given it's own "upload" table).
#8
Related: #226853: upload.module hard-codes 'upload files' permission check
#9
This is definitely a bug. The documentation for hook_file_download is suffering from the same problem, and does not indicate that the hook should not return anything (or NULL) for files not controlled by the current module, so here's a patch to fix that (applies to the docs in contrib).
Also, $file is used both as a string and as an object inside of upload_file_download(), and although PHP allows it, it's bad practice to use the same variable for many purposes. Since we're already patching upload_file_download(), we might as well fix it.
#10
flobruit I made some changes to your docs patch and went ahead and committed. I think you're also correct to use a different variable for the filepath.
#11
I've committed this to CVS HEAD. I'll leave it up to Gabor to commit to DRUPAL-6. I think it is safe to do, but some more eyeballs wouldn't hurt.
#12
Looks like Dries was quick to commit this to Drupal 7. The function argument was renamed from $file to $filepath, which is a great thing, since $file is an object later in the same function. But the db_query() argument is $file which does not have any value at that time (it should also be $filepath as far as I see), or otherwise this will never work. Lets fix in D7 and then backport the right fix.
#13
-1 for my patch reviewing karma. here's the fix.
#14
-1 for my variable renaming skills. The fix is good.
#15
This is a simple bug fix. Lets commit.
#16
Private downloads are currently broken in the 7.x branch because of this, and we have no tests to prove it.
#17
I've just committed the bug fix but we still need tests for it.
#18
Moving to "tests" component for more visibility.
#19
@webchick: shouldn't we try to get this to 6 before (priority-wise)?
#20
Moving back to 6.x, looks like we don't have a committable patch for that branch yet.
#21
I've opened a separate issue for tests so we can concentrate on the Drupal 6 breakage here: #284269: Tests for private file transfers
#22
here's HEAD's patch for D6.
#23
Did some research and found that people experienced the same bug in 2006 and it was fixed in the then current version (4.7.x): http://drupal.org/node/59648 (looks like it was not forward ported).
Anyway Drupal 6.4 was released with this fix in there, which seems like a better idea:
<?php/**
* Implementation of hook_file_download().
*/
function upload_file_download($filepath) {
$filepath = file_create_path($filepath);
$result = db_query("SELECT f.*, u.nid FROM {files} f INNER JOIN {upload} u ON f.fid = u.fid WHERE filepath = '%s'", $filepath);
if ($file = db_fetch_object($result)) {
if (user_access('view uploaded files') && ($node = node_load($file->nid)) && node_access('view', $node)) {
return array(
'Content-Type: ' . $file->filemime,
'Content-Length: ' . $file->filesize,
);
}
else {
return -1;
}
}
}
?>
Note that it does not only check whether the user has the 'view uploaded files' permission, it also checks, whether the user has access to the related node. There are other fixes released in the 6.4 security release, which could be missing from Drupal 7: http://cvs.drupal.org/viewvc.py/drupal/drupal/modules/upload/upload.modu...
BTW there is already an open issue for this node_access() check with the use case of assigning the upload to multiple nodes with different access checks: http://drupal.org/node/295586 so it is not the best end of all things.
#24
here's a patch based on Gábor's code from D6.
#25
The last submitted patch failed testing.
#26
We really, really need tests for this. #352236: Finish moving upload.module to DB:TNG broke the upload permissions again. I've created a new issue to fix the problem, #371206: upload_file_download() Blocks Access to Files Upload Doesn't Own.
#27
Hmm, seems like my patch linked in #26 is the same problem drewish fixes in #24. Either patch will work fine since they're identical in implementation.
Here's a suite for testing private downloads and upload module. Tests the following cases:
- Privileged user is allowed to view files uploaded through upload module.
- Privileged user is not prevented from viewing files not uploaded through upload module.
- Privileged user is not allowed to view an unmanaged file (since no module is available to set headers).
- Unprivileged user is not allowed to view files uploaded through upload module.
I've wrapped back in #371206: upload_file_download() Blocks Access to Files Upload Doesn't Own to make it so that the tests pass, since otherwise they wouldn't as upload.module currently has the bug reported in that issue.
I'll be happy to never see this problem again.
#28
My test for checking "Access Denied" on unmanaged files was backward (I was checking for 200 instead of 404). This patch correctly tests unmanaged files and fixes a spacing issue.
#29
The last submitted patch failed testing.
#30
Updated patch. Also fixes two regressions introduced in #517814: File API Stream Wrapper Conversion (1: uploaded files were always made public, and 2: wrong variable name used in file_build_uri()).
#31
these changes look really good but i'd like to make this query a bit more specific so it doesn't look like it's replicating file_load():
+ $file = db_query("SELECT f.*, u.nid FROM {file} f INNER JOIN {upload} u ON f.fid = u.fid WHERE uri = :uri", array(':uri' => $uri))->fetchObject();to:
+ $file = db_query("SELECT f.filemime, f.filesize, u.nid FROM {file} f INNER JOIN {upload} u ON f.fid = u.fid WHERE uri = :uri", array(':uri' => $uri))->fetchObject();#32
Done.
#33
Sorry, this is better.
#34
And this is even better.
#35
The last submitted patch failed testing.
#36
Reroll.
#37
The last submitted patch failed testing.
#38
A testbot glitch?
#39
This patch works for me.
I tried running the upload test in the patch without the actual changes and I get failures.
Running the upload test with the full patch runs without any problem.
#40
The last submitted patch failed testing.
#41
The tests broke due to an accidental commit in #118345: Revamp hook_user_form/_register/_validate/_submit/_insert/_update (see #38).
#42
The last submitted patch failed testing.
#43
Reroll.
#44
I think I would rather fix this in 7.x with #563000: Remove upload module and provide upgrade path to file. So, assuming that patch makes it in, this is a 6.x-only bug.
#45
sterwa requested that failed test be re-tested.
#46
The D6 issue was fixed in this commit:
http://drupalcode.org/viewvc/drupal/drupal/modules/upload/upload.module?...
There CVS commit message was simply “Drupal 6.4.”, i.e. there was no issue reference. The 6.4 release notes refers to SA-2008-047 that mentions “various Upload module vulnerabilities”.
#47
Automatically closed -- issue fixed for 2 weeks with no activity.