Krimson (http://krimson.be) asked me to do a quickscan of this module, with a focus on unauthorised access to private files.

Private Upload (PU) works by

- vetoing a download in hook_file_download by returning -1 for "private files" to which the user has no access.
- not interfering by returning NULL for files not governed by PU or for "private files" to which the user has access.

Other modules (eg upload) are responsible for allowing the upload by returning headers from their hook_file_download implementation.

PU determines which files should be private by doing (some odd) string manipulation; it tests whether the path starts with the private directory path.

This means that PU can be bypassed by passing a string that *is* a valid path on the filesystem in the private directory, but does not start with the private upload directory (yes, canonicalization is a pita).

This can be achieved by passing an extra pathseparator:

wget http://example.com/?q=system/files//private/[confidential_file]

Now, this particular bypass also causes the upload module to not send headers and so results in a 404 when upload is the only file module sending headers. It might be possible to generate a bypass purely with PU and Upload, but that was not my goal.

Any other module sending headers for existing files (such as IMCE) will result in Drupal serving a private/confidential file.

I recommend testing whether a file resides in the private directory by using filesystem functions. file_check_location is a likely candidate.

As this module has no stable releases, this issue can be handled in public.

Please do not yet make a new release; there's another issue that also occurs in modules with releases. I'll add an issue for private upload once those are fixed.

Comments

burningdog’s picture

burningdog CreditAttribution: burningdog commented

I couldn't reproduce. I attempted to download a private file by using the extra path separator:

wget http://example.com/?q=system/files//private/[confidential_file]

...but I still got an Access Denied, which is what I expect to get. The reason that access gets denied is because of the function that is used to check access, which is called like so:

  $private_dir = variable_get('private_upload_path', 'private');
  if(_private_upload_starts_with($file, $private_dir)) {

The checking function is:

/**
 * Utility
 * @return bool: does str1 start with str2
 */
function _private_upload_starts_with( $str, $start ) {
  if( count($str) == 0 ) return false; // avoid false positive.
  return strstr($str, $start) == $str;  
}

The documentation for _private_upload_starts_with() incorrectly states, "does str1 start with str2" when in fact it uses the strstr function to check if the name of the private upload path is present in the file download path, and if it is, it denies access.

Yes, we could check for the existence of the file using a filesystem function before we grant access to it, but given how private_upload checks if a file is private, using the filesystem function won't solve any particular problem.

Heine’s picture

Heine CreditAttribution: Heine commented

If you install a module (such as IMCE) that always responds positively in it's hook_file_download, PU will not prevent download.

burningdog’s picture

burningdog CreditAttribution: burningdog commented

Hmmm...that's strange - I thought that if a single module denied access to a file, then it didn't matter how many allowed access. I'll install IMCE now and take a look.

mrbubbs’s picture

mrbubbs CreditAttribution: mrbubbs commented

Roger, did you happen to get to test this yet? Would changing module weight have any effect on file access once a module denies access?

berliner’s picture

berliner CreditAttribution: berliner commented

Reading the code of file_download() in includes/file.inc (http://api.drupal.org/api/drupal/includes--file.inc/function/file_downlo...) it is indeed supposed to work as described in #3

berliner’s picture

berliner CreditAttribution: berliner commented

I tested this with the dev version and could reproduce the behaviour. A simple cast to boolean mysteriously fixed it for me, mysteriously because I verified before that the result of strstr is really of type boolean so the cast should be unnecessary:

/**
* Utility
* @return bool: does str1 start with str2
*/
function _private_upload_starts_with( $str, $start ) {
  if( count($str) == 0 ) return false; // avoid false positive.
  return (boolean) strstr($str, $start) == $str;  
}

The reason Roger can't reproduce this might be different php versions maybe? I ran this on 5.2.13 with Drupal 6.19 and Private Upload 6.x-1.x-dev (2010-Jul-11). Anyway, you could use strpos($str, $start) !== FALSE. That works (at least for me) without any casting needed and simply checks whether $start is contained in $str.

Also I installed IMCE, verified that its hook_file_download gets really called and returns file headers. The access control of private_upload works nevertheless which is consistent with the documentation of hook_file_download().

Apart from that it would be needed that private_upload returns file headers instead of NULL to be consistent with hook_file_download.