blog.Resource

Archive:

News-Feeds:


RSS 2.0
RSS 0.91
RDF
ATOM 0.3
April 3, 2011

[Coding] Doing Filename Checks Securely

By: Steffen Gebert

Recently a security issue in TYPO3 has been fixed, where it was possible to circumvent checks, which should ensure file names to match specific patterns (e.g. denying .php file extensions to be uploaded or renamed to).

As this problem is heavily caused by PHP's laxity, this blog entry aims to provide some explanations to you as developer to prevent you from placing similar security holes in your software or TYPO3 extensions.

Imagine you are programming a very basic file upload script and use the following piece of code:

$filename = $_POST['filename'];
$contents = $_POST['contents'];
file_put_contents($filename, $contents);

It is obvious that this code enables attackers to place arbitrary files on the server. So passing "evil.php" as a file name and sending PHP code, an attacker can execute any PHP code on your server.

So you might want to prevent saving .php files and other executable extensions by checking the file extension: 

if (0 === preg_match("/(.*)\.(php|php3|php4|php5)$/i", $filename) {
    file_put_contents($filename, $contents);
} else {
    die('Invalid file name given');
}

By using a whitelist approach, you can limit your upload script to allow only image file extensions: 

if (1 === preg_match("/(.*)\.(jpg|gif|png)$/i", $filename) {
    file_put_contents($filename, $contents);
} else {
    die('Invalid file name given');
}

What I also read very frequently in the web is that file inclusions using the following code can be treated as "safe": 

include('./themes/' . $_GET['filename'] . '.php'); 

But this code is far from being secure. The code snippet does not only expose a directory traversal vulnerability ('../../../evil/more-evil'). Even the trailing '.php' will not secure the include. As arbitrary .php files placed on the server can be executed, you are already in trouble. Not only if you have .php files containing malicious code on your server, but also if .php files are included which are meant to be included in another context.

Still the list of possible ways to exploit this "safe" code is not finished. Also the code examples above which are supposed to do a proper validation of the user provided file name are vulnerable. Let me explain you why.

This is where control characters come in. The most well-known control characters are \n (carriage return) or \t (horizontal tab). Less frequently used (or known) in PHP is the NUL(L) character (or zero-byte char), which can be written as 0x00, chr(0) or "\0" in PHP. In the first mentioned examples, "evil-file.php\0.jpg" could be submitted as value for $_POST['filename'].

Such a file name will pass the whitelist and the blacklist checks but "evil-file.php" will be used as target file name by file_put_contents().

In the include example, an attacker could use "../avatars/user-1234.jpg\0rest-doesnt-matter" to execute the contents of the "user-1234.jpg" file. This file could have been previously uploaded as the attacker's profile image in your imaginary web application or TYPO3 extension. Of course, the file could not contain image data, but malicious PHP code.

The remaining question is: Why do some functions, like the mentioned preg_match() or substring() work as expected and treat the \0 as a normal character whilst other functions treat it as end of the string?

The reason is that PHP itself is programmed in C, which treats the \0 as string terminator and ignores all following characters. And PHP relies on C's (or the operating system's) file system functions. That's why only file system functions are affected by this problem.

A list of affected functions and some further reading can e.g. be found at madirish.net.

So keep in mind to also check for NUL character, when dealing with user-input, especially when passed to file system functions:

$filename = str_replace(chr(0), '', $filename);

or using the [[:cntrl:]] class in regular expressions 

if (preg_match('/[[:cntrl:]]/', $filename)) {
    return FALSE;
}

The TYPO3 API provides two static functions in t3lib_div for file name and path checks: 

Usually this behavior to not react on control characters is called "binary-safe". So you think that file systems function that are explicitly marked as binary-safe in PHP manual do not expose the mentioned problems? Unfortunately not. Especially file_put_contents() is explicitly denoted to be binary-safe. However, this only holds for the to-be-written file contents but not for the $filename parameter. This parameter has to be checked and secured by you as the programmer!


comments

comment #1
Gravatar: Steffen Müller Steffen Müller April 4, 2011 00:35
Thanks for the very descriptive article.

comment #2
Gravatar: Ingo Augsten Ingo Augsten April 4, 2011 09:39
Thanks Steffen, very good writing style, easy to understand.

comment #3
Gravatar: maholtz maholtz April 4, 2011 11:46
Thanks for this very helpful article! For me it was an reminder at the right time :)

comment #4
Gravatar: Norbert Norbert April 4, 2011 12:38
I think this is a big problem of Typo3, that all files are in public web folder, including plugins where can be many badly written scripts. At least user uploads should definitely be outside the web root and served by a controller, not directly by the PHP processor.

comment #5
Gravatar: April 4, 2011 14:02
@Norbert
Any badly programmed extension is a risk.
Serving static files through PHP is IMHO not a good idea. Also TYPO3 Phoenix doesn't do this (however public files get symlinked). It's PHP's overhead, which can be saved.

The File Abstraction Layer project, which hopefully gets ready for integration in TYPO3 4.6, could solve the "all files in public folders".

Steffen

comment #6
Gravatar: Steffen Gebert Steffen Gebert April 4, 2011 14:02
Above comment is mine.. the name field doesn't seem to be really required ;-)

comment #7
Gravatar: Patrick Rodacker Patrick Rodacker April 4, 2011 14:48
Thank you Steffen for your effort. Very important topic and good written article. Keep on moving ;-)

comment #8
Gravatar: Samir Best Samir Best April 18, 2011 15:55
Thanks for your article. It is easy to understand.

comment #9
Gravatar: internetbureau internetbureau April 26, 2011 15:16
Great article!
Thnx for sharing!

Sorry, comments are closed for this post.