Fix manager privilege escalation

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);

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);

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);

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;
         }
 

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':

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<string, string>
+     */
+    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;
+    }
 }

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<int, array{id:int, name:string, type:string, default_text:string, value_id:int|null, value:string|null}>
      */
@@ -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 = [];

core/src/Support/TvBindingGuard.php

@@ -0,0 +1,97 @@
+<?php
+
+namespace EvolutionCMS\Support;
+
+/**
+ * Keeps the TV bindings that run code out of the hands of managers who may edit TVs but not PHP.
+ *
+ * @EVAL passes its argument to eval() and @SELECT puts it into a query, in a TV's input options,
+ * default value and output options. save_template alone is enough to store them, so they need the
+ * permission that already grants arbitrary PHP (save_snippet).
+ */
+final class TvBindingGuard
+{
+    /** Definition fields (POST name => 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<int|string, string|null> $desired TV id => submitted value
+     * @param array<int|string, string|null> $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<string, mixed> $submitted values keyed by column
+     * @param array<string, mixed> $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;
+    }
+}

core/tests/Unit/Manager/WebUserEmailRequiredMarkerTest.php

@@ -1,8 +1,19 @@
 <?php
 
-it('marks the web user email label as required in the form', function () {
+/*
+| main.css styles ".warning" as a ~100%-width inline-block whenever it sits as a direct child of
+| td:first-child in these forms, for a label whose whole text is the warning. The email row only
+| wraps the "*" in .warning, so that rule stretched the lone asterisk across the cell and pushed
+| the label text out to the right behind it. The label is wrapped in an outer span so .warning is
+| no longer a direct child of the td and the rule stops matching.
+*/
+
+it('marks the web user email label as required in the form, with the asterisk wrapped', function () {
     $formPath = dirname(__DIR__, 4) . '/manager/actions/mutate_web_user.dynamic.php';
     $form = file_get_contents($formPath);
 
-    expect($form)->toContain('<span class="warning">*</span> <?php echo $_lang[\'user_email\']; ?>:');
+    expect($form)->toContain('<span><span class="warning">*</span> <?php echo $_lang[\'user_email\']; ?>:</span>')
+        // 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('<span class="warning">*</span> <?php echo $_lang[\'user_email\']; ?>:</td>');
 });

core/tests/Unit/Security/DocumentPreviewSandboxTest.php

@@ -0,0 +1,21 @@
+<?php
+
+/*
+| The resource preview iframe loads the front-end's rendering of whatever content a manager
+| (possibly a limited editor, not the one previewing) saved on the resource. Without a sandbox,
+| same-origin script there can reach window.top, read the manager's CSRF meta tag, and ride the
+| previewing manager's session into a same-origin request. This pins the mitigation in place.
+*/
+
+it('sandboxes the resource preview iframe without allow-same-origin', function () {
+    $source = file_get_contents(dirname(__DIR__, 4) . '/manager/views/page/3.blade.php');
+
+    expect($source)->toContain('id="previewIframe"')
+        ->and($source)->toMatch('/<iframe[^>]*id="previewIframe"[^>]*sandbox="([^"]*)"/');
+
+    preg_match('/<iframe[^>]*id="previewIframe"[^>]*sandbox="([^"]*)"/', $source, $matches);
+    $tokens = explode(' ', trim($matches[1]));
+
+    expect($tokens)->not->toContain('allow-same-origin')
+        ->and($tokens)->toContain('allow-scripts');
+});