diff --git a/changelog/security-fetch-ssrf-redirect.md b/changelog/security-fetch-ssrf-redirect.md new file mode 100644 index 000000000..f61d01e05 --- /dev/null +++ b/changelog/security-fetch-ssrf-redirect.md @@ -0,0 +1 @@ +- Security: `{fetch}` no longer follows HTTP redirects for remote resources while a security policy is active, preventing an open redirect on a trusted host from bypassing `trusted_uri` (CWE-918 server-side request forgery) diff --git a/changelog/security-html-image-escaping.md b/changelog/security-html-image-escaping.md new file mode 100644 index 000000000..72c1616ab --- /dev/null +++ b/changelog/security-html-image-escaping.md @@ -0,0 +1 @@ +- Security: `{html_image}` now escapes the `file`, `path_prefix`, `href`/`link`, `width` and `height` attributes (it already escaped `alt` and pass-through attributes), and `{html_select_date}` casts `day_size`/`month_size`/`year_size` to int (matching `{html_select_time}`), preventing untrusted values passed into these attributes from breaking out of the generated HTML (CWE-79) diff --git a/changelog/security-symlink-path-traversal.md b/changelog/security-symlink-path-traversal.md new file mode 100644 index 000000000..b7393b6df --- /dev/null +++ b/changelog/security-symlink-path-traversal.md @@ -0,0 +1 @@ +- Security: prevent symlinks inside a trusted `secure_dir`/template directory from being used to read files outside of it (CWE-22 path traversal), affecting `{include}` and `{fetch}` of local files diff --git a/libs/plugins/function.fetch.php b/libs/plugins/function.fetch.php index 4a3e88196..45d4b4f3b 100644 --- a/libs/plugins/function.fetch.php +++ b/libs/plugins/function.fetch.php @@ -191,7 +191,24 @@ function smarty_function_fetch($params, $template) return; } } else { - $content = @file_get_contents($params[ 'file' ]); + if ($protocol && isset($template->smarty->security_policy)) { + // Remote resource (e.g. https://) reached through file_get_contents(). + // isTrustedUri() only validates the initial URL, but file_get_contents() + // follows redirects by default, so an open redirect on an otherwise + // trusted host could be used to reach a non-trusted target (SSRF). + // Disable redirect-following while a security policy is in effect. + $context = stream_context_create( + array( + 'http' => array( + 'follow_location' => 0, + 'max_redirects' => 1, + ), + ) + ); + $content = @file_get_contents($params[ 'file' ], false, $context); + } else { + $content = @file_get_contents($params[ 'file' ]); + } if ($content === false) { throw new SmartyException("{fetch} cannot read resource '" . $params[ 'file' ] . "'"); } diff --git a/libs/plugins/function.html_image.php b/libs/plugins/function.html_image.php index 71bc63864..ab43773fc 100644 --- a/libs/plugins/function.html_image.php +++ b/libs/plugins/function.html_image.php @@ -75,7 +75,7 @@ function smarty_function_html_image($params, Smarty_Internal_Template $template) break; case 'link': case 'href': - $prefix = ''; + $prefix = ''; $suffix = ''; break; default: @@ -153,6 +153,11 @@ function smarty_function_html_image($params, Smarty_Internal_Template $template) $width = round($width * $_resize); $height = round($height * $_resize); } - return $prefix . '' . $alt . '' . $suffix; + // $alt and the pass-through attributes ($extra) are already escaped above; + // escape the remaining value-context params at output time so untrusted + // values cannot break out of the attribute (CWE-79). The unescaped $file/ + // $width/$height are still used for getimagesize()/DPI math above. + return $prefix . '' . $alt
+           . '' . $suffix; } diff --git a/libs/plugins/function.html_select_date.php b/libs/plugins/function.html_select_date.php index d9c571976..c9c801329 100644 --- a/libs/plugins/function.html_select_date.php +++ b/libs/plugins/function.html_select_date.php @@ -131,9 +131,6 @@ function smarty_function_html_select_date($params, Smarty_Internal_Template $tem case 'day_value_format': case 'month_format': case 'month_value_format': - case 'day_size': - case 'month_size': - case 'year_size': case 'all_extra': case 'day_extra': case 'month_extra': @@ -151,6 +148,13 @@ function smarty_function_html_select_date($params, Smarty_Internal_Template $tem case 'year_id': $$_key = (string)$_value; break; + case 'day_size': + case 'month_size': + case 'year_size': + // numeric HTML size attribute; cast to int (consistent with + // html_select_time) so it cannot break out of size="…" (CWE-79) + $$_key = (int)$_value; + break; case 'display_days': case 'display_months': case 'display_years': diff --git a/libs/sysplugins/smarty_security.php b/libs/sysplugins/smarty_security.php index 49ae2a386..dd8941399 100644 --- a/libs/sysplugins/smarty_security.php +++ b/libs/sysplugins/smarty_security.php @@ -590,12 +590,34 @@ private function _updateResourceDir($oldDir, $newDir) */ private function _checkDir($filepath, $dirs) { - $directory = dirname($this->smarty->_realpath($filepath, true)) . DIRECTORY_SEPARATOR; + // Resolve the canonical, symlink-free path of the requested file so that + // a symlink located inside a trusted directory cannot be abused to read + // a file outside of it (CWE-22 path traversal). Smarty::_realpath() only + // normalizes the path as a string and does not follow symlinks, so we + // fall back to it only when the file does not yet exist on disk (e.g. + // config/cache paths that are validated before being written). + $realpath = @realpath($filepath); + $resolved = $realpath !== false ? $realpath : $this->smarty->_realpath($filepath, true); + $directory = dirname($resolved) . DIRECTORY_SEPARATOR; + + // Canonicalize the trusted directories the same way. This keeps + // legitimate symlinked deployment paths working (e.g. a Capistrano-style + // "current" release symlink, or macOS' /var -> /private/var): both the + // file and the trusted directories are compared after symlinks have been + // resolved. + $trusted = array(); + foreach ($dirs as $dir => $unused) { + $trusted[ $dir ] = true; + if (($dirRealpath = @realpath($dir)) !== false) { + $trusted[ rtrim($dirRealpath, '\\/') . DIRECTORY_SEPARATOR ] = true; + } + } + $_directory = array(); if (!preg_match('#[\\\\/][.][.][\\\\/]#', $directory)) { while (true) { // test if the directory is trusted - if (isset($dirs[ $directory ])) { + if (isset($trusted[ $directory ])) { return $_directory; } // abort if we've reached root diff --git a/tests/UnitTests/SecurityTests/SecurityTest.php b/tests/UnitTests/SecurityTests/SecurityTest.php index 51eb38c6a..8e815aa1e 100644 --- a/tests/UnitTests/SecurityTests/SecurityTest.php +++ b/tests/UnitTests/SecurityTests/SecurityTest.php @@ -375,6 +375,87 @@ public function testSmartyTemplateObject() { $this->smarty->display('string:{$smarty.template_object}'); } + /** + * A symlink located inside a trusted secure_dir must not be usable to read + * a file outside of it (CWE-22 path traversal via symlink). + */ + public function testSymlinkEscapeFromSecureDirIsRejected() + { + list($secureDir, $outsideFile) = $this->createSymlinkFixture('secret-outside-content'); + $link = $secureDir . DIRECTORY_SEPARATOR . 'finance_doc'; + if (!@symlink($outsideFile, $link)) { + $this->markTestSkipped('Unable to create symlinks on this platform'); + } + + $this->smarty->security_policy->secure_dir = array($secureDir . DIRECTORY_SEPARATOR); + + $this->expectException('SmartyException'); + $this->expectExceptionMessage('not trusted file path'); + // Use forward slashes: backslashes in a double-quoted template string are + // interpreted as escape sequences (\f, \r, ...), which would corrupt a + // Windows path. Forward slashes work on every platform. + $this->smarty->fetch('string:{include file="' . str_replace('\\', '/', $link) . '"}'); + } + + /** + * A symlink that stays inside the trusted secure_dir must keep working, so + * legitimate (e.g. deployment) symlinks are not broken by the fix above. + */ + public function testSymlinkWithinSecureDirIsAllowed() + { + list($secureDir) = $this->createSymlinkFixture('secret-outside-content'); + $target = $secureDir . DIRECTORY_SEPARATOR . 'real.tpl'; + file_put_contents($target, 'inside-content'); + $link = $secureDir . DIRECTORY_SEPARATOR . 'linked.tpl'; + if (!@symlink($target, $link)) { + $this->markTestSkipped('Unable to create symlinks on this platform'); + } + + $this->smarty->security_policy->secure_dir = array($secureDir . DIRECTORY_SEPARATOR); + + // Forward slashes so backslashes in a Windows path are not mistaken for + // escape sequences inside the double-quoted template string. + $this->assertEquals('inside-content', $this->smarty->fetch('string:{include file="' . str_replace('\\', '/', $link) . '"}')); + } + + /** + * Builds a temporary directory tree for the symlink tests: a (canonicalized) + * secure directory plus a file located outside of it. The tree is removed in + * tearDown(). Returns array(secureDir, outsideFile). + */ + private function createSymlinkFixture($outsideContent) + { + $base = sys_get_temp_dir() . DIRECTORY_SEPARATOR . 'smarty_symlink_' . getmypid() . '_' . uniqid(); + $secureDir = $base . DIRECTORY_SEPARATOR . 'secure'; + mkdir($secureDir, 0777, true); + $outsideFile = $base . DIRECTORY_SEPARATOR . 'outside.txt'; + file_put_contents($outsideFile, $outsideContent); + + // Canonicalize so secure_dir is symlink-free (sys_get_temp_dir() itself + // may sit under a symlink, e.g. /var -> /private/var on macOS). + $this->symlinkFixtureDir = realpath($base); + return array(realpath($secureDir), realpath($outsideFile)); + } + + /** @var string|null temp dir created by createSymlinkFixture(), removed in tearDown */ + private $symlinkFixtureDir = null; + + protected function tearDown(): void + { + if (!empty($this->symlinkFixtureDir) && is_dir($this->symlinkFixtureDir)) { + $it = new \RecursiveIteratorIterator( + new \RecursiveDirectoryIterator($this->symlinkFixtureDir, \FilesystemIterator::SKIP_DOTS), + \RecursiveIteratorIterator::CHILD_FIRST + ); + foreach ($it as $entry) { + ($entry->isDir() && !$entry->isLink()) ? rmdir($entry->getPathname()) : unlink($entry->getPathname()); + } + rmdir($this->symlinkFixtureDir); + $this->symlinkFixtureDir = null; + } + parent::tearDown(); + } + } class mysecuritystaticclass diff --git a/tests/UnitTests/TemplateSource/TagTests/PluginFunction/PluginFunctionFetchTest.php b/tests/UnitTests/TemplateSource/TagTests/PluginFunction/PluginFunctionFetchTest.php index 13e9f3a3d..ae5ae1167 100644 --- a/tests/UnitTests/TemplateSource/TagTests/PluginFunction/PluginFunctionFetchTest.php +++ b/tests/UnitTests/TemplateSource/TagTests/PluginFunction/PluginFunctionFetchTest.php @@ -82,4 +82,99 @@ public function testFetchSecurity2() $this->smarty->fetch('string:{fetch file="/templates/../etc/passwd"}'); } + /** + * When a security policy is in effect, {fetch} of a remote resource must not + * follow redirects, otherwise an open redirect on a trusted host could be + * used to bypass trusted_uri and reach an internal target (SSRF, CWE-918). + * + * @runInSeparateProcess + * @preserveGlobalState disabled + */ + public function testFetchRemoteDisablesRedirectsUnderSecurity() + { + FetchContextCaptureStreamWrapper::$capturedOptions = null; + stream_wrapper_register('ssrftest', 'FetchContextCaptureStreamWrapper'); + try { + $this->smarty->enableSecurity(); + $this->smarty->security_policy->trusted_uri[] = '/^ssrftest:\/\/allowed$/'; + + $result = $this->smarty->fetch('string:{fetch file="ssrftest://allowed/data"}'); + + $this->assertSame('BODY', $result); + $this->assertIsArray(FetchContextCaptureStreamWrapper::$capturedOptions); + $this->assertArrayHasKey('http', FetchContextCaptureStreamWrapper::$capturedOptions); + $this->assertSame(0, FetchContextCaptureStreamWrapper::$capturedOptions['http']['follow_location']); + $this->assertLessThanOrEqual(1, FetchContextCaptureStreamWrapper::$capturedOptions['http']['max_redirects']); + } finally { + stream_wrapper_unregister('ssrftest'); + } + } + + /** + * Without a security policy there is no trusted_uri to bypass, so the + * redirect-disabling stream context is not applied (backwards compatible). + * + * @runInSeparateProcess + * @preserveGlobalState disabled + */ + public function testFetchRemoteKeepsDefaultBehaviorWithoutSecurity() + { + FetchContextCaptureStreamWrapper::$capturedOptions = null; + stream_wrapper_register('ssrftest', 'FetchContextCaptureStreamWrapper'); + try { + $result = $this->smarty->fetch('string:{fetch file="ssrftest://allowed/data"}'); + + $this->assertSame('BODY', $result); + $this->assertSame(array(), FetchContextCaptureStreamWrapper::$capturedOptions); + } finally { + stream_wrapper_unregister('ssrftest'); + } + } + +} + +/** + * Minimal custom stream wrapper used by the fetch SSRF tests: it records the + * stream context options that {fetch} passes to file_get_contents() and returns + * a fixed body so the call succeeds without touching the network. + */ +class FetchContextCaptureStreamWrapper +{ + /** @var resource|null populated by PHP when a context is passed */ + public $context; + + /** @var array|null options captured from the context on the last open */ + public static $capturedOptions = null; + + private $read = false; + + public function stream_open($path, $mode, $options, &$opened_path) + { + self::$capturedOptions = isset($this->context) ? stream_context_get_options($this->context) : array(); + return true; + } + + public function stream_read($count) + { + if ($this->read) { + return ''; + } + $this->read = true; + return 'BODY'; + } + + public function stream_eof() + { + return $this->read; + } + + public function stream_stat() + { + return array(); + } + + public function url_stat($path, $flags) + { + return array(); + } } diff --git a/tests/UnitTests/TemplateSource/TagTests/PluginFunction/PluginFunctionHtmlImageTest.php b/tests/UnitTests/TemplateSource/TagTests/PluginFunction/PluginFunctionHtmlImageTest.php new file mode 100644 index 000000000..69231d320 --- /dev/null +++ b/tests/UnitTests/TemplateSource/TagTests/PluginFunction/PluginFunctionHtmlImageTest.php @@ -0,0 +1,84 @@ +setUpSmarty(__DIR__); + } + + public function testInit() + { + $this->cleanDirs(); + } + + /** + * Passing both width and height skips the getimagesize() lookup, so no real + * image file is needed to render the tag. + */ + private function render($params) + { + $tpl = $this->smarty->createTemplate('eval:{html_image file=$file width=$width height=$height href=$href path_prefix=$path_prefix}'); + $tpl->assign($params + array( + 'file' => 'pic.jpg', + 'width' => 44, + 'height' => 68, + 'href' => '', + 'path_prefix' => '', + )); + return $tpl->fetch(); + } + + public function testHrefIsEscaped() + { + $result = $this->render(array('href' => '">')); + $this->assertStringNotContainsString('', 'path_prefix' => '">')); + $this->assertStringNotContainsString('