6

PHP lets you instantiate classes from variables or array entries, like this:

class Foo {}
$className = 'Foo';
new $className();
$someArray = ['class_name' => 'Foo'];
new $someArray['class_name']();

Distressingly, some of my coworkers - on multiple projects I've worked on at multiple companies - have used this language feature with class names supplied from user input as a way of instantiating one of several possible subclasses depending upon a type specified in the request. I've seen code along these lines...

new $_GET['product_type']($_GET['product_id']);

This, besides being a maintenance headache, is obviously stupid and dangerous; you're letting an attacker instantiate an arbitrary class. But how dangerous? What attacks are there that just use built-in PHP classes (and therefore require no detailed knowledge of the application code) that could be used against endpoints like the one above?

I'm looking for the nastiest attack anyone can come up with that I can use to scare any future coworkers who use this horrible anti-pattern.

Mark Amery
  • 1,827
  • 2
  • 14
  • 22
  • While yes, that can be bad, I'd like to see more of that code in the last snippet. – Mark Buffalo Jan 26 '16 at 15:07
  • @MarkBuffalo the exact snippet I wrote here was fictional; the real code was getting some JSON from php://input, parsing it, then (without validation) reaching in a few levels deep to find a class name and instantiating whatever class it found. But the effect was the same - a malicious caller who knew or guessed that this was happening could instantiate an arbitrary PHP class - either one from our application that was required or autoloadable, or a built-in one. I'm mostly interested in attacks using built-in classes, since they'll be easiest to reuse against other applications. – Mark Amery Jan 26 '16 at 15:10
  • 1
    @MarkAmery if you could give a better example as to something your coworkers would do we would be able to provide a better counter example as to why allowing arbitrary class instantiation is bad. – d0nut Jan 26 '16 at 15:31
  • you have the option to inject code also, more thant just the class

    new class; injected code; echo($_GET['product_id']) I could tear this code apart mwahahaha

     – TheHidden Jan 26 '16 at 15:37
  • 2
    @silverpenguin I don't see how you could inject arbitrary code here? – Mark Amery Jan 26 '16 at 15:44
  • @MarkAmery Is eval() allowed in the class constructor? If so, you likely have complete control over the website in question. – Mark Buffalo Jan 26 '16 at 15:47
  • @MarkBuffalo I don't follow. Sure, if there exists a class in the codebase that calls eval on one of its arguments, then I can exploit it to execute arbitrary code, but that seems like an extremely unlikely scenario. I'm not sure if you're misunderstanding the mechanics of how the code in the question works? – Mark Amery Jan 26 '16 at 15:51
  • 1
    Well, from what you said in your earlier comment about unchecked JSON, this kind of unchecked and unverified input is always a security vulnerability. But I can't see the rest of the code, so I can't show you if there are others. Is it correct that the built-in classes and functions are never instantiated by the coder? If that is the case, I don't think you'd be able to exploit it this way. – Mark Buffalo Jan 26 '16 at 15:56
  • 1
    @MarkBuffalo You're definitely misunderstanding the mechanics of this. The way the code here works is that the client passes a class name to the server, and the server (assuming, but not validating, that the class name is one of a few that it expects) instantiates the class with the given name. This allows the client to make the server instantiate any PHP class it likes, as long as it's built into PHP or already defined somewhere in the serverside application. The question is how that can be practically exploited, without a deep knowledge of the application. – Mark Amery Jan 26 '16 at 16:00
  • 1
    What happens if you have a constructor with side effects? Allowing a user to execute those side effects can certainly cause badness to occur. – Neil Smithline Jan 26 '16 at 16:12
  • @NeilSmithline absolutely - what I'm curious about is whether there are built-in classes whose constructors have side effects that could be exploited to cause badness. – Mark Amery Jan 26 '16 at 16:13
  • 1
    not a direct duplicate, but answers will be pretty much the same as here: Is PHP unserialize() exploitable without any 'interesting' methods? (the big difference to this question is __construct vs __wakeup, but both are about exploiting user-based class creation without considering custom classes) – tim Jan 26 '16 at 17:21

1 Answers1

2

I don't think that you can do anything with just this code.

But that does not mean that this is secure in any way. You give up the control flow of your application to the user, which is not a good idea.

For example, if you were to call close on the newly created object, an attacker would gain SSRF:

$obj = new $_GET['x']($_GET['y']);
$obj->close();

// SSRF: 
http://localhost/shell.php?x=SoapClient&y=http://localhost/called

Or if you had a class such as this, an attacker would gain session fixation:

class Session {
   function __construct($sessionId) {
       session_id($sessionId);
   }
}

// session fixation:    
http://localhost/shell.php?x=Session&y=123

Or you may have a class like this:

class File {
    function __construct($filename, $content) {
        $this->filename = $filename;
        $this->content = $content;
    }

    // [...]

    function save() {
        file_put_contents($this->filename, $this->content);
    }
}

And lets say the classes that users are supposed to create also have a save method (eg to save the object to the database). Now an attacker would have code execution.

It's easy to imagine a lot more scenarios like this, where either a constructor of a class does something dangerous, or a dangerous method of a class has the same name as a safe method of the class you expect.

It's likely that your current code base would allow an attacker to do at least something.

Depending on your code being secret is also not a good idea. Your code should be secure, even if the source code somehow leaked.

tim
  • 29,640
  • 7
  • 98
  • 121