diff --git a/core/factory/settings.php b/core/factory/settings.php index 07ca7228f9..5c15dd76dc 100644 --- a/core/factory/settings.php +++ b/core/factory/settings.php @@ -54,8 +54,9 @@ 'use_editor' => 1, 'editor_css_path' => '', 'filemanager_path' => '[(base_path)]', - 'upload_files' => 'bmp,ico,gif,jpeg,jpg,png,psd,tif,tiff,fla,flv,swf,aac,au,avi,css,cache,doc,docx,gz,gzip,htaccess,htm,html,js,mp3,mp4,mpeg,mpg,ods,odp,odt,pdf,ppt,pptx,rar,tar,tgz,txt,wav,wmv,xls,xlsx,xml,z,zip,JPG,JPEG,PNG,GIF,svg,tpl,webp,avif', - 'upload_images' => 'bmp,ico,gif,jpeg,jpg,png,psd,tif,tiff,svg,webp,avif', + // no svg (scriptable, same-origin), htaccess or flash; defaults only reach new installs + 'upload_files' => 'bmp,ico,gif,jpeg,jpg,png,psd,tif,tiff,aac,au,avi,css,cache,doc,docx,gz,gzip,htm,html,js,mp3,mp4,mpeg,mpg,ods,odp,odt,pdf,ppt,pptx,rar,tar,tgz,txt,wav,wmv,xls,xlsx,xml,z,zip,JPG,JPEG,PNG,GIF,tpl,webp,avif', + 'upload_images' => 'bmp,ico,gif,jpeg,jpg,png,psd,tif,tiff,webp,avif', 'upload_media' => 'au,avi,mp3,mp4,mpeg,mpg,wav,wmv', 'upload_maxsize' => '5000000', 'new_file_permissions' => '0644', diff --git a/core/functions/actions/files.php b/core/functions/actions/files.php index 3add29b8a5..d87bfcb12a 100644 --- a/core/functions/actions/files.php +++ b/core/functions/actions/files.php @@ -813,7 +813,7 @@ function checkToken() $token = false; } - if (isset($_SESSION['token']) && !empty($_SESSION['token']) && $_SESSION['token'] === $token) { + if (is_string($token) && isset($_SESSION['token']) && !empty($_SESSION['token']) && hash_equals($_SESSION['token'], $token)) { $rs = true; } else { $rs = false; @@ -830,7 +830,7 @@ function checkToken() */ function makeToken() { - $newToken = uniqid('', true); + $newToken = bin2hex(random_bytes(16)); // uniqid() is clock-derived, not random $_SESSION['token'] = $newToken; return $newToken; diff --git a/core/src/Controllers/Users/EditOrNewUser.php b/core/src/Controllers/Users/EditOrNewUser.php index 38f6950ef1..0f50a961c4 100644 --- a/core/src/Controllers/Users/EditOrNewUser.php +++ b/core/src/Controllers/Users/EditOrNewUser.php @@ -157,7 +157,7 @@ public function process(): bool if ($userData['stay'] != '') { $a = ($userData['stay'] == '2') ? "88&id={$user->getKey()}" : "87"; - $this->parameters['url'] = "index.php?a={$a}&r=2&stay=" . $userData['stay']; + $this->parameters['url'] = "index.php?a={$a}&r=2&stay=" . (int)$userData['stay']; } else { $this->parameters['url'] = "index.php?a=88&id={$user->getKey()}"; } diff --git a/core/src/Core.php b/core/src/Core.php index f7dd4269fa..c2e1388a98 100644 --- a/core/src/Core.php +++ b/core/src/Core.php @@ -3595,7 +3595,7 @@ public function getChildIds($id, $depth = 10, $children = []) /** * Displays a javascript alert message in the web browser and quit * - * @param string $msg Message to show + * @param string $msg Message to show, as plain text * @param string $url URL to redirect to */ public function webAlertAndQuit($msg, $url = '') @@ -3648,7 +3648,7 @@ function __alertQuit() { -

" . $msg . '

+

" . htmlspecialchars((string)$msg, ENT_QUOTES, $manager_charset) . '

'; exit; diff --git a/core/tests/Unit/Security/FileManagerAndAlertHardeningTest.php b/core/tests/Unit/Security/FileManagerAndAlertHardeningTest.php new file mode 100644 index 0000000000..97d00d6cf2 --- /dev/null +++ b/core/tests/Unit/Security/FileManagerAndAlertHardeningTest.php @@ -0,0 +1,80 @@ +toContain('$newToken = bin2hex(random_bytes(16));') + ->toContain("hash_equals(\$_SESSION['token'], \$token)") + ->and($source)->not->toContain("uniqid('', true)"); +}); + +it('requires the session CSRF token for every media browser act except the page and its thumbnails', function () { + $entry = repoSource('manager/media/browser/mcpuk/browse.php'); + + expect($entry) + ->toContain("in_array(\$act, ['browser', 'thumb'], true)") + ->toContain('hash_equals(csrf_token(), $token)'); + + // the check must run before the browser is instantiated + expect(strpos($entry, 'hash_equals(csrf_token()'))->toBeLessThan(strpos($entry, 'new browser(')); + + // ... and the scripts have to send it: every request URL is built by baseGetData() + expect(repoSource('manager/media/browser/mcpuk/tpl/tpl_javascript.php')) + ->toContain('browser.csrfToken = "";'); + expect(repoSource('manager/media/browser/mcpuk/js/browser/misc.js')) + ->toContain('data += "&_token=" + encodeURIComponent(this.csrfToken);'); +}); + +it('escapes the message webAlertAndQuit writes into the alert page', function () { + $source = repoSource('core/src/Core.php'); + + expect($source) + ->toContain("

\" . htmlspecialchars((string)\$msg, ENT_QUOTES, \$manager_charset) . '

") + ->and($source)->not->toContain("

\" . \$msg . '

"); +}); + +it('casts the stay parameter before echoing it into a redirect', function (string $file) { + $source = repoSource($file); + + expect($source)->not->toMatch('/stay=[\'"] \. \$_POST\[\'stay\'\]/'); +})->with(array_map( + static fn (string $path) => 'manager/processors/' . basename($path), + glob(dirname(__DIR__, 4) . '/manager/processors/save_*.processor.php') +)); + +it('casts the backup manager tab index before writing it into a script block', function () { + expect(repoSource('manager/actions/bkmanager.static.php')) + ->toContain("tpDBM.setSelectedIndex( ' . (int)\$_GET['tab'] . ' );"); +}); + +it('ships upload defaults without scriptable or server-config extensions', function () { + // the factory file needs a booted manager, so read the two lines instead of requiring it + $source = repoSource('core/factory/settings.php'); + + foreach (['upload_files', 'upload_images'] as $key) { + expect(preg_match("/'$key' => '([^']*)'/", $source, $m))->toBe(1); + $list = explode(',', $m[1]); + expect(array_intersect($list, ['svg', 'htaccess', 'swf', 'fla', 'flv', 'php', 'phtml', 'phar'])) + ->toBe([], "$key allows a dangerous extension"); + } +}); diff --git a/manager/actions/bkmanager.static.php b/manager/actions/bkmanager.static.php index c76b81ba5c..8ad2eb50f3 100755 --- a/manager/actions/bkmanager.static.php +++ b/manager/actions/bkmanager.static.php @@ -603,7 +603,7 @@ class=""> $tab = get_by_key($_GET, 'tab', false); if (is_numeric($tab)) { - echo ''; + echo ''; } include_once EVO_MANAGER_PATH . "includes/footer.inc.php"; // send footer diff --git a/manager/media/browser/mcpuk/browse.php b/manager/media/browser/mcpuk/browse.php index 163159b6eb..588cc7c612 100755 --- a/manager/media/browser/mcpuk/browse.php +++ b/manager/media/browser/mcpuk/browse.php @@ -23,5 +23,17 @@ function returnNoPermissionsMessage($role) { if( $_GET['type'] == 'images' && !EvolutionCMS()->hasPermission('file_manager') && !EvolutionCMS()->hasPermission('assets_images')) returnNoPermissionsMessage('assets_images'); if( $_GET['type'] == 'files' && !EvolutionCMS()->hasPermission('file_manager') && !EvolutionCMS()->hasPermission('assets_files')) returnNoPermissionsMessage('assets_files'); +// Only the page itself and the thumbnails it embeds are fetched without a token; every other +// act, including reads, is scripted through browser.baseGetData() and carries the session one. +$act = isset($_GET['act']) ? $_GET['act'] : 'browser'; +if (!in_array($act, ['browser', 'thumb'], true)) { + $token = isset($_REQUEST['_token']) && is_string($_REQUEST['_token']) ? $_REQUEST['_token'] : ''; + if ($token === '' || !hash_equals(csrf_token(), $token)) { + header('HTTP/1.1 403 Forbidden'); + header('Content-Type: text/plain; charset=utf-8'); + die(json_encode(['error' => 'Invalid CSRF token.'])); + } +} + $browser = new browser($modx); $browser->action(); diff --git a/manager/media/browser/mcpuk/js/browser/misc.js b/manager/media/browser/mcpuk/js/browser/misc.js index 11d5645d70..3e899d0c06 100755 --- a/manager/media/browser/mcpuk/js/browser/misc.js +++ b/manager/media/browser/mcpuk/js/browser/misc.js @@ -339,6 +339,8 @@ browser.baseGetData = function(act) { data += "&act=" + act; if (this.cms) data += "&cms=" + this.cms; + if (this.csrfToken) + data += "&_token=" + encodeURIComponent(this.csrfToken); return data; }; diff --git a/manager/media/browser/mcpuk/tpl/tpl_javascript.php b/manager/media/browser/mcpuk/tpl/tpl_javascript.php index 47fb459624..a532d3f192 100755 --- a/manager/media/browser/mcpuk/tpl/tpl_javascript.php +++ b/manager/media/browser/mcpuk/tpl/tpl_javascript.php @@ -43,6 +43,7 @@ browser.opener.TinyMCE4 = "get['field']) ?>"; browser.cms = "cms) ?>"; +browser.csrfToken = ""; _.kuki.domain = "config['cookieDomain']) ?>"; _.kuki.path = "config['cookiePath']) ?>"; _.kuki.prefix = "config['cookiePrefix']) ?>"; diff --git a/manager/processors/save_content.processor.php b/manager/processors/save_content.processor.php index e5b2459247..226e1ec937 100755 --- a/manager/processors/save_content.processor.php +++ b/manager/processors/save_content.processor.php @@ -476,7 +476,7 @@ } else { $a = ($_POST['stay'] == '2') ? "27&id=$key" : "4&pid=$parentId"; } - $redirectUrl = "index.php?a=" . $a . "&r=1&stay=" . $_POST['stay']; + $redirectUrl = "index.php?a=" . $a . "&r=1&stay=" . (int)$_POST['stay']; } else { $redirectUrl = "index.php?a=3&id=$key&r=1"; } @@ -689,7 +689,7 @@ // document $a = ($_POST['stay'] == '2') ? "27&id=$id" : "4&pid=$parentId"; } - $redirectUrl = "index.php?a=" . $a . "&r=1&stay=" . $_POST['stay'] . $add_path; + $redirectUrl = "index.php?a=" . $a . "&r=1&stay=" . (int)$_POST['stay'] . $add_path; } else { $redirectUrl = "index.php?a=3&id=$id&r=1" . $add_path; } diff --git a/manager/processors/save_htmlsnippet.processor.php b/manager/processors/save_htmlsnippet.processor.php index dafcd8d350..3207d8385c 100755 --- a/manager/processors/save_htmlsnippet.processor.php +++ b/manager/processors/save_htmlsnippet.processor.php @@ -97,7 +97,7 @@ // finished emptying cache - redirect if ($_POST['stay'] != '') { $a = ($_POST['stay'] == '2') ? "78&id=$id" : "77"; - $header = "Location: index.php?a=" . $a . "&tab=2&stay=" . $_POST['stay']; + $header = "Location: index.php?a=" . $a . "&tab=2&stay=" . (int)$_POST['stay']; header($header); } else { $header = "Location: index.php?a=76&tab=2"; @@ -137,7 +137,7 @@ // finished emptying cache - redirect if ($_POST['stay'] != '') { $a = ($_POST['stay'] == '2') ? "78&id=$id" : "77"; - $header = "Location: index.php?a=" . $a . "&r=2&stay=" . $_POST['stay']; + $header = "Location: index.php?a=" . $a . "&r=2&stay=" . (int)$_POST['stay']; header($header); } else { evo()->unlockElement(3, $id); diff --git a/manager/processors/save_module.processor.php b/manager/processors/save_module.processor.php index 4a4d51ebe6..0ff9fa4e8b 100755 --- a/manager/processors/save_module.processor.php +++ b/manager/processors/save_module.processor.php @@ -144,7 +144,7 @@ // finished emptying cache - redirect if ($_POST['stay'] != '') { $a = ($_POST['stay'] == '2') ? "108&id=$newid" : "107"; - $header = "Location: index.php?a=" . $a . "&r=2&stay=" . $_POST['stay']; + $header = "Location: index.php?a=" . $a . "&r=2&stay=" . (int)$_POST['stay']; header($header); } else { $header = "Location: index.php?a=76&tab=5&r=2"; @@ -202,7 +202,7 @@ // finished emptying cache - redirect if ($_POST['stay'] != '') { $a = ($_POST['stay'] == '2') ? "108&id=$id" : "107"; - $header = "Location: index.php?a=" . $a . "&r=2&stay=" . $_POST['stay']; + $header = "Location: index.php?a=" . $a . "&r=2&stay=" . (int)$_POST['stay']; header($header); } else { $modx->unlockElement(6, $id); diff --git a/manager/processors/save_plugin.processor.php b/manager/processors/save_plugin.processor.php index b56faa84e7..f4787cb1d6 100755 --- a/manager/processors/save_plugin.processor.php +++ b/manager/processors/save_plugin.processor.php @@ -131,7 +131,7 @@ // finished emptying cache - redirect if ($_POST['stay'] != '') { $a = ($_POST['stay'] == '2') ? "102&id=$newid" : '101'; - $header = 'Location: index.php?a=' . $a . '&r=2&stay=' . $_POST['stay']; + $header = 'Location: index.php?a=' . $a . '&r=2&stay=' . (int)$_POST['stay']; header($header); } else { $header = 'Location: index.php?a=76&tab=4&r=2'; @@ -186,7 +186,7 @@ // finished emptying cache - redirect if ($_POST['stay'] != '') { $a = ($_POST['stay'] == '2') ? "102&id=$id" : '101'; - $header = 'Location: index.php?a=' . $a . '&r=2&stay=' . $_POST['stay']; + $header = 'Location: index.php?a=' . $a . '&r=2&stay=' . (int)$_POST['stay']; header($header); } else { $modx->unlockElement(5, $id); diff --git a/manager/processors/save_snippet.processor.php b/manager/processors/save_snippet.processor.php index a0e9889638..bf33f880f9 100755 --- a/manager/processors/save_snippet.processor.php +++ b/manager/processors/save_snippet.processor.php @@ -127,7 +127,7 @@ // finished emptying cache - redirect if ($_POST['stay'] != '') { $a = ($_POST['stay'] == '2') ? "22&id=$newid" : "23"; - $header = "Location: index.php?a=" . $a . "&r=2&stay=" . $_POST['stay']; + $header = "Location: index.php?a=" . $a . "&r=2&stay=" . (int)$_POST['stay']; header($header); } else { $header = "Location: index.php?a=76&tab=3&r=2"; @@ -167,7 +167,7 @@ // finished emptying cache - redirect if ($_POST['stay'] != '') { $a = ($_POST['stay'] == '2') ? "22&id=$id" : "23"; - $header = "Location: index.php?a=" . $a . "&r=2&stay=" . $_POST['stay']; + $header = "Location: index.php?a=" . $a . "&r=2&stay=" . (int)$_POST['stay']; header($header); } else { $modx->unlockElement(4, $id); diff --git a/manager/processors/save_template.processor.php b/manager/processors/save_template.processor.php index a241410593..251b977946 100755 --- a/manager/processors/save_template.processor.php +++ b/manager/processors/save_template.processor.php @@ -255,7 +255,7 @@ function writeTemplateFile($templatealias, $extension, $content, $mayCreate) // finished emptying cache - redirect if ($_POST['stay'] != '') { $a = ($_POST['stay'] == '2') ? "16&id=$newid" : "19"; - $header = "Location: index.php?a=" . $a . "&r=2&stay=" . $_POST['stay']; + $header = "Location: index.php?a=" . $a . "&r=2&stay=" . (int)$_POST['stay']; header($header); } else { $header = "Location: index.php?a=76&r=2"; @@ -371,7 +371,7 @@ function writeTemplateFile($templatealias, $extension, $content, $mayCreate) // finished emptying cache - redirect if ($_POST['stay'] != '') { $a = ($_POST['stay'] == '2') ? "16&id=$id" : "19"; - $header = "Location: index.php?a=" . $a . "&r=2&stay=" . $_POST['stay']; + $header = "Location: index.php?a=" . $a . "&r=2&stay=" . (int)$_POST['stay']; header($header); } else { EvolutionCMS()->unlockElement(1, $id); diff --git a/manager/processors/save_tmplvars.processor.php b/manager/processors/save_tmplvars.processor.php index f6ddb77e04..49312f1782 100755 --- a/manager/processors/save_tmplvars.processor.php +++ b/manager/processors/save_tmplvars.processor.php @@ -99,7 +99,7 @@ // finished emptying cache - redirect if ($_POST['stay'] != '') { $a = ($_POST['stay'] == '2') ? "301&id=$newid" : "300"; - $header = "Location: index.php?a=" . $a . "&r=2&stay=" . $_POST['stay']; + $header = "Location: index.php?a=" . $a . "&r=2&stay=" . (int)$_POST['stay']; header($header); } else { $header = "Location: index.php?a=76&tab=1&r=2"; @@ -163,7 +163,7 @@ // finished emptying cache - redirect if ($_POST['stay'] != '') { $a = ($_POST['stay'] == '2') ? "301&id=$id" : "300"; - $header = "Location: index.php?a=" . $a . "&r=2&stay=" . $_POST['stay'] . "&or=" . $origin . "&oid=" . $originId; + $header = "Location: index.php?a=" . $a . "&r=2&stay=" . (int)$_POST['stay'] . "&or=" . $origin . "&oid=" . $originId; header($header); } else { $modx->unlockElement(2, $id);