Skip to content

Commit

Permalink
Fixed sandbox for methods
Browse files Browse the repository at this point in the history
  • Loading branch information
@hason
hason committed May 10, 2019
1 parent 2e1e952 commit 57742de
Show file tree
Hide file tree
Showing 3 changed files with 125 additions and 20 deletions.
59 changes: 44 additions & 15 deletions ext/twig/twig.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -714,6 +714,7 @@ PHP_FUNCTION(twig_template_get_attributes)
char *class_name = NULL;
zval *tmp_class;
char *type_name;
zval *propertySandboxException;

if (zend_parse_parameters(ZEND_NUM_ARGS() TSRMLS_CC, "ozz|asbb", &template, &object, &zitem, &arguments, &type, &type_len, &isDefinedTest, &ignoreStrictCheck) == FAILURE) {
return;
Expand Down Expand Up @@ -923,10 +924,15 @@ PHP_FUNCTION(twig_template_get_attributes)
}
if ($this->env->hasExtension('\Twig\Extension\SandboxExtension')) {
$this->env->getExtension('\Twig\Extension\SandboxExtension')->checkPropertyAllowed($object, $item);
try {
$this->env->getExtension('\Twig\Extension\SandboxExtension')->checkPropertyAllowed($object, $item);
} catch (Twig_Sandbox_SecurityError $propertySandboxException) {
}
}
return $object->$item;
if (!isset($propertySandboxException)) {
return $object->$item;
}
}
}
*/
Expand All @@ -944,14 +950,17 @@ PHP_FUNCTION(twig_template_get_attributes)
if (TWIG_CALL_SB(TWIG_PROPERTY_CHAR(template, "env" TSRMLS_CC), "hasExtension", "Twig_Extension_Sandbox" TSRMLS_CC)) {
TWIG_CALL_ZZ(TWIG_CALL_S(TWIG_PROPERTY_CHAR(template, "env" TSRMLS_CC), "getExtension", "Twig_Extension_Sandbox" TSRMLS_CC), "checkPropertyAllowed", object, zitem TSRMLS_CC);
}
if (EG(exception)) {
if (EG(exception) && TWIG_INSTANCE_OF_USERLAND(EG(exception), "Twig_Sandbox_SecurityError" TSRMLS_CC)) {
propertySandboxException = EG(exception);
EG(exception) = NULL;
} else if (EG(exception)) {
efree(item);
return;
} else {
ret = TWIG_PROPERTY(object, zitem TSRMLS_CC);
efree(item);
RETURN_ZVAL(ret, 1, 0);
}

ret = TWIG_PROPERTY(object, zitem TSRMLS_CC);
efree(item);
RETURN_ZVAL(ret, 1, 0);
}
}
/*
Expand Down Expand Up @@ -1025,6 +1034,10 @@ PHP_FUNCTION(twig_template_get_attributes)
return false;
}
if (isset($propertySandboxException)) {
throw $propertySandboxException;
}
if ($ignoreStrictCheck || !$this->env->isStrictVariables()) {
return null;
}
Expand All @@ -1045,6 +1058,11 @@ PHP_FUNCTION(twig_template_get_attributes)
efree(item);
RETURN_FALSE;
}
if (Z_TYPE_P(propertySandboxException) == IS_OBJECT) {
efree(item);
EG(exception) = propertySandboxException;
return;
}
if (ignoreStrictCheck || !TWIG_CALL_BOOLEAN(TWIG_PROPERTY_CHAR(template, "env" TSRMLS_CC), "isStrictVariables" TSRMLS_CC)) {
efree(item);
return;
Expand All @@ -1066,7 +1084,11 @@ PHP_FUNCTION(twig_template_get_attributes)
}
*/
MAKE_STD_ZVAL(zmethod);
ZVAL_STRING(zmethod, method, 1);
if (call) {
ZVAL_STRING(zmethod, "__call", 1);
} else {
ZVAL_STRING(zmethod, method, 1);
}
if (TWIG_CALL_SB(TWIG_PROPERTY_CHAR(template, "env" TSRMLS_CC), "hasExtension", "Twig_Extension_Sandbox" TSRMLS_CC)) {
TWIG_CALL_ZZ(TWIG_CALL_S(TWIG_PROPERTY_CHAR(template, "env" TSRMLS_CC), "getExtension", "Twig_Extension_Sandbox" TSRMLS_CC), "checkMethodAllowed", object, zmethod TSRMLS_CC);
}
Expand All @@ -1083,26 +1105,33 @@ PHP_FUNCTION(twig_template_get_attributes)
try {
$ret = call_user_func_array([$object, $method], $arguments);
} catch (\BadMethodCallException $e) {
if ($call && isset($propertySandboxException)) {
throw $propertySandboxException;
}
if ($call && ($ignoreStrictCheck || !$this->env->isStrictVariables())) {
return null;
}
throw $e;
}
*/
ret = TWIG_CALL_USER_FUNC_ARRAY(object, method, arguments TSRMLS_CC);
if (EG(exception) && TWIG_INSTANCE_OF(EG(exception), spl_ce_BadMethodCallException TSRMLS_CC)) {
efree(tmp_method_name_get);
efree(tmp_method_name_is);
efree(lcItem);
if (call && EG(exception) && TWIG_INSTANCE_OF(EG(exception), spl_ce_BadMethodCallException TSRMLS_CC)) {
if (Z_TYPE_P(propertySandboxException) == IS_OBJECT) {
efree(item);
EG(exception) = propertySandboxException;
return;
}
if (ignoreStrictCheck || !TWIG_CALL_BOOLEAN(TWIG_PROPERTY_CHAR(template, "env" TSRMLS_CC), "isStrictVariables" TSRMLS_CC)) {
efree(tmp_method_name_get);
efree(tmp_method_name_is);
efree(lcItem);efree(item);
zend_clear_exception(TSRMLS_C);
return;
}
}
free_ret = 1;
efree(tmp_method_name_get);
efree(tmp_method_name_is);
efree(lcItem);
}
/*
// @deprecated in 1.28
if ($object instanceof Twig_TemplateInterface) {
Expand Down
30 changes: 27 additions & 3 deletions src/Template.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
use Twig\Error\Error;
use Twig\Error\LoaderError;
use Twig\Error\RuntimeError;
use Twig\Sandbox\SecurityError;

/**
* Default base class for compiled templates.
Expand Down Expand Up @@ -516,6 +517,7 @@ final protected function getContext($context, $item, $ignoreStrictCheck = false)
* @return mixed The attribute value, or a Boolean when $isDefinedTest is true, or null when the attribute is not set and $ignoreStrictCheck is true
*
* @throws RuntimeError if the attribute does not exist and Twig is running in strict mode and $isDefinedTest is false
* @throws SecurityError if the attribute is not allowed
*
* @internal
*/
Expand Down Expand Up @@ -591,17 +593,23 @@ protected function getAttribute($object, $item, array $arguments = [], $type = s
}

// object property
$propertySandboxException = null;
if (self::METHOD_CALL !== $type && !$object instanceof self) { // \Twig\Template does not have public properties, and we don't want to allow access to internal ones
if (isset($object->$item) || \array_key_exists((string) $item, $object)) {
if ($isDefinedTest) {
return true;
}

if ($this->env->hasExtension('\Twig\Extension\SandboxExtension')) {
$this->env->getExtension('\Twig\Extension\SandboxExtension')->checkPropertyAllowed($object, $item);
try {
$this->env->getExtension('\Twig\Extension\SandboxExtension')->checkPropertyAllowed($object, $item);
} catch (SecurityError $propertySandboxException) {
}
}

return $object->$item;
if (!isset($propertySandboxException)) {
return $object->$item;
}
}
}

Expand Down Expand Up @@ -668,6 +676,10 @@ protected function getAttribute($object, $item, array $arguments = [], $type = s
return false;
}

if (isset($propertySandboxException)) {
throw $propertySandboxException;
}

if ($ignoreStrictCheck || !$this->env->isStrictVariables()) {
return;
}
Expand All @@ -680,7 +692,15 @@ protected function getAttribute($object, $item, array $arguments = [], $type = s
}

if ($this->env->hasExtension('\Twig\Extension\SandboxExtension')) {
$this->env->getExtension('\Twig\Extension\SandboxExtension')->checkMethodAllowed($object, $method);
try {
$this->env->getExtension('\Twig\Extension\SandboxExtension')->checkMethodAllowed($object, $call ? '__call' : $method);
} catch (SecurityError $e) {
if ($call && isset($propertySandboxException)) {
throw $propertySandboxException;
}

throw $e;
}
}

// Some objects throw exceptions when they have __call, and the method we try
Expand All @@ -692,6 +712,10 @@ protected function getAttribute($object, $item, array $arguments = [], $type = s
$ret = \call_user_func_array([$object, $method], $arguments);
}
} catch (\BadMethodCallException $e) {
if ($call && isset($propertySandboxException)) {
throw $propertySandboxException;
}

if ($call && ($ignoreStrictCheck || !$this->env->isStrictVariables())) {
return;
}
Expand Down
56 changes: 54 additions & 2 deletions test/Twig/Tests/Extension/SandboxTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,9 @@ protected function setUp()
'1_basic7' => '{{ cycle(["foo","bar"], 1) }}',
'1_basic8' => '{{ obj.getfoobar }}{{ obj.getFooBar }}',
'1_basic9' => '{{ obj.foobar }}{{ obj.fooBar }}',
'1_basic10' => '{{ obj.baz }}',
'1_basic11' => '{{ obj.xyz }}',
'1_basic12' => '{{ obj.bac }}',
'1_basic' => '{% if obj.foo %}{{ obj.foo|upper }}{% endif %}',
'1_layout' => '{% block content %}{% endblock %}',
'1_child' => "{% extends \"1_layout\" %}\n{% block content %}\n{{ \"a\"|json_encode }}\n{% endblock %}",
Expand Down Expand Up @@ -258,6 +261,35 @@ public function testSandboxAllowFunctionsCaseInsensitive()
}
}

public function testSandboxRunGetterInsteadOfUnallowedProperty()
{
$twig = $this->getEnvironment(true, array(), self::$templates, array(), array(), array('FooObject' => 'getBaz'));
FooObject::reset();
$this->assertEquals('baz', $twig->load('1_basic10')->render(self::$params), 'Sandbox allow some methods');
$this->assertEquals(1, FooObject::$called['getBaz'], 'Sandbox only calls method getBaz');
}

public function testSandboxRunCallInsteadOfUnallowedProperty()
{
$twig = $this->getEnvironment(true, array(), self::$templates, array(), array(), array('FooObject' => '__call'));
FooObject::reset();
$this->assertEquals('xyz', $twig->load('1_basic11')->render(self::$params), 'Sandbox allow a method (__call())');
$this->assertEquals(1, FooObject::$called['__call'], 'Sandbox only calls method __call');
}

public function testSandboxRunCallInsteadOfUnallowedPropertyAndThrowException()
{
$twig = $this->getEnvironment(true, array(), self::$templates, [], [], ['FooObject' => '__call']);
FooObject::reset();
try {
$twig->load('1_basic12')->render(self::$params);
$this->fail('Sandbox throws a SecurityError exception if an unallowed property is called in the template through a method (__call)');
} catch (SecurityError $e) {
$this->assertEquals('Calling "bac" property on a "FooObject" object is not allowed.', $e->getRawMessage());
$this->assertEquals(1, FooObject::$called['__call'], 'Sandbox only calls method __call');
}
}

public function testSandboxLocallySetForAnInclude()
{
self::$templates = [
Expand Down Expand Up @@ -327,13 +359,17 @@ protected function getEnvironment($sandboxed, $options, $templates, $tags = [],

class FooObject
{
public static $called = ['__toString' => 0, 'foo' => 0, 'getFooBar' => 0];
public static $called = ['__call' => 0, '__toString' => 0, 'foo' => 0, 'getBaz' => 0, 'getFooBar' => 0];

public $bar = 'bar';

public $baz = 'baz';

public $bac = 'bac';

public static function reset()
{
self::$called = ['__toString' => 0, 'foo' => 0, 'getFooBar' => 0];
self::$called = ['__call' => 0, '__toString' => 0, 'foo' => 0, 'getBaz' => 0, 'getFooBar' => 0];
}

public function __toString()
Expand Down Expand Up @@ -361,4 +397,20 @@ public function getAnotherFooObject()
{
return new self();
}

public function getBaz()
{
++self::$called['getBaz'];
return $this->baz;
}

public function __call($name, $arguments)
{
++self::$called['__call'];
if ('bac' === strtolower($name)) {
throw new BadMethodCallException();
}

return $name;
}
}

0 comments on commit 57742de

Please sign in to comment.