Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions core/factory/settings.php
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand Down
4 changes: 2 additions & 2 deletions core/functions/actions/files.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down
2 changes: 1 addition & 1 deletion core/src/Controllers/Users/EditOrNewUser.php
Original file line number Diff line number Diff line change
Expand Up @@ -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()}";
}
Expand Down
4 changes: 2 additions & 2 deletions core/src/Core.php
Original file line number Diff line number Diff line change
Expand Up @@ -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 = '')
Expand Down Expand Up @@ -3648,7 +3648,7 @@ function __alertQuit() {
</script>
</head>
<body>
<p>" . $msg . '</p>
<p>" . htmlspecialchars((string)$msg, ENT_QUOTES, $manager_charset) . '</p>
</body>
</html>';
exit;
Expand Down
80 changes: 80 additions & 0 deletions core/tests/Unit/Security/FileManagerAndAlertHardeningTest.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,80 @@
<?php

/*
|--------------------------------------------------------------------------
| File manager tokens, media browser CSRF, alert escaping
|--------------------------------------------------------------------------
|
| The file manager guards its destructive GET actions with a one-shot session token, which was
| built from uniqid() - a clock value, not a random one. The media browser (mcpuk) had no CSRF
| check at all and relied on SameSite alone. webAlertAndQuit() wrote its message into the page
| verbatim, so any caller interpolating request data into an alert was a reflected XSS. The
| ?stay= and ?tab= parameters were concatenated into Location headers and a script block; they
| are only ever small integers.
|
*/

function repoSource(string $relative): string
{
return (string)file_get_contents(dirname(__DIR__, 4) . '/' . $relative);
}

it('builds the file manager token from random bytes and compares it in constant time', function () {
$source = repoSource('core/functions/actions/files.php');

expect($source)
->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 = "<?php echo text::jsValue(csrf_token()) ?>";');
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("<p>\" . htmlspecialchars((string)\$msg, ENT_QUOTES, \$manager_charset) . '</p>")
->and($source)->not->toContain("<p>\" . \$msg . '</p>");
});

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");
}
});
2 changes: 1 addition & 1 deletion manager/actions/bkmanager.static.php
Original file line number Diff line number Diff line change
Expand Up @@ -603,7 +603,7 @@ class="<?= $_style['icon_save'] ?>"></i> <?= $_lang["bkmgr_snapshot_submit"] ?>

$tab = get_by_key($_GET, 'tab', false);
if (is_numeric($tab)) {
echo '<script type="text/javascript">tpDBM.setSelectedIndex( ' . $_GET['tab'] . ' );</script>';
echo '<script type="text/javascript">tpDBM.setSelectedIndex( ' . (int)$_GET['tab'] . ' );</script>';
}

include_once EVO_MANAGER_PATH . "includes/footer.inc.php"; // send footer
12 changes: 12 additions & 0 deletions manager/media/browser/mcpuk/browse.php
Original file line number Diff line number Diff line change
Expand Up @@ -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();
2 changes: 2 additions & 0 deletions manager/media/browser/mcpuk/js/browser/misc.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
};

Expand Down
1 change: 1 addition & 0 deletions manager/media/browser/mcpuk/tpl/tpl_javascript.php
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@
browser.opener.TinyMCE4 = "<?= text::jsValue($this->get['field']) ?>";
<?php ENDIF ?>
browser.cms = "<?php echo text::jsValue($this->cms) ?>";
browser.csrfToken = "<?php echo text::jsValue(csrf_token()) ?>";
_.kuki.domain = "<?php echo text::jsValue($this->config['cookieDomain']) ?>";
_.kuki.path = "<?php echo text::jsValue($this->config['cookiePath']) ?>";
_.kuki.prefix = "<?php echo text::jsValue($this->config['cookiePrefix']) ?>";
Expand Down
4 changes: 2 additions & 2 deletions manager/processors/save_content.processor.php
Original file line number Diff line number Diff line change
Expand Up @@ -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";
}
Expand Down Expand Up @@ -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;
}
Expand Down
4 changes: 2 additions & 2 deletions manager/processors/save_htmlsnippet.processor.php
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -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);
Expand Down
4 changes: 2 additions & 2 deletions manager/processors/save_module.processor.php
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -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);
Expand Down
4 changes: 2 additions & 2 deletions manager/processors/save_plugin.processor.php
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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);
Expand Down
4 changes: 2 additions & 2 deletions manager/processors/save_snippet.processor.php
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -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);
Expand Down
4 changes: 2 additions & 2 deletions manager/processors/save_template.processor.php
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -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);
Expand Down
4 changes: 2 additions & 2 deletions manager/processors/save_tmplvars.processor.php
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -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);
Expand Down