Skip to content

fix(security): resolve @FILE and @INCLUDE paths before trusting them #2446

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
elcreator wants to merge 1 commit into
base: 3.5.x
Choose a base branch
from
Open

fix(security): resolve @FILE and @INCLUDE paths before trusting them #2446

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
55 changes: 33 additions & 22 deletions core/functions/actions/mutate_content.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -540,29 +540,34 @@ function getTVDisplayFormat($name, $value, $format, $paramstring = "", $tvtype =
$widget_output = '';
$o = '';
/* If we are loading a file */
if (substr($params['output'], 0, 5) == "@FILE") {
$file_name = EVO_BASE_PATH . trim(substr($params['output'], 6));
if (!file_exists($file_name)) {
$widget_output = $file_name . ' does not exist';
$output = (string) get_by_key($params, 'output', '');
/* Both paths are resolved and checked for containment before
they are used - @INCLUDE runs the file, and the output
options this comes from are edited under a permission of
their own, not the one that grants arbitrary PHP. */
if (substr($output, 0, 5) == "@FILE") {
$file_name = $modx->atBindFilePath(substr($output, 6));
if ($file_name === false) {
$widget_output = 'Could not retrieve file for TV ' . $name . '.';
} else {
$widget_output = file_get_contents($file_name);
}
} elseif (substr($params['output'], 0, 8) == '@INCLUDE') {
$file_name = EVO_BASE_PATH . trim(substr($params['output'], 9));
if (!file_exists($file_name)) {
$widget_output = $file_name . ' does not exist';
} elseif (substr($output, 0, 8) == '@INCLUDE') {
$file_name = $modx->atBindFilePath(substr($output, 9));
if ($file_name === false) {
$widget_output = 'Could not retrieve file for TV ' . $name . '.';
} else {
/* The included file needs to set $widget_output. Can be string, array, object */
include $file_name;
}
} elseif (substr($params['output'], 0, 6) == '@CHUNK' && $value !== '') {
$chunk_name = trim(substr($params['output'], 7));
} elseif (substr($output, 0, 6) == '@CHUNK' && $value !== '') {
$chunk_name = trim(substr($output, 7));
$widget_output = $modx->getChunk($chunk_name);
} elseif (substr($params['output'], 0, 5) == '@EVAL' && $value !== '') {
$eval_str = trim(substr($params['output'], 6));
} elseif (substr($output, 0, 5) == '@EVAL' && $value !== '') {
$eval_str = trim(substr($output, 6));
$widget_output = eval($eval_str);
} elseif ($value !== '') {
$widget_output = $params['output'];
$widget_output = $output;
} else {
$widget_output = '';
}
Expand Down Expand Up @@ -899,16 +904,16 @@ function renderFormElement(
$custom_output = '';
/* If we are loading a file */
if (substr($field_elements, 0, 5) == "@FILE") {
$file_name = EVO_BASE_PATH . trim(substr($field_elements, 6));
if (!file_exists($file_name)) {
$custom_output = $file_name . ' does not exist';
$file_name = $modx->atBindFilePath(substr($field_elements, 6));
if ($file_name === false) {
$custom_output = 'Could not retrieve file for this TV.';
} else {
$custom_output = file_get_contents($file_name);
}
} elseif (substr($field_elements, 0, 8) == '@INCLUDE') {
$file_name = EVO_BASE_PATH . trim(substr($field_elements, 9));
if (!file_exists($file_name)) {
$custom_output = $file_name . ' does not exist';
$file_name = $modx->atBindFilePath(substr($field_elements, 9));
if ($file_name === false) {
$custom_output = 'Could not retrieve file for this TV.';
} else {
ob_start();
include $file_name;
Expand Down Expand Up @@ -950,9 +955,15 @@ function renderFormElement(
} else {
$custom = explode(":", $field_type);
$custom_output = '';
$file_name = EVO_BASE_PATH . 'assets/tvs/' . $custom['1'] . '/' . $custom['1'] . '.customtv.php';
if (!file_exists($file_name)) {
$custom_output = $file_name . ' does not exist';
// The widget name comes out of the TV's type column and lands in a
// path that is then included, so it goes through the same
// containment rule as every other binding.
$widget = (string) get_by_key($custom, 1, '');
$file_name = $widget === ''
? false
: $modx->atBindFilePath('assets/tvs/' . $widget . '/' . $widget . '.customtv.php');
if ($file_name === false) {
$custom_output = 'Could not retrieve the custom TV widget.';
} else {
ob_start();
include $file_name;
Expand Down
55 changes: 33 additions & 22 deletions core/functions/tv.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -516,30 +516,35 @@ function getTVDisplayFormat($name, $value, $format, $paramstring = '', $tvtype =
case 'custom_widget':
$widget_output = '';
/* If we are loading a file */
if (strpos($params['output'], '@FILE') === 0) {
$file_name = EVO_BASE_PATH . trim(substr($params['output'], 6));
if (!is_file($file_name)) {
$widget_output = $file_name . ' does not exist';
$output = (string) get_by_key($params, 'output', '');
/* Both paths are resolved and checked for containment before
they are used - @INCLUDE runs the file, and the output
options this comes from are edited under a permission of
their own, not the one that grants arbitrary PHP. */
if (strpos($output, '@FILE') === 0) {
$file_name = $modx->atBindFilePath(substr($output, 6));
if ($file_name === false) {
$widget_output = 'Could not retrieve file for TV ' . $name . '.';
} else {
$widget_output = file_get_contents($file_name);
}
} elseif (strpos($params['output'], '@INCLUDE') === 0) {
$file_name = EVO_BASE_PATH . trim(substr($params['output'], 9));
if (!is_file($file_name)) {
$widget_output = $file_name . ' does not exist';
} elseif (strpos($output, '@INCLUDE') === 0) {
$file_name = $modx->atBindFilePath(substr($output, 9));
if ($file_name === false) {
$widget_output = 'Could not retrieve file for TV ' . $name . '.';
} else {
/* The included file needs to set $widget_output. Can be string, array, object */
include $file_name;
}
} elseif ($value !== '') {
if (strpos($params['output'], '@CHUNK') === 0) {
$chunk_name = trim(substr($params['output'], 7));
if (strpos($output, '@CHUNK') === 0) {
$chunk_name = trim(substr($output, 7));
$widget_output = $modx->getChunk($chunk_name);
} elseif (strpos($params['output'], '@EVAL') === 0) {
$eval_str = trim(substr($params['output'], 6));
} elseif (strpos($output, '@EVAL') === 0) {
$eval_str = trim(substr($output, 6));
$widget_output = eval($eval_str);
} else {
$widget_output = $params['output'];
$widget_output = $output;
}
} else {
$widget_output = '';
Expand Down Expand Up @@ -893,16 +898,16 @@ function renderFormElement(
$custom_output = '';
/* If we are loading a file */
if (strpos($field_elements, '@FILE') === 0) {
$file_name = EVO_BASE_PATH . trim(substr($field_elements, 6));
if (!file_exists($file_name)) {
$custom_output = $file_name . ' does not exist';
$file_name = $modx->atBindFilePath(substr($field_elements, 6));
if ($file_name === false) {
$custom_output = 'Could not retrieve file for this TV.';
} else {
$custom_output = file_get_contents($file_name);
}
} elseif (strpos($field_elements, '@INCLUDE') === 0) {
$file_name = EVO_BASE_PATH . trim(substr($field_elements, 9));
if (!file_exists($file_name)) {
$custom_output = $file_name . ' does not exist';
$file_name = $modx->atBindFilePath(substr($field_elements, 9));
if ($file_name === false) {
$custom_output = 'Could not retrieve file for this TV.';
} else {
ob_start();
include $file_name;
Expand Down Expand Up @@ -942,9 +947,15 @@ function renderFormElement(
} // end switch statement
} else {
$custom = explode(':', $field_type);
$file_name = EVO_BASE_PATH.'assets/tvs/'.$custom['1'].'/'.$custom['1'].'.customtv.php';
if (!is_file($file_name)) {
$custom_output = $file_name . ' does not exist';
// The widget name comes out of the TV's type column and lands in a
// path that is then included, so it goes through the same
// containment rule as every other binding.
$widget = (string) get_by_key($custom, 1, '');
$file_name = $widget === ''
? false
: $modx->atBindFilePath('assets/tvs/' . $widget . '/' . $widget . '.customtv.php');
if ($file_name === false) {
$custom_output = 'Could not retrieve the custom TV widget.';
} else {
ob_start();
include $file_name;
Expand Down
72 changes: 45 additions & 27 deletions core/src/Core.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6433,7 +6433,7 @@ public function isSafeCode($phpcode = '', $safe_functions = '')
* @return string|false
* @since 3.5.8
*/
private function resolveAtBindFilePath($candidate)
public function resolveAtBindFilePath($candidate)
{
$resolved = realpath($candidate);
if ($resolved === false || !is_file($resolved)) {
Expand Down Expand Up @@ -6463,6 +6463,37 @@ private function resolveAtBindFilePath($candidate)
return $resolved;
}

/**
* The file an @FILE or @INCLUDE binding names, or false.
*
* One containment rule for every binding. It used to be four, and only one
* of them resolved the path before checking it.
*
* @param string $relative the binding's argument, as typed
* @param string[] $searchPaths prefixes below EVO_BASE_PATH, in order
* @return string|false
* @since 3.5.8
*/
public function atBindFilePath($relative, array $searchPaths = [''])
{
$relative = trim((string) $relative);
$relative = ltrim(str_replace(chr(92), '/', $relative), '/');

if ($relative === '') {
return false;
}

foreach ($searchPaths as $path) {
$resolved = $this->resolveAtBindFilePath(EVO_BASE_PATH . $path . $relative);

if ($resolved !== false) {
return $resolved;
}
}

return false;
}

public function atBindFileContent($str = '')
{

Expand All @@ -6486,16 +6517,15 @@ public function atBindFileContent($str = '')

$errorMsg = "Could not retrieve string '" . $str . "'.";

$search_path = ['assets/tvs/', 'assets/chunks/', 'assets/templates/', $this->getConfig('rb_base_url') . 'files/', ''];
foreach ($search_path as $path) {
$file_path = $this->resolveAtBindFilePath(EVO_BASE_PATH . $path . $str);

if ($file_path !== false) {
break;
}
}
$file_path = $this->atBindFilePath($str, [
'assets/tvs/',
'assets/chunks/',
'assets/templates/',
$this->getConfig('rb_base_url') . 'files/',
''
]);

if (!$file_path) {
if ($file_path === false) {
return $errorMsg;
}

Expand Down Expand Up @@ -6830,25 +6860,13 @@ public function atBindInclude($str = '')
}

$str = substr($str, 9);
$str = trim($str);
$str = str_replace('\\', '/', $str);
$str = ltrim($str, '/');

$tpl_dir = 'assets/templates/';

if (strpos($str, EVO_MANAGER_PATH) === 0) {
return false;
}

if (is_file(EVO_BASE_PATH . $str)) {
$file_path = EVO_BASE_PATH . $str;
} elseif (is_file(EVO_BASE_PATH . "{$tpl_dir}{$str}")) {
$file_path = EVO_BASE_PATH . $tpl_dir . $str;
} else {
return false;
}
// This includes rather than reads, so the path settles on is the
// path PHP executes. The old check ran on the string as typed, which
// a `..` walked straight past.
$file_path = $this->atBindFilePath($str, ['', 'assets/templates/']);

if (!$file_path || !is_file($file_path)) {
if ($file_path === false) {
return false;
}

Expand Down
87 changes: 87 additions & 0 deletions core/tests/Unit/Security/ParserEvalHardeningTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
| - mergeConditionalTagsContent() <@IF:...> conditional tags
| - _getSGVar() [[$_GET(x)]] superglobal reads
| - atBindFileContent() @FILE: template includes
| - atBindInclude() @INCLUDE: template includes, which run the file
|
| All three are reachable from content that the parser re-scans across passes, so a snippet echoing
| request data can carry a payload into them without any editing privilege. These tests drive the
Expand Down Expand Up @@ -234,3 +235,89 @@ function parserHardeningCore(): Core
}
});
});

describe('@INCLUDE binding', function () {

test('directory traversal outside the base path is refused', function () {
$core = parserHardeningCore();

// This one does not read the file, it includes it - so a path that
// escapes the tree is arbitrary code execution, not a disclosure.
expect($core->atBindInclude('@INCLUDE ' . str_repeat('../', 20) . 'Windows/win.ini'))->toBeFalse()
->and($core->atBindInclude('@INCLUDE ' . str_repeat('../', 20) . 'etc/passwd'))->toBeFalse();
});

test('a traversal back into the manager directory is refused', function () {
$core = parserHardeningCore();

// The old check asked whether the string as typed started with the
// manager path, so anything reaching it by way of `..` walked past.
expect($core->atBindInclude('@INCLUDE assets/../manager/index.php'))->toBeFalse()
->and($core->atBindInclude('@INCLUDE manager/index.php'))->toBeFalse();
});

test('a file inside the tree is still included', function () {
$core = parserHardeningCore();

$relative = 'evo_atinclude_' . bin2hex(random_bytes(6)) . '.php';
$absolute = EVO_BASE_PATH . $relative;
file_put_contents($absolute, '<?php echo "included-body";');

try {
expect($core->atBindInclude('@INCLUDE ' . $relative))->toBe('included-body')
// A traversal that normalises back inside the tree still resolves.
->and($core->atBindInclude('@INCLUDE assets/../' . $relative))->toBe('included-body');
} finally {
@unlink($absolute);
}
});

test('a missing file is refused rather than fatal', function () {
$core = parserHardeningCore();

expect($core->atBindInclude('@INCLUDE assets/evo_no_such_' . bin2hex(random_bytes(6)) . '.php'))
->toBeFalse()
->and($core->atBindInclude('@INCLUDE '))->toBeFalse();
});

test('content with no binding is returned untouched', function () {
$core = parserHardeningCore();

expect($core->atBindInclude('plain content'))->toBe('plain content');
});
});

describe('the shared binding path resolver', function () {

test('a directory is not a file a binding may name', function () {
$core = parserHardeningCore();

expect($core->atBindFilePath('.'))->toBeFalse()
->and($core->atBindFilePath(''))->toBeFalse()
->and($core->atBindFilePath(' '))->toBeFalse();
});

test('search path prefixes are tried in order', function () {
$core = parserHardeningCore();

$name = 'evo_bindpath_' . bin2hex(random_bytes(6)) . '.txt';
$absolute = EVO_BASE_PATH . $name;
file_put_contents($absolute, 'x');

try {
expect($core->atBindFilePath($name, ['']))->toBe(str_replace(chr(92), '/', realpath($absolute)))
// A prefix the file is not under does not find it.
->and($core->atBindFilePath($name, ['assets/']))->toBeFalse();
} finally {
@unlink($absolute);
}
});

test('a backslash separator is accepted and still contained', function () {
$core = parserHardeningCore();

// Windows style separators reach this from hand written bindings.
expect($core->atBindFilePath(str_repeat('..' . chr(92), 20) . 'Windows' . chr(92) . 'win.ini'))
->toBeFalse();
});
});
Loading