using a registry to include files

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

Post Reply
SidewinderX
Forum Contributor
Posts: 407
Joined: Fri Jul 16, 2004 9:04 pm
Location: NY

using a registry to include files

Post by SidewinderX »

Below is a very simple example of what I am doing. Essentially I have a "front controller" which validates an incoming http get request against a white list. The http request is a file that should be included. If it is valid, the request is stored in a registry. Once it is stored in the registry, an instance of another class is called - that class is responsible for including the validated request (which is now stored in the registry). Aside from some abstraction that may seem complete useless in my code below, is it safe to use data stored in the registry (which is a valid http request) to include the file? I cannot see anything wrong with it even though Zend Studio is complaining about an "unsafe use of variable in call include()/require()." I just think thats because Zend Studio does not know where the variable is coming from.

Code: Select all

<?php
 
class Singleton
{
    private static $instance;
    public $file;
 
    private function __construct ($file)
    {
        $this->file = $file;
    }
 
    public static function init ($file = null)
    {
        if (! isset(self::$instance)) {
            self::$instance = new self($file);
        }
        return self::$instance;
    }
}
 
class Includer
{
 
    public function __construct ()
    {
        $config = Singleton::init();
        require_once ($config->file);
    }
}
 
$valid_files = array("myfile" , "yourfile" , "foo" , "bar");
$requested_file = $_GET['file'];
if (in_array($requested_file, $valid_files)) {
    $file = $requested_file . ".php";
    $config = Singleton::init($file);
    new Includer();
}
?>
So, is there anything unsafe about line 28?

Thank you,
John
User avatar
Mordred
DevNet Resident
Posts: 1579
Joined: Sun Sep 03, 2006 5:19 am
Location: Sofia, Bulgaria

Re: using a registry to include files

Post by Mordred »

The code analysis tool cannot grasp your code (particularly that you've checked the filename) so it complains. The code looks okay to me, so ignore.
SidewinderX
Forum Contributor
Posts: 407
Joined: Fri Jul 16, 2004 9:04 pm
Location: NY

Re: using a registry to include files

Post by SidewinderX »

Thanks Mordred, thats what I assumed.
User avatar
Christopher
Site Administrator
Posts: 13596
Joined: Wed Aug 25, 2004 7:54 pm
Location: New York, NY, US

Re: using a registry to include files

Post by Christopher »

I would recommend not using globals wrapped in globals, and instead use objects:

Code: Select all

// use a config object to load the configurtion
$config = new Config_Ini('/path/to/config.ini');    // or XML or DB or whatever
$config->valid_files = array("myfile" , "yourfile" , "foo" , "bar");   // or set it directly
 
// somewhere later do something like
$fc = new FrontController($request, $config);
$fc->dispatch();
(#10850)
SidewinderX
Forum Contributor
Posts: 407
Joined: Fri Jul 16, 2004 9:04 pm
Location: NY

Re: using a registry to include files

Post by SidewinderX »

arborint wrote:I would recommend not using globals wrapped in globals
By globals wrapped in globals I am assuming you mean an http request wrapped in a singleton? This was a concern of mine, but if the data is properly validated, I cannot think of a reason not to use it. I am interested to know why you would not recommend it.
User avatar
Christopher
Site Administrator
Posts: 13596
Joined: Wed Aug 25, 2004 7:54 pm
Location: New York, NY, US

Re: using a registry to include files

Post by Christopher »

Maybe I am missing something, but this:

Code: Select all

$config = Singleton::init($file);
new Includer();
just seems like a crazy Rube Goldberg device to do this:

Code: Select all

require_once $file;
(#10850)
alex.barylski
DevNet Evangelist
Posts: 6267
Joined: Tue Dec 21, 2004 5:00 pm
Location: Winnipeg

Re: using a registry to include files

Post by alex.barylski »

I was wondering the same thing...

Why use a singleton to store nothing but a simple config value???

Your code was extremely confusing to me...so while it might make sense to you now...in 9 months time you'll be scratching your head wondering what the hell.

No reason to use a singleton IMHO.
SidewinderX
Forum Contributor
Posts: 407
Joined: Fri Jul 16, 2004 9:04 pm
Location: NY

Re: using a registry to include files

Post by SidewinderX »

Oh. I completely concur with your point. The code above was just an example, not my actual implementation, hence the "aside from some abstraction that may seem complete useless in my code below."
Post Reply