Skip to content

Commit ab0ecf4

Browse files
authored
Merge pull request #2493 from elcreator/fix-manager-privilege-escalation
Fix manager privilege escalation
2 parents 91d9a89 + 068766e commit ab0ecf4

23 files changed

Lines changed: 523 additions & 39 deletions

‎core/functions/actions/files.php‎

Lines changed: 33 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -418,10 +418,38 @@ function fileManagerRemoveLink($path)
418418
}
419419
}
420420

421+
if(!function_exists('fileManagerIsExecutableName')) {
422+
/**
423+
* Whether a file name would be run by the web server or changes how it serves a folder:
424+
* a PHP-like extension anywhere in the name ("shell.php.jpg" runs under AddHandler), or
425+
* one of the per-directory configuration files.
426+
*
427+
* @param string $name
428+
* @return bool
429+
*/
430+
function fileManagerIsExecutableName($name)
431+
{
432+
$base = strtolower(basename(str_replace(chr(92), '/', (string) $name)));
433+
if (in_array($base, ['.htaccess', '.htpasswd', '.user.ini', '.env', 'web.config'], true)) {
434+
return true;
435+
}
436+
$parts = explode('.', $base);
437+
array_shift($parts);
438+
439+
foreach ($parts as $part) {
440+
if (preg_match('/^(?:php\d*|phps|phtml|pht|phar|inc|cgi|pl|py|sh|asp|aspx|jsp)$/', trim($part))) {
441+
return true;
442+
}
443+
}
444+
445+
return false;
446+
}
447+
}
448+
421449
if(!function_exists('fileManagerExtractZip')) {
422450
/**
423451
* Extracts $file into $path, skipping any entry that would land outside it, go through a
424-
* symlink, or reach a protected folder.
452+
* symlink, reach a protected folder, or be a server-executable file.
425453
*
426454
* @param string $file
427455
* @param string $path
@@ -461,6 +489,10 @@ function fileManagerExtractZip($file, $path, array $protectedPaths = [], $dirMod
461489
if (!fileManagerIsSafeWriteTarget($root, $target) || fileManagerPathIsProtected($target, $protectedPaths)) {
462490
continue;
463491
}
492+
// the upload form refuses these; unpacking must not be the way around it
493+
if (!$isDir && fileManagerIsExecutableName(end($segments))) {
494+
continue;
495+
}
464496
if ($isDir) {
465497
if (!is_dir($target)) {
466498
mkdir($target, $dirMode, true);

‎core/functions/actions/mutate_content.php‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -547,7 +547,7 @@ function getTVDisplayFormat($name, $value, $format, $paramstring = "", $tvtype =
547547
their own, not the one that grants arbitrary PHP. */
548548
if (substr($output, 0, 5) == "@FILE") {
549549
$file_name = $modx->atBindFilePath(substr($output, 6));
550-
if ($file_name === false) {
550+
if ($file_name === false || !$modx->atBindFileIsReadable($file_name)) {
551551
$widget_output = 'Could not retrieve file for TV ' . $name . '.';
552552
} else {
553553
$widget_output = file_get_contents($file_name);
@@ -905,7 +905,7 @@ function renderFormElement(
905905
/* If we are loading a file */
906906
if (substr($field_elements, 0, 5) == "@FILE") {
907907
$file_name = $modx->atBindFilePath(substr($field_elements, 6));
908-
if ($file_name === false) {
908+
if ($file_name === false || !$modx->atBindFileIsReadable($file_name)) {
909909
$custom_output = 'Could not retrieve file for this TV.';
910910
} else {
911911
$custom_output = file_get_contents($file_name);

‎core/functions/tv.php‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -523,7 +523,7 @@ function getTVDisplayFormat($name, $value, $format, $paramstring = '', $tvtype =
523523
their own, not the one that grants arbitrary PHP. */
524524
if (strpos($output, '@FILE') === 0) {
525525
$file_name = $modx->atBindFilePath(substr($output, 6));
526-
if ($file_name === false) {
526+
if ($file_name === false || !$modx->atBindFileIsReadable($file_name)) {
527527
$widget_output = 'Could not retrieve file for TV ' . $name . '.';
528528
} else {
529529
$widget_output = file_get_contents($file_name);
@@ -899,7 +899,7 @@ function renderFormElement(
899899
/* If we are loading a file */
900900
if (strpos($field_elements, '@FILE') === 0) {
901901
$file_name = $modx->atBindFilePath(substr($field_elements, 6));
902-
if ($file_name === false) {
902+
if ($file_name === false || !$modx->atBindFileIsReadable($file_name)) {
903903
$custom_output = 'Could not retrieve file for this TV.';
904904
} else {
905905
$custom_output = file_get_contents($file_name);

‎core/src/Core.php‎

Lines changed: 41 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -6615,11 +6615,15 @@ public function resolveAtBindFilePath($candidate)
66156615
return false;
66166616
}
66176617

6618-
$manager = realpath(EVO_MANAGER_PATH);
6619-
if ($manager !== false) {
6620-
$manager = rtrim(str_replace(DIRECTORY_SEPARATOR, '/', $manager), '/') . '/';
6621-
if (strpos($resolved, $manager) === 0) {
6622-
return false;
6618+
// the manager and the core hold source, configuration and .env, none of it content
6619+
foreach ([EVO_MANAGER_PATH, EVO_CORE_PATH] as $protected) {
6620+
$protected = realpath($protected);
6621+
if ($protected !== false) {
6622+
$protected = rtrim(str_replace(DIRECTORY_SEPARATOR, '/', $protected), '/') . '/';
6623+
// a core that is not below the base (a relocated one) is not what the base serves
6624+
if ($protected !== $base && strpos($protected, $base) === 0 && strpos($resolved, $protected) === 0) {
6625+
return false;
6626+
}
66236627
}
66246628
}
66256629

@@ -6657,6 +6661,37 @@ public function atBindFilePath($relative, array $searchPaths = [''])
66576661
return false;
66586662
}
66596663

6664+
/**
6665+
* Whether a file resolved by atBindFilePath() may be handed out as text by @FILE or the
6666+
* file_get_contents modifier: no source (PHP-like files, wherever they live: snippets,
6667+
* plugins, modules), no hidden files or folders, no generated or backup data.
6668+
*
6669+
* @param string|false $path
6670+
* @return bool
6671+
*/
6672+
public function atBindFileIsReadable($path)
6673+
{
6674+
if ($path === false || $path === '') {
6675+
return false;
6676+
}
6677+
$path = str_replace(DIRECTORY_SEPARATOR, '/', (string) $path);
6678+
$base = realpath(EVO_BASE_PATH);
6679+
$relative = $base === false ? $path : ltrim(substr($path, strlen(rtrim(str_replace(DIRECTORY_SEPARATOR, '/', $base), '/'))), '/');
6680+
6681+
foreach (explode('/', $relative) as $segment) {
6682+
if (strpos($segment, '.') === 0) {
6683+
return false;
6684+
}
6685+
}
6686+
if (preg_match('~^assets/(?:cache|backup)(?:/|$)~i', $relative)) {
6687+
return false;
6688+
}
6689+
$ext = '.' . strtolower(pathinfo($path, PATHINFO_EXTENSION));
6690+
$denied = array_merge(self::AT_BIND_FILE_DENIED_EXTENSIONS, ['.pht', '.cgi', '.pl', '.py', '.sh', '.env', '.ini', '.sql', '.log', '.bak', '.conf', '.pem', '.key']);
6691+
6692+
return !in_array($ext, $denied, true);
6693+
}
6694+
66606695
public function atBindFileContent($str = '')
66616696
{
66626697

@@ -6688,7 +6723,7 @@ public function atBindFileContent($str = '')
66886723
''
66896724
]);
66906725

6691-
if ($file_path === false) {
6726+
if ($file_path === false || !$this->atBindFileIsReadable($file_path)) {
66926727
return $errorMsg;
66936728
}
66946729

‎core/src/Legacy/Modifiers.php‎

Lines changed: 12 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1123,17 +1123,19 @@ public function getValueFromPreset($key, $value, $cmd, $opt)
11231123
if (!is_file($value)) {
11241124
return $value;
11251125
}
1126-
$value = realpath($value);
1127-
if (strpos($value, EVO_MANAGER_PATH) !== false) {
1128-
exit('Can not read core file');
1129-
}
1130-
$ext = strtolower(substr($value, -4));
1131-
if ($ext === '.php') {
1132-
exit('Can not read php file');
1133-
}
1134-
if ($ext === '.cgi') {
1135-
exit('Can not read cgi file');
1126+
// same rules as @FILE: inside the installation, not manager/ or core/, no source or secrets
1127+
$resolved = $modx->resolveAtBindFilePath($value);
1128+
if ($resolved === false) {
1129+
exit('Can not read file: outside the site, or inside manager/ or core/');
1130+
}
1131+
if (!$modx->atBindFileIsReadable($resolved)) {
1132+
$reason = 'hidden, generated or backup file';
1133+
if (preg_match('/\.(php\d*|phps|phtml|pht|phar|inc|cgi|pl|py|sh|env|ini|sql|log|bak|conf|pem|key)$/i', $resolved, $matches)) {
1134+
$reason = 'denied extension .' . strtolower($matches[1]);
1135+
}
1136+
exit('Can not read file: ' . $reason);
11361137
}
1138+
$value = $resolved;
11371139

11381140
return file_get_contents($value);
11391141
case 'filesize':

‎core/src/Services/DocumentSaveService.php‎

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@
1212
use EvolutionCMS\Support\DocumentSave\PublishState;
1313
use EvolutionCMS\Support\DocumentSave\TemplateVariableInput;
1414
use EvolutionCMS\Support\DocumentSave\TemplateVariableValues;
15+
use EvolutionCMS\Support\TvBindingGuard;
1516
use Illuminate\Support\Facades\DB;
1617

1718
/**
@@ -81,6 +82,15 @@ public function save(array $input): DocumentSaveResult
8182
$tvs = TemplateVariableValues::forTemplate($template, $id, !$ctx->isAdministrator(), $ctx->managerDocgroups);
8283
$tvValues = TemplateVariableInput::values($tvs, $input);
8384

85+
// @EVAL and @SELECT in a TV value run code when it is rendered: that takes the PHP permission
86+
$storedValues = [];
87+
foreach ($tvs as $tv) {
88+
$storedValues[$tv['id']] = $tv['value'];
89+
}
90+
if (!TvBindingGuard::allowsValues($ctx->can('save_snippet'), $tvValues, $storedValues, (string) ($input['ta'] ?? ''), (string) ($existing->content ?? ''), $this->textFields($input), $existing ? $this->textFields($existing->getAttributes()) : [])) {
91+
throw new DocumentSaveDenied($ctx->lang('error_no_privileges'));
92+
}
93+
8494
$now = $ctx->now;
8595
$pubDate = empty($input['pub_date']) ? 0 : $ctx->toTimestamp((string) $input['pub_date']);
8696
$unpubDate = empty($input['unpub_date']) ? 0 : $ctx->toTimestamp((string) $input['unpub_date']);
@@ -300,4 +310,19 @@ private function transaction(callable $callback): mixed
300310
{
301311
return SiteContent::resolveConnection()->transaction($callback);
302312
}
313+
314+
/**
315+
* The text fields a TV binding can pull in with [*field*].
316+
*
317+
* @return array<string, string>
318+
*/
319+
private function textFields(array $source): array
320+
{
321+
$fields = [];
322+
foreach (['pagetitle', 'longtitle', 'description', 'introtext', 'menutitle', 'link_attributes', 'alias'] as $name) {
323+
$fields[$name] = (string) ($source[$name] ?? '');
324+
}
325+
326+
return $fields;
327+
}
303328
}

‎core/src/Support/DocumentSave/TemplateVariableValues.php‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@ final class TemplateVariableValues
1313
{
1414
/**
1515
* TVs of a template with the value stored for the document. Non-administrators only
16-
* get the TVs without access rules or whose document belongs to one of their groups.
16+
* get the TVs without access rules or whose access rows name one of their document groups.
1717
*
1818
* @return array<int, array{id:int, name:string, type:string, default_text:string, value_id:int|null, value:string|null}>
1919
*/
@@ -31,12 +31,12 @@ public static function forTemplate(int $template, int $documentId, bool $restric
3131
->where('site_tmplvar_templates.templateid', $template)
3232
->orderBy('site_tmplvars.rank');
3333

34+
// the TV's own access rows decide, not the groups of the document it is saved on
3435
if ($restrictToGroups) {
35-
$query->leftJoin('document_groups', 'site_tmplvar_contentvalues.contentid', '=', 'document_groups.document')
36-
->where(function ($q) use ($managerGroups) {
37-
$q->whereNull('site_tmplvar_access.documentgroup')
38-
->orWhereIn('document_groups.document_group', $managerGroups);
39-
});
36+
$query->where(function ($q) use ($managerGroups) {
37+
$q->whereNull('site_tmplvar_access.documentgroup')
38+
->orWhereIn('site_tmplvar_access.documentgroup', $managerGroups);
39+
});
4040
}
4141

4242
$rows = [];
Lines changed: 97 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,97 @@
1+
<?php
2+
3+
namespace EvolutionCMS\Support;
4+
5+
/**
6+
* Keeps the TV bindings that run code out of the hands of managers who may edit TVs but not PHP.
7+
*
8+
* @EVAL passes its argument to eval() and @SELECT puts it into a query, in a TV's input options,
9+
* default value and output options. save_template alone is enough to store them, so they need the
10+
* permission that already grants arbitrary PHP (save_snippet).
11+
*/
12+
final class TvBindingGuard
13+
{
14+
/** Definition fields (POST name => column) a binding can sit in. */
15+
public const FIELDS = ['elements' => 'elements', 'default_text' => 'default_text', 'display_params' => 'display_params'];
16+
17+
public static function hasCodeBinding(?string $value): bool
18+
{
19+
$value = (string) $value;
20+
// the parser trims with trim(), which also drops NUL and vertical tab, and matches a command
21+
// by prefix ("@EVALreturn 1;" runs), so no boundary may be required after the name.
22+
// @INHERIT hands its argument back to the parser, so it is looked through.
23+
for ($depth = 0; $depth < 20; $depth++) {
24+
$value = ltrim($value, " \t\n\r\0\x0B");
25+
if (preg_match('/^@{1,2}(?:EVAL|SELECT)/i', $value)) {
26+
return true;
27+
}
28+
if (!preg_match('/^@INHERIT/i', $value)) {
29+
return false;
30+
}
31+
$value = substr($value, 8);
32+
}
33+
34+
return true; // deeper than any real value nests: do not guess
35+
}
36+
37+
/**
38+
* A TV value or resource field that can reach a code binding. Bindings nest (@INHERIT hands
39+
* its argument back to the binding parser), so one is also caught behind another binding.
40+
*/
41+
public static function reachesCodeBinding(?string $value): bool
42+
{
43+
return self::hasCodeBinding($value);
44+
}
45+
46+
/**
47+
* Whether a manager may store these TV values and the resource content. Values that did not
48+
* change are kept, so an existing binding stays editable around by whoever may edit the page.
49+
*
50+
* @param array<int|string, string|null> $desired TV id => submitted value
51+
* @param array<int|string, string|null> $stored TV id => current value
52+
*/
53+
public static function allowsValues(bool $mayWritePhp, array $desired, array $stored, string $content = '', string $storedContent = '', array $fields = [], array $storedFields = []): bool
54+
{
55+
if ($mayWritePhp) {
56+
return true;
57+
}
58+
// content only runs as a binding when a TV pulls it in with @DOCUMENT, and then from its start
59+
if ($content !== $storedContent && self::hasCodeBinding($content)) {
60+
return false;
61+
}
62+
// [*field*] in a binding's argument pulls in any other field of the page, so each is checked
63+
foreach ($fields as $name => $value) {
64+
$value = (string) $value;
65+
if (self::hasCodeBinding($value) && $value !== (string) ($storedFields[$name] ?? '')) {
66+
return false;
67+
}
68+
}
69+
foreach ($desired as $id => $value) {
70+
$value = (string) $value;
71+
if (self::reachesCodeBinding($value) && $value !== (string) ($stored[$id] ?? '')) {
72+
return false;
73+
}
74+
}
75+
76+
return true;
77+
}
78+
79+
/**
80+
* @param array<string, mixed> $submitted values keyed by column
81+
* @param array<string, mixed> $stored current values keyed by column, empty for a new TV
82+
*/
83+
public static function allows(bool $mayWritePhp, array $submitted, array $stored = []): bool
84+
{
85+
if ($mayWritePhp) {
86+
return true;
87+
}
88+
foreach (self::FIELDS as $column) {
89+
$value = (string) ($submitted[$column] ?? '');
90+
if (self::hasCodeBinding($value) && $value !== (string) ($stored[$column] ?? '')) {
91+
return false;
92+
}
93+
}
94+
95+
return true;
96+
}
97+
}
Lines changed: 13 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,19 @@
11
<?php
22

3-
it('marks the web user email label as required in the form', function () {
3+
/*
4+
| main.css styles ".warning" as a ~100%-width inline-block whenever it sits as a direct child of
5+
| td:first-child in these forms, for a label whose whole text is the warning. The email row only
6+
| wraps the "*" in .warning, so that rule stretched the lone asterisk across the cell and pushed
7+
| the label text out to the right behind it. The label is wrapped in an outer span so .warning is
8+
| no longer a direct child of the td and the rule stops matching.
9+
*/
10+
11+
it('marks the web user email label as required in the form, with the asterisk wrapped', function () {
412
$formPath = dirname(__DIR__, 4) . '/manager/actions/mutate_web_user.dynamic.php';
513
$form = file_get_contents($formPath);
614

7-
expect($form)->toContain('<span class="warning">*</span> <?php echo $_lang[\'user_email\']; ?>:');
15+
expect($form)->toContain('<span><span class="warning">*</span> <?php echo $_lang[\'user_email\']; ?>:</span>')
16+
// the bare, unwrapped form is what main.css's td:first-child > .warning rule stretches
17+
// to ~100% width, pushing the label text off to the right behind it
18+
->and($form)->not->toContain('<span class="warning">*</span> <?php echo $_lang[\'user_email\']; ?>:</td>');
819
});
Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
<?php
2+
3+
/*
4+
| The resource preview iframe loads the front-end's rendering of whatever content a manager
5+
| (possibly a limited editor, not the one previewing) saved on the resource. Without a sandbox,
6+
| same-origin script there can reach window.top, read the manager's CSRF meta tag, and ride the
7+
| previewing manager's session into a same-origin request. This pins the mitigation in place.
8+
*/
9+
10+
it('sandboxes the resource preview iframe without allow-same-origin', function () {
11+
$source = file_get_contents(dirname(__DIR__, 4) . '/manager/views/page/3.blade.php');
12+
13+
expect($source)->toContain('id="previewIframe"')
14+
->and($source)->toMatch('/<iframe[^>]*id="previewIframe"[^>]*sandbox="([^"]*)"/');
15+
16+
preg_match('/<iframe[^>]*id="previewIframe"[^>]*sandbox="([^"]*)"/', $source, $matches);
17+
$tokens = explode(' ', trim($matches[1]));
18+
19+
expect($tokens)->not->toContain('allow-same-origin')
20+
->and($tokens)->toContain('allow-scripts');
21+
});

0 commit comments

Comments
 (0)