This is my first site using the private file system, but as far as I can tell, the only thing that happens when you switch from public to private is that:

  • your urls change
  • hook_file_download() gets fired when a file is requested. So now you can implement this hook in your code (or get a module that does) and decide whether to serve the file or not.

The Imagecached images get sent to imagecache_cache_private(), which checks to make sure the user has access to the preset before serving the image. But it doesn't check anything else. I believe this means that you could request any image on the site if you knew the name of the preset and the name of the image because Imagecache doesn't check for file download permissions. To demonstrate: is denied by hook_file_download, but will be displayed if you have access to the preset 'extra_large'.

in imagecache_cache_private() I think there should be a call to module_invoke_all('file_download', $source, $preset) to ensure that the user has access to the original file as well as the preset. The attached patch adds that check.

Members fund testing for the Drupal project. Drupal Association Learn more


csevb10’s picture

Status: Needs review » Needs work

First, this patch makes complete sense, and I would definitely consider it a security/privacy hole. I think the approach is correct to solving the issue, I think just a small tweak is required for it to be in working order.

The module_invoke_all call will return an array. The default call to module_invoke_all for file_download uses the following check: if (in_array(-1, $headers)). It seems like it'd make sense to do the same thing here.

kscheirer’s picture

Status: Needs work » Needs review
642 bytes

good point, attached a new patch.

csevb10’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me now.

drewish’s picture

here are patches that actually apply. i think we might need to revisit this later because we're discarding the headers returned by hook_file_download(). but for now it's an improvement.

drewish’s picture

Status: Reviewed & tested by the community » Fixed

committed to HEAD.

kscheirer’s picture

thanks drewish, rolled patched from the wrong document root by mistake.

drewish’s picture

oh the problem was that that the first patch had to be applied and then the second, rather than just applying one patch. also for future refererence you should report security issues to the security team. it helps insure that issues like this don't languish unnoticed.

Status: Fixed » Closed (fixed)

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

rose2’s picture

I do agree that you should attach a new patch for it, if the first one doesn't work. You may also search it for further clarification and information. thanks for the post. [scottsdale security systems]