T09 · Insecure Skill Coding Practices
Error
- Location
- src/deploy-manager.js:33
- Finding
- Shell Command Injection Through Deployment Configuration<![CDATA[ ## Vulnerability Details **File Location**: `src/deploy-manager.js:33-35, 52-58, 94-96, 106-108, 119-121, 132` **Vulnerability Type**: OS command injection through unsafe shell interpolation **Risk Level**: High ### Vulnerable Code ```javascript const { stdout, stderr } = await execAsync( `docker build -t ${this.config.imageName}:latest .` ); ``` ```javascript // Stop the old container await execAsync(`docker stop ${this.config.containerName} 2>nul || true`); await execAsync(`docker rm ${this.config.containerName} 2>nul || true`); // Start the new container const { stdout } = await execAsync( `docker run -d -p ${this.config.port}:${this.config.port} ` + `--name ${this.config.containerName} ${this.config.imageName}:latest` ); ``` ```javascript const { stdout } = await execAsync( `docker ps -q -f name=${this.config.containerName}` ); ``` ```javascript const { stdout } = await execAsync( `netstat -an | findstr :${this.config.port}` ); ``` ```javascript const { stdout } = await execAsync( `docker logs --tail ${lines} ${this.config.containerName}` ); ``` ```javascript await execAsync(`docker stop ${this.config.containerName}`); ``` ### Technical Analysis The application executes Docker and operating-system commands through `child_process.exec`. Unlike an argument-based process API, `exec` passes the constructed string to a command shell. The following caller-controlled values are directly interpolated into shell commands without validation or escaping: - `config.imageName` - `config.containerName` - `config.port` - The `lines` argument passed to `getLogs()` An attacker who can control any of these values can introduce shell metacharacters such as command separators, substitutions, redirections, or pipelines. The shell will interpret the injected content as an additional command rather than as a Docker argument. The constructor also applies `...config` after the default properties, so arbitrary supplied values remain accepted wit ...[truncated 1748 chars]
- Remediation
- <![CDATA[ ## Remediation Suggestions 1. Replace `exec` with `execFile` or `spawn` and pass every command argument as a separate array element: ```javascript import { execFile } from 'child_process'; import { promisify } from 'util'; const execFileAsync = promisify(execFile); await execFileAsync('docker', [ 'build', '-t', `${this.config.imageName}:latest`, '.' ]); await execFileAsync('docker', [ 'run', '-d', '-p', `${port}:${port}`, '--name', this.config.containerName, `${this.config.imageName}:latest` ]); ``` 2. Do not use shell operators such as `|| true` for expected failure handling. Catch expected `docker stop` and `docker rm` errors in JavaScript instead. 3. Validate configuration before storing or using it: - Require `port` to be an integer from 1 through 65535. - Require `lines` to be a bounded positive integer. - Enforce Docker-compatible allowlists for image and container names. - Reject whitespace, control characters, shell metacharacters, and unexpected types. 4. Avoid enabling the `shell` option when using `spawn` or `execFile`. 5. Run the deployment process under a dedicated, least-privileged account. Limit Docker daemon access wherever possible because Docker control can imply host-level control. 6. Add security tests that submit shell metacharacters and verify that they are rejected and never create additional processes. ]]>