diff --git a/core/src/Controllers/SystemInfo.php b/core/src/Controllers/SystemInfo.php
index 660dcae672..18b8e606cc 100644
--- a/core/src/Controllers/SystemInfo.php
+++ b/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 = '' . $label . '';
+ $label = '' . $label . '';
}
$details = sprintf($this->managerTheme->getLexicon('opcache_memory_details'),
$this->formatBytes($used),
diff --git a/core/src/ManagerTheme.php b/core/src/ManagerTheme.php
index ec759f4048..8aa184097f 100644
--- a/core/src/ManagerTheme.php
+++ b/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(),
diff --git a/core/src/Middleware/VerifyCsrfToken.php b/core/src/Middleware/VerifyCsrfToken.php
index f46dc63ac6..5bbd8e4b3e 100644
--- a/core/src/Middleware/VerifyCsrfToken.php
+++ b/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]));
}
/**
diff --git a/core/src/Support/Paginate.php b/core/src/Support/Paginate.php
index 44052b8a58..d32b873caf 100644
--- a/core/src/Support/Paginate.php
+++ b/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);
}
/**
diff --git a/core/tests/Unit/Security/BackupManagerShellTest.php b/core/tests/Unit/Security/BackupManagerShellTest.php
index c4ca9a4d39..0fd81ab766 100644
--- a/core/tests/Unit/Security/BackupManagerShellTest.php
+++ b/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();
diff --git a/core/tests/Unit/Security/BladeStoredXssTest.php b/core/tests/Unit/Security/BladeStoredXssTest.php
index bb51aaeda1..1523d71e3c 100644
--- a/core/tests/Unit/Security/BladeStoredXssTest.php
+++ b/core/tests/Unit/Security/BladeStoredXssTest.php
@@ -52,7 +52,7 @@ function renderBladeFragment(string $template, array $data = []): string
*/
function storedXssPayload(): string
{
- return 'Import failed
bad value:'; + return 'Import failed
bad value:'; } 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(''; + . '
row 12:'; $rendered = (string) safe_html($stored); @@ -30,7 +30,7 @@ // ...but the injected element is inert text, not an element. expect($rendered)->not->toContain('') - ->and($rendered)->toContain('<script>alert(document.cookie)</script>'); + ->and($rendered)->toContain('<script>alert(document.domain)</script>'); }); test('attributes cannot survive the allow list', function () { diff --git a/core/tests/Unit/Support/ArithmeticExpressionTest.php b/core/tests/Unit/Support/ArithmeticExpressionTest.php index 9eda282bed..f892f35972 100644 --- a/core/tests/Unit/Support/ArithmeticExpressionTest.php +++ b/core/tests/Unit/Support/ArithmeticExpressionTest.php @@ -86,15 +86,24 @@ }); }); +/** + * Spells a string as PHP octal escapes, so a payload holds no letters for the callers' filter to + * strip. Built at runtime to keep obfuscated call literals - an antivirus signature - out of the file. + */ +function octalEscape(string $value): string +{ + return implode('', array_map(static fn (string $char) => '\\' . decoct(ord($char)), str_split($value))); +} + describe('rejects everything that is not arithmetic', function () { // Every payload below survives `preg_replace('@([a-zA-Z\n\r\t\s])@', '', $filter)` - the filter // the callers apply before handing the string over - because it contains no letters at all. $payloads = [ - 'octal escaped system() call' => '"\163\171\163\164\145\155"("\151\144")', - 'octal escaped phpinfo' => '"\160\150\160\151\156\146\157"()', - 'backtick shell operator' => '1 . `\151\144`', - 'variable variable' => '${"\137\107\105\124"}', + 'octal escaped call with an argument' => '"' . octalEscape('strrev') . '"("' . octalEscape('x') . '")', + 'octal escaped call without arguments' => '"' . octalEscape('phpversion') . '"()', + 'backtick shell operator' => '1 . `' . octalEscape('true') . '`', + 'variable variable' => '${"' . octalEscape('_GET') . '"}', 'statement separator' => '1;print_r($_SERVER)', 'superglobal read' => '$_SERVER', 'string concatenation' => '"1"."2"', @@ -125,9 +134,9 @@ $marker = sys_get_temp_dir() . DIRECTORY_SEPARATOR . 'evo_arith_' . bin2hex(random_bytes(6)); // file_put_contents("