Corveda / PHPSandbox

A PHP-based sandboxing library with a full suite of configuration and validation options.
https://phpsandbox.org
Other
220 stars 46 forks source link

Callable string variable invalid operations #26

Open etmpoon opened 2 years ago

etmpoon commented 2 years ago

When a variable is passed to a function, the sandbox wraps it with a "wrap" function. If the variable is a string with a callable value like "time", the "wrap" function returns a SandboxedString object. The target function receives the SandboxedString object instead of the string. Understand that the SandboxedString object was trying to prevent unsafe code to run by a callable variable. However, it has following issues.

1) The gettype function in the target function returns "object" instead of "string". The var_dump and var_export functions are okay. 2) Strict comparison of input values in the target function fail because they are objects instead of strings. 3) I don't know if this could have any security concern because the SandboxedString passed into custom function contains a lot of information.

Following code replicates the issue. The gettype in obj_b->getValue returns "object". The strict comparison in obj_a->test returns "false" even the same strings are submitted to the function.

echo '001';
echo '<br>';

$execode = '';
$appcode = '';

$execode = 'class obj_a {' .
            '   function test($x, $y, $cusfun) {' .
            '       $a = $cusfun[\'b\']->getValue($x);' .
            '       $b = $cusfun[\'b\']->getValue($y);' .
            '       return ($a === $b);' .
            '   }' .
            '}' .
            'class obj_b {' .
            '   function getValue($x) {' .
            '       echo gettype($x) . \'<br>\';' .
            '       return $x;' .
            '   }' .
            '}' .
            '$cusfun[\'a\'] = new obj_a();' .
            '$cusfun[\'b\'] = new obj_b();';

$appcode = 'return $cusfun;';

echo '002';
echo '<br>';

$sandboxcusfun = new PHPSandbox\PHPSandbox;

$sandboxcusfun->setOptions(['allow_classes'=>1]);
$sandboxcusfun->defineVar('cusfun',null);

$cusfun = $sandboxcusfun->execute($execode . $appcode);

echo '003';
echo '<br>';
var_dump($cusfun);
echo '<br>';

$r = 'nothing';
if ($cusfun['a'] && method_exists($cusfun['a'], 'test')) {
    $r = $cusfun['a']->test('ob_clean','ob_clean',$cusfun);
}

echo '005';
echo '<br>';

var_dump($r);
echo '<br>';

echo '006';
echo '<br>';
etmpoon commented 2 years ago

I tried in_array with strict mode which failed. Other functions may fail because it's actually passing an object instead of a string into the functions.

Could someone explain why the PHPSandbox needs to wrap function inputs by a "wrap" function which returns a SandboxedString object when the input value is callable?

The PHPSandbox will check all function calls in sandbox code even it's a variable function call like $x(). I think it's not required to pass the SandboxedString into functions except that the sandbox allows "eval" in sandbox code or any external function eval input values,

Could we add a setting like allow_callable_fun_input to allow user select whether to wrap function inputs?

The code below replicates in_array with strict mode doesn't work in sandbox. The output was "false" even "time" (callable value) is in the input array.

echo '001a';
echo '<br>';

$execode = '';
$appcode = '';

$execode = 'class obj_a {' .
            '   function exeit($x, $y) {' .
            '       $x = in_array($x,$y,true);' .
            '       return $x;' .
            '   }' .
            '}' .
            '$cusfun[\'a\'] = new obj_a();';

$appcode = 'return $cusfun;';

echo '002';
echo '<br>';

$sandboxcusfun = new PHPSandbox\PHPSandbox;
$sandboxcusfun->setOptions(['allow_classes'=>1]);
$sandboxcusfun->defineVar('cusfun',null);

$cusfun = $sandboxcusfun->execute($execode . $appcode);

echo '003';
echo '<br>';
var_dump($cusfun);
echo '<br>';

echo var_export($cusfun,true);
echo '<br>';

$r1 = 'nothing';
if ($cusfun['a'] && method_exists($cusfun['a'], 'exeit')) {
    $r1 = $cusfun['a']->exeit('time',['time','test']);
}

echo '005';
echo '<br>';

var_dump($r1);
echo '<br>';

echo '006';
echo '<br>';
fieryprophet commented 2 years ago

SandboxedString was an attempt at preventing ways of introducing callable code into the sandbox via clever use of string prefixing and such but obviously introduces issues of its own. I will research a flag for disabling SandboxedString in the sandbox with the caveat that doing so will of course potentially introduce sandbox escape vectors but some use cases simply needed the additional flexibility. Continued improvement of SandboxedString edge-cases (in this case overriding the native gettype() function to account for SandboxedStrings) is also probably needed.