This is safe, right?

Discussions of secure PHP coding. Security in software is important, so don't be afraid to ask. And when answering: be anal. Nitpick. No security vulnerability is too small.

Moderator: General Moderators

User avatar
VladSun
DevNet Master
Posts: 4313
Joined: Wed Jun 27, 2007 9:44 am
Location: Sofia, Bulgaria

Post by VladSun »

Mordred wrote:Yes it would work, perhaps you should try it first?
Yes, I should :)
Mordred wrote:And what does register_globals have to do with this at all?
Just a similar example about using prefixes...
There are 10 types of people in this world, those who understand binary and those who don't
User avatar
VladSun
DevNet Master
Posts: 4313
Joined: Wed Jun 27, 2007 9:44 am
Location: Sofia, Bulgaria

Post by VladSun »

Game over :)

Well, this gave me an expected result, but not the one you've expected :) It seems that there is some kind of path "optimization" in include().
I was surprised to see that even if you have none existing directories in your include path one could include a file without errors.
This behaviour seems to be so, only if you have "dir1/dir2/../../dir1/dir2/file" structure in your include path.

That's how my "contents/secret.txt" could be included - by using "XXX/../../contents/secret" where XXX could be anything.
It is extended to:

Code: Select all

include("contents/pre_XXX/../../contents/secret.txt");
At the same time:

Code: Select all

root@mail:/www/ipclassify.relef.net# ls contents/pre_XXX/../../contents/secret.txt
/usr/bin/ls: contents/pre_XXX/../../contents/secret.txt: No such file or directory

Code: Select all

php -v
PHP 5.2.1 (cli) (built: Mar 30 2007 00:22:49)
Copyright (c) 1997-2007 The PHP Group
Zend Engine v2.2.0, Copyright (c) 1998-2007 Zend Technologies
I think it should be considered a PHP bug.

If it is what Mordred and Zoxive tried to point, then it's true. I was more concerned about including files like "/etc/passwd", not files in a directory which is viewable by default.
There are 10 types of people in this world, those who understand binary and those who don't
User avatar
Zoxive
Forum Regular
Posts: 974
Joined: Fri Apr 01, 2005 4:37 pm
Location: Bay City, Michigan

Post by Zoxive »

That is normal ../ behavior. Thats what i was trying to say the hole time. Instead I wasn't saying `XXX` i was saying Just `/`.

What helped out on your server was you have Magic Quotes on, and the Suffix of `.txt` So only .txt files on your server could of been included.
User avatar
VladSun
DevNet Master
Posts: 4313
Joined: Wed Jun 27, 2007 9:44 am
Location: Sofia, Bulgaria

Post by VladSun »

Zoxive wrote:That is normal ../ behavior. Thats what i was trying to say the hole time. Instead I wasn't saying `XXX` i was saying Just `/`.

What helped out on your server was you have Magic Quotes on, and the Suffix of `.txt` So only .txt files on your server could of been included.
No, it is not ...
It is "normal" only in PHP ...

In Windows, Linux, DOS and other OSs attempts to use such paths will raise an error. I've even tried it on Perl - error again ...

Is it normal to have "cd /home/none_existing_dir/../../home/existing_dir/" working? I don't think so ...

The "prefix" solution would work if PHP include() behaved as it should.
There are 10 types of people in this world, those who understand binary and those who don't
User avatar
VladSun
DevNet Master
Posts: 4313
Joined: Wed Jun 27, 2007 9:44 am
Location: Sofia, Bulgaria

Post by VladSun »

Anyway, Mordred, Zoxive, thanks for the info :)
There are 10 types of people in this world, those who understand binary and those who don't
toasty2
Forum Contributor
Posts: 361
Joined: Wed Aug 03, 2005 10:28 am
Location: Arkansas, USA

Post by toasty2 »

So, what all should I improve/change?
User avatar
Christopher
Site Administrator
Posts: 13596
Joined: Wed Aug 25, 2004 7:54 pm
Location: New York, NY, US

Post by Christopher »

Code: Select all

$a = trim('/.', preg_replace('/[^a-zA-Z0-9\-\_\.\/]/', '', $_GET['a']));
$path = $base . $a . $extension;
if (file_exists($path)) [
     include $path;
} else {
     // error
}
(#10850)
User avatar
Mordred
DevNet Resident
Posts: 1579
Joined: Sun Sep 03, 2006 5:19 am
Location: Sofia, Bulgaria

Post by Mordred »

arborint wrote:

Code: Select all

$a = trim('/.', preg_replace('/[^a-zA-Z0-9\-\_\.\/]/', '', $_GET['a']));
$path = $base . $a . $extension;
if (file_exists($path)) [
     include $path;
} else {
     // error
}

This is not correct, one can still do fake_prefix/../stuff instead of /../stuff

I still insist that the corect way to handle this is with a switch, and not by using the variable in the include. The alternative, if done properly, would requre comparing realpath(dirname()) etc etc - i.e. too many things that can go wrong.
User avatar
VladSun
DevNet Master
Posts: 4313
Joined: Wed Jun 27, 2007 9:44 am
Location: Sofia, Bulgaria

Post by VladSun »

I agree with Mordred. I would never use such include(). Instead, I recommend to use integer IDs, which would be used as an index of an array containing the path/file to include. This way validation is very easy and *secure*, while keeping code short.
There are 10 types of people in this world, those who understand binary and those who don't
Post Reply