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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

quicksketch’s picture

Priority: Critical » Normal
Status: Active » Needs review

Thanks, this looks like a likely bug. I'll review next time I'm working on filefield.

isaac77’s picture

Thanks 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 the IF. 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:


$node = node_load($content['nid']);
if ($denied == FALSE && node_access('view', $node) == FALSE) {
  // You don't have permission to view the node this file is attached to.
  $denied = TRUE;
}

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.)

isaac77’s picture

Sorry, here's the patch. Seems to have gotten lost when I previewed the comment. Thanks again for your help!

criznach’s picture

Status: Needs review » Reviewed & tested by the community

Patch 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.

espie’s picture

Are 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.

criznach’s picture

Priority: Normal » Critical
quicksketch’s picture

Title: Incorrect node_access check? » Node access check for private files does not check node_access()
Status: Reviewed & tested by the community » Fixed

Thanks 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.

espie’s picture

Well, 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...

donquixote’s picture

It 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).

<?php
      if ($denied == FALSE && $node = node_load($content['nid'])) {
        if (!node_access('view', $node)) {
          // You don't have permission to view the node this file is attached to.
          $denied = TRUE;
        }
      }
?>

@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.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

nonzero’s picture

Status: Closed (fixed) » Needs review
FileSize
606 bytes

I 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.

grendzy’s picture

Status: Needs review » Closed (fixed)

nonzero: 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... )

j0nathan’s picture

subscribing...

nonzero’s picture

It 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.

quicksketch’s picture

Thanks nonzero, I committed your version since I appreciate the shorter syntax. I'll be making a new release today.

threexk’s picture

Why 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.

threexk’s picture

Looks like it hit the Security-news mailing list now. Maybe I was being impatient. Thanks to all for catching and fixing this.