Using $_GET vars directly in include

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
RobertGonzalez
Site Administrator
Posts: 14293
Joined: Tue Sep 09, 2003 6:04 pm
Location: Fremont, CA, USA

Using $_GET vars directly in include

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

Post by feyd »

Attempting request of non-existant file (and potentially exposing of path), execution unknown code, etc etc.
User avatar
RobertGonzalez
Site Administrator
Posts: 14293
Joined: Tue Sep 09, 2003 6:04 pm
Location: Fremont, CA, USA

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

Post 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.
User avatar
dbevfat
Forum Contributor
Posts: 126
Joined: Tue Jun 28, 2005 2:47 pm
Location: Ljubljana, Slovenia

Post 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
User avatar
RobertGonzalez
Site Administrator
Posts: 14293
Joined: Tue Sep 09, 2003 6:04 pm
Location: Fremont, CA, USA

Post 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.
User avatar
Maugrim_The_Reaper
DevNet Master
Posts: 2704
Joined: Tue Nov 02, 2004 5:43 am
Location: Ireland

Post 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...
User avatar
dbevfat
Forum Contributor
Posts: 126
Joined: Tue Jun 28, 2005 2:47 pm
Location: Ljubljana, Slovenia

Post 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.
User avatar
Jenk
DevNet Master
Posts: 3587
Joined: Mon Sep 19, 2005 6:24 am
Location: London

Post by Jenk »

That's assuming the directory topic_ exists.
User avatar
dbevfat
Forum Contributor
Posts: 126
Joined: Tue Jun 28, 2005 2:47 pm
Location: Ljubljana, Slovenia

Post 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.
User avatar
volka
DevNet Evangelist
Posts: 8391
Joined: Tue May 07, 2002 9:48 am
Location: Berlin, ger

Post by volka »

I agree, the include "works" on win xp if the file topic_ does not exist, tested with php 5.1.6
User avatar
Maugrim_The_Reaper
DevNet Master
Posts: 2704
Joined: Tue Nov 02, 2004 5:43 am
Location: Ireland

Post 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.
User avatar
RobertGonzalez
Site Administrator
Posts: 14293
Joined: Tue Sep 09, 2003 6:04 pm
Location: Fremont, CA, USA

Post 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.
User avatar
volka
DevNet Evangelist
Posts: 8391
Joined: Tue May 07, 2002 9:48 am
Location: Berlin, ger

Post 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.
User avatar
Jenk
DevNet Master
Posts: 3587
Joined: Mon Sep 19, 2005 6:24 am
Location: London

Post by Jenk »

I stand corrected.
Post Reply