diff --git a/docs/sections/tasks/execute.md b/docs/sections/tasks/execute.md index c6509f4b..c05b7b84 100644 --- a/docs/sections/tasks/execute.md +++ b/docs/sections/tasks/execute.md @@ -35,12 +35,13 @@ The `params` array will be joined with spaces and appended to the command. ### escape (default: true) -By default, the command and parameters are escaped using `escapeshellcmd()` for security: +By default, the command and each entry in `params` are escaped per token using `escapeshellarg()` so that arguments cannot break out into additional shell tokens (argument injection): ```php $data = [ - 'command' => 'bin/cake importer run', - 'escape' => true, // Default - commands are escaped + 'command' => 'bin/cake', + 'params' => ['importer', 'run'], + 'escape' => true, // Default - command and params escaped per token ]; ``` @@ -53,6 +54,24 @@ $data = [ ]; ``` +`escape => false` is hard-rejected unless `debug` is `true`. + +### Queue.executeAllowedCommands (production allow-list) + +When `debug` is disabled, the `command` value MUST appear verbatim in the `Queue.executeAllowedCommands` allow-list, otherwise the task throws before invoking `exec()`: + +```php +// config/app.php (or app_local.php) +'Queue' => [ + 'executeAllowedCommands' => [ + 'bin/cake', + '/usr/bin/php', + ], +], +``` + +If the allow-list is unset or empty in production, every Execute job is rejected. This protects against an attacker who can write to `queued_jobs` (DB-level compromise, or upstream code that pipes user input into `createJob('Queue.Execute', ...)`) from steering the exec call. + ### redirect (default: true) By default, stderr output is redirected to stdout (using `2>&1`). Disable if you need separate error handling: diff --git a/src/Queue/Task/ExecuteTask.php b/src/Queue/Task/ExecuteTask.php index a5d536b7..cdfcc0a7 100644 --- a/src/Queue/Task/ExecuteTask.php +++ b/src/Queue/Task/ExecuteTask.php @@ -90,16 +90,27 @@ public function run(array $data, int $jobId): void { throw new QueueException('Command escaping must be enabled when debug mode is off for security reasons'); } - $command = $data['command']; + $rawCommand = (string)$data['command']; + if (!Configure::read('debug')) { + $allowed = (array)Configure::read('Queue.executeAllowedCommands', []); + if (!$allowed || !in_array($rawCommand, $allowed, true)) { + throw new QueueException( + 'Command `' . $rawCommand . '` is not in Queue.executeAllowedCommands allow-list', + ); + } + } + if ($data['escape']) { - $command = escapeshellcmd($data['command']); + $command = escapeshellarg($rawCommand); + } else { + $command = $rawCommand; } if ($data['params']) { $params = $data['params']; if ($data['escape']) { foreach ($params as $key => $value) { - $params[$key] = escapeshellcmd($value); + $params[$key] = escapeshellarg((string)$value); } } $command .= ' ' . implode(' ', $params); diff --git a/tests/TestCase/Queue/Task/ExecuteTaskTest.php b/tests/TestCase/Queue/Task/ExecuteTaskTest.php index 6c72c129..22d94854 100644 --- a/tests/TestCase/Queue/Task/ExecuteTaskTest.php +++ b/tests/TestCase/Queue/Task/ExecuteTaskTest.php @@ -4,9 +4,11 @@ namespace Queue\Test\TestCase\Queue\Task; use Cake\Console\ConsoleIo; +use Cake\Core\Configure; use Cake\TestSuite\TestCase; use Exception; use Queue\Console\Io; +use Queue\Model\QueueException; use Queue\Queue\Task\ExecuteTask; use RuntimeException; use Shim\TestSuite\ConsoleOutput; @@ -57,7 +59,7 @@ public function setUp(): void { * @return void */ public function testRun() { - $this->Task->run(['command' => 'php -v'], 0); + $this->Task->run(['command' => 'php', 'params' => ['-v']], 0); $this->assertTextContains('PHP ', $this->out->output()); } @@ -68,13 +70,13 @@ public function testRun() { public function testRunFailureWithRedirect() { $exception = null; try { - $this->Task->run(['command' => 'fooooobbbaraar -eeee'], 0); + $this->Task->run(['command' => 'fooooobbbaraar', 'params' => ['-eeee']], 0); } catch (Exception $e) { $exception = $e; } $this->assertInstanceOf(Exception::class, $exception); - $this->assertSame('Failed with error code 127: `fooooobbbaraar -eeee 2>&1`', $exception->getMessage()); + $this->assertSame("Failed with error code 127: `'fooooobbbaraar' '-eeee' 2>&1`", $exception->getMessage()); $this->assertTextContains('Error (code 127)', $this->err->output()); $this->assertTextContains('fooooobbbaraar: not found', $this->out->output()); @@ -84,7 +86,7 @@ public function testRunFailureWithRedirect() { * @return void */ public function testRunFailureWithRedirectAndIgnoreCode() { - $this->Task->run(['command' => 'fooooobbbaraar -eeee', 'accepted' => []], 0); + $this->Task->run(['command' => 'fooooobbbaraar', 'params' => ['-eeee'], 'accepted' => []], 0); $this->assertTextContains('Success (code 127)', $this->out->output()); $this->assertTextContains('fooooobbbaraar: not found', $this->out->output()); @@ -97,9 +99,93 @@ public function testRunFailureWithoutRedirect() { $this->skipIf((bool)getenv('TRAVIS'), 'Not redirecting stderr to stdout prints noise to the CLI output in between test runs.'); $this->expectException(RuntimeException::class); - $this->expectExceptionMessage('Failed with error code 127: `fooooobbbaraar -eeee`'); + $this->expectExceptionMessage("Failed with error code 127: `'fooooobbbaraar' '-eeee'`"); - $this->Task->run(['command' => 'fooooobbbaraar -eeee', 'redirect' => false], 0); + $this->Task->run(['command' => 'fooooobbbaraar', 'params' => ['-eeee'], 'redirect' => false], 0); + } + + /** + * Per-token escapeshellarg neutralizes argument-injection payloads instead of + * letting them re-tokenize against the shell. The rogue param has to be quoted as + * a single argument and must not be interpreted as additional flags. + * + * @return void + */ + public function testRunEscapesArgumentInjectionPayloadAsSingleToken() { + $exception = null; + try { + $this->Task->run([ + 'command' => 'fooooobbbaraar', + 'params' => ['-r --some-flag /etc/passwd'], + ], 0); + } catch (Exception $e) { + $exception = $e; + } + + $this->assertInstanceOf(Exception::class, $exception); + $this->assertStringContainsString( + "'fooooobbbaraar' '-r --some-flag /etc/passwd'", + (string)$exception->getMessage(), + ); + } + + /** + * In production (debug=false) the command MUST be in the allow-list. + * Without it, the task throws before any exec() happens. + * + * @return void + */ + public function testRunRejectsCommandNotInAllowListWhenDebugDisabled() { + Configure::write('debug', false); + Configure::write('Queue.executeAllowedCommands', ['php']); + + $this->expectException(QueueException::class); + $this->expectExceptionMessage('Command `rm` is not in Queue.executeAllowedCommands allow-list'); + + try { + $this->Task->run(['command' => 'rm', 'params' => ['-rf', '/tmp/x']], 0); + } finally { + Configure::write('debug', true); + Configure::delete('Queue.executeAllowedCommands'); + } + } + + /** + * In production (debug=false) the empty/missing allow-list MUST reject every command. + * + * @return void + */ + public function testRunRejectsAnyCommandWhenAllowListEmptyAndDebugDisabled() { + Configure::write('debug', false); + Configure::delete('Queue.executeAllowedCommands'); + + $this->expectException(QueueException::class); + $this->expectExceptionMessage('Command `php` is not in Queue.executeAllowedCommands allow-list'); + + try { + $this->Task->run(['command' => 'php', 'params' => ['-v']], 0); + } finally { + Configure::write('debug', true); + } + } + + /** + * In production with the command listed, the task runs as normal. + * + * @return void + */ + public function testRunPassesWhenCommandInAllowListAndDebugDisabled() { + Configure::write('debug', false); + Configure::write('Queue.executeAllowedCommands', ['php']); + + try { + $this->Task->run(['command' => 'php', 'params' => ['-v']], 0); + } finally { + Configure::write('debug', true); + Configure::delete('Queue.executeAllowedCommands'); + } + + $this->assertTextContains('PHP ', $this->out->output()); } }