Skip to content

Commit

Permalink
Fix potential security vector
Browse files Browse the repository at this point in the history
- Fixes a potential security vector whereby if the value of a view model
  key references a PHP function, that function is called.
  - Now validates the callback to determine if it is a static method
    call or namespaced function call; if not, simply returns the value.
  • Loading branch information
weierophinney committed Sep 22, 2011
1 parent 2ee91d0 commit 6d0e085
Show file tree
Hide file tree
Showing 4 changed files with 52 additions and 2 deletions.
1 change: 1 addition & 0 deletions .vimproject
Expand Up @@ -79,6 +79,7 @@ mustache=/home/matthew/git/phly_mustache CD=. filter="*.php *.html *.xml *.txt"
template-with-pragma-and-partial.mustache
template-with-sections-and-delim-set.mustache
template-with-sub-view.mustache
template-referencing-php-function.mustache
}
}
}
Expand Down
31 changes: 29 additions & 2 deletions library/Phly/Mustache/Renderer.php
Expand Up @@ -364,14 +364,15 @@ public function getValue($key, $view)
}
if (is_object($view)) {
if (isset($view->$key)) {
if (is_callable($view->$key)) {
if (is_callable($view->$key) && $this->isValidCallback($view->$key)) {
return call_user_func($view->$key);
}
return $view->$key;
}
return '';
}
if (isset($view[$key])) {
if (is_callable($view[$key])) {
if (is_callable($view[$key]) && $this->isValidCallback($view[$key])) {
return call_user_func($view[$key]);
}
return $view[$key];
Expand Down Expand Up @@ -457,4 +458,30 @@ protected function handlePragmas($token, $data, $view)
}
}
}

/**
* Is the callback provided valid?
*
* @param callback $callback
* @return bool
*/
protected function isValidCallback($callback)
{
if (is_string($callback)) {
if (strstr($callback, '::')) {
// Referencing a static method call
return true;
}
if (strstr($callback, '\\')) {
// Referencing a namespaced function
return true;
}

// For security purposes, we don't want to call global functions
return false;
}

// Object or array callback -- always okay
return true;
}
}
21 changes: 21 additions & 0 deletions tests/PhlyTest/Mustache/MustacheTest.php
Expand Up @@ -552,4 +552,25 @@ protected function getRecursiveView()
),
);
}

/**
* @group injection-issues
*/
public function testArrayValuesThatReferToPHPBuiltInsShouldNotCallThem()
{
$test = $this->mustache->render('template-referencing-php-function', array(
'message' => 'time',
));
$this->assertEquals('time', trim($test));
}

/**
* @group injection-issues
*/
public function testObjectPropertiesThatReferToPHPBuiltInsShouldNotCallThem()
{
$model = (object) array('message' => 'time');
$test = $this->mustache->render('template-referencing-php-function', $model);
$this->assertEquals('time', trim($test));
}
}
@@ -0,0 +1 @@
{{message}}

0 comments on commit 6d0e085

Please sign in to comment.