9

Is it secure to use the following code:

require($_SERVER['DOCUMENT_ROOT'] . "/pages/" . $_GET['page'] . ".php")
iamart
  • 311
  • 3
  • 15

5 Answers5

12

No, it is not secure. Why?

Because sequence of two dots /../ means one directory back and the attacker could potentially include anything on your system, even above $_SERVER['DOCUMENT_ROOT']. (In an unfortunate configuration that means secret/sensitive OS config files.)

You have to IF or SWITCH for the allowed values to prevent malicious input. Example:

switch($_GET['page']) {
     case 'welcome': $page='welcome';
     case 'shop': $page='shop';
     default: $page='index';
}
require($_SERVER['DOCUMENT_ROOT'] . "/pages/" . $page . ".php")

Also check out in_array() for a little easier filtration.

Rok Kralj
  • 46,826
  • 10
  • 71
  • 80
2

StackOverflow has a useful Q&A for how to sanitize user input with PHP. It's a few years old, but the principles haven't changed at all.

The quick answer is: if you can avoid the problem in the first place, you're better off.

Show us how you're trying to use this, and we may be able to offer suggestions for improvement.

Community
  • 1
  • 1
ghoti
  • 45,319
  • 8
  • 65
  • 104
  • The way I read it, the question was "Is this secure", to which the right answer is "No, never" and a wrong answer is "perhaps, if sanitized". – ghoti Jul 26 '12 at 20:39
  • it's the same of second way of Mikey1980. – iamart Jul 26 '12 at 21:00
  • @ghoti Yes -- that is always "true" -- but the linked question does not talk about this case at all. It's good advice, but needs an expansion to be relevant here .. –  Jul 26 '12 at 21:03
1

It's not secure. You can use array with allowed values. For example

$allowed_pages = array('index', 'test', 'my_page')
if (!in_array($_GET['page'], $allowed_pages)){
    echo 'good bye';
    die();
} else {
   //
}
yAnTar
  • 4,269
  • 9
  • 47
  • 73
-1

If you trust all the files in the pages dir try:

if (in_array($_GET['page'],glob("/pages/*.php"))) {
   require($_SERVER['DOCUMENT_ROOT'] . "/pages/" . $_GET['page'] . ".php");
} else echo "Nice try hacker!";

Here's another solution using parts of a function I use to clean uploaded filenames:

OPTION #2 thanks Daniel, Rok!

$page = preg_replace('/[^a-zA-Z0-9_ %\[\]\.\(\)%&-]/s', '', $_GET['page']);
$filename = $_SERVER['DOCUMENT_ROOT'] . "/pages/" . str_replace("/",'',$page) . ".php";
if (file_exists($filename)) {
    require($filename);
} else echo "Nice try hacker!";

Note that this will only work if there are no special characters in your file names

Mikey1980
  • 971
  • 4
  • 15
  • 24
  • 1
    Globing whole directory is slow. It is better to use file_exists() on just the requested file. Your code also has a few bugs. – Rok Kralj Jul 26 '12 at 19:59
  • your right `file_exists()` is WAY faster when I tested on a huge dir but honestly with less than 100 files it made no difference, at least on on my box... also fixed the missing `)` and `;` – Mikey1980 Jul 26 '12 at 20:11
  • 3
    Your second example is just as vurneable as the original OP's code. The thing is a little more tricky. – Rok Kralj Jul 26 '12 at 20:51
  • 1
    I didn't understand meaning of first way, but second way is the same I have :D – iamart Jul 26 '12 at 20:58
  • I know my edit isn't the most secure but the odds of an attacker knowing where to find PHP scripts on your server are unlikely, for a basic templating system this should suffice – Mikey1980 Jul 26 '12 at 21:03
  • @Mikey1980: `str_replace` won't work, since the slashes may be escaped. Check out [the OWASP page linked above](https://www.owasp.org/index.php/Path_Traversal) for examples. – Daniel Pryden Jul 26 '12 at 22:11
  • To help others I've clean up my entire answer, thanks to Rok and Daniel for all the help.. like the OP I'm hear to learn – Mikey1980 Jul 26 '12 at 22:33
  • @Mikey1980: I see you've switched from `str_replace` to `preg_replace`, but unless I'm reading it wrong I don't think your regex is correct. But overall I think you're taking the wrong tack here: the correct approach is to *whitelist* the valid pages, not try to filter out everything that could be harmful (which turns into a game of [walls and ladders](http://blogs.msdn.com/b/oldnewthing/archive/2012/01/17/10257351.aspx) which you are unlikely to win). – Daniel Pryden Jul 28 '12 at 00:59
-3

use regExp to check your $_GET['page']!

Kirill Speransky
  • 5
  • 4