Security hole with private file system?
| Project: | ImageCache |
| Version: | 6.x-2.x-dev |
| Component: | Code |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed |
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.
| Attachment | Size |
|---|---|
| imagecache_file_download.patch | 613 bytes |
#1
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.
#2
good point, attached a new patch.
#3
Looks good to me now.
#4
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.
#5
committed to HEAD.
#6
thanks drewish, rolled patched from the wrong document root by mistake.
#7
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.
#8
Automatically closed -- issue fixed for 2 weeks with no activity.
#9
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]