diff --git a/core/functions/actions/files.php b/core/functions/actions/files.php index 789590611b..640918e97e 100644 --- a/core/functions/actions/files.php +++ b/core/functions/actions/files.php @@ -418,10 +418,38 @@ function fileManagerRemoveLink($path) } } +if(!function_exists('fileManagerIsExecutableName')) { + /** + * Whether a file name would be run by the web server or changes how it serves a folder: + * a PHP-like extension anywhere in the name ("shell.php.jpg" runs under AddHandler), or + * one of the per-directory configuration files. + * + * @param string $name + * @return bool + */ + function fileManagerIsExecutableName($name) + { + $base = strtolower(basename(str_replace(chr(92), '/', (string) $name))); + if (in_array($base, ['.htaccess', '.htpasswd', '.user.ini', '.env', 'web.config'], true)) { + return true; + } + $parts = explode('.', $base); + array_shift($parts); + + foreach ($parts as $part) { + if (preg_match('/^(?:php\d*|phps|phtml|pht|phar|inc|cgi|pl|py|sh|asp|aspx|jsp)$/', trim($part))) { + return true; + } + } + + return false; + } +} + if(!function_exists('fileManagerExtractZip')) { /** * Extracts $file into $path, skipping any entry that would land outside it, go through a - * symlink, or reach a protected folder. + * symlink, reach a protected folder, or be a server-executable file. * * @param string $file * @param string $path @@ -461,6 +489,10 @@ function fileManagerExtractZip($file, $path, array $protectedPaths = [], $dirMod if (!fileManagerIsSafeWriteTarget($root, $target) || fileManagerPathIsProtected($target, $protectedPaths)) { continue; } + // the upload form refuses these; unpacking must not be the way around it + if (!$isDir && fileManagerIsExecutableName(end($segments))) { + continue; + } if ($isDir) { if (!is_dir($target)) { mkdir($target, $dirMode, true); diff --git a/core/functions/actions/mutate_content.php b/core/functions/actions/mutate_content.php index 3985badd14..fb6ff17252 100644 --- a/core/functions/actions/mutate_content.php +++ b/core/functions/actions/mutate_content.php @@ -547,7 +547,7 @@ function getTVDisplayFormat($name, $value, $format, $paramstring = "", $tvtype = 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) { + if ($file_name === false || !$modx->atBindFileIsReadable($file_name)) { $widget_output = 'Could not retrieve file for TV ' . $name . '.'; } else { $widget_output = file_get_contents($file_name); @@ -905,7 +905,7 @@ function renderFormElement( /* If we are loading a file */ if (substr($field_elements, 0, 5) == "@FILE") { $file_name = $modx->atBindFilePath(substr($field_elements, 6)); - if ($file_name === false) { + if ($file_name === false || !$modx->atBindFileIsReadable($file_name)) { $custom_output = 'Could not retrieve file for this TV.'; } else { $custom_output = file_get_contents($file_name); diff --git a/core/functions/tv.php b/core/functions/tv.php index 08f011b9d0..8b3dfd4e20 100644 --- a/core/functions/tv.php +++ b/core/functions/tv.php @@ -523,7 +523,7 @@ function getTVDisplayFormat($name, $value, $format, $paramstring = '', $tvtype = 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) { + if ($file_name === false || !$modx->atBindFileIsReadable($file_name)) { $widget_output = 'Could not retrieve file for TV ' . $name . '.'; } else { $widget_output = file_get_contents($file_name); @@ -899,7 +899,7 @@ function renderFormElement( /* If we are loading a file */ if (strpos($field_elements, '@FILE') === 0) { $file_name = $modx->atBindFilePath(substr($field_elements, 6)); - if ($file_name === false) { + if ($file_name === false || !$modx->atBindFileIsReadable($file_name)) { $custom_output = 'Could not retrieve file for this TV.'; } else { $custom_output = file_get_contents($file_name); diff --git a/core/src/Core.php b/core/src/Core.php index a780e97bae..9873372c15 100644 --- a/core/src/Core.php +++ b/core/src/Core.php @@ -6615,11 +6615,15 @@ public function resolveAtBindFilePath($candidate) return false; } - $manager = realpath(EVO_MANAGER_PATH); - if ($manager !== false) { - $manager = rtrim(str_replace(DIRECTORY_SEPARATOR, '/', $manager), '/') . '/'; - if (strpos($resolved, $manager) === 0) { - return false; + // the manager and the core hold source, configuration and .env, none of it content + foreach ([EVO_MANAGER_PATH, EVO_CORE_PATH] as $protected) { + $protected = realpath($protected); + if ($protected !== false) { + $protected = rtrim(str_replace(DIRECTORY_SEPARATOR, '/', $protected), '/') . '/'; + // a core that is not below the base (a relocated one) is not what the base serves + if ($protected !== $base && strpos($protected, $base) === 0 && strpos($resolved, $protected) === 0) { + return false; + } } } @@ -6657,6 +6661,37 @@ public function atBindFilePath($relative, array $searchPaths = ['']) return false; } + /** + * Whether a file resolved by atBindFilePath() may be handed out as text by @FILE or the + * file_get_contents modifier: no source (PHP-like files, wherever they live: snippets, + * plugins, modules), no hidden files or folders, no generated or backup data. + * + * @param string|false $path + * @return bool + */ + public function atBindFileIsReadable($path) + { + if ($path === false || $path === '') { + return false; + } + $path = str_replace(DIRECTORY_SEPARATOR, '/', (string) $path); + $base = realpath(EVO_BASE_PATH); + $relative = $base === false ? $path : ltrim(substr($path, strlen(rtrim(str_replace(DIRECTORY_SEPARATOR, '/', $base), '/'))), '/'); + + foreach (explode('/', $relative) as $segment) { + if (strpos($segment, '.') === 0) { + return false; + } + } + if (preg_match('~^assets/(?:cache|backup)(?:/|$)~i', $relative)) { + return false; + } + $ext = '.' . strtolower(pathinfo($path, PATHINFO_EXTENSION)); + $denied = array_merge(self::AT_BIND_FILE_DENIED_EXTENSIONS, ['.pht', '.cgi', '.pl', '.py', '.sh', '.env', '.ini', '.sql', '.log', '.bak', '.conf', '.pem', '.key']); + + return !in_array($ext, $denied, true); + } + public function atBindFileContent($str = '') { @@ -6688,7 +6723,7 @@ public function atBindFileContent($str = '') '' ]); - if ($file_path === false) { + if ($file_path === false || !$this->atBindFileIsReadable($file_path)) { return $errorMsg; } diff --git a/core/src/Legacy/Modifiers.php b/core/src/Legacy/Modifiers.php index d8141a9ccb..fff17dc0b3 100644 --- a/core/src/Legacy/Modifiers.php +++ b/core/src/Legacy/Modifiers.php @@ -1123,17 +1123,19 @@ public function getValueFromPreset($key, $value, $cmd, $opt) if (!is_file($value)) { return $value; } - $value = realpath($value); - if (strpos($value, EVO_MANAGER_PATH) !== false) { - exit('Can not read core file'); - } - $ext = strtolower(substr($value, -4)); - if ($ext === '.php') { - exit('Can not read php file'); - } - if ($ext === '.cgi') { - exit('Can not read cgi file'); + // same rules as @FILE: inside the installation, not manager/ or core/, no source or secrets + $resolved = $modx->resolveAtBindFilePath($value); + if ($resolved === false) { + exit('Can not read file: outside the site, or inside manager/ or core/'); + } + if (!$modx->atBindFileIsReadable($resolved)) { + $reason = 'hidden, generated or backup file'; + if (preg_match('/\.(php\d*|phps|phtml|pht|phar|inc|cgi|pl|py|sh|env|ini|sql|log|bak|conf|pem|key)$/i', $resolved, $matches)) { + $reason = 'denied extension .' . strtolower($matches[1]); + } + exit('Can not read file: ' . $reason); } + $value = $resolved; return file_get_contents($value); case 'filesize': diff --git a/core/src/Services/DocumentSaveService.php b/core/src/Services/DocumentSaveService.php index d30ef82752..9ba02fd7d1 100644 --- a/core/src/Services/DocumentSaveService.php +++ b/core/src/Services/DocumentSaveService.php @@ -12,6 +12,7 @@ use EvolutionCMS\Support\DocumentSave\PublishState; use EvolutionCMS\Support\DocumentSave\TemplateVariableInput; use EvolutionCMS\Support\DocumentSave\TemplateVariableValues; +use EvolutionCMS\Support\TvBindingGuard; use Illuminate\Support\Facades\DB; /** @@ -81,6 +82,15 @@ public function save(array $input): DocumentSaveResult $tvs = TemplateVariableValues::forTemplate($template, $id, !$ctx->isAdministrator(), $ctx->managerDocgroups); $tvValues = TemplateVariableInput::values($tvs, $input); + // @EVAL and @SELECT in a TV value run code when it is rendered: that takes the PHP permission + $storedValues = []; + foreach ($tvs as $tv) { + $storedValues[$tv['id']] = $tv['value']; + } + if (!TvBindingGuard::allowsValues($ctx->can('save_snippet'), $tvValues, $storedValues, (string) ($input['ta'] ?? ''), (string) ($existing->content ?? ''), $this->textFields($input), $existing ? $this->textFields($existing->getAttributes()) : [])) { + throw new DocumentSaveDenied($ctx->lang('error_no_privileges')); + } + $now = $ctx->now; $pubDate = empty($input['pub_date']) ? 0 : $ctx->toTimestamp((string) $input['pub_date']); $unpubDate = empty($input['unpub_date']) ? 0 : $ctx->toTimestamp((string) $input['unpub_date']); @@ -300,4 +310,19 @@ private function transaction(callable $callback): mixed { return SiteContent::resolveConnection()->transaction($callback); } + + /** + * The text fields a TV binding can pull in with [*field*]. + * + * @return array + */ + private function textFields(array $source): array + { + $fields = []; + foreach (['pagetitle', 'longtitle', 'description', 'introtext', 'menutitle', 'link_attributes', 'alias'] as $name) { + $fields[$name] = (string) ($source[$name] ?? ''); + } + + return $fields; + } } diff --git a/core/src/Support/DocumentSave/TemplateVariableValues.php b/core/src/Support/DocumentSave/TemplateVariableValues.php index 77d3b674f3..52201163cd 100644 --- a/core/src/Support/DocumentSave/TemplateVariableValues.php +++ b/core/src/Support/DocumentSave/TemplateVariableValues.php @@ -13,7 +13,7 @@ final class TemplateVariableValues { /** * TVs of a template with the value stored for the document. Non-administrators only - * get the TVs without access rules or whose document belongs to one of their groups. + * get the TVs without access rules or whose access rows name one of their document groups. * * @return array */ @@ -31,12 +31,12 @@ public static function forTemplate(int $template, int $documentId, bool $restric ->where('site_tmplvar_templates.templateid', $template) ->orderBy('site_tmplvars.rank'); + // the TV's own access rows decide, not the groups of the document it is saved on if ($restrictToGroups) { - $query->leftJoin('document_groups', 'site_tmplvar_contentvalues.contentid', '=', 'document_groups.document') - ->where(function ($q) use ($managerGroups) { - $q->whereNull('site_tmplvar_access.documentgroup') - ->orWhereIn('document_groups.document_group', $managerGroups); - }); + $query->where(function ($q) use ($managerGroups) { + $q->whereNull('site_tmplvar_access.documentgroup') + ->orWhereIn('site_tmplvar_access.documentgroup', $managerGroups); + }); } $rows = []; diff --git a/core/src/Support/TvBindingGuard.php b/core/src/Support/TvBindingGuard.php new file mode 100644 index 0000000000..1c17e6c841 --- /dev/null +++ b/core/src/Support/TvBindingGuard.php @@ -0,0 +1,97 @@ + column) a binding can sit in. */ + public const FIELDS = ['elements' => 'elements', 'default_text' => 'default_text', 'display_params' => 'display_params']; + + public static function hasCodeBinding(?string $value): bool + { + $value = (string) $value; + // the parser trims with trim(), which also drops NUL and vertical tab, and matches a command + // by prefix ("@EVALreturn 1;" runs), so no boundary may be required after the name. + // @INHERIT hands its argument back to the parser, so it is looked through. + for ($depth = 0; $depth < 20; $depth++) { + $value = ltrim($value, " \t\n\r\0\x0B"); + if (preg_match('/^@{1,2}(?:EVAL|SELECT)/i', $value)) { + return true; + } + if (!preg_match('/^@INHERIT/i', $value)) { + return false; + } + $value = substr($value, 8); + } + + return true; // deeper than any real value nests: do not guess + } + + /** + * A TV value or resource field that can reach a code binding. Bindings nest (@INHERIT hands + * its argument back to the binding parser), so one is also caught behind another binding. + */ + public static function reachesCodeBinding(?string $value): bool + { + return self::hasCodeBinding($value); + } + + /** + * Whether a manager may store these TV values and the resource content. Values that did not + * change are kept, so an existing binding stays editable around by whoever may edit the page. + * + * @param array $desired TV id => submitted value + * @param array $stored TV id => current value + */ + public static function allowsValues(bool $mayWritePhp, array $desired, array $stored, string $content = '', string $storedContent = '', array $fields = [], array $storedFields = []): bool + { + if ($mayWritePhp) { + return true; + } + // content only runs as a binding when a TV pulls it in with @DOCUMENT, and then from its start + if ($content !== $storedContent && self::hasCodeBinding($content)) { + return false; + } + // [*field*] in a binding's argument pulls in any other field of the page, so each is checked + foreach ($fields as $name => $value) { + $value = (string) $value; + if (self::hasCodeBinding($value) && $value !== (string) ($storedFields[$name] ?? '')) { + return false; + } + } + foreach ($desired as $id => $value) { + $value = (string) $value; + if (self::reachesCodeBinding($value) && $value !== (string) ($stored[$id] ?? '')) { + return false; + } + } + + return true; + } + + /** + * @param array $submitted values keyed by column + * @param array $stored current values keyed by column, empty for a new TV + */ + public static function allows(bool $mayWritePhp, array $submitted, array $stored = []): bool + { + if ($mayWritePhp) { + return true; + } + foreach (self::FIELDS as $column) { + $value = (string) ($submitted[$column] ?? ''); + if (self::hasCodeBinding($value) && $value !== (string) ($stored[$column] ?? '')) { + return false; + } + } + + return true; + } +} diff --git a/core/tests/Unit/Manager/WebUserEmailRequiredMarkerTest.php b/core/tests/Unit/Manager/WebUserEmailRequiredMarkerTest.php index 2ce0feb2e8..38ab4025c1 100644 --- a/core/tests/Unit/Manager/WebUserEmailRequiredMarkerTest.php +++ b/core/tests/Unit/Manager/WebUserEmailRequiredMarkerTest.php @@ -1,8 +1,19 @@ toContain('* :'); + expect($form)->toContain('* :') + // the bare, unwrapped form is what main.css's td:first-child > .warning rule stretches + // to ~100% width, pushing the label text off to the right behind it + ->and($form)->not->toContain('* :'); }); diff --git a/core/tests/Unit/Security/DocumentPreviewSandboxTest.php b/core/tests/Unit/Security/DocumentPreviewSandboxTest.php new file mode 100644 index 0000000000..6e8d0a721c --- /dev/null +++ b/core/tests/Unit/Security/DocumentPreviewSandboxTest.php @@ -0,0 +1,21 @@ +toContain('id="previewIframe"') + ->and($source)->toMatch('/]*id="previewIframe"[^>]*sandbox="([^"]*)"/'); + + preg_match('/]*id="previewIframe"[^>]*sandbox="([^"]*)"/', $source, $matches); + $tokens = explode(' ', trim($matches[1])); + + expect($tokens)->not->toContain('allow-same-origin') + ->and($tokens)->toContain('allow-scripts'); +}); diff --git a/core/tests/Unit/Security/FileManagerExecutableNameTest.php b/core/tests/Unit/Security/FileManagerExecutableNameTest.php new file mode 100644 index 0000000000..4d706e8e3d --- /dev/null +++ b/core/tests/Unit/Security/FileManagerExecutableNameTest.php @@ -0,0 +1,11 @@ +toBeTrue(); +})->with(['shell.php', 'shell.PHP.jpg', 'a/b/x.phtml', '.htaccess', '.user.ini', 'x.phar.png', 'dir\x.php5']); + +it('accepts ordinary files', function (string $name) { + expect(fileManagerIsExecutableName($name))->toBeFalse(); +})->with(['photo.jpg', 'notes.txt', 'archive.tar.gz', 'php-logo.png', 'readme']); diff --git a/core/tests/Unit/Security/MediaBrowserDoubleExtensionTest.php b/core/tests/Unit/Security/MediaBrowserDoubleExtensionTest.php new file mode 100644 index 0000000000..1a9ef5ab99 --- /dev/null +++ b/core/tests/Unit/Security/MediaBrowserDoubleExtensionTest.php @@ -0,0 +1,47 @@ +newInstanceWithoutConstructor(); + foreach (['types' => ['images' => $types], 'config' => ['deniedExts' => $denied]] as $name => $value) { + $property = new ReflectionProperty(uploader::class, $name); + $property->setAccessible(true); + $property->setValue($uploader, $value); + } + + return $uploader; +} + +function uploaderAllowsName(uploader $uploader, string $name): bool +{ + $method = new ReflectionMethod(uploader::class, 'validateFilename'); + $method->setAccessible(true); + + return $method->invoke($uploader, $name, 'images'); +} + +it('rejects executable extensions hidden before the final extension', function (string $name) { + $uploader = uploaderForNames('jpg png gif', 'exe php phtml'); + + expect(uploaderAllowsName($uploader, $name))->toBeFalse(); +})->with(['shell.php.jpg', 'shell.PHP.jpg', 'a.b.phtml.png', 'shell.php.', 'dir\\shell.php.gif']); + +it('accepts ordinary image names', function (string $name) { + $uploader = uploaderForNames('jpg png gif', 'exe php phtml'); + + expect(uploaderAllowsName($uploader, $name))->toBeTrue(); +})->with(['photo.jpg', 'my.holiday.photo.PNG', 'php-logo.gif']); + +it('still rejects a denied or unlisted final extension', function (string $name) { + $uploader = uploaderForNames('jpg png gif', 'exe php phtml'); + + expect(uploaderAllowsName($uploader, $name))->toBeFalse(); +})->with(['shell.php', 'doc.txt']); diff --git a/core/tests/Unit/Security/ParserEvalHardeningTest.php b/core/tests/Unit/Security/ParserEvalHardeningTest.php index 8b55b9dac2..8cd8518748 100644 --- a/core/tests/Unit/Security/ParserEvalHardeningTest.php +++ b/core/tests/Unit/Security/ParserEvalHardeningTest.php @@ -238,6 +238,28 @@ function parserHardeningCore(): Core @unlink($absolute); } }); + + test('files under the core directory are refused, also through traversal', function () { + $core = parserHardeningCore(); + + // an earlier test may have pinned the constants to a tree whose core is not below its base + if (strpos(rtrim(EVO_CORE_PATH, '/') . '/', rtrim(EVO_BASE_PATH, '/') . '/core/') !== 0) { + $this->markTestSkipped('EVO_CORE_PATH is not below EVO_BASE_PATH/core here'); + } + + $relative = 'core/evo_atfile_' . bin2hex(random_bytes(6)) . '.txt'; + $absolute = EVO_BASE_PATH . $relative; + is_dir(dirname($absolute)) || mkdir(dirname($absolute), 0777, true); + file_put_contents($absolute, 'secret'); + + try { + expect($core->atBindFileContent('@FILE:' . $relative))->not->toBe('secret') + ->and($core->atBindFileContent('@FILE:assets/../' . $relative))->not->toBe('secret') + ->and($core->atBindFilePath($relative))->toBeFalse(); + } finally { + @unlink($absolute); + } + }); }); describe('@INCLUDE binding', function () { @@ -325,3 +347,34 @@ function parserHardeningCore(): Core ->toBeFalse(); }); }); + +describe('@FILE readability', function () { + + test('source, hidden, generated and secret files are not handed out as text', function (string $relative) { + $core = parserHardeningCore(); + $absolute = EVO_BASE_PATH . $relative; + is_dir(dirname($absolute)) || mkdir(dirname($absolute), 0777, true); + file_put_contents($absolute, 'x'); + + try { + expect($core->atBindFileIsReadable($core->atBindFilePath($relative)))->toBeFalse(); + } finally { + @unlink($absolute); + } + })->with(['assets/snippets/evo_x.php', 'assets/plugins/evo_x.phtml', 'assets/evo_x/.secret.txt', 'assets/cache/evo_x.txt', 'assets/backup/evo_x.sql', 'assets/evo_x.env']); + + test('ordinary content files stay readable', function () { + $core = parserHardeningCore(); + $relative = 'assets/templates/evo_x_' . bin2hex(random_bytes(4)) . '.html'; + $absolute = EVO_BASE_PATH . $relative; + is_dir(dirname($absolute)) || mkdir(dirname($absolute), 0777, true); + file_put_contents($absolute, '

ok

'); + + try { + expect($core->atBindFileIsReadable($core->atBindFilePath($relative)))->toBeTrue() + ->and($core->atBindFileIsReadable(false))->toBeFalse(); + } finally { + @unlink($absolute); + } + }); +}); diff --git a/core/tests/Unit/Security/TvBindingGuardTest.php b/core/tests/Unit/Security/TvBindingGuardTest.php new file mode 100644 index 0000000000..edc15d6378 --- /dev/null +++ b/core/tests/Unit/Security/TvBindingGuardTest.php @@ -0,0 +1,59 @@ +toBeTrue(); +})->with(['@EVAL return 1;', ' @eval return 1;', '@@EVAL return time();', "@SELECT id FROM t"]); + +it('lets harmless values through', function (?string $value) { + expect(TvBindingGuard::hasCodeBinding($value))->toBeFalse(); +})->with([null, '', 'Red==red||Blue==blue', '@CHUNK header', '@FILE assets/x.html', 'mail@EVALUATE.com', 'text @EVAL later']); + +it('refuses a new code binding without the PHP permission', function (string $field) { + expect(TvBindingGuard::allows(false, [$field => '@EVAL return 1;']))->toBeFalse(); +})->with(['elements', 'default_text', 'display_params']); + +it('allows a code binding for a manager who may write PHP', function () { + expect(TvBindingGuard::allows(true, ['elements' => '@EVAL return 1;']))->toBeTrue(); +}); + +it('keeps an existing binding editable around it, but not changeable, without the permission', function () { + $stored = ['elements' => '@EVAL return 1;']; + + expect(TvBindingGuard::allows(false, ['elements' => '@EVAL return 1;', 'default_text' => 'x'], $stored))->toBeTrue() + ->and(TvBindingGuard::allows(false, ['elements' => '@EVAL system("id");'], $stored))->toBeFalse(); +}); + +it('catches a code binding in a TV value even behind another binding', function (string $value) { + expect(TvBindingGuard::reachesCodeBinding($value))->toBeTrue(); +})->with(['@EVAL return 1;', '@INHERIT @EVAL return 1;', "@INHERIT\n@eval x();", '@@SELECT 1']); + +it('leaves ordinary TV text alone', function (string $value) { + expect(TvBindingGuard::reachesCodeBinding($value))->toBeFalse(); +})->with(['', 'write to me@select.com', '@CHUNK header', 'plain']); + +it('refuses new code in TV values or resource content without the PHP permission', function () { + expect(TvBindingGuard::allowsValues(false, [3 => '@INHERIT @EVAL 1;'], []))->toBeFalse() + ->and(TvBindingGuard::allowsValues(false, [], [], '@EVAL return 1;', 'old'))->toBeFalse() + ->and(TvBindingGuard::allowsValues(true, [3 => '@EVAL 1;'], [], '@EVAL 1;'))->toBeTrue() + ->and(TvBindingGuard::allowsValues(false, [3 => '@EVAL 1;'], [3 => '@EVAL 1;']))->toBeTrue() + ->and(TvBindingGuard::allowsValues(false, [3 => 'text'], [3 => '@EVAL 1;']))->toBeTrue(); +}); + +it('catches the forms the parser accepts but a word-boundary check misses', function (string $value) { + expect(TvBindingGuard::hasCodeBinding($value))->toBeTrue(); +})->with([ + 'no boundary' => '@EVALreturn 1;', + 'select no boundary' => '@SELECTid FROM x', + 'inherit chain' => '@INHERIT@EVAL x', + 'nested inherit' => "@INHERIT @INHERIT\n@evalx", + 'nul prefix' => "\0@EVAL x", + 'vertical tab prefix' => "\x0B@EVAL x", +]); + +it('checks the page text fields a binding can pull in with a placeholder', function () { + expect(TvBindingGuard::allowsValues(false, [], [], '', '', ['pagetitle' => '@EVALreturn 1;'], ['pagetitle' => 'old']))->toBeFalse() + ->and(TvBindingGuard::allowsValues(false, [], [], '', '', ['pagetitle' => 'Hello'], ['pagetitle' => 'old']))->toBeTrue() + ->and(TvBindingGuard::allowsValues(true, [], [], '', '', ['pagetitle' => '@EVAL 1;'], []))->toBeTrue(); +}); diff --git a/core/tests/Unit/Support/DocumentSave/TemplateVariableValuesTest.php b/core/tests/Unit/Support/DocumentSave/TemplateVariableValuesTest.php index ed36a77ea7..f3cc8824ad 100644 --- a/core/tests/Unit/Support/DocumentSave/TemplateVariableValuesTest.php +++ b/core/tests/Unit/Support/DocumentSave/TemplateVariableValuesTest.php @@ -60,15 +60,18 @@ function bootTvFixture(): Capsule ->and(array_keys($rows[0]))->toBe(['id', 'name', 'type', 'default_text', 'value_id', 'value']); })->skip(!extension_loaded('pdo_sqlite'), 'pdo_sqlite is required'); -test('a restricted tv is hidden from a user whose document is not in its group', function () { +test('a restricted tv is hidden from a manager outside its own access group', function () { bootTvFixture(); - expect(array_column(TemplateVariableValues::forTemplate(1, 100, true, [5]), 'id'))->toBe([2, 1]); - + // TV 3 is restricted to document group 9. A manager in group 5 does not get it, even once + // the document itself is put into group 5 - the document's groups are not the TV's groups. Capsule::table('document_groups')->insert(['document_group' => 5, 'document' => 100]); Capsule::table('site_tmplvar_contentvalues')->insert(['tmplvarid' => 3, 'contentid' => 100, 'value' => 'v3']); - expect(array_column(TemplateVariableValues::forTemplate(1, 100, true, [5]), 'id'))->toBe([2, 1, 3]); + expect(array_column(TemplateVariableValues::forTemplate(1, 100, true, [5]), 'id'))->toBe([2, 1]); + + // Only membership in the TV's own access group (9) exposes it. + expect(array_column(TemplateVariableValues::forTemplate(1, 100, true, [9]), 'id'))->toBe([2, 1, 3]); })->skip(!extension_loaded('pdo_sqlite'), 'pdo_sqlite is required'); test('sync writes only the differences and leaves other documents alone', function () { diff --git a/core/vendor/composer/autoload_classmap.php b/core/vendor/composer/autoload_classmap.php index 2d46a890c9..d50e7b6ae1 100644 --- a/core/vendor/composer/autoload_classmap.php +++ b/core/vendor/composer/autoload_classmap.php @@ -1443,6 +1443,7 @@ 'EvolutionCMS\\Support\\SqliteDumper' => $baseDir . '/src/Support/SqliteDumper.php', 'EvolutionCMS\\Support\\SystemSettingPathNormalizer' => $baseDir . '/src/Support/SystemSettingPathNormalizer.php', 'EvolutionCMS\\Support\\TemplateFileEngines' => $baseDir . '/src/Support/TemplateFileEngines.php', + 'EvolutionCMS\\Support\\TvBindingGuard' => $baseDir . '/src/Support/TvBindingGuard.php', 'EvolutionCMS\\TemplateProcessor' => $baseDir . '/src/TemplateProcessor.php', 'EvolutionCMS\\Tracy\\ConnectionTiming' => $baseDir . '/src/Tracy/ConnectionTiming.php', 'EvolutionCMS\\Tracy\\Debugger' => $baseDir . '/src/Tracy/Debugger.php', diff --git a/core/vendor/composer/autoload_static.php b/core/vendor/composer/autoload_static.php index 8c0e8dd377..4bd613dbec 100644 --- a/core/vendor/composer/autoload_static.php +++ b/core/vendor/composer/autoload_static.php @@ -2105,6 +2105,7 @@ class ComposerStaticInit925fea465a58fa69f06ccf2629003e87 'EvolutionCMS\\Support\\SqliteDumper' => __DIR__ . '/../..' . '/src/Support/SqliteDumper.php', 'EvolutionCMS\\Support\\SystemSettingPathNormalizer' => __DIR__ . '/../..' . '/src/Support/SystemSettingPathNormalizer.php', 'EvolutionCMS\\Support\\TemplateFileEngines' => __DIR__ . '/../..' . '/src/Support/TemplateFileEngines.php', + 'EvolutionCMS\\Support\\TvBindingGuard' => __DIR__ . '/../..' . '/src/Support/TvBindingGuard.php', 'EvolutionCMS\\TemplateProcessor' => __DIR__ . '/../..' . '/src/TemplateProcessor.php', 'EvolutionCMS\\Tracy\\ConnectionTiming' => __DIR__ . '/../..' . '/src/Tracy/ConnectionTiming.php', 'EvolutionCMS\\Tracy\\Debugger' => __DIR__ . '/../..' . '/src/Tracy/Debugger.php', diff --git a/manager/actions/mutate_web_user.dynamic.php b/manager/actions/mutate_web_user.dynamic.php index abd32d0855..363aaacf99 100755 --- a/manager/actions/mutate_web_user.dynamic.php +++ b/manager/actions/mutate_web_user.dynamic.php @@ -356,7 +356,12 @@ function evoRenderTvImageCheck(a) { - * : + + * : diff --git a/manager/includes/mutate_settings.ajax.php b/manager/includes/mutate_settings.ajax.php index ccfeb0ef5b..ba547ce1cf 100644 --- a/manager/includes/mutate_settings.ajax.php +++ b/manager/includes/mutate_settings.ajax.php @@ -15,6 +15,17 @@ $str = ''; $emptyCache = false; +// page 118 is a plain view, so nothing before this file checked what the manager may do +$core = EvolutionCMS(); +if ($action === 'setsetting' && strpos($key, '_hide_') !== 0 && !$core->hasPermission('settings')) { + $action = ''; // only the dismiss flags of notices are open to every manager +} elseif ($action === 'updateplugin') { + $needed = $key === '_delete_' ? 'delete_plugin' : 'save_plugin'; + if (!$core->hasPermission($needed) || ($key !== '_delete_' && $key !== 'disabled')) { + $action = ''; + } +} + switch (true) { case ($action == 'get' && preg_match('/^[A-z0-9_-]+$/', $lang) && file_exists(EVO_CORE_PATH . 'lang/' . $lang . '/global.php')): { diff --git a/manager/media/browser/mcpuk/core/browser.php b/manager/media/browser/mcpuk/core/browser.php index 59f7dabf0a..c08479fc2e 100755 --- a/manager/media/browser/mcpuk/core/browser.php +++ b/manager/media/browser/mcpuk/core/browser.php @@ -496,7 +496,7 @@ protected function act_rename() $this->errorMsg("A file or folder with that name already exists."); } $ext = file::getExtension($newName); - if (!$this->validateExtension($ext, $this->type)) { + if (!$this->validateFilename($newName, $this->type)) { $this->errorMsg("Denied file extension."); } if (is_array($evtOut) && !empty($evtOut)) { @@ -622,7 +622,7 @@ protected function act_cp_cbd() $error[] = $this->label("The file '{file}' does not exist.", $replace); } elseif (substr($base, 0, 1) == ".") { $error[] = "$base: " . $this->label("File name shouldn't begins with '.'"); - } elseif (!$this->validateExtension($ext, $type)) { + } elseif (!$this->validateFilename($base, $type)) { $error[] = "$base: " . $this->label("Denied file extension."); } elseif (file_exists("$dir/$base")) { $error[] = "$base: " . $this->label("A file or folder with that name already exists."); @@ -711,7 +711,7 @@ protected function act_mv_cbd() $error[] = $this->label("The file '{file}' does not exist.", $replace); } elseif (substr($base, 0, 1) == ".") { $error[] = "$base: " . $this->label("File name shouldn't begins with '.'"); - } elseif (!$this->validateExtension($ext, $type)) { + } elseif (!$this->validateFilename($base, $type)) { $error[] = "$base: " . $this->label("Denied file extension."); } elseif (file_exists("$dir/$base")) { $error[] = "$base: " . $this->label("A file or folder with that name already exists."); diff --git a/manager/media/browser/mcpuk/core/uploader.php b/manager/media/browser/mcpuk/core/uploader.php index 8069b8f522..b24a393f9f 100755 --- a/manager/media/browser/mcpuk/core/uploader.php +++ b/manager/media/browser/mcpuk/core/uploader.php @@ -406,9 +406,13 @@ protected function checkUploadedFile(array $aFile = null) return $this->label("File name shouldn't begins with '.'"); // EXTENSION CHECK - elseif (!$this->validateExtension($extension, $this->type)) + elseif (!$this->validateFilename($file['name'], $this->type)) return $this->label("Denied file extension."); + // RASTER IMAGE NAMES MUST HOLD IMAGE DATA OF THE KIND THEY CLAIM + elseif (!$this->imageDataMatchesExtension($extension, $file['tmp_name'])) + return $this->label("Unknown error."); + // SPECIAL DIRECTORY TYPES CHECK (e.g. *img) elseif (preg_match('/^\*([^ ]+)(.*)?$/s', $typePatt, $patt)) { list($typePatt, $type, $params) = $patt; @@ -473,6 +477,57 @@ protected function checkInputDir($dir, $inclType = true, $existing = true) return (is_dir($path) && is_readable($path)) ? $return : false; } + /** + * A name that claims a raster image format has to hold that format: getimagesize() must read + * it and report the type the extension stands for. + * + * @param string $ext + * @param string $path + * @return bool + */ + protected function imageDataMatchesExtension($ext, $path) + { + $kinds = [ + 'jpg' => [IMAGETYPE_JPEG], 'jpeg' => [IMAGETYPE_JPEG], 'jpe' => [IMAGETYPE_JPEG], + 'png' => [IMAGETYPE_PNG], 'gif' => [IMAGETYPE_GIF], 'bmp' => [IMAGETYPE_BMP], + 'webp' => [IMAGETYPE_WEBP], 'ico' => [IMAGETYPE_ICO], 'avif' => [defined('IMAGETYPE_AVIF') ? IMAGETYPE_AVIF : -1], + 'tif' => [IMAGETYPE_TIFF_II, IMAGETYPE_TIFF_MM], 'tiff' => [IMAGETYPE_TIFF_II, IMAGETYPE_TIFF_MM], + 'psd' => [IMAGETYPE_PSD], + ]; + $ext = strtolower(trim($ext)); + if (!isset($kinds[$ext])) { + return true; + } + $info = @getimagesize($path); + + return $info !== false && in_array($info[2], $kinds[$ext], true); + } + + /** + * Like validateExtension() for a whole file name: every inner dot-separated part is also checked + * against the denied list, since Apache's AddHandler runs PHP for "shell.php.jpg". + * + * @param string $name + * @param string $type + * @return bool + */ + protected function validateFilename($name, $type) + { + $parts = explode('.', basename(str_replace('\\', '/', (string)$name))); + $ext = array_pop($parts); + array_shift($parts); + + $denied = strtolower(text::clearWhitespaces($this->config['deniedExts'])); + $denied = strlen($denied) ? explode(' ', $denied) : []; + foreach ($parts as $part) { + if (in_array(strtolower(trim($part)), $denied, true)) { + return false; + } + } + + return $this->validateExtension($ext, $type); + } + /** * @param $ext * @param $type diff --git a/manager/processors/save_tmplvars.processor.php b/manager/processors/save_tmplvars.processor.php index 49312f1782..ebf119cad8 100755 --- a/manager/processors/save_tmplvars.processor.php +++ b/manager/processors/save_tmplvars.processor.php @@ -21,6 +21,7 @@ $originId = isset($_REQUEST['oid']) ? (int)$_REQUEST['oid'] : null; $currentdate = time() + $modx->config['server_offset_time']; $properties = $_POST['properties']; +$tvBindingFields = ['elements' => $elements, 'default_text' => $default_text, 'display_params' => $params]; //Kyle Jaebker - added category support if (empty($_POST['newcategory']) && $_POST['categoryid'] > 0) { @@ -47,6 +48,11 @@ "id" => $id ]); + // @EVAL and @SELECT run code, which save_template alone does not grant + if (!\EvolutionCMS\Support\TvBindingGuard::allows($modx->hasPermission('save_snippet'), $tvBindingFields)) { + $modx->webAlertAndQuit($_lang["error_no_privileges"]); + } + // disallow duplicate names for new tvs if (EvolutionCMS\Models\SiteTmplvar::where('name', '=', $name)->first()) { $modx->getManagerApi()->saveFormValues(300); @@ -113,6 +119,12 @@ "id" => $id ]); + // @EVAL and @SELECT run code, which save_template alone does not grant + $storedTv = EvolutionCMS\Models\SiteTmplvar::find($id); + if (!\EvolutionCMS\Support\TvBindingGuard::allows($modx->hasPermission('save_snippet'), $tvBindingFields, $storedTv ? $storedTv->getAttributes() : [])) { + $modx->webAlertAndQuit($_lang["error_no_privileges"]); + } + // disallow duplicate names for tvs if (EvolutionCMS\Models\SiteTmplvar::where('name', '=', $name)->where('id', '!=', $id)->first()) { $modx->getManagerApi()->saveFormValues(300); diff --git a/manager/views/page/3.blade.php b/manager/views/page/3.blade.php index 0df1b896d3..28d0b5da60 100644 --- a/manager/views/page/3.blade.php +++ b/manager/views/page/3.blade.php @@ -497,7 +497,10 @@ class="' . $_style['icon_move'] . '">' . $icon_pub_unpub : '') . (evo()- @if(!empty($show_preview))
{{ ManagerTheme::getLexicon('preview') }}
- + {{-- sandboxed without allow-same-origin: previewed content runs scripts but gets an opaque + origin, so it cannot reach window.top, read the manager's CSRF meta tag, or ride the + manager's session into a same-origin request. --}} +
@endif @endsection