Security: MCP tool execution uses proc_open without output sanitization #5
Description
Description
The McpApiController::executeToolViaArtisan() method executes artisan commands via proc_open and returns the raw output to the API response without proper error handling or output sanitization.
Location
src/Api/Controllers/McpApiController.php:459-490
Security Concerns
-
Process execution without timeout: The
proc_opencall has no timeout mechanism, potentially allowing long-running processes to tie up server resources (denial of service). -
Error stream ignored:
stderr(pipe 2) is opened but its contents are discarded. Error messages from the subprocess could contain sensitive information and are not being logged properly. -
No output size limit: The response from
stream_get_contents($pipes[1])has no size limit, potentially allowing memory exhaustion if the MCP server returns extremely large output. -
Command construction relies on hardcoded map: While the
$commandMapprovides some protection, any changes to this map need careful security review.
Recommended Fix
protected function executeToolViaArtisan(string $server, string $tool, array $arguments): mixed
{
// Add timeout
$timeout = config('api.mcp.execution_timeout', 30);
// ... existing code ...
// Set non-blocking and use stream_select for timeout
stream_set_blocking($pipes[1], false);
stream_set_blocking($pipes[2], false);
$output = '';
$errorOutput = '';
$startTime = time();
while (true) {
$read = [$pipes[1], $pipes[2]];
$write = $except = null;
if (stream_select($read, $write, $except, 1) === false) {
break;
}
if (time() - $startTime > $timeout) {
proc_terminate($process);
throw new \RuntimeException('MCP server execution timed out');
}
// Read with size limits
$output .= fread($pipes[1], 1024 * 1024); // 1MB max
$errorOutput .= fread($pipes[2], 1024 * 64); // 64KB max
if (strlen($output) > 10 * 1024 * 1024) { // 10MB absolute max
proc_terminate($process);
throw new \RuntimeException('Output size limit exceeded');
}
// ... check if process has ended ...
}
// Log stderr for debugging (sanitized)
if (!empty($errorOutput)) {
Log::warning('MCP server stderr output', [
'server' => $server,
'error' => Str::limit($errorOutput, 1000),
]);
}
}Priority
Medium - While the command map provides protection against arbitrary command execution, the lack of timeouts and output limits could be exploited for DoS attacks.