search

palakmonad / timthumb

Automatically exported from code.google.com/p/timthumb
0 stars 0 forks source link

Zero day vulnerability that gives remote attacker shell access #212

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago 
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/

Original issue reported on code.google.com by mmaun...@gmail.com on 2 Aug 2011 at 1:20

Attachments:

GoogleCodeExporter commented 9 years ago 
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");

Original comment by j...@fullspeedmarketing.com on 2 Aug 2011 at 6:11

GoogleCodeExporter commented 9 years ago 
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.

Original comment by j...@fullspeedmarketing.com on 2 Aug 2011 at 6:15

GoogleCodeExporter commented 9 years ago 
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

Original comment by mmaun...@gmail.com on 2 Aug 2011 at 6:33

GoogleCodeExporter commented 9 years ago 
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.

Original comment by d...@dd32.id.au on 2 Aug 2011 at 6:50

Attachments:

GoogleCodeExporter commented 9 years ago 
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.

Original comment by mmaun...@gmail.com on 2 Aug 2011 at 7:13

GoogleCodeExporter commented 9 years ago 
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.

Original comment by BinaryMoon on 2 Aug 2011 at 8:26

GoogleCodeExporter commented 9 years ago 
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)

Original comment by BinaryMoon on 2 Aug 2011 at 8:41

GoogleCodeExporter commented 9 years ago 
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.

Original comment by donn...@gmail.com on 2 Aug 2011 at 9:04

GoogleCodeExporter commented 9 years ago 
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.

Original comment by BinaryMoon on 2 Aug 2011 at 12:45

GoogleCodeExporter commented 9 years ago 
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.

Original comment by dev.cold...@gmail.com on 2 Aug 2011 at 1:23

GoogleCodeExporter commented 9 years ago 
(untested) pseudocode for better allowed sites matching: 
http://pastebin.com/RstGh0ZX

Original comment by demitrio...@gmail.com on 2 Aug 2011 at 6:15

GoogleCodeExporter commented 9 years ago 
(untested) pseudocode for better temporary file management: 
http://pastebin.com/E5RFwRwR

Original comment by demitrio...@gmail.com on 2 Aug 2011 at 6:36

GoogleCodeExporter commented 9 years ago 
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!

Original comment by werner.k...@gmail.com on 3 Aug 2011 at 7:15

GoogleCodeExporter commented 9 years ago 
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.

Original comment by BinaryMoon on 3 Aug 2011 at 8:26

GoogleCodeExporter commented 9 years ago 
Use sys_get_temp_dir() as cache dir. IMHO this is also necessary.

Original comment by siegfr...@gmail.com on 3 Aug 2011 at 11:33

GoogleCodeExporter commented 9 years ago 
Issue 218 has been merged into this issue.

Original comment by BinaryMoon on 3 Aug 2011 at 12:31

GoogleCodeExporter commented 9 years ago

Original comment by BinaryMoon on 3 Aug 2011 at 2:10

GoogleCodeExporter commented 9 years ago 
@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.

Original comment by Alberge...@gmail.com on 3 Aug 2011 at 7:04

GoogleCodeExporter commented 9 years ago 
[deleted comment]
GoogleCodeExporter commented 9 years ago 
If I force check the file extension to be in jpg/gif/png, will that prevent the 
php files from being uploaded?

Original comment by Aeolo...@gmail.com on 4 Aug 2011 at 2:16

GoogleCodeExporter commented 9 years ago 
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'.

Original comment by BinaryMoon on 4 Aug 2011 at 8:49

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

Original comment by ad...@letusbuzz.com on 4 Aug 2011 at 9:24

GoogleCodeExporter commented 9 years ago 
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

Original comment by BinaryMoon on 4 Aug 2011 at 9:28

GoogleCodeExporter commented 9 years ago 
@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.

Original comment by y...@gingerlime.com on 5 Aug 2011 at 8:51

GoogleCodeExporter commented 9 years ago 
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

Original comment by BinaryMoon on 5 Aug 2011 at 10:18

GoogleCodeExporter commented 9 years ago 
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.

Original comment by y...@gingerlime.com on 5 Aug 2011 at 10:34

GoogleCodeExporter commented 9 years ago 
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.

Original comment by BinaryMoon on 5 Aug 2011 at 10:46

GoogleCodeExporter commented 9 years ago 
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.

Original comment by y...@gingerlime.com on 5 Aug 2011 at 11:05

GoogleCodeExporter commented 9 years ago 
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

Original comment by BinaryMoon on 5 Aug 2011 at 11:59

GoogleCodeExporter commented 9 years ago 
Use a local mac address as the salt.

Original comment by kristian...@gmail.com on 5 Aug 2011 at 6:03

GoogleCodeExporter commented 9 years ago 
I think we can close this now.

Original comment by mmaun...@gmail.com on 5 Aug 2011 at 10:22