Upload.module hard-codes 'view uploaded files' permission check

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: files upload table).

Is this for 7.x-dev only, or would it work on 6.x or 5.x, too?
Thanks!

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.

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

Related: #226853: upload.module hard-codes 'upload files' permission check

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.

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.

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.

-1 for my variable renaming skills. The fix is good.

This is a simple bug fix. Lets commit.

Private downloads are currently broken in the 7.x branch because of this, and we have no tests to prove it.

I've just committed the bug fix but we still need tests for it.

Moving to "tests" component for more visibility.

@webchick: shouldn't we try to get this to 6 before (priority-wise)?

Moving back to 6.x, looks like we don't have a committable patch for that branch yet.

I've opened a separate issue for tests so we can concentrate on the Drupal 6 breakage here: #284269: Tests for private file transfers

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:

/**
 * 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.

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.

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.

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.

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

Done.

Sorry, this is better.

And this is even better.

Reroll.

A testbot glitch?

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.

The tests broke due to an accidental commit in #118345: Revamp hook_user_form/_register/_validate/_submit/_insert/_update (see #38).

Reroll.

I think I would rather fix this in 7.x with #563000: Provide upgrade path to file. So, assuming that patch makes it in, this is a 6.x-only bug.

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