ImageCache makes private filesystem straightforward accesible (security issue #1172)

very interesting one. subscribe

subscribe

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 

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.

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.

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

http://drupal.org/node/809184

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.

subscribe

subscribe

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.

"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.

Your right iam sorry, guess iam just frustrated.

Subscribing. Thanks

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.

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

sub

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.

Tagging as D6 stable release blocker.

@drewish Can we close this issue?

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