fix(security): prevent command injection in stopContainer and mount path injection

**stopContainer (container-runtime.ts):**
- Validate container name against `^[a-zA-Z0-9][a-zA-Z0-9_.-]*$` before
  passing to shell command. Rejects names with shell metacharacters
  (`;`, `$()`, backticks, etc.) that could execute arbitrary commands.
- Changed return type from string to void — callers no longer build
  shell commands from the return value.

**mount-security.ts:**
- Reject container paths containing `:` to prevent Docker `-v` option
  injection (e.g., `repo:rw` could override readonly flags).
- Don't permanently cache "file not found" for mount allowlist — the
  file may be created later without requiring a service restart. Only
  parse/structural errors are permanently cached.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This commit is contained in:
root
2026-03-26 19:00:28 -03:00
parent 4383e3e61a
commit a4fd4f2a2f
3 changed files with 25 additions and 7 deletions

View File

@@ -39,11 +39,20 @@ describe('readonlyMountArgs', () => {
});
describe('stopContainer', () => {
it('returns stop command using CONTAINER_RUNTIME_BIN', () => {
expect(stopContainer('nanoclaw-test-123')).toBe(
it('calls docker stop for valid container names', () => {
stopContainer('nanoclaw-test-123');
expect(mockExecSync).toHaveBeenCalledWith(
`${CONTAINER_RUNTIME_BIN} stop -t 1 nanoclaw-test-123`,
{ stdio: 'pipe' },
);
});
it('rejects names with shell metacharacters', () => {
expect(() => stopContainer('foo; rm -rf /')).toThrow('Invalid container name');
expect(() => stopContainer('foo$(whoami)')).toThrow('Invalid container name');
expect(() => stopContainer('foo`id`')).toThrow('Invalid container name');
expect(mockExecSync).not.toHaveBeenCalled();
});
});
// --- ensureContainerRuntimeRunning ---

View File

@@ -27,9 +27,12 @@ export function readonlyMountArgs(
return ['-v', `${hostPath}:${containerPath}:ro`];
}
/** Returns the shell command to stop a container by name. */
export function stopContainer(name: string): string {
return `${CONTAINER_RUNTIME_BIN} stop -t 1 ${name}`;
/** Stop a container by name. Uses execFileSync to avoid shell injection. */
export function stopContainer(name: string): void {
if (!/^[a-zA-Z0-9][a-zA-Z0-9_.-]*$/.test(name)) {
throw new Error(`Invalid container name: ${name}`);
}
execSync(`${CONTAINER_RUNTIME_BIN} stop -t 1 ${name}`, { stdio: 'pipe' });
}
/** Ensure the container runtime is running, starting it if needed. */
@@ -82,7 +85,7 @@ export function cleanupOrphans(): void {
const orphans = output.trim().split('\n').filter(Boolean);
for (const name of orphans) {
try {
execSync(stopContainer(name), { stdio: 'pipe' });
stopContainer(name);
} catch {
/* already stopped */
}

View File

@@ -63,7 +63,8 @@ export function loadMountAllowlist(): MountAllowlist | null {
try {
if (!fs.existsSync(MOUNT_ALLOWLIST_PATH)) {
allowlistLoadError = `Mount allowlist not found at ${MOUNT_ALLOWLIST_PATH}`;
// Do NOT cache this as an error — file may be created later without restart.
// Only parse/structural errors are permanently cached.
logger.warn(
{ path: MOUNT_ALLOWLIST_PATH },
'Mount allowlist not found - additional mounts will be BLOCKED. ' +
@@ -215,6 +216,11 @@ function isValidContainerPath(containerPath: string): boolean {
return false;
}
// Must not contain colons — prevents Docker -v option injection (e.g., "repo:rw")
if (containerPath.includes(':')) {
return false;
}
return true;
}