What steps will reproduce the problem? 1. Use the default timthumb.php with default $allowedSites settings. 2. Load remote file http://blogger.com.example.com/attack.php file so it gets stored in cache dir 3. Access /wp-content/themes/yourtheme/timthumbdir/cache/attack.php and it will execute on most servers

What is the expected output? What do you see instead?

If attack.php is a hacker shell app like Alucar shell, you have access to the server with whatever priveleges the web server account has. e.g. you can read /etc/passwd

What version of the product are you using? On what operating system?

I'm using 1.28 but it looks like trunk is vulnerable. You're using strpos to check for a valid external host:

if (strpos (strtolower ($url_info['host']), $site) !== false) {

Please provide any additional information below.

Even though this is easily fixable, it probably affects thousands of existing WP installations where timthumb.php was bundled with a theme. I would immediately contact theme developers.

This is already in the wild and may have been for some time now. My server was compromised earlier today. I tracked it down to timthumb.php and confirmed the attack script was in the timthumb cache directory.

A file containing a base64 encoded Alucar shell was uploaded, executed and the attacker used the shell to inject ads into my blog. He/she may have done a lot more damage that I'm not aware of yet.

A screenshot of the shell running on my blog is attached.

More details on my blog (I'm filing this bug first though): http://markmaunder.com/

I've gone ahead and added the following code to line 719 to check the filename.

if(preg_match('/.php$/',$localfilepath,$bad_filenames) == 1) display_error("someone is doing something they shouldn't");

Upon thinking a couple more minutes about it, you should check to make sure the filename ends in a graphic format rather than trying to track down every possible executable suffix someone might have on their server.

I think you're using an older version of timthumb.php. Check what's in trunk:

http://code.google.com/p/timthumb/source/browse/trunk/timthumb.php

Indeed the original attack here required a version which was pre r141, so more than a few days old.

There still exists a problem here however, in that files from any domain can be used if part of the domain exists within the domain. For example, currently 'www.wordpress.com.dd32.id.au' will pass through the $allowedSites checks as it contains 'Wordpress.com'.

Attached is a patch which uses a basic regular expression to check to see if it's actually that domain (or a subdomain).

There is a further potential issue here, If an attacker uploads an item which passes as a image (ie. bypasses r141's mime type check - EASY to do with PHP, simply prefix the content of the file with 'GIF8' and PHP thinks it's a gif), AND the server is setup insecurely (Some servers pass ALL files through PHP, or at least, files which have no extension/mime type specified) then there's still the potential for code execution.

I recommend timthumb uses a caching mechanism other than storing remotely loaded files under the wordpress root. You could use /tmp with a cleanup mechanism. It's easier than trying to anticipate the infinite number of ways that visitors being able to write to a web accessible directory could be exploited.

There were two patches this weekend. One which checks the file type and one which removes the file extension on cached data which should mean the files can not be executed.

The idea of storing external files in the /tmp directory is tempting so I will think about that as well. Another option would be for users to change the cache directory path.

I have asked on Twitter for help with hardening security for the script a few times. I am not an expert in this area (clearly) so would value any help I can get in the matter.

just committed another update which (I think) fixes the issue mentioned above regarding using domains that aren't clean (eg http://wordpress.com.hacker.com)

In the allowedSites check you should probably check the end of the hostname matches the current checked host. I'd use substr and strpos to do that.

I have changed the allowed domain check to use a regex as suggested by Mark in the separate patch submission so the latest version 'should' resolve the domain issue.

The php file execution on the Apache server is triggered by the filetype, given by the name. Because of that, I would strongly recommend to do a whitelist filetype check.

(untested) pseudocode for better allowed sites matching: http://pastebin.com/RstGh0ZX

(untested) pseudocode for better temporary file management: http://pastebin.com/E5RFwRwR

I'm quite surprised, I would expect a cache directory (not intended to be web-accessed) protected by an htaccess ("deny all"). Yes, this will only protect apache users, but it's a good start.

For the others, a way would be to hash the filename (using local SALT), sothat nobody except the script knows how the uploaded file may be called.

Finally, if not already the case, the cache directory should not be browseable. Universal way to avoid this: add an empty index.php file.

Hope this makes sense!

Demitrio - I tried your code and it didn't work on my test server because safe mode was enabled. I did modify it a bit to remove a bunch of the errors created but it still didn't work properly. Besides this, I am not sure of the benefit of your suggestion since the script currently does very similar things, only in the cache directory?

Werner - thanks for the comments. I think a lot of these things are up to the developers of themes and plugins to implement (.htaccess and empty index.php). Both good ideas though and I will add them to the docs. The salt idea is also awesome, but I can't see the average user changing these things either. Most users purchase a theme and TimThumb comes installed, they are not going to think to change the salt themselves. If you can think of a way to generate a unique salt per machine then I would be happy to use that instead.

Use sys_get_temp_dir() as cache dir. IMHO this is also necessary.

Issue 218 has been merged into this issue.

@BinaryMoon The problem with using ./cache/ for storage is that you are pulling untrusted files over the internet and placing them in a location that is almost sure to be directly accessible and executable by the web server. This is the essence of the vulnerability.

Software that accepts uploads has to go to great pains to ensure that the uploaded files cannot be executed. MIME checks are not sufficient as mentioned in comment #4. Even domain checks may not be of use if I can create a wordpress.com blog to host malicious files.

Using /tmp/ is really much safer, since it won't be beneath the document root of the web server.

Comment deleted

If I force check the file extension to be in jpg/gif/png, will that prevent the php files from being uploaded?

The script already checks the file type so that's not really a solution.

Alberge - I totally understand that already, I'm not arguing against it. What I questioned above is that the code provided uploaded the files to the temp directory and then copied to the cache directory, which defeats the purpose of using the temp directory to start with.

I tried implementing the code in the temp directory on my timthumb test server and got safe mode errors, which makes me wonder if this is viable since the code needs to 'just work'.

@BinaryMoon - What about renaming the cache file like "image.png" to "image.png.txt". No one will be able to execute this file.

Great minds think alike - check out the latest commit - I made this change about 80 minutes ago :)

http://code.google.com/p/timthumb/source/detail?r=149

@BinaryMoon - re #14 - I've made a comment on another thread (issue 217) about this. On wordpress you might be able to pull one of the security defines (e.g. NONCE_KEY, SECURE_AUTH_KEY) as a salt. If those are not available or empty, then perhaps you can generate one and place it somewhere as a variable/define - maybe inside this (now empty) index.php file in the cache folder? and then pull its value from timthumb... Of course you'd need to make sure it is not leaked. and you should probably use hmac instead of md5 too, but that's a relatively small improvement compared to using a secret key / salt in the first place.

I considered this as well.

The goal of TimThumb is to work in any application - WordPress is not guaranteed. As such I decided to use the SERVER[DOCUMENT_ROOT] as the salt.

It's not a perfect solution but it does add an extra layer of protection that should slow people further.

This was added in revision 148 - http://code.google.com/p/timthumb/source/detail?r=148

the server document root is a highly predictable value though, so really not recommended for use as a secret key.

You really should try to get or generate a pseudo-random value. If you can't get one from e.g. Wordpress, then you can always generate one yourself, and store it somewhere (inaccessible). I know it's probably not too elegant, but I think it's potentially more robust than relying on the fact that the cache folder is not externally accessible.

I agree that it's guessable, but it's a lot harder to guess than the old version which had no attempt at a salt at all. I haven't stopped thinking about the security and will likely change things further in a future update.

Great. Looks like there's a bunch of people here who are willing to help sharing ideas and code on how to do that.

I think it's definitely better than the old version, but I wonder if adding the document root really makes it "a lot harder to guess". It's a reasonably predictable value in many environments.

I think the document root is pretty good. I am not a server guru but in my experience the most common pattern is /home/xxxxx/public_html/directory/path/here/ - and that xxxxx could be anything - possibly unrelated to the website (a persons name, a company name, often abbreviated to 6 letters, etc). I'm not saying it's impossible to guess, just a lot harder.

As an aside, it looks like Mark - who first announced the bug - has rewritten the script, and that it may be introduced as TimThumb 2

Use a local mac address as the salt.

I think we can close this now.