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:

example.com/files/image.png is denied by hook_file_download, but

example.com/files/imagecache/extra_large/image.png 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.

CommentFileSizeAuthor
#4 imagecache_505904.D6.patch803 bytesdrewish
#4 imagecache_505904.D5.patch742 bytesdrewish
#2 505904_imagecache_file_download.patch642 byteskscheirer
imagecache_file_download.patch613 byteskscheirer
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

csevb10’s picture

csevb10 CreditAttribution: csevb10 commented
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

kscheirer
Primary language English
Location Vallejo
CreditAttribution: kscheirer commented
Status: Needs work » Needs review
FileSize
505904_imagecache_file_download.patch642 bytes

good point, attached a new patch.

csevb10’s picture

csevb10 CreditAttribution: csevb10 commented
Status: Needs review » Reviewed & tested by the community

Looks good to me now.

drewish’s picture

drewish CreditAttribution: drewish commented
FileSize
imagecache_505904.D5.patch742 bytes
imagecache_505904.D6.patch803 bytes

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

drewish CreditAttribution: drewish commented
Status: Reviewed & tested by the community » Fixed

committed to HEAD.

kscheirer’s picture

kscheirer
Primary language English
Location Vallejo
CreditAttribution: kscheirer commented

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

drewish’s picture

drewish CreditAttribution: drewish commented

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

rose2 CreditAttribution: rose2 commented

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]