diff --git a/core/functions/actions/mutate_content.php b/core/functions/actions/mutate_content.php index d065ae51de..3985badd14 100644 --- a/core/functions/actions/mutate_content.php +++ b/core/functions/actions/mutate_content.php @@ -540,29 +540,34 @@ function getTVDisplayFormat($name, $value, $format, $paramstring = "", $tvtype = $widget_output = ''; $o = ''; /* If we are loading a file */ - if (substr($params['output'], 0, 5) == "@FILE") { - $file_name = EVO_BASE_PATH . trim(substr($params['output'], 6)); - if (!file_exists($file_name)) { - $widget_output = $file_name . ' does not exist'; + $output = (string) get_by_key($params, 'output', ''); + /* Both paths are resolved and checked for containment before + they are used - @INCLUDE runs the file, and the output + options this comes from are edited under a permission of + their own, not the one that grants arbitrary PHP. */ + if (substr($output, 0, 5) == "@FILE") { + $file_name = $modx->atBindFilePath(substr($output, 6)); + if ($file_name === false) { + $widget_output = 'Could not retrieve file for TV ' . $name . '.'; } else { $widget_output = file_get_contents($file_name); } - } elseif (substr($params['output'], 0, 8) == '@INCLUDE') { - $file_name = EVO_BASE_PATH . trim(substr($params['output'], 9)); - if (!file_exists($file_name)) { - $widget_output = $file_name . ' does not exist'; + } elseif (substr($output, 0, 8) == '@INCLUDE') { + $file_name = $modx->atBindFilePath(substr($output, 9)); + if ($file_name === false) { + $widget_output = 'Could not retrieve file for TV ' . $name . '.'; } else { /* The included file needs to set $widget_output. Can be string, array, object */ include $file_name; } - } elseif (substr($params['output'], 0, 6) == '@CHUNK' && $value !== '') { - $chunk_name = trim(substr($params['output'], 7)); + } elseif (substr($output, 0, 6) == '@CHUNK' && $value !== '') { + $chunk_name = trim(substr($output, 7)); $widget_output = $modx->getChunk($chunk_name); - } elseif (substr($params['output'], 0, 5) == '@EVAL' && $value !== '') { - $eval_str = trim(substr($params['output'], 6)); + } elseif (substr($output, 0, 5) == '@EVAL' && $value !== '') { + $eval_str = trim(substr($output, 6)); $widget_output = eval($eval_str); } elseif ($value !== '') { - $widget_output = $params['output']; + $widget_output = $output; } else { $widget_output = ''; } @@ -899,16 +904,16 @@ function renderFormElement( $custom_output = ''; /* If we are loading a file */ if (substr($field_elements, 0, 5) == "@FILE") { - $file_name = EVO_BASE_PATH . trim(substr($field_elements, 6)); - if (!file_exists($file_name)) { - $custom_output = $file_name . ' does not exist'; + $file_name = $modx->atBindFilePath(substr($field_elements, 6)); + if ($file_name === false) { + $custom_output = 'Could not retrieve file for this TV.'; } else { $custom_output = file_get_contents($file_name); } } elseif (substr($field_elements, 0, 8) == '@INCLUDE') { - $file_name = EVO_BASE_PATH . trim(substr($field_elements, 9)); - if (!file_exists($file_name)) { - $custom_output = $file_name . ' does not exist'; + $file_name = $modx->atBindFilePath(substr($field_elements, 9)); + if ($file_name === false) { + $custom_output = 'Could not retrieve file for this TV.'; } else { ob_start(); include $file_name; @@ -950,9 +955,15 @@ function renderFormElement( } else { $custom = explode(":", $field_type); $custom_output = ''; - $file_name = EVO_BASE_PATH . 'assets/tvs/' . $custom['1'] . '/' . $custom['1'] . '.customtv.php'; - if (!file_exists($file_name)) { - $custom_output = $file_name . ' does not exist'; + // The widget name comes out of the TV's type column and lands in a + // path that is then included, so it goes through the same + // containment rule as every other binding. + $widget = (string) get_by_key($custom, 1, ''); + $file_name = $widget === '' + ? false + : $modx->atBindFilePath('assets/tvs/' . $widget . '/' . $widget . '.customtv.php'); + if ($file_name === false) { + $custom_output = 'Could not retrieve the custom TV widget.'; } else { ob_start(); include $file_name; diff --git a/core/functions/tv.php b/core/functions/tv.php index 475c67acfc..08f011b9d0 100644 --- a/core/functions/tv.php +++ b/core/functions/tv.php @@ -516,30 +516,35 @@ function getTVDisplayFormat($name, $value, $format, $paramstring = '', $tvtype = case 'custom_widget': $widget_output = ''; /* If we are loading a file */ - if (strpos($params['output'], '@FILE') === 0) { - $file_name = EVO_BASE_PATH . trim(substr($params['output'], 6)); - if (!is_file($file_name)) { - $widget_output = $file_name . ' does not exist'; + $output = (string) get_by_key($params, 'output', ''); + /* Both paths are resolved and checked for containment before + they are used - @INCLUDE runs the file, and the output + options this comes from are edited under a permission of + their own, not the one that grants arbitrary PHP. */ + if (strpos($output, '@FILE') === 0) { + $file_name = $modx->atBindFilePath(substr($output, 6)); + if ($file_name === false) { + $widget_output = 'Could not retrieve file for TV ' . $name . '.'; } else { $widget_output = file_get_contents($file_name); } - } elseif (strpos($params['output'], '@INCLUDE') === 0) { - $file_name = EVO_BASE_PATH . trim(substr($params['output'], 9)); - if (!is_file($file_name)) { - $widget_output = $file_name . ' does not exist'; + } elseif (strpos($output, '@INCLUDE') === 0) { + $file_name = $modx->atBindFilePath(substr($output, 9)); + if ($file_name === false) { + $widget_output = 'Could not retrieve file for TV ' . $name . '.'; } else { /* The included file needs to set $widget_output. Can be string, array, object */ include $file_name; } } elseif ($value !== '') { - if (strpos($params['output'], '@CHUNK') === 0) { - $chunk_name = trim(substr($params['output'], 7)); + if (strpos($output, '@CHUNK') === 0) { + $chunk_name = trim(substr($output, 7)); $widget_output = $modx->getChunk($chunk_name); - } elseif (strpos($params['output'], '@EVAL') === 0) { - $eval_str = trim(substr($params['output'], 6)); + } elseif (strpos($output, '@EVAL') === 0) { + $eval_str = trim(substr($output, 6)); $widget_output = eval($eval_str); } else { - $widget_output = $params['output']; + $widget_output = $output; } } else { $widget_output = ''; @@ -893,16 +898,16 @@ function renderFormElement( $custom_output = ''; /* If we are loading a file */ if (strpos($field_elements, '@FILE') === 0) { - $file_name = EVO_BASE_PATH . trim(substr($field_elements, 6)); - if (!file_exists($file_name)) { - $custom_output = $file_name . ' does not exist'; + $file_name = $modx->atBindFilePath(substr($field_elements, 6)); + if ($file_name === false) { + $custom_output = 'Could not retrieve file for this TV.'; } else { $custom_output = file_get_contents($file_name); } } elseif (strpos($field_elements, '@INCLUDE') === 0) { - $file_name = EVO_BASE_PATH . trim(substr($field_elements, 9)); - if (!file_exists($file_name)) { - $custom_output = $file_name . ' does not exist'; + $file_name = $modx->atBindFilePath(substr($field_elements, 9)); + if ($file_name === false) { + $custom_output = 'Could not retrieve file for this TV.'; } else { ob_start(); include $file_name; @@ -942,9 +947,15 @@ function renderFormElement( } // end switch statement } else { $custom = explode(':', $field_type); - $file_name = EVO_BASE_PATH.'assets/tvs/'.$custom['1'].'/'.$custom['1'].'.customtv.php'; - if (!is_file($file_name)) { - $custom_output = $file_name . ' does not exist'; + // The widget name comes out of the TV's type column and lands in a + // path that is then included, so it goes through the same + // containment rule as every other binding. + $widget = (string) get_by_key($custom, 1, ''); + $file_name = $widget === '' + ? false + : $modx->atBindFilePath('assets/tvs/' . $widget . '/' . $widget . '.customtv.php'); + if ($file_name === false) { + $custom_output = 'Could not retrieve the custom TV widget.'; } else { ob_start(); include $file_name; diff --git a/core/src/Core.php b/core/src/Core.php index d937b554ac..b8c1614cb7 100644 --- a/core/src/Core.php +++ b/core/src/Core.php @@ -6433,7 +6433,7 @@ public function isSafeCode($phpcode = '', $safe_functions = '') * @return string|false * @since 3.5.8 */ - private function resolveAtBindFilePath($candidate) + public function resolveAtBindFilePath($candidate) { $resolved = realpath($candidate); if ($resolved === false || !is_file($resolved)) { @@ -6463,6 +6463,37 @@ private function resolveAtBindFilePath($candidate) return $resolved; } + /** + * The file an @FILE or @INCLUDE binding names, or false. + * + * One containment rule for every binding. It used to be four, and only one + * of them resolved the path before checking it. + * + * @param string $relative the binding's argument, as typed + * @param string[] $searchPaths prefixes below EVO_BASE_PATH, in order + * @return string|false + * @since 3.5.8 + */ + public function atBindFilePath($relative, array $searchPaths = ['']) + { + $relative = trim((string) $relative); + $relative = ltrim(str_replace(chr(92), '/', $relative), '/'); + + if ($relative === '') { + return false; + } + + foreach ($searchPaths as $path) { + $resolved = $this->resolveAtBindFilePath(EVO_BASE_PATH . $path . $relative); + + if ($resolved !== false) { + return $resolved; + } + } + + return false; + } + public function atBindFileContent($str = '') { @@ -6486,16 +6517,15 @@ public function atBindFileContent($str = '') $errorMsg = "Could not retrieve string '" . $str . "'."; - $search_path = ['assets/tvs/', 'assets/chunks/', 'assets/templates/', $this->getConfig('rb_base_url') . 'files/', '']; - foreach ($search_path as $path) { - $file_path = $this->resolveAtBindFilePath(EVO_BASE_PATH . $path . $str); - - if ($file_path !== false) { - break; - } - } + $file_path = $this->atBindFilePath($str, [ + 'assets/tvs/', + 'assets/chunks/', + 'assets/templates/', + $this->getConfig('rb_base_url') . 'files/', + '' + ]); - if (!$file_path) { + if ($file_path === false) { return $errorMsg; } @@ -6830,25 +6860,13 @@ public function atBindInclude($str = '') } $str = substr($str, 9); - $str = trim($str); - $str = str_replace('\\', '/', $str); - $str = ltrim($str, '/'); - - $tpl_dir = 'assets/templates/'; - - if (strpos($str, EVO_MANAGER_PATH) === 0) { - return false; - } - if (is_file(EVO_BASE_PATH . $str)) { - $file_path = EVO_BASE_PATH . $str; - } elseif (is_file(EVO_BASE_PATH . "{$tpl_dir}{$str}")) { - $file_path = EVO_BASE_PATH . $tpl_dir . $str; - } else { - return false; - } + // This includes rather than reads, so the path settles on is the + // path PHP executes. The old check ran on the string as typed, which + // a `..` walked straight past. + $file_path = $this->atBindFilePath($str, ['', 'assets/templates/']); - if (!$file_path || !is_file($file_path)) { + if ($file_path === false) { return false; } diff --git a/core/tests/Unit/Security/ParserEvalHardeningTest.php b/core/tests/Unit/Security/ParserEvalHardeningTest.php index 1475ab835d..0115594201 100644 --- a/core/tests/Unit/Security/ParserEvalHardeningTest.php +++ b/core/tests/Unit/Security/ParserEvalHardeningTest.php @@ -10,6 +10,7 @@ | - mergeConditionalTagsContent() <@IF:...> conditional tags | - _getSGVar() [[$_GET(x)]] superglobal reads | - atBindFileContent() @FILE: template includes +| - atBindInclude() @INCLUDE: template includes, which run the file | | All three are reachable from content that the parser re-scans across passes, so a snippet echoing | request data can carry a payload into them without any editing privilege. These tests drive the @@ -234,3 +235,89 @@ function parserHardeningCore(): Core } }); }); + +describe('@INCLUDE binding', function () { + + test('directory traversal outside the base path is refused', function () { + $core = parserHardeningCore(); + + // This one does not read the file, it includes it - so a path that + // escapes the tree is arbitrary code execution, not a disclosure. + expect($core->atBindInclude('@INCLUDE ' . str_repeat('../', 20) . 'Windows/win.ini'))->toBeFalse() + ->and($core->atBindInclude('@INCLUDE ' . str_repeat('../', 20) . 'etc/passwd'))->toBeFalse(); + }); + + test('a traversal back into the manager directory is refused', function () { + $core = parserHardeningCore(); + + // The old check asked whether the string as typed started with the + // manager path, so anything reaching it by way of `..` walked past. + expect($core->atBindInclude('@INCLUDE assets/../manager/index.php'))->toBeFalse() + ->and($core->atBindInclude('@INCLUDE manager/index.php'))->toBeFalse(); + }); + + test('a file inside the tree is still included', function () { + $core = parserHardeningCore(); + + $relative = 'evo_atinclude_' . bin2hex(random_bytes(6)) . '.php'; + $absolute = EVO_BASE_PATH . $relative; + file_put_contents($absolute, 'atBindInclude('@INCLUDE ' . $relative))->toBe('included-body') + // A traversal that normalises back inside the tree still resolves. + ->and($core->atBindInclude('@INCLUDE assets/../' . $relative))->toBe('included-body'); + } finally { + @unlink($absolute); + } + }); + + test('a missing file is refused rather than fatal', function () { + $core = parserHardeningCore(); + + expect($core->atBindInclude('@INCLUDE assets/evo_no_such_' . bin2hex(random_bytes(6)) . '.php')) + ->toBeFalse() + ->and($core->atBindInclude('@INCLUDE '))->toBeFalse(); + }); + + test('content with no binding is returned untouched', function () { + $core = parserHardeningCore(); + + expect($core->atBindInclude('plain content'))->toBe('plain content'); + }); +}); + +describe('the shared binding path resolver', function () { + + test('a directory is not a file a binding may name', function () { + $core = parserHardeningCore(); + + expect($core->atBindFilePath('.'))->toBeFalse() + ->and($core->atBindFilePath(''))->toBeFalse() + ->and($core->atBindFilePath(' '))->toBeFalse(); + }); + + test('search path prefixes are tried in order', function () { + $core = parserHardeningCore(); + + $name = 'evo_bindpath_' . bin2hex(random_bytes(6)) . '.txt'; + $absolute = EVO_BASE_PATH . $name; + file_put_contents($absolute, 'x'); + + try { + expect($core->atBindFilePath($name, ['']))->toBe(str_replace(chr(92), '/', realpath($absolute))) + // A prefix the file is not under does not find it. + ->and($core->atBindFilePath($name, ['assets/']))->toBeFalse(); + } finally { + @unlink($absolute); + } + }); + + test('a backslash separator is accepted and still contained', function () { + $core = parserHardeningCore(); + + // Windows style separators reach this from hand written bindings. + expect($core->atBindFilePath(str_repeat('..' . chr(92), 20) . 'Windows' . chr(92) . 'win.ini')) + ->toBeFalse(); + }); +}); diff --git a/core/tests/Unit/Security/TvBindingFilePathTest.php b/core/tests/Unit/Security/TvBindingFilePathTest.php new file mode 100644 index 0000000000..9c5b2d3ec0 --- /dev/null +++ b/core/tests/Unit/Security/TvBindingFilePathTest.php @@ -0,0 +1,91 @@ + str_replace( + "\r", + '', + (string) file_get_contents(dirname(__DIR__, 4) . '/' . $file) +); + +$copies = [ + 'core/functions/tv.php', + 'core/functions/actions/mutate_content.php', +]; + +test('no TV binding builds a path by concatenation any more', function () use ($read, $copies) { + foreach ($copies as $file) { + expect($read($file))->not->toContain('EVO_BASE_PATH . trim(substr('); + } +}); + +test('both copies resolve @FILE and @INCLUDE through the shared rule', function () use ($read, $copies) { + foreach ($copies as $file) { + $source = $read($file); + + // custom_widget (front end) and custom_tv (manager form), four call + // sites across the two files. + // custom_widget x2, custom_tv x2, and the custom_tv: file. + expect(substr_count($source, '$modx->atBindFilePath('))->toBe(5) + ->and($source)->toContain('atBindFilePath(substr($output, 6))') + ->and($source)->toContain('atBindFilePath(substr($output, 9))') + ->and($source)->toContain('atBindFilePath(substr($field_elements, 6))') + ->and($source)->toContain('atBindFilePath(substr($field_elements, 9))'); + } +}); + +test('a refused binding no longer echoes the path it tried', function () use ($read, $copies) { + // The message used to be the absolute server path plus " does not exist", + // which told a visitor where the installation lives on disk. + foreach ($copies as $file) { + expect($read($file))->not->toContain("\$file_name . ' does not exist'"); + } +}); + +test('the custom TV widget name cannot walk out of assets/tvs', function () use ($read, $copies) { + // $field_type is "custom_tv:", and the file it names is included. + foreach ($copies as $file) { + expect($read($file)) + ->toContain("atBindFilePath('assets/tvs/' . \$widget . '/' . \$widget . '.customtv.php')") + ->and($read($file))->not->toContain("EVO_BASE_PATH.'assets/tvs/'.\$custom['1']"); + } +}); + +test('a TV with no output option does not raise a notice', function () use ($read, $copies) { + // $params['output'] was read unconditionally, and display_params is empty + // for most TVs. + foreach ($copies as $file) { + $source = $read($file); + + expect($source)->toContain("\$output = (string) get_by_key(\$params, 'output', '');") + ->and($source)->not->toContain("\$params['output']"); + } +}); + +test('Core exposes one containment rule for every binding', function () use ($read) { + $core = $read('core/src/Core.php'); + + expect($core)->toContain('public function atBindFilePath($relative, array $searchPaths') + ->and($core)->toContain('public function resolveAtBindFilePath($candidate)') + // atBindInclude() decided containment on the string as typed, which a + // `..` segment walked past - the bug fixed for @FILE in 3.5.8. + ->and($core)->not->toContain('if (strpos($str, EVO_MANAGER_PATH) === 0)') + ->and($core)->toContain("\$this->atBindFilePath(\$str, ['', 'assets/templates/']);"); +});