Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Massive security vulnerability with Autoloaders #6

Open
nickl- opened this issue Aug 5, 2012 · 28 comments
Open

Massive security vulnerability with Autoloaders #6

nickl- opened this issue Aug 5, 2012 · 28 comments
Labels
Milestone

Comments

@nickl-
Copy link
Member

nickl- commented Aug 5, 2012

This is huge!!!

Makes XSS and SQLI seem like way too much effort in comparison.

Exploit: PHP include & require exposes the current context to the included file.

i.o.w. you have unrestricted access to $this == (the autoloader instance) from within the include

HOWTO:

Add the following inside a file which will be included by the autoloader

<?php
     echo "<h2>Reflection knows:</h2><pre>",  \ReflectionObject::export($this, true),  "</pre>";

Result:

from using Symfony's UniversalClassLoader, for example, but same goes for any other autoloader as I have not seen one with even the slightest consideration for this vulnerability:

Reflection knows:

<?php
/**
 * UniversalClassLoader implements a "universal" autoloader for PHP 5.3.
 *
 * It is able to load classes that use either:
 *
 *  * The technical interoperability standards for PHP 5.3 namespaces and
 *    class names (https://github.com/php-fig/fig-standards/blob/master/accepted/PSR-0.md);
 *
 *  * The PEAR naming convention for classes (http://pear.php.net/).
 *
 * Classes from a sub-namespace or a sub-hierarchy of PEAR classes can be
 * looked for in a list of locations to ease the vendoring of a sub-set of
 * classes for large projects.
 *
 * Example usage:
 *
 *     $loader = new UniversalClassLoader();
 *
 *     // register classes with namespaces
 *     $loader->registerNamespaces(array(
 *         'Symfony\Component' => __DIR__.'/component',
 *         'Symfony'           => __DIR__.'/framework',
 *         'Sensio'            => array(__DIR__.'/src', __DIR__.'/vendor'),
 *     ));
 *
 *     // register a library using the PEAR naming convention
 *     $loader->registerPrefixes(array(
 *         'Swift_' => __DIR__.'/Swift',
 *     ));
 *
 *
 *     // to enable searching the include path (e.g. for PEAR packages)
 *     $loader->useIncludePath(true);
 *
 *     // activate the autoloader
 *     $loader->register();
 *
 * In this example, if you try to use a class in the Symfony\Component
 * namespace or one of its children (Symfony\Component\Console for instance),
 * the autoloader will first look for the class under the component/
 * directory, and it will then fallback to the framework/ directory if not
 * found before giving up.
 *
 * @author Fabien Potencier 
 *
 * @api
 */
Object of class [  class Symfony\Component\ClassLoader\UniversalClassLoader ] {
  @@ /path/to/src/ClassLoader/UniversalClassLoader.php 61-319

  - Constants [0] {
  }

  - Static properties [0] {
  }

  - Static methods [0] {
  }

  - Properties [5] {
    Property [  private $namespaces ]
    Property [  private $prefixes ]
    Property [  private $namespaceFallbacks ]
    Property [  private $prefixFallbacks ]
    Property [  private $useIncludePath ]
  }

  - Dynamic properties [0] {
  }

  - Methods [17] {
    /**
     * Turns on searching the include for class files. Allows easy loading
     * of installed PEAR packages
     *
     * @param Boolean $useIncludePath
     */
    Method [  public method useIncludePath ] {
      @@ /path/to/src/ClassLoader/UniversalClassLoader.php 75 - 78

      - Parameters [1] {
        Parameter #0 [  $useIncludePath ]
      }
    }

    /**
     * Can be used to check if the autoloader uses the include path to check
     * for classes.
     *
     * @return Boolean
     */
    Method [  public method getUseIncludePath ] {
      @@ /path/to/src/ClassLoader/UniversalClassLoader.php 86 - 89
    }

    /**
     * Gets the configured namespaces.
     *
     * @return array A hash with namespaces as keys and directories as values
     */
    Method [  public method getNamespaces ] {
      @@ /path/to/src/ClassLoader/UniversalClassLoader.php 96 - 99
    }

    /**
     * Gets the configured class prefixes.
     *
     * @return array A hash with class prefixes as keys and directories as values
     */
    Method [  public method getPrefixes ] {
      @@ /path/to/src/ClassLoader/UniversalClassLoader.php 106 - 109
    }

    /**
     * Gets the directory(ies) to use as a fallback for namespaces.
     *
     * @return array An array of directories
     */
    Method [  public method getNamespaceFallbacks ] {
      @@ /path/to/src/ClassLoader/UniversalClassLoader.php 116 - 119
    }

    /**
     * Gets the directory(ies) to use as a fallback for class prefixes.
     *
     * @return array An array of directories
     */
    Method [  public method getPrefixFallbacks ] {
      @@ /path/to/src/ClassLoader/UniversalClassLoader.php 126 - 129
    }

    /**
     * Registers the directory to use as a fallback for namespaces.
     *
     * @param array $dirs An array of directories
     *
     * @api
     */
    Method [  public method registerNamespaceFallbacks ] {
      @@ /path/to/src/ClassLoader/UniversalClassLoader.php 138 - 141

      - Parameters [1] {
        Parameter #0 [  array $dirs ]
      }
    }

    /**
     * Registers a directory to use as a fallback for namespaces.
     *
     * @param string $dir A directory
     */
    Method [  public method registerNamespaceFallback ] {
      @@ /path/to/src/ClassLoader/UniversalClassLoader.php 148 - 151

      - Parameters [1] {
        Parameter #0 [  $dir ]
      }
    }

    /**
     * Registers directories to use as a fallback for class prefixes.
     *
     * @param array $dirs An array of directories
     *
     * @api
     */
    Method [  public method registerPrefixFallbacks ] {
      @@ /path/to/src/ClassLoader/UniversalClassLoader.php 160 - 163

      - Parameters [1] {
        Parameter #0 [  array $dirs ]
      }
    }

    /**
     * Registers a directory to use as a fallback for class prefixes.
     *
     * @param string $dir A directory
     */
    Method [  public method registerPrefixFallback ] {
      @@ /path/to/src/ClassLoader/UniversalClassLoader.php 170 - 173

      - Parameters [1] {
        Parameter #0 [  $dir ]
      }
    }

    /**
     * Registers an array of namespaces
     *
     * @param array $namespaces An array of namespaces (namespaces as keys and locations as values)
     *
     * @api
     */
    Method [  public method registerNamespaces ] {
      @@ /path/to/src/ClassLoader/UniversalClassLoader.php 182 - 187

      - Parameters [1] {
        Parameter #0 [  array $namespaces ]
      }
    }

    /**
     * Registers a namespace.
     *
     * @param string       $namespace The namespace
     * @param array|string $paths     The location(s) of the namespace
     *
     * @api
     */
    Method [  public method registerNamespace ] {
      @@ /path/to/src/ClassLoader/UniversalClassLoader.php 197 - 200

      - Parameters [2] {
        Parameter #0 [  $namespace ]
        Parameter #1 [  $paths ]
      }
    }

    /**
     * Registers an array of classes using the PEAR naming convention.
     *
     * @param array $classes An array of classes (prefixes as keys and locations as values)
     *
     * @api
     */
    Method [  public method registerPrefixes ] {
      @@ /path/to/src/ClassLoader/UniversalClassLoader.php 209 - 214

      - Parameters [1] {
        Parameter #0 [  array $classes ]
      }
    }

    /**
     * Registers a set of classes using the PEAR naming convention.
     *
     * @param string       $prefix The classes prefix
     * @param array|string $paths  The location(s) of the classes
     *
     * @api
     */
    Method [  public method registerPrefix ] {
      @@ /path/to/src/ClassLoader/UniversalClassLoader.php 224 - 227

      - Parameters [2] {
        Parameter #0 [  $prefix ]
        Parameter #1 [  $paths ]
      }
    }

    /**
     * Registers this instance as an autoloader.
     *
     * @param Boolean $prepend Whether to prepend the autoloader or not
     *
     * @api
     */
    Method [  public method register ] {
      @@ /path/to/src/ClassLoader/UniversalClassLoader.php 236 - 239

      - Parameters [1] {
        Parameter #0 [  $prepend = false ]
      }
    }

    /**
     * Loads the given class or interface.
     *
     * @param string $class The name of the class
     */
    Method [  public method loadClass ] {
      @@ /path/to/src/ClassLoader/UniversalClassLoader.php 246 - 251

      - Parameters [1] {
        Parameter #0 [  $class ]
      }
    }

    /**
     * Finds the path to the file where the class is defined.
     *
     * @param string $class The name of the class
     *
     * @return string|null The path, if found
     */
    Method [  public method findFile ] {
      @@ /path/to/src/ClassLoader/UniversalClassLoader.php 260 - 318

      - Parameters [1] {
        Parameter #0 [  $class ]
      }
    }
  }
}

So how do you like them apples? With access to the autoloader there's no limit to the evil that can be done from here.

@nickl-
Copy link
Member Author

nickl- commented Aug 5, 2012

Another example: Composer\Autoload\ClassLoader

But you catch my drift.

Reflection knows:

<?php
/**
 * ClassLoader implements a PSR-0 class loader
 *
 * See https://github.com/php-fig/fig-standards/blob/master/accepted/PSR-0.md
 *
 *     $loader = new \Composer\Autoload\ClassLoader();
 *
 *     // register classes with namespaces
 *     $loader->add('Symfony\Component', __DIR__.'/component');
 *     $loader->add('Symfony',           __DIR__.'/framework');
 *
 *     // activate the autoloader
 *     $loader->register();
 *
 *     // to enable searching the include path (eg. for PEAR packages)
 *     $loader->setUseIncludePath(true);
 *
 * In this example, if you try to use a class in the Symfony\Component
 * namespace or one of its children (Symfony\Component\Console for instance),
 * the autoloader will first look for the class under the component/
 * directory, and it will then fallback to the framework/ directory if not
 * found before giving up.
 *
 * This class is loosely based on the Symfony UniversalClassLoader.
 *
 * @author Fabien Potencier 
 * @author Jordi Boggiano 
 */
Object of class [  class Composer\Autoload\ClassLoader ] {
  @@ /Users/inspirex/code/flow/work/aRESTedAPI/vendor/composer/ClassLoader.php 43-205

  - Constants [0] {
  }

  - Static properties [0] {
  }

  - Static methods [0] {
  }

  - Properties [4] {
    Property [  private $prefixes ]
    Property [  private $fallbackDirs ]
    Property [  private $useIncludePath ]
    Property [  private $classMap ]
  }

  - Dynamic properties [0] {
  }

  - Methods [11] {
    Method [  public method getPrefixes ] {
      @@ /Users/inspirex/code/flow/work/aRESTedAPI/vendor/composer/ClassLoader.php 50 - 53
    }

    Method [  public method getFallbackDirs ] {
      @@ /Users/inspirex/code/flow/work/aRESTedAPI/vendor/composer/ClassLoader.php 55 - 58
    }

    Method [  public method getClassMap ] {
      @@ /Users/inspirex/code/flow/work/aRESTedAPI/vendor/composer/ClassLoader.php 60 - 63
    }

    /**
     * @param array $classMap Class to filename map
     */
    Method [  public method addClassMap ] {
      @@ /Users/inspirex/code/flow/work/aRESTedAPI/vendor/composer/ClassLoader.php 68 - 75

      - Parameters [1] {
        Parameter #0 [  array $classMap ]
      }
    }

    /**
     * Registers a set of classes
     *
     * @param string       $prefix The classes prefix
     * @param array|string $paths  The location(s) of the classes
     */
    Method [  public method add ] {
      @@ /Users/inspirex/code/flow/work/aRESTedAPI/vendor/composer/ClassLoader.php 83 - 100

      - Parameters [2] {
        Parameter #0 [  $prefix ]
        Parameter #1 [  $paths ]
      }
    }

    /**
     * Turns on searching the include path for class files.
     *
     * @param bool $useIncludePath
     */
    Method [  public method setUseIncludePath ] {
      @@ /Users/inspirex/code/flow/work/aRESTedAPI/vendor/composer/ClassLoader.php 107 - 110

      - Parameters [1] {
        Parameter #0 [  $useIncludePath ]
      }
    }

    /**
     * Can be used to check if the autoloader uses the include path to check
     * for classes.
     *
     * @return bool
     */
    Method [  public method getUseIncludePath ] {
      @@ /Users/inspirex/code/flow/work/aRESTedAPI/vendor/composer/ClassLoader.php 118 - 121
    }

    /**
     * Registers this instance as an autoloader.
     *
     * @param bool $prepend Whether to prepend the autoloader or not
     */
    Method [  public method register ] {
      @@ /Users/inspirex/code/flow/work/aRESTedAPI/vendor/composer/ClassLoader.php 128 - 131

      - Parameters [1] {
        Parameter #0 [  $prepend = false ]
      }
    }

    /**
     * Unregisters this instance as an autoloader.
     */
    Method [  public method unregister ] {
      @@ /Users/inspirex/code/flow/work/aRESTedAPI/vendor/composer/ClassLoader.php 136 - 139
    }

    /**
     * Loads the given class or interface.
     *
     * @param  string    $class The name of the class
     * @return bool|null True, if loaded
     */
    Method [  public method loadClass ] {
      @@ /Users/inspirex/code/flow/work/aRESTedAPI/vendor/composer/ClassLoader.php 147 - 154

      - Parameters [1] {
        Parameter #0 [  $class ]
      }
    }

    /**
     * Finds the path to the file where the class is defined.
     *
     * @param string $class The name of the class
     *
     * @return string|null The path, if found
     */
    Method [  public method findFile ] {
      @@ /Users/inspirex/code/flow/work/aRESTedAPI/vendor/composer/ClassLoader.php 163 - 204

      - Parameters [1] {
        Parameter #0 [  $class ]
      }
    }
  }
}

@nickl-
Copy link
Member Author

nickl- commented Aug 5, 2012

A Solution:

<?php
            call_user_func(function () use ($file) {
                ob_start();
                require $file;
                ob_end_clean();
            });

This also ensures no source mistakenly outputted by php due to faulty script tags and effectively disables your phissing capabilities which exposed this vulnerability in the first place.

nickl- added a commit to nickl-/ClassLoader that referenced this issue Aug 5, 2012
While doing development on Respect/Loader a massive security vulnerability was discovered which has the possibility to have huge repercussions as it gives any include file scope to hijack the autoloader.
see Respect/Loader#6 for more information.

This fix will also prevent an included script from auto-outputting anything as a result from the include call which will prevent any unwanted source code from ever being revealed as a result of faulty tags or phising for information when a script manages to be included by someone trying to exploit the application.
nickl- added a commit to nickl-/composer that referenced this issue Aug 5, 2012
While doing development on Respect/Loader a massive security vulnerability was discovered which has the possibility to have huge repercussions as it gives any include file scope to hijack the autoloader.
see Respect/Loader#6 for more information.

This fix will also prevent an included script from auto-outputting anything as a result from the include call which will prevent any unwanted source code from ever being revealed as a result of faulty tags or phising for information when a script manages to be included by someone trying to exploit the application.
@alganet
Copy link
Member

alganet commented Aug 6, 2012

Great job! That was unexpected.

We really need to hide the loader instance with no option to turn this off, but there are ways to code an application that controls the output buffer enough to prevent injections outside the loader (using Respect\Template that would be easy), and those guys would want to turn that off for performance, so I belive the ob_* should be optional. What do you think? @augustohp @henriquemoody

I've imagined three use cases for this exploit: infected componentes being used (someone using a component which was hacked to be evil), remote file inclusion through apache log inclusion or something like this and folder permission problems).

The case 2 and 3 can be avoided with open_basedir and proper folder permissions. We could check this on the Loader instantiation. The case 1 is harder and the hacked component could be even not aware of that (since it's possible to inject malicious code without breaking tests and it's possible that some commit passes without full code review).

Any other use case I haven't considered? We should document those to guide our implementation.

@nickl-
Copy link
Member Author

nickl- commented Aug 7, 2012

If you are able to access the classmap there's really no end to the liberties it affords you and all it would take is a strategically placed php file and managing to inject a single class_exists(). This would more than do the trick and allow you lots of room to play as class_exists is expected to fail yet autoloaders do not take this into account and it really only fails after it has had the opportunity to try every option available to it before it happily tries again.

We really need to do some benchmarks although speculating that it would cause a bit of overhead can be imagined. That said I am not too concerned about the performance on the first run and would gladly sacrifice a few cycles towards peace of mind knowing that the cache is based on a secure replication of the current state which reflects the components actually required by the application. It might very well mean that you could create the cache in an offline or DMZ environment and not be exposed to any of these vulnerabilities when in the cloud. Bottom line: rather than spending time on perfecting something that is already broken. But it all boils down to benchmarking which I think should be started post haste as these will be the deciding factors ultimately. Agree?

@nickl-
Copy link
Member Author

nickl- commented Aug 7, 2012

If anyone else has access to other autoloaders at hand please post the results here so we can help them get fixed before the word gets out and the script kiddies run wild with it.

I vote we first identify and fix all those affected before we kindle the furnace with brilliant ideas of mischief.

@alganet
Copy link
Member

alganet commented Aug 7, 2012

I'm more focused identifying how a user could damage the system when he gets in. Getting inside the autoloader is easy, but that doesn't mean that he will be able to do something. For now, the problem only happens when an unexcpected malicious file is autoloaded, and that can be prevented (with open_basedir, for example). Wondering if it's possible to exploit that in different ways.

@mxrth
Copy link

mxrth commented Aug 7, 2012

Just out of interest: what is the actual attack vector described in this issue?
I don't see how the autoloader is responsible for securing included scripts or even able to do so.
Just consider the following script being loaded by the autoloader as an example: https://gist.github.com/3289058
The autoloader can do absolutely nothing against an attacker controlled file, or did I miss sth. completely?

@alganet
Copy link
Member

alganet commented Aug 7, 2012

@DrAk3 I'm not sure either and I also would like to understand this issue better, and more important, how this exploit differs from any other file inclusion exploit.

It seems that the main difference is that the attacker has access to the autoloader instance from inside and can bypass its visibility declarations, being able to access any private and protected attributes and methods. He can't override methods from that, which makes your gist look harmless, but often autoloaders have some kind of path and cache control which can be changed (since they often rely on protected properties).

Summing it up: fixing this prevents your autoloader from being changed by classes you load, which is something we already expected from any autoloader and now seems to have a rough edge.

From the inside, anything is possible. The attacker could change the autoloader to load a different, hacked Doctrine connection class and steal connection information right from the instance configuration, for example. What I would like to know is how an attacker could in fact trick any system to load the malicious code.

@nickl-
Copy link
Member Author

nickl- commented Aug 8, 2012

@DrAk3 how did you manage to get loader to load your attack? For a PSR-0 autoloader it is hardly necessary to specifically make a call to loadfile or include yourself manually, the loader will find you and it is eager to do so.

You may have been mislead by my choice of showing the content definition from ReflectionObject::export($this, true) of the loader as seen in the examples above. Maybe a var_dump($this) or the popular print_r($this) would've gotten the point across better or facilitated a better explanation, we can only hope.

As oppose to your solution/attack where you managed to read the content of the loader source code, after having inbound knowledge of its location and assumed read access privileges granted all you've accomplished is to obtain some reading material for your favourite syntax highlighted pager, but the server is still far from being yours you may be able to run your own copy at will from there but that is about it.

Instead what you see here is the the result from a script file i.e. your attack.php being loaded automagically by the autoloader but where this differs from yours is that we are in the context of tho object instance, we are the autoloader, the reference $this = the autoloader instance currently working/serving the application. We do not care where the source file is located, we do not have to eval or do anything else, we have total control over the loader and share the same privileges it has on the system.

To use your example:

<?php

      /** We don't need an instance of the loader */
     $loader = new TestLoader();
     /** nor do we have to settle for it's publicly exposed interfaces alone */
     $loader->loadFile('attack.php');

     /** Because we are the loader instance itself */
     $this->loadFile('attack.php'); // publicly accessible facility as per 
                                    // previous example but from the $this reference

     /** but also we enjoy complete access to it's resources protected and private as well */
     $this->_private_classmap[] = 'add/mapping/to/this/file.php';
     $this->_log_writer = $my_dev_slash_null_writer;  // ssssshh don't tell anyone

     /** Something you will not be privileged to from your approach */
     file_put_content('for_prying_eyes.only', var_export($this->_current_cache));

I hope you see now why it is MASSIVE! Your system can be compromised and you will never know about it and once I've had these liberties I can easily guarantee my continued presence whether you restart, clear cache, even fixing the vulnerability later will have no effect. This is not something I hope to see happening at all!

@alganet wrote:

What I would like to know is how an attacker could in fact trick any system to load the malicious code.

All I need is to discover one factory that prompts an exception telling me class/type/object not found and the door is open, I would imagine that you can easily inject the script from a remote resource with no need for physical access on the filesystem. I really hope they realize the extend of this vulnerability and start fixing the auto-loaders it would really be a pity if they wait for someone to make an example of one first before they catch a wake-up, too late. =(

I take no responsibility for what happens!!! washing hands

@cs278
Copy link

cs278 commented Aug 8, 2012

If you can control the content of a file the autoloader loads the door is already blown open. Wrapping the call inside an anonymous function isn't going to save you, if there is a global variable containing the loader or its a singleton you can access it again. Or if you do apply your fix:

<?php

$bt = debug_backtrace();

while ($frame = array_shift($bt))
{
    if (isset($frame['object']) && $frame['object'] instanceof \Composer\Autoload\ClassLoader)
    {
        $loader = $frame['object'];

        $class = new ReflectionObject($loader);
        $prop = $class->getProperty('classMap');
        $prop->setAccessible(true);

        $map = $prop->getValue($loader);

        /* Do some evil, replace Symfony's request object with some really horrid code. */
        $map['Symfony\\Component\\HttpFoundation\\Request'] = '/path/to/some/scary/code.php';

        $prop->setValue($loader, $map);

        break;
    }
}

@nickl-
Copy link
Member Author

nickl- commented Aug 8, 2012

All it does is limit the scope/context by which the the script-file is included. Complete access from inside vs the use of reflection from outside. There are more tactics we can employ to obfuscate the resources further, for the security conscious but only once the context has been reduced.

By discarding the output buffer you also effectively prevent any output from code automatically executed on include and you will have to continue in the dark or find more legitimate angles.

@cs278 Are you saying this is not a notable vulnerability or do you only consider the fix deficient?

I am still reluctant to discuss or explore the exploits at this point until the frameworks have had a chance to remedy the scenario. We will have ample time to explore the boundaries and convince the naysayers please focus your efforts at informing the owners of exploitable auto loaders in use. At the moment we focus on fixing, this WILL NOT turn into a platform encouraging abuse.

Your support and understanding is appreciated, thank you in advance...

@webmozart
Copy link

@nickl- How is this a security issue? You assume that the class loader loads a malicious file - how does the exploiter get that malicious file there and register it in the first place? Exposing the $this variable does not seem like an issue to me... even if you secure that, there are a zillion more ways to exploit a server once you're able to let it execute malicious code.

@nickl-
Copy link
Member Author

nickl- commented Aug 8, 2012

@bschussek You don't necessarily have to have the file there, remember include/require would be just as happy with any the resource. I'm not sure what you mean by the exploit needs to register? I don't see any other includes that need to register anything anywhere.

The question thus far has not been on how, this is a vulnerability, in our opinion and we would see preventative measures be placed for WHEN someone attempts this exploit. as they would firstly not be in the scope of the loaders context itself but would not be able to auto-execute anything that can output any information to screen. That is going to be a hell of a lot more difficult then what I currently accomplished by knowing that I am dealing with the symfony class loader.

If you don't consider this a vulnerability then I am relieved. We have not explored the extend of this exploit in full as a courtesy to yourselves. Whatever your decision it appears there is enough eager hax0rs and spectators here who will want to see just how far we can take it. It's not an attack at the Symfony loader specifically, it is purely by chance that yours were at hand when we discovered this. We need to run this through it's paces for the sake of Respect/Loader and happy pandas everywhere.

Thank you for sharing your thoughts and please feel that you are always welcome to discuss anything else that comes to mind, we appreciate your opinion. On the off chance that you maybe feel confident enough would you be inclined to make us a friendly challenge/wager to prove you wrong. All in good spirit =) and you may rest assure that you will be informed if we find anything worth noting. Apologies for any undue inconvenience, personally it would suite me just fine if we don't manage to break anything, I'm strongly doubtful at that though.

@mxrth
Copy link

mxrth commented Aug 9, 2012

I've investigated this issue a little bit and I'll try to clarify some points:

  1. (@nickl-) As soon as an attacker can execute any code, there is nothing we can do about it: the application is taken over. Period.
  2. (@bschussek) There is a possible attack vector triggering such an issue: I've tried to describe it in a blogpost: http://drak3.devmx.de/blog/2012/08/08/autoloaded-remote-file-inclusion/
    It's basically a (remote) file inclusion attack that depends on a special autoloader (psr-0 loaders are such autoloaders) and can lead to arbitrary code execution in some special cases. See the blogpost for more details.
  3. The fix proposed here is not the right way imho: as said in 1. the inclusion of arbitrary files has to be prevented. The issue could either be fixed in php directly or in the classloader-implementations by validating the passed classname.

@nickl-
Copy link
Member Author

nickl- commented Aug 10, 2012

@DrAk3 spoil sports :p so much for the challenge then. =)

You could also use the data resource:

<?php
include "data://text/plain;base64,PD9waHAgZWNobyAnSGVsbG8gV29ybGQnOw==";

Will output Hello World

As much as I am grateful for everyone's efforts with protecting us from injections this is not the vulnerability I am concerned with. What bothers me is the fact that the included script is erroneously (IMO) imported into the class loader context and granted full access to manipulate its state Can it be that we are the only ones who think this is a huge concern, anyone else?

Why exactly would they bother to add scope limiters on the properties you may ask:

<?php
/** composer */
private $prefixes;
private $fallbackDirs;
private $useIncludePath;
private $classMap;

/** symfony */
private $namespaces;
private $prefixes;
private $namespaceFallbacks;
private $prefixFallbacks;
private $useIncludePath;

What is the point if you're just going to give it away to every script you include? Surely its usage as a design choice was not for cosmetic purposes alone! We can only hope...

Keep in mind I am not saying that this is wrong and you are welcome to call require 'statically_defined_script_to_include.php'; in your own class method, you may even manipulate the state you expose as this is intended and since you know what it is that you are including, not quite the case for autoloaders, their purpose is to reliably include anything you may require so that you do not have to. So you would assume that they'd take a little more precaution since they cannot know what it is that they are including. Well it turns out that they don't.

If this is not a mistake then it must be intentional, right. We can therefor accept that any script included by these autoloaders may legitimately modify any context variables they see fit, as intended and catered for and continue to use them like this. The only use case though that I can think of, is for undetectable, otherwise you would extend the class loader of course, undermining of the normal operations or the modification of what should be considered the unreliable state of said class loader. Why else would it be allowed, can you motivate this?

Before someone compares this with Reflection again may I suggest you consider one thing, that when you are using Reflection you know that you are manipulating an object in ways it is not intended. You know this is done at your own risk and therefor cannot shift blame to the developers for not considering the ways that it breaks. This is not the same thing as intentionally or unknowingly allowing you to break the machine especially when we can avoid that. As with any feature you are encouraged to submit any of these bugs you come across, you may also request it be documented properly, which usually gets resolved timeously. These are some of the key factors our decisions for choosing one library above another are based on, am I correct? Personally I also want some assurance that any security related problems would be dealt with post haste but ymmv.

@nickl-
Copy link
Member Author

nickl- commented Aug 10, 2012

Wikipedia on Application Security:

Application security encompasses measures taken throughout the application's life-cycle to prevent exceptions in the security policy of an application or the underlying system (vulnerabilities) through flaws in the design, development, deployment, upgrade, or maintenance of the application.

Applications only control the use of resources granted to them, and not which resources are granted to them. They, in turn, determine the use of these resources by users of the application through application security.

Which I agree with and also applies to these concerns: it doesn't matter where the script came from what matters is how it gets included and how it gets exposed that may or may not expose a security vulnerability.

@DrAk3 Based on this definition which clearly states that the application doesn't control which resources are given to it. Your proposal only addresses the ClassName the loader receives, which it needs to use appropriately. This does not however fix the vulnerability from receiving a faulty script. Since it has no choice about what it is given it better well deal with it properly. The problem here has nothing to do with the remote file either on the contrary if the proposed fix was to ignore remote files we would've failed as it should not be concerned about what resource it is given.

The question here is really whether it poses a security threat that the internal classmap (or any other state property) is modifiable by the script being included as the loader is accountable for what it exposes and how it uses the resources given.

The fact of the matter is that we are dealing with a class loader and it needs to load classes, right?
Based on this assumption you may additionally ask:

Should the inclusion of a class script file result in any output?
I can't think of any reason why.

Is it a security concern that an incorrectly tagged config class included, for example, results in the display of authentication information?
You damn right it is.

Should this be considered a vulnerability exploitable on the loader?
You damn right it is as it concerns controlling of the resource it was given end of discussion.
The vulnerability lies in the fact that the loader exposed the information by including the faulty script not the fact that the script was faulty.

The same goes for @DrAk3's haxor being able to display information to confirm that it's remote resource was included by outputting a message, the output is a result of the loader including the remote script.

Is it a security threat if the script included did not result in a new class identifiable by the name requested?
No matter how good its intentions the loader obviously included something it was not supposed to and it should probably have a closer look at what it just did there as it was a result of it's use of the resource given and rightfully its responsibility.

Surely it can't all fall on the loader's lap to resolve?
The way I see it, and this is also the definition from Wikipedia,

  • if a library exposes functionality or information that can damage or be used to exploit the application or any part of the system as a whole it is a security risk.
  • if a library does not properly consider what it is given and in turn make proper use thereof then it's a bug and needs to be fixed regardless of whether its a security risk or not.
  • if a library does not consider the limits or how it uses the resources at its disposal then it needs to be optimized and is just begging to be exploited, regardless of whether its a security risk or not.

You will notice that there is no consideration for chance, since no matter how minute the chance of something happening is not an excuse to be tardy. Just do it right and we can all sleep easy.

What do you say?

@webmozart
Copy link

@nickl- Nick, we are making totally different assumptions:

You assume that code loaded through the class loader might be malicious. Malicious, because good code doesn't use $this in the global space as it can never know for sure what $this refers to. You say you want to protect that malicious code from modifying the internal state of the class loader.

The fundamental problem in your assumption is that you should never, ever, execute malicious code. If you execute malicious code, the access to $this or printing to stdout is not the problem. You could rewrite all the core classes in the cache and completely hijack the behavior of the complete application.

That is why we assume that you only install trustworthy, "good" (see above) classes. If you don't, we can't protect you, not by hiding $this and not by discarding the output.

@DrAk3 A ticket has been opened for this on the PHP bug tracker: https://bugs.php.net/bug.php?id=62789

@webmozart
Copy link

@nickl- While I appreciate your efforts, please stop advertising this as a huge security leak. It is not. This is like asking everyone to put his wallet into a safe instead of locking the front door of his apartment.

@nickl-
Copy link
Member Author

nickl- commented Aug 10, 2012

@bschussek

You assume that code loaded through the class loader might be malicious. Malicious, because good code doesn't use $this in the global space as it can never know for sure what $this refers to.

<?php
if (isset($this) && is_a($this, 'Symfony\\Component\\ClassLoader\\UniversalClassLoader'))
        echo "I'm pretty damn sure that \$this is UniversalClassLoader from Symfony but ymmv";

Malicious? Why are you only concerned when something malicious exploits your security vulnerability? I don't recall ever saying malicious. I did say damage though which my very good hearted exploit might very easily do when I start modifying the resources at my disposal and you go and change any of the private members when upgrading. I hope you consider that everything is now public and that you need to keep things the same for us please.

You say good code doesn't use $this? I don't understand what makes it good or bad, you argue that making $this available is good but I must be a good developer and not use it, thats bad? Seems like double standards to me. If I didn't want you to be bad I won't need to give you a lecture about it, I just simply make it impossible, now is that also bad or was it good or bad no it must be good, see now you really confused me! Bad!

You say you want to protect that malicious code from modifying the internal state of the class loader.

No I simply say I want to protect the internal state of the class loader as I believe the information in this context bares a security risk if not reliable. I cannot trust my own security precautions if I am uncertain about my loader.
I thought that you unknowingly exposed it by mistake but you ensure me it is intentional.

The fundamental problem in your assumption is that you should never, ever, execute malicious code. If you execute malicious code, the access to $this or printing to stdout is not the problem. You could rewrite all the core classes in the cache and completely hijack the behavior of the complete application.

You say it's only fundamentally a problem if it is malicious, otherwise it's fine, right? =)

Loaded question alert: You cannot argue that it is wrong without admitting that it is a security vulnerability and there are risks involved exploiting it. You shouldn't have to tell me that you are vulnerable there, I can see that, you need to do something about it so that I can't expose your vulnerability.

That is why we assume that you only install trustworthy, "good" (see above) classes.

Would you consider UniversalClassLoader trustworthy a "good" class? (see below) I am just asking to understand what you mean and where to set the bar. Could you maybe motivate why this is a good thing which I am unable to see perhaps. We will be doing some benchmarks soon if performance is a concern, I am sure we can find something more optimized perhaps.

Unfortunately you have given me nothing to substantiate any benefit or reasons why exposing it is a good thing. Maybe we have completely different views on what is good practice. Feel free to elaborate.

If you don't, we can't protect you, not by hiding $this and not by discarding the output.

Don't worry about protecting me you're the one that needs protection. All I ask of you is load my classes in a reliable and consistent way without fault, it is not your job to protect but you are expected not to f-up!

@nickl- While I appreciate your efforts, please stop advertising this as a huge security leak. It is not.

Please don't put words in my mouth. This is a massive security vulnerability and you don't have to agree with me, you can ask any panda they already know.

This issue is not about you it's about Respect/Loader and who are you to dictate what we may and may not consider secure. If you sleep happy at night knowing you have an exploitable security vulnerability then that is up to you, we may care about what PHP does and provide our developers with the most intuitive and simple solutions as PHP intended, that is Respect. Why would you want us to also compromise our security because you said so? sic

The patch we submitted to you as well as informing you about the discovery was purely a curtesy and no hard feelings. Unless you can show that anything is false that is written here, which I will gladly rectify if that be the case, then I don't see why this issue should be any of your concern. As said before your opinion is always welcome and appreciated and if you think we are wrong please help us right.

We have waited, to give you a chance to respond, before doing actual exploits. During the course these will start appearing here and will be reflected in our test cases as well. Since you are not concerned we would also want you not to be concerned here. Keep in mind this is for Respect/Loader and mean you no harm.

To conclude:

This is like asking everyone to put his wallet into a safe instead of locking the front door of his apartment.

I am more an; if you keep your safe locked, you may leave the front door wide open; kinda guy. - for real

@webmozart
Copy link

I'm not saying that exposing the $this variable is good. I'm saying that hiding it is not a security measure. It does not protect the application in any way.

No I simply say I want to protect the internal state of the class loader as I believe the information in this context bares a security risk if not reliable.

You cannot. If you run code that wants to fool around, it will (think reflection, rewriting PHP files etc.). Don't run code that you don't trust.

@nickl-
Copy link
Member Author

nickl- commented Aug 10, 2012

@bschussek

I'm not saying that exposing the $this variable is good. I'm saying that hiding it is not a security measure. It does not protect the application in any way.

If it is securing it's context by not calling include from a class method (exposing the context) but from a function (no context to expose) instead, how is that not a security measure?

You cannot. If you run code that wants to fool around, it will (think reflection, rewriting PHP files etc.). Don't run code that you don't trust.

If you followed my looong elaborate explanations, don't worry it's understandable. There's a saying, which sums it up, I think about building malls but it could be more general, I am not sure.

It goes

If you build it they will use it

I cannot trust UniversalClassLoader (or any class loader that follows this pattern as a matter of fact) because its context is insecure.

I agree with you whole heartedly but you also don't have the right to call any code untrustworthy because they chose to use what you make available. That is not fair! Allowing others to change the context makes the loader untrustworthy not the other way around I am afraid.

@nickl-
Copy link
Member Author

nickl- commented Aug 10, 2012

@bschussek

I'm saying that hiding it is not a security measure. It does not protect the application in any way.

An auto loader has no concern for protecting the application, you do not have to worry about that at all. This is no why we use an auto loader.

@nickl-

Don't worry about protecting me you're the one that needs protection. All I ask of you is load my classes in a reliable and consistent way without fault, it is not your job to protect but you are expected not to f-up!

Is that too much to ask, I think not.

@schmittjoh
Copy link

There is no point in "securing" the class loader. It just results in a performance hit, and as soon as you can execute code, you can simply define any class you like. There is no need to temper with the internal state of the class loader to make it load classes from a different place, just define the class in the file that you want to write your "attack" code.

<?php

// attack.php

namespace Symfony\Component\Whatever;

// Yeah, I can overwrite the class here, whatever autoloader is never invoked
// for this class as it is already defined.
class Foo { }

Now, remote file inclusion is a different matter, but I have yet to see the code where a class name depends on user data, and if it does, you better have that data sanitized where it enters your system.

If you want to make some mitigation in your own class loader, feel free to do so, but I don't think that we should make any change to the symfony, nor composer's class loader. It is only a false sense of security anyway, at least imho and you can of course disagree with my assessment. Nonetheless, I would also like to ask you to respect our position on this matter.

@nickl-
Copy link
Member Author

nickl- commented Aug 10, 2012

@schmittjoh Do you have benchmarks to support this supposed performance hit?

Also bare in mind that the following would be a equally viable fix:

<?php
class ClassLoader {
///
}

/** context-less load function */
function loadFile($file) {
    include $file;
}

I am not willing to accept performance at the price of security.

That said I wish I am able to get the message across. This is not about false sense of security this is not about trying to break the class loader this is not about hacking or injecting this is not about a magic solution to fix security or have less of a chance to be hacked. The lack of any of these are also no excuse to write insecure code if I may add.

Why is it acceptable to expose the context? It beats me???? I seriously thought this was oversight and that it was exposed unknowingly by mistake yet you are all expressing that it is intentional, not for any valid or good reason but also with no regard.

My concern is that I share a code base trying to live happy with other modules expecting everyone to do their part and be security conscious while anyone of us are freely allowed to manipulate the auto loader state legally without constraint. Goes beyond reason!

If you can prove there is a performance knock or any other benefit whatsoever to do the include via the class method intentionally instead of a function, wrapped, declared, closure whatever, coming to think of it you are also welcome to call it from a designated class for all I care. What is the motivation for exposing the class loader as there are obvious drawbacks from intentionally exposing the context. I must be missing something profound here which you are prive to that I am not being told please can someone help me see the reason for doing this?

All that said to each their own, no disrespect, it really doesn't bother me I have several class loaders of my own so I have no reason to try and convince you, neither is that the purpose for this, only trying to understand What The!

Can you help me understand?

@schmittjoh
Copy link

I do not regard this a "security" issue, at all.

Exposing $this or not, intentionally or as a side-effect, it does not matter. If you want to get access to the class loader, you can get it no matter what we do. You can use debug_backtrace which provides you the object, and from there use reflection to modify the internal state. If you are so inclined, there is no fix in the world that we can make to prevent this. So, to me all it does is adding an additonal method/function call for nothing.

The only sane thing to do here is: If you do not trust code, do not use it.

@nickl-
Copy link
Member Author

nickl- commented Aug 10, 2012

As much as I am willing and eager to get to this point and say: "You know what, I agree,. you have a valid point there." alas... I cannot. =(

@schmittjoh

Exposing $this or not, intentionally or as a side-effect, it does not matter. If you want to get access to the class loader, you can get it no matter what we do.

Accessing instance is not the same as being instance, no matter what you do with reflection or where you find the reference you can never say I am now that object instance which is why this is massive, in my eyes.

Using reflection to gain access to an object for its unintentional values you know what you are doing is not allowed, receiving resources by calling a method, instantiating a class, calling a function passing by reference as well as included in context are all intentional constructs of the PHP language. The difference is you are allowing access to the instance saying it is fine go ahead and change it we grant you the liberty and we have taken it into consideration.

Call me a purist then but if you are not prepared for the changes I'm legally going to make, because you say I can then you'll have to fix that part because I am not doing anything wrong here, you are saying its allowed. This is a powerful construct which is the only way to obtain this privilege but if we don't show the common sense to use it correctly it will be taken away. Mark my words...

Totally disregarding the fact of course that you have a responsibility to the PHP community as a whole to set the example, as many will follow suite. This only a small example which you are responsible for as I make a leap to give reason for ALL class loaders being broken this way, excuse my blatancy.

and Spiderman comes to mind:

With great power comes great responsibility

Thank you for the explanation @schmittjoh much appreciated feel free to pop by again I for one am excited to see what the actual benchmarks will produce. =)

@cs278
Copy link

cs278 commented Aug 10, 2012

The point is that once the attacker can get your code to load his code, he owns you. Putting up speed bumps to slow him down is just pointless, you should have secured the initial entry point.

@nickl-
Copy link
Member Author

nickl- commented Aug 16, 2012

@cs278 The point, actually, has nothing to do with an attacker. Are we talking about the same thing here? Repeating myself once more: The question is really if you consider security === attack and nothing else.

What you are saying is that once we ensure that you cannot pass a url as a classname or even better configure php.ini and turn allow_url_include off, then we are safe and we can totally disregard security from then on. Why bother limiting the scope of what you expose by declaring properties and methods private or protected at all, who cares for encapsulation or singletons man why use OOP at all they are merely speed bumps. Can you see that this might be construed as ridiculous? If we do go this route mind you then this issue is solved, well done.

Anyway:
I will be doing realistic benchmarks soon if there are other valid concerns or anything else that you would like to learn from the results, now would be a good time to let me know.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

6 participants