I'm opening this issue as a notice to all users having ImageCache enabled in a site with private download method
nevertheless I won't disclosure its details until further the next week, since quicksketch said that it should be solved (probably) by tomorrow

for further reference on why I'm partially disclosing it refer to #791044-11: What would the role of the Security Team precisely be?

CommentFileSizeAuthor
#7 imagecache_796384.patch1.14 KBdrewish
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

arhak’s picture

Issue tags: +security

tagging

arhak’s picture

bellow is the original report sent to the Security Team on January 25, 2010
Note that this security hole can be exploited from an anonymous account

ImageCache module leaves a public callback (access=>TRUE) opened even when download method is private
leading to the straightforward access to protected files EVEN being out of the webservers reach
conflictive code is detailed in the explanation below

How to reproduce it:
- fresh Drupal 6.15 (with clean urls)
- admin/build/modules
-- enable dhtml_menu-6.x-3.5
-- enable imageapi_gd (imageapi-6.x-1.6)
-- enable imagecache_ui (imagecache-6.x-2.0-beta10)
- admin/settings/file-system
-- Download method: Private
- admin/build/imagecache/add
-- Preset namespace: test
-- Add Scale: 50%x50% (admin/build/imagecache/1/add/imagecache_scale)
-- Note the sample image appears under "system/files": http://localhost/dev/imagecache/system/files/imagecache/test/imagecache_sample.png?1264462815
- logout or open another browser (anonymous user)
-- visit the same image but under "sites/default/files" instead:
---- http://localhost/dev/imagecache/sites/default/files/imagecache/test/imagecache_sample.png
---- http://localhost/dev/imagecache/?q=sites/default/files/imagecache/test/imagecache_sample.png
>>>> the image is served! (WOW!)
-- re-visit the image under "system/files" (now as anonymous user)
---- http://localhost/dev/imagecache/system/files/imagecache/test/imagecache_sample.png
>>>> the image is not server (as expected)

- login as uid1
- admin/settings/file-system
-- File system path: ../../../non-www/bug_imagecache
-- Download method: Private
- re-visit admin/build/imagecache/1 and re-save the form (so the respective directory be created at the new location)
- delete the previous "files" directory (sites/default/files)
- logout or open another browser (anonymous user)
- re-visit the image as being under "sites/default/files"
-- http://localhost/dev/imagecache/sites/default/files/imagecache/test/imagecache_sample.png
-- http://localhost/dev/imagecache/?q=sites/default/files/imagecache/test/imagecache_sample.png
>>>> the image is served!! (WOW!!)
- clear the cache (or just menu rebuild)
- refresh the stolen image
>>>> the image is not server (as expected)
- visit the image like this
-- http://localhost/dev/imagecache/?q=../../../non-www/bug_imagecache/imagecache/test/imagecache_sample.png
>>>> the image is served!!! (WOW!!!)

Explanation:
- regardless:
-- the way the image was uploaded (upload module, filefield, or even FTP)
-- the private download being under the web server or not
-- the imagecache permissions not assigned to anyone
- facts:
-- imagecache_menu declares two callbacks

  // standard imagecache callback.
  $items[file_directory_path() .'/imagecache'] = array(
    'page callback' => 'imagecache_cache',
    'access callback' => TRUE,
    'type' => MENU_CALLBACK
  );
  // private downloads imagecache callback
  $items['system/files/imagecache'] = array(
    'page callback' => 'imagecache_cache_private',
    'access callback' => TRUE,
    'type' => MENU_CALLBACK
  );

-- even when download method is private the public callback works (security hole)
-- note that having clean-urls enabled allows to take control via htaccess, but the non-clean-urls path will remain opened
-- ANOTHER related issue (which shouldn't be reported because it disclosures the security hole)
changing the "File system path" while using "Download method: Private" REQUIRES a menu rebuild
(actually it requires a menu rebuild for any change made, even when Download method is Public, but in that case it wouldn't be a security hole but a simple bug)

marcoka’s picture

very interesting one. subscribe

grendzy’s picture

subscribe

drewish’s picture

There was a really screwy message thread that got this to me originally. I tried replying to the sender but it was not the original reporter. Here's what I'd said:

> -------- Original Message --------
> Subject: [security] Fw: ImageCache makes private filesystem straightforward accesible
> Date: Mon, 25 Jan 2010 19:53:31 -0500
> From: Antonio de Rojas
> Reply-To: security@drupal.org
> To:
>
> BTW, I forgot to mention that I searched the queue of ImageCache for related issues
> it seems that this can be discovered following steps at http://drupal.org/node/456796#comment-1608454
> which was posted on May 2009 and last subscribed yesterday
>  
> ----- Original Message -----
> From: Antonio de Rojas
> To: security@drupal.org
> Cc: arhaks@gmail.com
> Sent: Monday, 25 January, 2010 7:35 PM
> Subject: ImageCache makes private filesystem straightforward accesible
> ImageCache module leaves a public callback (access=>TRUE) opened even when download method is private
> leading to the straightforward access to protected files EVEN being out of the webservers reach
> conflictive code is detailed in the explanation below
>  
> How to reproduce it:
> - fresh Drupal 6.15 (with clean urls)
> - admin/build/modules
> -- enable dhtml_menu-6.x-3.5
> -- enable imageapi_gd (imageapi-6.x-1.6)
> -- enable imagecache_ui (imagecache-6.x-2.0-beta10)
> - admin/settings/file-system   
> -- Download method: Private
> - admin/build/imagecache/add
> -- Preset namespace: test
> -- Add Scale: 50%x50% (admin/build/imagecache/1/add/imagecache_scale)
> -- Note the sample image appears under "system/files": http://localhost/dev/imagecache/system/files/imagecache/test/imagecache_...
> - logout or open another browser (anonymous user)
> -- visit the same image but under "sites/default/files" instead:
> ---- http://localhost/dev/imagecache/sites/default/files/imagecache/test/imag...
> ---- http://localhost/dev/imagecache/?q=sites/default/files/imagecache/test/i...
> >>>> the image is served! (WOW!)

I don't really think that's a wow, it's the documented behavior which is why we recommended putting the files directory outside of the webroot. After moving the files directory I'm not able to access the images using this technique.
 
>
> -- re-visit the image under "system/files" (now as anonymous user)
> ---- http://localhost/dev/imagecache/system/files/imagecache/test/imagecache_...
> >>>> the image is not server (as expected)
>  
> - login as uid1
> - admin/settings/file-system
> -- File system path: ../../../non-www/bug_imagecache
> -- Download method: Private
> - re-visit admin/build/imagecache/1 and re-save the form (so the respective directory be created at the new location)
> - delete the previous "files" directory (sites/default/files)
> - logout or open another browser (anonymous user)
> - re-visit the image as being under "sites/default/files"
> -- http://localhost/dev/imagecache/sites/default/files/imagecache/test/imag...
> -- http://localhost/dev/imagecache/?q=sites/default/files/imagecache/test/i...
> >>>> the image is served!! (WOW!!)

I can't replicate this. Are you sure you really deleted the files directory? I setup my test site in the webroot e.g "http://localhost". Does it depend on being in the subdirectory?
 
>
> - clear the cache (or just menu rebuild)
> - refresh the stolen image
> >>>> the image is not server (as expected)
> - visit the image like this
> -- http://localhost/dev/imagecache/?q=../../../non-www/bug_imagecache/image...
> >>>> the image is served!!! (WOW!!!)

Okay, I can reproduce this and I agree that it is a bug. It looks like you could probably also get http://localhost/dev/?q=../../../non-www/bug_imagecache/imagecache/test/... to work as well since the q part overrides the path. 
 
>
>  
>
> Explanation:
> - regardless:
> -- the way the image was uploaded (upload module, filefield, or even FTP)
> -- the private download being under the web server or not
> -- the imagecache permissions not assigned to anyone
> - facts:
> -- imagecache_menu declares two callbacks
>   // standard imagecache callback.
>   $items[file_directory_path() .'/imagecache'] = array(
>     'page callback' => 'imagecache_cache',
>     'access callback' => TRUE,
>     'type' => MENU_CALLBACK
>   );
>   // private downloads imagecache callback
>   $items['system/files/imagecache'] = array(
>     'page callback' => 'imagecache_cache_private',
>     'access callback' => TRUE,
>     'type' => MENU_CALLBACK
>   );
> -- even when download method is private the public callback works (security hole)
> -- note that having clean-urls enabled allows to take control via htaccess, but the non-clean-urls path will remain opened
> -- ANOTHER related issue (which shouldn't be reported because it disclosures the security hole)
> changing the "File system path" while using "Download method: Private" REQUIRES a menu rebuild
> (actually it requires a menu rebuild for any change made, even when Download method is Public, but in that case it wouldn't be a security hole but a simple bug)
>  

Looking at system_menu() it also always defines 'system/files'. The real problem seems to be the public menu callback. I seems to me that changing the access callback on that to check the variable would suffice. What do you think?
 
>
> regards,
> Antonio
> BTW, I'm user arhak at d.o. (http://drupal.org/user/334209)
>  
> PS: the project page shows imagecache-6.x-2.0-beta9 as recommended while the LATEST release is actually imagecache-6.x-2.0-beta10 http://drupal.org/node/67819/release this seem to be an infrastructure bug doing wrong version comparisons (which leads to users downloading improper versions, and therefore next releases of this module wouldn't be the recommended one)
>   

Yeah people have mentioned that. I suppose I should just post a 2.0 release when we fix this.
andrew 

arhak’s picture

I seems to me that changing the access callback on that to check the variable would suffice. What do you think?

IMO a menu rebuild would be required

drewish’s picture

Status: Active » Needs review
FileSize
1.14 KB
arhak’s picture

@#7 oh, I see your point, looks good to me

arhak’s picture

BTW, that fixes the public access issue
but what about accessing files under a different path? e.g. ?q=../../../non-www/...
wouldn't that need some sort of check to verify the requested file is under ImageCache's control (i.e. some preset subdir) ?

drewish’s picture

Version: 6.x-2.0-beta10 » 6.x-2.x-dev

Yeah you're correct we shouldn't be registering menu callbacks for paths outside of the system root. Considering that it might be best to just follow the plan you'd described of adding some conditions to imagecache_menu() so we only register the public file transfer callback when public file transfers are enabled and the path is sane. We'd probably need to add a #submit handler to the system_file_system_settings form.

arhak’s picture

that was my first thought, because once there, you can react to path changes (in addition to "private vs public" changes)
nevertheless, what would be left for installation profiles then?
programmatically setting up private downloads wouldn't trigger the #submit handler
but requiring to rebuild the menu in such cases doesn't sounds that bizarre

thoughts?

arhak’s picture

I liked your 'access callback' approach because (once you need a proxy function/path):
- it is indifferent to whether the change comes from UI submission, code setup or even a DB backup restoration
- it doesn't requires menu rebuild (which might partially fail due to 3rd party code misusing the locking framework e.g. #251792 & #246653)
- it makes simpler to test if that back-door is really closed (getting ride of it when download method is private might leave the question on whether it could be achievable to let it fully open by mistake)

in addition, I think another sort of checking would be nice, for instance

/**
 * Check if a file is really located inside $directory. Should be used to make
 * sure a file specified is really located within the directory to prevent
 * exploits.
 ...
 */
function file_check_location($source, $directory = '') {
  ...
  $directory = realpath($directory);
  if ($directory && strpos($source, $directory) !== 0) {
    return 0;
  }
  ...
}

PS: note that file_check_location will figure the realpath of both operands to determine whether source really is under the target directory (which should properly work with ?q=../../non-www/...)

drewish’s picture

We currently have the issue with the menus needing a rebuild after the files directory changes. Since we currently don't deal with that I suppose we could just go for an access callback that just checks for a file_directory_path() that has no "../" or "/.." s in it.

arhak’s picture

when having private download method with a non-www directory I would prefer to have a "../../non-www/imagecache" path rather than an absolute path which reveals the directory structure of the server and therefore exposes its vulnerabilities (based on OS for instance)

removing dotted paths doesn't seems to be the best solution (IMO), since Drupal allows relative paths and still avoids exploits by means of file_check_location to ensure the addressed file(s) resides within the intended "files" directory
IMO ImageCache would need to do something similar but adding its files/imagecache subdirectory to avoid the user getting unintended sibling files
then it would be valid to access ?q=../non-www/mysite-files/imagecache/... if ../non-www/mysite-files is actually the "files" directory

just throwing my thoughts

EugenMayer’s picture

Please have a look at that API additoin to imagecache which makes fixing this issue trivial.

http://drupal.org/node/809184

drewish’s picture

arhak, I wasn't clear, the access callback I was describing was only for the public file menu item so we didn't inadvertently allow directories outside the webroot. For private transfers we don't need to worry about it, and shouldn't worry about it for exactly the reasons you mention.

arhak’s picture

#15 I don't clearly see how it could solve this issue

#13, #16
so, you're saying:
- leave the rebuild menu issue for later (I'm thinking about a hook_requirements to guard its consistency)
- create the menu callback depending on the download method (conditionally, never both at the same time)
- and both callbacks remain the same as they currently are?

how would intruders be stopped from getting ?q=system/files/imagecache/../out-of-imagecache-domain?
this is what I can't see without relying in something like file_check_location

izmeez’s picture

subscribe

idflood’s picture

subscribe

EugenMayer’s picture

As imagecache is accessing the file on the Filesystem, then "copying it" ( derivate ) to a place, which is open in the public ( {filefolder}/imagecache ) and does NOT create a file-entity for that file, you wont be able to protect this derivates as they are transfered using apache directly, if the user just guesses the filepath using yourdomain.tld/sites/default/files/imagecache/100x100/private.png .

Using http://drupal.org/node/809184 for correcting the access url for the preset and http://drupal.org/node/810642 for moving the preset into a private directory ( files/private/imagecache/100x100/private.png ) you can solve this issue. I have already implemented this. All you need then is putting a global htacces deny all file into private.

Yet, using the current approach, the access url is changed to use a menu-callback to check rights. This works out for the first part, while the implementation is not extensible at all ( thats what the first patch is for ). What imagecache does miss completely is to secure the derivates path. Eventhough you can place the "files" folder out of the range of apache, so you can never access derivates directly, this will end in a total mess. As all files NOT using the fileapi, but using imagecache will land there - without having a menu-callback to access them, as they are not part of the fileapi.

But i guess those feature will dry and die in the queue like the others do.

arhak’s picture

@#20 I'm not saying that your approach wouldn't work, I'm saying it is not "clearly" solving this security issue, rather it is incorporating new features, which will require more testing (most of all for backward compatibility with current configurations user might have implemented already)
and therefore, they might be wonderful, but will require a longer live-cycle to be committed

But i guess those feature will dry and die in the queue like the others do.

regretfully, this module hasn't been under active development for 6.x, and D7 is right there...
thus, I guess it would be fine to get a decent fix for this security hole before moving towards more elaborated and extensible approaches (which in turn might bring their own security holes in the meantime they are getting tuned and tested)

EDIT: typos

EugenMayer’s picture

"its not clearly solving this issue" . Sorry, could it be, that you simply dont have time to think over it? New features? Those to things are API`s ! Not a single feature is in there, all the features "might come" could be implemented using other modules.

to be honest, i simply see that people dont take their time to inverstigate, whether they focus on Drupal 7 or something else.

fixed for me, do whatever you want.

arhak’s picture

@#22 wait EugenMayer, you're getting angry at me, but I'm not even a maintainer of this module
I'm not officially declaring that your approach won't be considered, I just expressed my opinion

to be honest, i simply see that people dont take their time to inverstigate, whether they focus on Drupal 7 or something else.

you're totally right, particularly I'm not in a good mood to battle for some improvement on ImageCache 6.x
when I've been battling for over four months to fix a simple security hole and it hasn't been done yet

EugenMayer’s picture

Your right iam sorry, guess iam just frustrated.

gausarts’s picture

Subscribing. Thanks

EugenMayer’s picture

just to be sure people dont wait - iam not going to work on this issue. I fixed it the way described above and tested it. It works for some months in production.

Iam not interested in wasting my time in this issue, like in the others. so if you are waiting for a approach here, i suppose you have to do it yourself.

Sorry.

Fabianx’s picture

Assigned: Unassigned » Fabianx

Hi,

Having just switched a site from public to private I took some interest in this issue.

First: Patching this is better than not patching this at all - regardless if the approach used is still having issues.

The policy for security changes is always:

- Make it work for most common cases first (but leave issue open to work on).

Compare this with an open door:

- Sure you can say that if that door is open and you put a door before it, that you can still use an axe to break it open and that you can still forge a second key to open that door.

However no one would come to the idea or conclusion that it is bad to install that door, because it still has issues.

The other comparison (menu cache issue) would here to say:

* Okay we add a door, but leave it opened until the next (menu) cache clear.

This will eventually happen and then the door is closed.

And if there is still a backdoor (../) it is still not a good idea to leave the front door open, too.

So:

Approach:

Lets first fix this with a patch like (pseudo code):

hook_menu() {
if (file_download_method == PUBLIC) {
$items[] = array() // Public handler
}

$items[] = array() // Private handler
}

This should work for most cases and close this door. (after a menu cache)

Now lets work on the menu_cache issue (door open before menu_cache run).

What does drupal core do if there is a security issue?

- Bug the user each time with a huge message.

What do node access modules do if a rebuild is needed?

- Bug the user each time with a huge message.

Okay, how can we detect a fs change?

Easy with a variable:

hook_menu() {
if (file_download_method == PUBLIC) {
$items[] = array() // Public handler
}

$items[] = array() // Private handler

variable_set('imagecache_stored_dl_method', $file_download_method);
}

Then test in hook_init():

if ( variable_get('imagecache_stored_dl_method', NULL) != variable_get('file_download_method', PUBLIC) && user_access('administer system') &&('is an admin loctation')) {
drupal_set_message('Warning: Your system is insecure. You need to clear the menu cache before imageache will stop serving from public directory.');
}

Also possibly add a message to status report.

If a user ignores that besides having made an important system change -> Same as with security updates. His responsibility.

So now we have public access closed.

And this easy patch just should go in so that it is fixed for most cases, but does not break anything.

Lets just put a door before that.

Are there any remaining issues now someone can think of?

So that we can _then_ work on making it a door out of steel, etc.

Gonna create the real patch for this probably on Monday.

Best Wishes,

Fabian

benone’s picture

sub

drewish’s picture

Status: Needs review » Active

Committing the patch from #7 since it fixes part of this. #1135480: When file system path is changed, imagecache breaks took care of rebuilding the menus when the file system settings change mentioned in #13.

EugenMayer, I'm going to mark #809184: Add API to modífy imagecache path prefix as a dupe of #570132: Support CDN rewriting of ImageCache paths and go review and commit the later.

Leaving this as active so we can deal with the .. issue.

fizk’s picture

fizk’s picture

@drewish Can we close this issue?

fizk’s picture

Assigned: Fabianx » Unassigned
EugenMayer’s picture

This issue ist def. not closed / fixed, just to be sure that kind of clear :)