Page 1 of 2

Using $_GET vars directly in include

Posted: Tue Sep 12, 2006 4:17 pm
by RobertGonzalez
What are the potential implications of this code, as you see it here?

Code: Select all

<?php
include( "folder_name/topic_" . $HTTP_GET_VARS['id'] . ".php");
?>
I am trying to prepare my Director for a request of mine (that all web development go through our IS department at the very least, if we cannot handle all development) and I have noticed this in some of the code. The PHP set up is handling this pretty well, but are there ways to break this somehow?

Posted: Tue Sep 12, 2006 4:28 pm
by feyd
Attempting request of non-existant file (and potentially exposing of path), execution unknown code, etc etc.

Posted: Tue Sep 12, 2006 5:05 pm
by RobertGonzalez
The error settings on the PHP install are pretty good, in that I have tried to throw a lot of stuff at it and it just rolls right over it. Basically if the page cannot get included it just displays a page with content where the page would be.

How can I convince my boss that this is not a good way to go? I am thinking that an approach of 'the customer experience may not be to their satisfaction with blank page loads, no input validation and no communication'. I am just thinking that you never ever want to take user input and throw it into an include. But I can't seem to find a way to break this to my boss's satisfaction. Oh well, I guess I am not that good of a hacker...

Posted: Tue Sep 12, 2006 5:10 pm
by feyd
Everah wrote:How can I convince my boss that this is not a good way to go? I am thinking that an approach of 'the customer experience may not be to their satisfaction with blank page loads, no input validation and no communication'. I am just thinking that you never ever want to take user input and throw it into an include. But I can't seem to find a way to break this to my boss's satisfaction. Oh well, I guess I am not that good of a hacker...
The fact that it creates internal errors alone is enough in my book. Whether they are displayed or not is secondary. Showing a "blank page" is a VERY bad user experience. There's blindly including a page based solely on user input too, which you've already noted. Granted, the potentially REALLY bad stuff is less with this variant on the theme, but it's still a bad practice no matter where you go.

Posted: Tue Sep 12, 2006 5:26 pm
by dbevfat
Is it just me being tired, or it's true that with this code you can include (and therefore execute) any php file on the system, provided the serving process has read access to it? Think $_GET["id"] = "../../../path/to/anyfile". This is really bad, even if there is "no way" for the attacker to put his own file on the server.

A bit offtopic about putting files on the server: it's possible for an attacker to make a fake upload. By "fake" I mean that there is no actual form on the web page that would have input=file field. The upload is just a HTTP POST multipart/form-data to any php script. The file ends inside php-upload-dir. It has a randomly generated name and no extension, so it's not a real threat in this (or maybe any) case, but there it is -- a file on the server.

g'nite

Posted: Tue Sep 12, 2006 6:04 pm
by RobertGonzalez
feyd wrote:
Everah wrote:How can I convince my boss that this is not a good way to go? I am thinking that an approach of 'the customer experience may not be to their satisfaction with blank page loads, no input validation and no communication'. I am just thinking that you never ever want to take user input and throw it into an include. But I can't seem to find a way to break this to my boss's satisfaction. Oh well, I guess I am not that good of a hacker...
The fact that it creates internal errors alone is enough in my book. Whether they are displayed or not is secondary. Showing a "blank page" is a VERY bad user experience. There's blindly including a page based solely on user input too, which you've already noted. Granted, the potentially REALLY bad stuff is less with this variant on the theme, but it's still a bad practice no matter where you go.
Cool. I think we're on the same page here. Thanks feyd.

Posted: Wed Sep 13, 2006 2:54 am
by Maugrim_The_Reaper
It's bad practice (and probably fairly common) but limited in terms of what goes wrong.
Is it just me being tired, or it's true that with this code you can include (and therefore execute) any php file on the system, provided the serving process has read access to it?
The prefix prior to the GET variable defeats that - self limiting. Remove the prefix path however...

Posted: Wed Sep 13, 2006 6:02 am
by dbevfat
Maugrim_The_Reaper wrote:The prefix prior to the GET variable defeats that - self limiting.
No, I'm quite sure it doesn't.

Code: Select all

$id = "/../../../dir/file";
include("folder_name/topic_" . $id . ".php");
this code tries to include "folder_name/topic_/../../../dir/file.php", right? Which means that "../dir/file.php" gets included, since "directory/.."-pair cancel each other out. Put in enough "../"s, and you're at the top of the tree. Now all you have to do is climb down.

Posted: Wed Sep 13, 2006 6:18 am
by Jenk
That's assuming the directory topic_ exists.

Posted: Wed Sep 13, 2006 6:41 am
by dbevfat
No, "topic_" doesn't have to exist. I just tested it and it works -- you can include any file on the system, provided that:
- the file ends with ".php",
- you have read access to it.

Posted: Wed Sep 13, 2006 10:02 am
by volka
I agree, the include "works" on win xp if the file topic_ does not exist, tested with php 5.1.6

Posted: Wed Sep 13, 2006 10:16 am
by Maugrim_The_Reaper
Ah, that explains it then...on WinXP ;).

Kudos in any case... I'll promote it to very bad and possibly plain evil.

Posted: Wed Sep 13, 2006 10:38 am
by RobertGonzalez
Thanks guys. On a side note, does anyone know what the default php.ini setting for log_errors is? I think that for the project I am working it would be helpful to have alook at the PHP generated errors. I have never really looked into the PHP error logs so I am not sure if they are on by default and, if they are on, where the log resides.

Posted: Wed Sep 13, 2006 10:46 am
by volka
Maugrim_The_Reaper wrote:Ah, that explains it then...on WinXP ;).
naa, same with PHP 5.1.6-pl2-gentoo (cli) (built: Sep 10 2006 12:06:24)

Code: Select all

require 'directory/does/not/exist/../../../../test.html';
test.html is included even with 4 levels of non-existing directories prepended.

Posted: Wed Sep 13, 2006 10:48 am
by Jenk
I stand corrected.