Use of is_callable causes arbitrary code execution #138

Closed
JonathanAquino opened this Issue Mar 15, 2013 · 5 comments

Comments

Projects
None yet
2 participants
@JonathanAquino

Although it is supposed to be safe to pass arrays of strings into a mustache template, if you pass an array of two items containing a class name and a function name, Mustache will execute that function on that class.

<?php

require_once 'lib/Mustache/Autoloader.php';
Mustache_Autoloader::register();

class MustacheTest extends PHPUnit_Framework_TestCase {

    public function test() {
        $mustacheEngine = new Mustache_Engine();
        $strings = array('DateTime', 'getLastErrors');
        $this->assertEquals('DateTimegetLastErrors',
                $mustacheEngine->render('{{#strings}}{{.}}{{/strings}}',
                    array('strings' => $strings)));
    }

}

Expected: Mustache outputs "DateTimegetLastErrors"
Actual: Mustache outputs "" and executes DateTime::getLastErrors()

I believe that Mustache is doing this because it checks is_callable() on the array, and is_callable() returns true.

This is also a problem for applications that define their own class autoloader, because is_callable(), when passed an array of two strings, will try to autoload the first string as a class.

@bobthecow

This comment has been minimized.

Show comment Hide comment
@bobthecow

bobthecow Mar 15, 2013

Owner

That's absolutely correct: https://github.com/bobthecow/mustache.php/wiki/Callable

And it's expected. The only other way to call static methods is to pass a Foo::Bar string, and that's even worse.

Owner

bobthecow commented Mar 15, 2013

That's absolutely correct: https://github.com/bobthecow/mustache.php/wiki/Callable

And it's expected. The only other way to call static methods is to pass a Foo::Bar string, and that's even worse.

@JonathanAquino

This comment has been minimized.

Show comment Hide comment
@JonathanAquino

JonathanAquino Mar 15, 2013

Often these strings are coming from user input. What's to prevent a user from giving strings that coincide with a class name and function name in my application?

I recommend that array('SomeClass', 'methodName') be removed from the list of callable values, to prevent security issues.

Often these strings are coming from user input. What's to prevent a user from giving strings that coincide with a class name and function name in my application?

I recommend that array('SomeClass', 'methodName') be removed from the list of callable values, to prevent security issues.

bobthecow added a commit that referenced this issue Mar 23, 2013

Add a 'strict_callables' option to Mustache_Engine
Prevents treating array('ClassName', 'methodName') or array($instance, 'methodName') as section or interpolation callables. This helps protect against arbitrary code evaluation when user input is passed directly into the template.

This setting is recommended for PHP 5.3+, and all PHP 5.2 projects not using section or interpolation lambdas.

The default value is false to preserve backwards compatibility.

See #138

bobthecow added a commit that referenced this issue Mar 23, 2013

Add a 'strict_callables' option to Mustache_Engine
Prevents treating array('ClassName', 'methodName') or array($instance, 'methodName') as section or interpolation callables. This helps protect against arbitrary code evaluation when user input is passed directly into the template.

This setting is recommended for PHP 5.3+, and all PHP 5.2 projects not using section or interpolation lambdas.

The default value is false to preserve backwards compatibility.

See #138
@bobthecow

This comment has been minimized.

Show comment Hide comment
@bobthecow

bobthecow Mar 23, 2013

Owner

Yep. That's a valid concern. Do you think c160134 would do the trick?

Owner

bobthecow commented Mar 23, 2013

Yep. That's a valid concern. Do you think c160134 would do the trick?

@JonathanAquino

This comment has been minimized.

Show comment Hide comment
@JonathanAquino

JonathanAquino Mar 25, 2013

Yes, a strict_callables option would be helpful, I think. Thanks very much.

Yes, a strict_callables option would be helpful, I think. Thanks very much.

@bobthecow

This comment has been minimized.

Show comment Hide comment
@bobthecow

bobthecow Mar 25, 2013

Owner

Strict callables has been released in v2.2.0. Thanks for your help.

Owner

bobthecow commented Mar 25, 2013

Strict callables has been released in v2.2.0. Thanks for your help.

@bobthecow bobthecow closed this Mar 25, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment