Skip to content
Permalink
Browse files

Enforce that relative paths are contained within the template paths.

If user input was used to generate a view path, they could use
a relative path and try to escape the template path directories. Check
the paths if they look like bad things are going to happen.
  • Loading branch information...
markstory committed Jul 16, 2014
1 parent 31ed8c3 commit 0c7720e8f8f795233b5809419bfa024c4bed9eb1
Showing with 53 additions and 12 deletions.
  1. +24 −5 src/View/View.php
  2. +29 −7 tests/TestCase/View/ViewTest.php
@@ -850,21 +850,40 @@ protected function _getViewFileName($name = null) {
return $name;
}
$name = trim($name, DS);
} elseif ($name[0] === '.') {
$name = $this->viewPath . DS . $subDir . $name;
} elseif (!$plugin || $this->viewPath !== $this->name) {
$name = $this->viewPath . DS . $subDir . $name;
}
}
$paths = $this->_paths($plugin);
foreach ($paths as $path) {
foreach ($this->_paths($plugin) as $path) {
if (file_exists($path . $name . $this->_ext)) {
return $path . $name . $this->_ext;
return $this->_checkFilePath($path . $name . $this->_ext, $path);
}
}
throw new Error\MissingViewException(array('file' => $name . $this->_ext));
}
/**
* Check that a view file path does not go outside of the defined template paths.
*
* Only paths that contain `..` will be checked, as they are the ones most likely to
* have the ability to resolve to files outside of the template paths.
*
* @param string $file The path to the template file.
* @param string $path Base path that $file should be inside of.
* @return string The file path
*/
protected function _checkFilePath($file, $path) {
if (strpos($file, '..') === false) {
return $file;
}
$absolute = realpath($file);
if (strpos($absolute, $path) !== 0) {
throw new Error\MissingViewException(array('file' => $file));

This comment has been minimized.

Copy link
@dereuromark

dereuromark Jul 16, 2014

Member

I wonder if it would be useful to add some text as well. Missing View would make most think that there is a typo - and they would get crazy about why Cake cannot find it although it is really there. We should add a note about the "invalid" view file path here.

This comment has been minimized.

Copy link
@markstory

markstory Jul 16, 2014

Author Member

Ok, I can make a specific error for this case.

}
return $absolute;
}
/**
* Splits a dot syntax plugin name into its plugin and filename.
* If $name does not have a dot, then index 0 will be null.
@@ -132,12 +132,13 @@ public function paths($plugin = null, $cached = true) {
}
/**
* Test only function to return instance scripts.
* Setter for extension.
*
* @return array Scripts
* @param string $ext The extension
* @return void
*/
public function scripts() {
return $this->_scripts;
public function ext($ext) {
$this->_ext = $ext;
}
}
@@ -475,7 +476,8 @@ public function testCamelCasePluginGetTemplate() {
* @return void
*/
public function testGetViewFileNames() {
$viewOptions = ['plugin' => null,
$viewOptions = [
'plugin' => null,
'name' => 'Pages',
'viewPath' => 'Pages'
];
@@ -492,7 +494,7 @@ public function testGetViewFileNames() {
$result = $View->getViewFileName('/Posts/index');
$this->assertPathEquals($expected, $result);
$expected = TEST_APP . 'TestApp' . DS . 'Template' . DS . 'Pages' . DS . '..' . DS . 'Posts' . DS . 'index.ctp';
$expected = TEST_APP . 'TestApp' . DS . 'Template' . DS . 'Posts' . DS . 'index.ctp';
$result = $View->getViewFileName('../Posts/index');
$this->assertPathEquals($expected, $result);
@@ -512,6 +514,26 @@ public function testGetViewFileNames() {
$this->assertPathEquals($expected, $result);
}
/**
* Test that getViewFileName() protects against malicious directory traversal.
*
* @expectedException Cake\View\Error\MissingViewException
* @return void
*/
public function testGetViewFileNameDirectoryTraversal() {
$viewOptions = [
'plugin' => null,
'name' => 'Pages',
'viewPath' => 'Pages',
];
$request = $this->getMock('Cake\Network\Request');
$response = $this->getMock('Cake\Network\Response');
$view = new TestView(null, null, null, $viewOptions);
$view->ext('.php');
$view->getViewFileName('../../../../bootstrap');
}
/**
* Test getting layout filenames
*
@@ -1127,7 +1149,7 @@ public function testViewFileName() {
$result = $View->getViewFileName('../Element/test_element');
$this->assertRegExp('/Element(\/|\\\)test_element.ctp/', $result);
$expected = TEST_APP . 'TestApp' . DS . 'Template' . DS . 'Posts' . DS . '..' . DS . 'Posts' . DS . 'index.ctp';
$expected = TEST_APP . 'TestApp' . DS . 'Template' . DS . 'Posts' . DS . 'index.ctp';
$result = $View->getViewFileName('../Posts/index');
$this->assertPathEquals($expected, $result);
}

0 comments on commit 0c7720e

Please sign in to comment.
You can’t perform that action at this time.