Thanks for a super important module!
I think there might be a mistake in the way the module checks to see if a user has access to a node that a file is attached to.
On line 183 in filefield.module, shouldn't the test be
if (... node_access('view', $node) == FALSE))
rather than
if (... node_access('view', $node))
The current code looks like this:
if ($denied == FALSE && $node = node_load($content['nid']) && node_access('view', $node)) {
// You don't have permission to view the node this file is attached to.
$denied = TRUE;
}
Sorry if I'm missing something, and thanks for your help!
I came across this problem on Drupal 6.13 and Filefield 6.x-3.1, with private files enabled. If a node with a filefield was un-published, anonymous users were correctly denied access to the node itself. Yet they were able to view the file if they went directly to it, via /system/files/filepath
Comment | File | Size | Author |
---|---|---|---|
#11 | filefield-516104-10.patch | 606 bytes | nonzero |
#3 | filefield-node-access-fix-516104-3.patch | 617 bytes | isaac77 |
Comments
Comment #1
quicksketchThanks, this looks like a likely bug. I'll review next time I'm working on filefield.
Comment #2
isaac77 CreditAttribution: isaac77 commentedThanks for looking at this.
In addition to adding the
node_access('view', $node) == FALSE
test, I had to set$node = node_load($content['nid'])
before theIF
. Without that second change things still didn't seem to work on my site (with private files enabled).So the new code, which is working on my site, is:
Not sure it's necessary for a small change like this, but a patch against 6.x-3.x-dev is attached. (Hope it is correct to path against that rather than 6.x-3.1.)
Comment #3
isaac77 CreditAttribution: isaac77 commentedSorry, here's the patch. Seems to have gotten lost when I previewed the comment. Thanks again for your help!
Comment #4
criznach CreditAttribution: criznach commentedPatch works for me.
I've confirmed and tested this patch with two similar setups with private files, content access, and filefield 3.0 and 3.1. The 3.0 site correctly restricts file access, while the 3.1 site grants file access to anonymous users. Files are protected after patch.
Comment #5
espie CreditAttribution: espie commentedAre you kidding ? this is a security issue. Big regression.
For the story, I was using drupal5 with uploads, and I'm switching to drupal6 trying out filefield.
This "feature" thoroughly breaks my site, where people shouldn't be able to acces attached content if they can't access the parent node.
Please consider making a release with just this.
Comment #6
criznach CreditAttribution: criznach commentedComment #7
quicksketchThanks isaac77, patch much appreciated. Committed and I'll make a new release after cleaning up other bugs in the queue.
This bug was introduced in #494176: Less restrictive hook_file_download(). I don't use private files since they are utterly ridiculous in their implementation (a surefire way to make pages take 5-20 times as long to load, as it's a Drupal bootstrap for every file). So I depend on contributor patches for maintaining it. It just so happens the last patch I accepted didn't work quite as advertised.
Comment #8
espie CreditAttribution: espie commentedWell, if you think private files are bad, you should do something about making them better (I agree that a full bootstrap for each file might be too much).
Unfortunately, they're necessary for some application.
What would you consider ? The trouble is, you mostly need access to the database to check grants... especially with respect to users.
I could envision building .htaccess files each time new files get uploaded/permissions changes, but what do you check ? In most cases, drupal users are not really logged in by more than a cookie on the navigator, I don't know how I can check that...
Comment #9
donquixote CreditAttribution: donquixote commentedIt doesn't really matter, but... if you do it like this, you can eventually save the time for node_load. Plus, you make sure you don't call node_access with an empty node (not really necessary, node_access does the check for itself already).
@isaac77 (#2): Seems like PHP lazy evaluation does not work the way we expect...
----------
About private files.
I found this article,
http://www.drupalcoder.com/story/406-mixing-private-and-public-downloads...
My own implementation looks similar, but I don't rewrite to system/files.
Comment #11
nonzero CreditAttribution: nonzero commentedI changed my line 183 to
if ($denied == FALSE && ($node = node_load($content['nid'])) && !node_access('view', $node)) {
and it works fine now. Note the parentheses around the center condition. && has precedence over =. Similar to donquixote's fix, this short circuit may be slightly more efficient than isaac77's patch since it doesn't call node_load unnecessarily.
Comment #12
grendzy CreditAttribution: grendzy commentednonzero: This has been fixed in the dev release.
isaac77: In the future I think it would be better to report issues like this confidentially, please see How to report a security issue.
Thanks!
(p.s. if you want just the patch for this issue it's here: http://cvs.drupal.org/viewvc.py/drupal/contributions/modules/filefield/f... )
Comment #13
j0nathan CreditAttribution: j0nathan commentedsubscribing...
Comment #14
nonzero CreditAttribution: nonzero commentedIt appears that the "stable" release has still not incorporated the fix in this bug report. I use FileField for access controlled file sharing quite often for production sites, so I think it's important this critical security issue gets committed to the stable release soon. If someone else agrees, please change the status from "closed" to something else. Thanks.
Comment #15
quicksketchThanks nonzero, I committed your version since I appreciate the shorter syntax. I'll be making a new release today.
Comment #16
threexk CreditAttribution: threexk commentedWhy wasn't this reported as a security issue? (Posted to security advisories list, marked as security update in Update Status module, etc.) Sounds like it exposed files that should be protected.
Comment #17
threexk CreditAttribution: threexk commentedLooks like it hit the Security-news mailing list now. Maybe I was being impatient. Thanks to all for catching and fixing this.