Local File Inclusion Vulnerability

[Spoofed Existence]

All Kohana versions I have investigated so far are vulnerable to a local file inclusion vulnerability in the default configuration. While ".." segments of an URI are removed properly, after removing this the segments are trim'ed. So, ".. " (two periods, followed by a space) is not stripped but later converted into "..". Since any number of directories are allowed to be specified, any file with the extension php could be included remotely.

To see this happen, create a file "/tmp/test.php" with PHP code "<?php die('test');". Then, request the following page (or leave out index.php if it's automatically used by rewriteurl): http://thehost/index.php/%20../%20../%20../%20../%20../%20../%20../%20../%20../tmp/test Possibly more " ../" segments will be needed. But finally the page will die with the message 'test'.

After the code is executed and the script continues, it looks for the controller class. When it's not found, it will display a 404 file not found. So usually you won't be able to see the output of the page, unless an error occurs or the page terminates the script.

This will have serious consequences if the user is able to write to a .php file. While this is not possible in the default configuration, it can very well be later. For instance, if an application writes user input to the logs, the user can write php code if it is not stripped by the item logging it. Then, a malicious user could include this file and execute his own PHP. The log items in the standard installation seem to be stripped properly. Also, this may pose security risks on shared hosts, where the PHP files are executed as different users. One user could write a file and include it from another user's script, possibly having read- or write-permissions where he shouldn't.

A workaround is to disallow a period in the segments. That is, remove the period from "$config_allowed?".

Spoofed Existence

[Shadowhand]

Wouldn't adding . to the list of trimmed characters solve this?

[Shadowhand]

Fixed in r3048 and r3049.

[Spoofed Existence]

This does *not* solve the problem. All you need now is an existing directory in a controllers directory. In the default setup, this would work: /index.php/additional_examples/%20../%20../%20../%20../%20../%20../%20../%20../%20../tmp/test

My advice is to strip out all ".." and "." segments in the filter_uri function in the router. After the trim, obviously.

Spoofed Existence

[Spoofed Existence]

Sorry... I don't understand this system... It should be reopened, but it said resolution deleted. Anyways... ;)

[Shadowhand]

Are you even testing this? If you use %20../, it won't be translated to ../ until after the controller has been found, so trim($uri, ' ./'), does solve the problem.

[Spoofed Existence]

Have I tested it? I downloaded the 2.1.3 version from here: http://trac.kohanaphp.com/browser/tags/2.1.3 and your fix was in it. Are you even testing it? Because I have given a direct example that works.

This, indeed, no longer works:

/index.php/ ../ ../ ../ ../ ../ ../ ../ ../ ../tmp/test

However, if you prepend a directory, nothing is trim'ed. Since trim only replaces from the beginning and ending.

The fix is even less working than I thought. Because you can simply prepend a tab character, which will not be removed by the line you edited, but will be removed at the trim in filter_uri...

So this still works: /index.php/%09../%20../%20../%20../%20../%20../%20../%20../%20../tmp/test

[Shadowhand]

Fixed in r3058 and r3059.

[Shadowhand]

Better fix in r3061.

[Spoofed Existence]

Nice try but... nope...

http://localhost/private/test/kohana_new/index.php/%09..%20/%20..%20/%20..%20/%20..%20/%20..%20/%20..%20/%20..%20/%20..%20/%20..%20/tmp/test

Like I said before, why not just block them out in filter_uri, after the trim..?

[Shadowhand]

Why are you using %20? that won't allow the vulnerability to happen...

[Shadowhand]

Sorry, I didn't say that very well:

If you put %20 in the URL, .. / is not the same as ../, so there is no exploit. Is there any possible way this could actually be exploited?

[Spoofed Existence]

Yes, the segment will become ".. ". Or whatever combination of whitespaces you want. But in filter_uri, it is trimmed, and those spaces are removed. So it will become ".." again. See Router.php lines 270 and the way it's called on line 134.

Good luck fixing it ;)

[JAAulde]

Spoofed Existence,

This is quite an interesting discussion and I am replying to it to get myself subscribed to the email updates. We've been discussing this live in IRC through the last several fixes/postings.

I see what you're saying in your last posting (07/11/08 10:29:31) but don't quite understand how it is a vulnerability. Can you verify that you were able to exploit r3601 ?

Thanks, Jim

[JAAulde]

Sorry... r3601

[JAAulde]

damnit... r3061

[Shadowhand]

The last place that the URI is used to set a file path is [http://trac.kohanaphp.com/browser/trunk/system/libraries/Router.php#L117 Router, line 118). Because the segments have not been trimed yet, it's not vulnerable.

[Spoofed Existence]

Yes, I can still exploit it. With the url I posted: /index.php/%09..%20/%20..%20/%20..%20/%20..%20/%20..%20/%20..%20/%20..%20/%20..%20/%20..%20/tmp/test

Let's see what happens. html::specialchars does nothing. It just remains "\t.. / .. / " etc. Replace obviously won't do anything to it either, since the patter is not matched because there's a space between the periods and the slash. The trim of slashes won't touch it either.

The explode in line 123 will make it an array like: array ( "\t.. ", " .. ", " .. ", " .. ", etc, "tmp", "test" ).

However in line 134, all segments will be set to the return value of filter_uri with the segment as parameter (line 270). This function does a trim on the segment. So the segments will then be an array: array ( "..", "..", "..", etc, "tmp", "test" );

Hence it's exploitable.

These line numbers are according to the latest version, r3601. On which I have tested it.

Good luck, Spoofed Existence

[Shadowhand]

Ah, I see. I was looking at trunk, not 2.1.3. Anyways, r3066 removes . from the list of allowed characters. Although I don't really like this fix, the 2.1 branch will be killed off in less than a month, and Router (trunk) can't replace Router (2.1.3) because Router was changed in some fundamental ways, filter_uri being removed completely as the biggest example of that.

Thanks for your help and patience!

[Shadowhand]

Fixes in r3067 and r3068 should also help.

[Spoofed Existence]

Ahh, that was my wrong, I'm worry. I didn't know what trunk even was. I simply downloaded the version I thought was newest. Sorry about that ;).

[JAAulde]

Spoofed Existence,

Thanks a lot for that breakdown of what happens. I am not familiar w/ a lot fo the core stuff anymore, so that helped me get it. Shadowhand knows the core stuff but as he said he was looking more at trunk which doesn't have a lot of that older functionality.

Anyway, appreciate your assistance and explanations. Keep looking for holes. :)

Jim

[Spoofed Existence]

Erm, actually, sorry about this Shadowhand... But your last fix won't help either :P. It made it a bit tougher, but still possible. Take the segment " .. ../ /". The ../ will be removed, but only once. So the result of this will be ".. /". Which will again become a "..". This is one of PHP's 'beauties': the string is passed once, and if replacing something creates a new item that should be replaced as well, it won't.

Disallowing the period will work of course, as long as people don't re-enable it.

[JAAulde]

Spoofed Existence,

With 2.2 on the way and promising to get rid of this problem, we're opting to remove . from the config by default, and let these fixes make it hard for the exploit to be executed *if* . is enabled by a dev.

Thanks, Jim

[Geert]

Check out r3073 (follow-up to r3072). Can't beat that regex, can you?

[anonymous]

Milestone 2.1.3 deleted