Skip to content

Commit 57742de

Browse files
hasonhason
committed
Fixed sandbox for methods
1 parent 2e1e952 commit 57742de

File tree

3 files changed

+125
-20
lines changed

3 files changed

+125
-20
lines changed

‎ext/twig/twig.c‎

Lines changed: 44 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -714,6 +714,7 @@ PHP_FUNCTION(twig_template_get_attributes)
714714
char *class_name = NULL;
715715
zval *tmp_class;
716716
char *type_name;
717+
zval *propertySandboxException;
717718

718719
if (zend_parse_parameters(ZEND_NUM_ARGS() TSRMLS_CC, "ozz|asbb", &template, &object, &zitem, &arguments, &type, &type_len, &isDefinedTest, &ignoreStrictCheck) == FAILURE) {
719720
return;
@@ -923,10 +924,15 @@ PHP_FUNCTION(twig_template_get_attributes)
923924
}
924925
925926
if ($this->env->hasExtension('\Twig\Extension\SandboxExtension')) {
926-
$this->env->getExtension('\Twig\Extension\SandboxExtension')->checkPropertyAllowed($object, $item);
927+
try {
928+
$this->env->getExtension('\Twig\Extension\SandboxExtension')->checkPropertyAllowed($object, $item);
929+
} catch (Twig_Sandbox_SecurityError $propertySandboxException) {
930+
}
927931
}
928932
929-
return $object->$item;
933+
if (!isset($propertySandboxException)) {
934+
return $object->$item;
935+
}
930936
}
931937
}
932938
*/
@@ -944,14 +950,17 @@ PHP_FUNCTION(twig_template_get_attributes)
944950
if (TWIG_CALL_SB(TWIG_PROPERTY_CHAR(template, "env" TSRMLS_CC), "hasExtension", "Twig_Extension_Sandbox" TSRMLS_CC)) {
945951
TWIG_CALL_ZZ(TWIG_CALL_S(TWIG_PROPERTY_CHAR(template, "env" TSRMLS_CC), "getExtension", "Twig_Extension_Sandbox" TSRMLS_CC), "checkPropertyAllowed", object, zitem TSRMLS_CC);
946952
}
947-
if (EG(exception)) {
953+
if (EG(exception) && TWIG_INSTANCE_OF_USERLAND(EG(exception), "Twig_Sandbox_SecurityError" TSRMLS_CC)) {
954+
propertySandboxException = EG(exception);
955+
EG(exception) = NULL;
956+
} else if (EG(exception)) {
948957
efree(item);
949958
return;
959+
} else {
960+
ret = TWIG_PROPERTY(object, zitem TSRMLS_CC);
961+
efree(item);
962+
RETURN_ZVAL(ret, 1, 0);
950963
}
951-
952-
ret = TWIG_PROPERTY(object, zitem TSRMLS_CC);
953-
efree(item);
954-
RETURN_ZVAL(ret, 1, 0);
955964
}
956965
}
957966
/*
@@ -1025,6 +1034,10 @@ PHP_FUNCTION(twig_template_get_attributes)
10251034
return false;
10261035
}
10271036
1037+
if (isset($propertySandboxException)) {
1038+
throw $propertySandboxException;
1039+
}
1040+
10281041
if ($ignoreStrictCheck || !$this->env->isStrictVariables()) {
10291042
return null;
10301043
}
@@ -1045,6 +1058,11 @@ PHP_FUNCTION(twig_template_get_attributes)
10451058
efree(item);
10461059
RETURN_FALSE;
10471060
}
1061+
if (Z_TYPE_P(propertySandboxException) == IS_OBJECT) {
1062+
efree(item);
1063+
EG(exception) = propertySandboxException;
1064+
return;
1065+
}
10481066
if (ignoreStrictCheck || !TWIG_CALL_BOOLEAN(TWIG_PROPERTY_CHAR(template, "env" TSRMLS_CC), "isStrictVariables" TSRMLS_CC)) {
10491067
efree(item);
10501068
return;
@@ -1066,7 +1084,11 @@ PHP_FUNCTION(twig_template_get_attributes)
10661084
}
10671085
*/
10681086
MAKE_STD_ZVAL(zmethod);
1069-
ZVAL_STRING(zmethod, method, 1);
1087+
if (call) {
1088+
ZVAL_STRING(zmethod, "__call", 1);
1089+
} else {
1090+
ZVAL_STRING(zmethod, method, 1);
1091+
}
10701092
if (TWIG_CALL_SB(TWIG_PROPERTY_CHAR(template, "env" TSRMLS_CC), "hasExtension", "Twig_Extension_Sandbox" TSRMLS_CC)) {
10711093
TWIG_CALL_ZZ(TWIG_CALL_S(TWIG_PROPERTY_CHAR(template, "env" TSRMLS_CC), "getExtension", "Twig_Extension_Sandbox" TSRMLS_CC), "checkMethodAllowed", object, zmethod TSRMLS_CC);
10721094
}
@@ -1083,26 +1105,33 @@ PHP_FUNCTION(twig_template_get_attributes)
10831105
try {
10841106
$ret = call_user_func_array([$object, $method], $arguments);
10851107
} catch (\BadMethodCallException $e) {
1108+
if ($call && isset($propertySandboxException)) {
1109+
throw $propertySandboxException;
1110+
}
1111+
10861112
if ($call && ($ignoreStrictCheck || !$this->env->isStrictVariables())) {
10871113
return null;
10881114
}
10891115
throw $e;
10901116
}
10911117
*/
10921118
ret = TWIG_CALL_USER_FUNC_ARRAY(object, method, arguments TSRMLS_CC);
1093-
if (EG(exception) && TWIG_INSTANCE_OF(EG(exception), spl_ce_BadMethodCallException TSRMLS_CC)) {
1119+
efree(tmp_method_name_get);
1120+
efree(tmp_method_name_is);
1121+
efree(lcItem);
1122+
if (call && EG(exception) && TWIG_INSTANCE_OF(EG(exception), spl_ce_BadMethodCallException TSRMLS_CC)) {
1123+
if (Z_TYPE_P(propertySandboxException) == IS_OBJECT) {
1124+
efree(item);
1125+
EG(exception) = propertySandboxException;
1126+
return;
1127+
}
10941128
if (ignoreStrictCheck || !TWIG_CALL_BOOLEAN(TWIG_PROPERTY_CHAR(template, "env" TSRMLS_CC), "isStrictVariables" TSRMLS_CC)) {
1095-
efree(tmp_method_name_get);
1096-
efree(tmp_method_name_is);
1097-
efree(lcItem);efree(item);
10981129
zend_clear_exception(TSRMLS_C);
10991130
return;
11001131
}
11011132
}
11021133
free_ret = 1;
1103-
efree(tmp_method_name_get);
1104-
efree(tmp_method_name_is);
1105-
efree(lcItem);
1134+
}
11061135
/*
11071136
// @deprecated in 1.28
11081137
if ($object instanceof Twig_TemplateInterface) {

‎src/Template.php‎

Lines changed: 27 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515
use Twig\Error\Error;
1616
use Twig\Error\LoaderError;
1717
use Twig\Error\RuntimeError;
18+
use Twig\Sandbox\SecurityError;
1819

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

593595
// object property
596+
$propertySandboxException = null;
594597
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
595598
if (isset($object->$item) || \array_key_exists((string) $item, $object)) {
596599
if ($isDefinedTest) {
597600
return true;
598601
}
599602

600603
if ($this->env->hasExtension('\Twig\Extension\SandboxExtension')) {
601-
$this->env->getExtension('\Twig\Extension\SandboxExtension')->checkPropertyAllowed($object, $item);
604+
try {
605+
$this->env->getExtension('\Twig\Extension\SandboxExtension')->checkPropertyAllowed($object, $item);
606+
} catch (SecurityError $propertySandboxException) {
607+
}
602608
}
603609

604-
return $object->$item;
610+
if (!isset($propertySandboxException)) {
611+
return $object->$item;
612+
}
605613
}
606614
}
607615

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

679+
if (isset($propertySandboxException)) {
680+
throw $propertySandboxException;
681+
}
682+
671683
if ($ignoreStrictCheck || !$this->env->isStrictVariables()) {
672684
return;
673685
}
@@ -680,7 +692,15 @@ protected function getAttribute($object, $item, array $arguments = [], $type = s
680692
}
681693

682694
if ($this->env->hasExtension('\Twig\Extension\SandboxExtension')) {
683-
$this->env->getExtension('\Twig\Extension\SandboxExtension')->checkMethodAllowed($object, $method);
695+
try {
696+
$this->env->getExtension('\Twig\Extension\SandboxExtension')->checkMethodAllowed($object, $call ? '__call' : $method);
697+
} catch (SecurityError $e) {
698+
if ($call && isset($propertySandboxException)) {
699+
throw $propertySandboxException;
700+
}
701+
702+
throw $e;
703+
}
684704
}
685705

686706
// Some objects throw exceptions when they have __call, and the method we try
@@ -692,6 +712,10 @@ protected function getAttribute($object, $item, array $arguments = [], $type = s
692712
$ret = \call_user_func_array([$object, $method], $arguments);
693713
}
694714
} catch (\BadMethodCallException $e) {
715+
if ($call && isset($propertySandboxException)) {
716+
throw $propertySandboxException;
717+
}
718+
695719
if ($call && ($ignoreStrictCheck || !$this->env->isStrictVariables())) {
696720
return;
697721
}

‎test/Twig/Tests/Extension/SandboxTest.php‎

Lines changed: 54 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,9 @@ protected function setUp()
3737
'1_basic7' => '{{ cycle(["foo","bar"], 1) }}',
3838
'1_basic8' => '{{ obj.getfoobar }}{{ obj.getFooBar }}',
3939
'1_basic9' => '{{ obj.foobar }}{{ obj.fooBar }}',
40+
'1_basic10' => '{{ obj.baz }}',
41+
'1_basic11' => '{{ obj.xyz }}',
42+
'1_basic12' => '{{ obj.bac }}',
4043
'1_basic' => '{% if obj.foo %}{{ obj.foo|upper }}{% endif %}',
4144
'1_layout' => '{% block content %}{% endblock %}',
4245
'1_child' => "{% extends \"1_layout\" %}\n{% block content %}\n{{ \"a\"|json_encode }}\n{% endblock %}",
@@ -258,6 +261,35 @@ public function testSandboxAllowFunctionsCaseInsensitive()
258261
}
259262
}
260263

264+
public function testSandboxRunGetterInsteadOfUnallowedProperty()
265+
{
266+
$twig = $this->getEnvironment(true, array(), self::$templates, array(), array(), array('FooObject' => 'getBaz'));
267+
FooObject::reset();
268+
$this->assertEquals('baz', $twig->load('1_basic10')->render(self::$params), 'Sandbox allow some methods');
269+
$this->assertEquals(1, FooObject::$called['getBaz'], 'Sandbox only calls method getBaz');
270+
}
271+
272+
public function testSandboxRunCallInsteadOfUnallowedProperty()
273+
{
274+
$twig = $this->getEnvironment(true, array(), self::$templates, array(), array(), array('FooObject' => '__call'));
275+
FooObject::reset();
276+
$this->assertEquals('xyz', $twig->load('1_basic11')->render(self::$params), 'Sandbox allow a method (__call())');
277+
$this->assertEquals(1, FooObject::$called['__call'], 'Sandbox only calls method __call');
278+
}
279+
280+
public function testSandboxRunCallInsteadOfUnallowedPropertyAndThrowException()
281+
{
282+
$twig = $this->getEnvironment(true, array(), self::$templates, [], [], ['FooObject' => '__call']);
283+
FooObject::reset();
284+
try {
285+
$twig->load('1_basic12')->render(self::$params);
286+
$this->fail('Sandbox throws a SecurityError exception if an unallowed property is called in the template through a method (__call)');
287+
} catch (SecurityError $e) {
288+
$this->assertEquals('Calling "bac" property on a "FooObject" object is not allowed.', $e->getRawMessage());
289+
$this->assertEquals(1, FooObject::$called['__call'], 'Sandbox only calls method __call');
290+
}
291+
}
292+
261293
public function testSandboxLocallySetForAnInclude()
262294
{
263295
self::$templates = [
@@ -327,13 +359,17 @@ protected function getEnvironment($sandboxed, $options, $templates, $tags = [],
327359

328360
class FooObject
329361
{
330-
public static $called = ['__toString' => 0, 'foo' => 0, 'getFooBar' => 0];
362+
public static $called = ['__call' => 0, '__toString' => 0, 'foo' => 0, 'getBaz' => 0, 'getFooBar' => 0];
331363

332364
public $bar = 'bar';
333365

366+
public $baz = 'baz';
367+
368+
public $bac = 'bac';
369+
334370
public static function reset()
335371
{
336-
self::$called = ['__toString' => 0, 'foo' => 0, 'getFooBar' => 0];
372+
self::$called = ['__call' => 0, '__toString' => 0, 'foo' => 0, 'getBaz' => 0, 'getFooBar' => 0];
337373
}
338374

339375
public function __toString()
@@ -361,4 +397,20 @@ public function getAnotherFooObject()
361397
{
362398
return new self();
363399
}
400+
401+
public function getBaz()
402+
{
403+
++self::$called['getBaz'];
404+
return $this->baz;
405+
}
406+
407+
public function __call($name, $arguments)
408+
{
409+
++self::$called['__call'];
410+
if ('bac' === strtolower($name)) {
411+
throw new BadMethodCallException();
412+
}
413+
414+
return $name;
415+
}
364416
}

0 commit comments

Comments
 (0)