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("", "x") spelled without a single letter. - $call = '"\146\151\154\145\137\160\165\164\137\143\157\156\164\145\156\164\163"("' + $call = '"' . octalEscape('file_put_contents') . '"("' . addcslashes($marker, "\\\"") - . '","\170")'; + . '","' . octalEscape('x') . '")'; ArithmeticExpression::evaluate($call); diff --git a/manager/actions/category_mgr/skin/edit.tpl.phtml b/manager/actions/category_mgr/skin/edit.tpl.phtml index 237e717fc0..7163ddeef1 100755 --- a/manager/actions/category_mgr/skin/edit.tpl.phtml +++ b/manager/actions/category_mgr/skin/edit.tpl.phtml @@ -38,7 +38,7 @@ - txt('delete'); ?> + txt('delete'); ?> diff --git a/manager/actions/logging.static.php b/manager/actions/logging.static.php index 99b8abe427..47d222a9d6 100755 --- a/manager/actions/logging.static.php +++ b/manager/actions/logging.static.php @@ -30,7 +30,7 @@ $logs_user = record_sort(array_unique_multi($logs, 'internalKey'), 'username'); foreach ($logs_user as $row) { $selectedtext = $row['internalKey'] == get_by_key($_REQUEST, 'searchuser') ? ' selected="selected"' : ''; - echo "\t\t" . '\n"; + echo "\t\t" . '\n"; } ?> @@ -66,7 +66,7 @@ $logs_items = record_sort(array_unique_multi($logs, 'itemid'), 'itemid'); foreach ($logs_items as $row) { $selectedtext = $row['itemid'] == get_by_key($_REQUEST, 'itemid') ? ' selected="selected"' : ''; - echo "\t\t" . '\n"; + echo "\t\t" . '\n"; } ?> @@ -82,7 +82,7 @@ $logs_names = record_sort(array_unique_multi($logs, 'itemname'), 'itemname'); foreach ($logs_names as $row) { $selectedtext = $row['itemname'] == get_by_key($_REQUEST, 'itemname') ? ' selected="selected"' : ''; - echo "\t\t" . '\n"; + echo "\t\t" . '\n"; } ?> @@ -92,7 +92,7 @@
+ value="getConfig('modx_charset')) ?>"/>
@@ -187,7 +187,20 @@ class=""> // Number of result to display on the page, will be in the LIMIT of the sql query also $int_num_result = is_numeric($_REQUEST['nrresults']) ? $_REQUEST['nrresults'] : EvolutionCMS()->getConfig('number_of_logs'); - $extargv = "&a=13&searchuser=" . get_by_key($_REQUEST, 'searchuser') . "&action=" . get_by_key($_REQUEST, 'action') . "&itemid=" . get_by_key($_REQUEST, 'itemid') . "&itemname=" . get_by_key($_REQUEST, 'itemname') . "&message=" . get_by_key($_REQUEST, 'message') . "&dateto=" . $_REQUEST['dateto'] . "&datefrom=" . $_REQUEST['datefrom'] . "&nrresults=" . $int_num_result . "&log_submit=" . $_REQUEST['log_submit']; // extra argv here (could be anything depending on your page) + // Paginate urldecodes this before escaping it into href="", so '%' is encoded once more to + // keep filter values that contain '&' or '=' intact. + $extargv = '&' . str_replace('%', '%25', http_build_query([ + 'a' => 13, + 'searchuser' => (string)get_by_key($_REQUEST, 'searchuser', '', 'is_scalar'), + 'action' => (string)get_by_key($_REQUEST, 'action', '', 'is_scalar'), + 'itemid' => (string)get_by_key($_REQUEST, 'itemid', '', 'is_scalar'), + 'itemname' => (string)get_by_key($_REQUEST, 'itemname', '', 'is_scalar'), + 'message' => (string)get_by_key($_REQUEST, 'message', '', 'is_scalar'), + 'dateto' => (string)get_by_key($_REQUEST, 'dateto', '', 'is_scalar'), + 'datefrom' => (string)get_by_key($_REQUEST, 'datefrom', '', 'is_scalar'), + 'nrresults' => (int)$int_num_result, + 'log_submit' => (string)get_by_key($_REQUEST, 'log_submit', '', 'is_scalar'), + ], '', '&', PHP_QUERY_RFC3986)); // build the sql $limit = $num_rows = $logs->count(); diff --git a/manager/actions/mutate_categories.dynamic.php b/manager/actions/mutate_categories.dynamic.php index 9bc3282407..be122ab6bf 100755 --- a/manager/actions/mutate_categories.dynamic.php +++ b/manager/actions/mutate_categories.dynamic.php @@ -10,12 +10,12 @@ $_module_params = [ 'module_version' => '1.0.0', 'module_params' => '', - 'module_id' => get_by_key($_GET, 'id', 0), + 'module_id' => (int)get_by_key($_GET, 'id', 0, 'is_scalar'), 'package_name' => 'Module_Categories_Manager', 'native_language' => 'de', 'name' => $_lang['manage_categories'], 'dirname' => EVO_MANAGER_URL, - 'url' => 'index.php?a=120&id=' . get_by_key($_GET, 'id', 0), + 'url' => 'index.php?a=120&id=' . (int)get_by_key($_GET, 'id', 0, 'is_scalar'), 'path' => realpath( __DIR__ ) . DIRECTORY_SEPARATOR . 'category_mgr' . DIRECTORY_SEPARATOR, 'inc_dir' => realpath( __DIR__ ) . DIRECTORY_SEPARATOR . 'category_mgr' . DIRECTORY_SEPARATOR . 'inc' . DIRECTORY_SEPARATOR, 'languages_dir' => realpath( __DIR__ ) . DIRECTORY_SEPARATOR . 'category_mgr' . DIRECTORY_SEPARATOR . 'lang' . DIRECTORY_SEPARATOR, diff --git a/manager/actions/mutate_menuindex_sort.dynamic.php b/manager/actions/mutate_menuindex_sort.dynamic.php index 6ab9c493d1..9e47f6d83b 100755 --- a/manager/actions/mutate_menuindex_sort.dynamic.php +++ b/manager/actions/mutate_menuindex_sort.dynamic.php @@ -82,7 +82,7 @@ $classes .= ($row['published']) ? ' publishedNode ' : ' unpublishedNode '; $classes = ($row['deleted']) ? ' deletedNode ' : $classes; $icon = $row['isfolder'] ? ' ' : ' '; - $resourcelist .= '
  • ' . $icon . $row['pagetitle'] . ' (' . $row['id'] . ')
  • '; + $resourcelist .= '
  • ' . $icon . entities($row['pagetitle'], evo()->getConfig('modx_charset')) . ' (' . (int)$row['id'] . ')
  • '; } $resourcelist .= '
    '; } else { diff --git a/manager/actions/mutate_web_user.dynamic.php b/manager/actions/mutate_web_user.dynamic.php index e5484584e6..abd32d0855 100755 --- a/manager/actions/mutate_web_user.dynamic.php +++ b/manager/actions/mutate_web_user.dynamic.php @@ -268,7 +268,7 @@ function evoRenderTvImageCheck(a) { " />

    - (' . $usernamedata['id'] . ')' : '') : $_lang['web_user_title']) ?> + getConfig('modx_charset')) . (isset($usernamedata['id']) ? '(' . (int)$usernamedata['id'] . ')' : '') : $_lang['web_user_title']) ?>

    diff --git a/manager/actions/web_user_management.static.php b/manager/actions/web_user_management.static.php index 551b37631e..e6328e8ea4 100644 --- a/manager/actions/web_user_management.static.php +++ b/manager/actions/web_user_management.static.php @@ -38,7 +38,7 @@ $role_options = ''; $roles = \EvolutionCMS\Models\UserRole::query()->select('id', 'name')->get()->toArray(); foreach ($roles as $row) { - $role_options .= ''; + $role_options .= ''; } // prepare data @@ -122,13 +122,13 @@ $listDocs[] = [ 'icon' => '', - 'name' => '' . $el['username'] . '', - 'user_full_name' => $el['fullname'], - 'email' => $el['email'], - 'role' => $el['name'] ?: ManagerTheme::getLexicon('no_user_role'), + 'name' => '' . htmlspecialchars((string)$el['username'], ENT_QUOTES, ManagerTheme::getCharset(), false) . '', + 'user_full_name' => htmlspecialchars((string)$el['fullname'], ENT_QUOTES, ManagerTheme::getCharset(), false), + 'email' => htmlspecialchars((string)$el['email'], ENT_QUOTES, ManagerTheme::getCharset(), false), + 'role' => $el['name'] ? htmlspecialchars((string)$el['name'], ENT_QUOTES, ManagerTheme::getCharset(), false) : ManagerTheme::getLexicon('no_user_role'), 'user_prevlogin' => $el['thislogin'] ? $modx->toDateFormat($el['thislogin']) : '-', 'user_logincount' => $el['logincount'], - 'user_block' => $el['blocked'] ? ManagerTheme::getLexicon('yes').' ' : '-', + 'user_block' => $el['blocked'] ? ManagerTheme::getLexicon('yes').' ' : '-', ]; } @@ -215,7 +215,7 @@ function menuAction(a) { - +
    diff --git a/manager/media/style/default/js/evo.js b/manager/media/style/default/js/evo.js index 7e03f98c99..669f901fde 100644 --- a/manager/media/style/default/js/evo.js +++ b/manager/media/style/default/js/evo.js @@ -1881,7 +1881,7 @@ keepMeAlive: function () { evo.get('includes/session_keepalive.php?tok=' + d.getElementById('sessTokenInput').value + '&o=' + Math.random(), function (r) { r = JSON.parse(r); - if (r.status !== 'ok') w.location.href = evo.EVO_MANAGER_URL + '?a=8'; + if (r.status !== 'ok') w.location.href = evo.EVO_MANAGER_URL + '?a=8&_token=' + encodeURIComponent(evo.tabsCsrfToken()); }); }, openWindow: function (a) { diff --git a/manager/processors/delete_template.processor.php b/manager/processors/delete_template.processor.php index aef451e224..2af9a51305 100755 --- a/manager/processors/delete_template.processor.php +++ b/manager/processors/delete_template.processor.php @@ -29,7 +29,7 @@ diff --git a/manager/processors/delete_tmplvars.processor.php b/manager/processors/delete_tmplvars.processor.php index 6073d7bb3c..4124c9991c 100755 --- a/manager/processors/delete_tmplvars.processor.php +++ b/manager/processors/delete_tmplvars.processor.php @@ -42,7 +42,7 @@ diff --git a/manager/views/frame/1.blade.php b/manager/views/frame/1.blade.php index 6ebba7a1f0..4a748205ed 100644 --- a/manager/views/frame/1.blade.php +++ b/manager/views/frame/1.blade.php @@ -314,7 +314,7 @@ function jsIconMarkup($icon) { @endif
  • - + {{ icon_html($_style['icon_logout']) }} {{ManagerTheme::getLexicon('logout')}}
  • diff --git a/manager/views/page/2.blade.php b/manager/views/page/2.blade.php index d34fdc4a90..76ace9dddb 100644 --- a/manager/views/page/2.blade.php +++ b/manager/views/page/2.blade.php @@ -462,7 +462,7 @@ : '') . ' - + ' . $_style['icon_logout'] . ' diff --git a/manager/views/page/user_roles/element_permission.blade.php b/manager/views/page/user_roles/element_permission.blade.php index a91633f323..388f687d7c 100644 --- a/manager/views/page/user_roles/element_permission.blade.php +++ b/manager/views/page/user_roles/element_permission.blade.php @@ -37,7 +37,7 @@
  • - +
  • diff --git a/manager/views/page/user_roles/permission.blade.php b/manager/views/page/user_roles/permission.blade.php index 40f86e9168..9b8c18758e 100644 --- a/manager/views/page/user_roles/permission.blade.php +++ b/manager/views/page/user_roles/permission.blade.php @@ -98,7 +98,7 @@ function changestate(element) { }, delete: function () { if (confirm("{{ ManagerTheme::getLexicon('confirm_delete_permission') }}") === true) { - document.location.href = "index.php?id=" + document.userform.id.value + "&a=135&action=delete"; + document.location.href = "index.php?id=" + document.userform.id.value + "&a=135&action=delete&_token={{ csrf_token() }}"; } }, cancel: function () { diff --git a/manager/views/page/user_roles/permissions_groups.blade.php b/manager/views/page/user_roles/permissions_groups.blade.php index 57f09c3ccf..7b34433401 100644 --- a/manager/views/page/user_roles/permissions_groups.blade.php +++ b/manager/views/page/user_roles/permissions_groups.blade.php @@ -58,7 +58,7 @@ function changestate(element) { }, delete: function () { if (confirm("{{ ManagerTheme::getLexicon('confirm_delete_category') }}") === true) { - document.location.href = "index.php?id=" + document.userform.id.value + "&a=136&action=delete"; + document.location.href = "index.php?id=" + document.userform.id.value + "&a=136&action=delete&_token={{ csrf_token() }}"; } }, cancel: function () { diff --git a/manager/views/page/user_roles/user_role.blade.php b/manager/views/page/user_roles/user_role.blade.php index 599c239be1..be8553d4af 100644 --- a/manager/views/page/user_roles/user_role.blade.php +++ b/manager/views/page/user_roles/user_role.blade.php @@ -201,7 +201,7 @@ function changestate(element) { }, delete: function () { if (confirm("{{ ManagerTheme::getLexicon('confirm_delete_role') }}") === true) { - document.location.href = "index.php?id=" + document.userform.id.value + "&a=35&action=delete"; + document.location.href = "index.php?id=" + document.userform.id.value + "&a=35&action=delete&_token={{ csrf_token() }}"; } }, cancel: function () {