From bfda542d87fd2316f8c70a6c2eca12a3b1c48376 Mon Sep 17 00:00:00 2001 From: Donach <39565367+Donach@users.noreply.github.com> Date: Thu, 30 Jul 2026 12:45:32 +0000 Subject: [PATCH] =?UTF-8?q?=F0=9F=9B=A1=EF=B8=8F=20Sentinel:=20[CRITICAL]?= =?UTF-8?q?=20Fix=20command=20injection=20vulnerability=20in=20execSync?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replaced vulnerable `execSync` calls with `execFileSync` in unix user utilities to prevent shell command injection. Pass dynamic arguments in the arguments array instead of string interpolation to completely bypass shell parsing. - Updates `packages/core/src/unix/id-lookups.ts` - Updates `packages/core/src/unix/user-manager.ts` - Updates test mocks - Logs findings in `.jules/sentinel.md` --- .jules/sentinel.md | 5 +++++ packages/core/src/unix/id-lookups.ts | 10 +++++----- packages/core/src/unix/user-manager.test.ts | 18 ++++++++++-------- packages/core/src/unix/user-manager.ts | 4 ++-- 4 files changed, 22 insertions(+), 15 deletions(-) diff --git a/.jules/sentinel.md b/.jules/sentinel.md index 3a5ef27423..5ca90f2929 100644 --- a/.jules/sentinel.md +++ b/.jules/sentinel.md @@ -7,3 +7,8 @@ **Vulnerability:** The daemon configuration file (`~/.agor/config.yaml`) and its parent directory (`~/.agor`) were created with default file permissions (e.g., `0o755`/`0o644`), which made them readable by other users on the system. This file stores extremely sensitive information such as API keys and master JWT secrets. **Learning:** Default Node.js filesystem operations (`fs.writeFile` and `fs.mkdir`) do not enforce strict permissions unless explicitly specified with a `mode` parameter. When handling sensitive files, relying on the system `umask` is insufficient. **Prevention:** Always specify `mode: 0o600` for sensitive files and `mode: 0o700` for their parent directories. Additionally, use `fs.chmod` to retroactively secure existing files and directories that might have been created with permissive defaults. + +## 2025-05-15 - [CRITICAL] Command Injection Risk via execSync +**Vulnerability:** Command injection vulnerability due to string interpolation in `execSync` commands across unix utilities (`id-lookups.ts` and `user-manager.ts`). Shell characters inside dynamic parameters like usernames or groupnames were vulnerable to shell breakout. +**Learning:** Node's `execSync` spawns a shell and evaluates the entire string. Double quoting (`"..."`) is insufficient against advanced shell characters (like backticks or `$()`). +**Prevention:** Always use `execFileSync` from `node:child_process` and pass dynamic parameters via the arguments array (with `--` double-dashes to avoid flag injection) to prevent a shell from being spawned entirely. Ensure test mocks are updated to mock `execFileSync` instead. diff --git a/packages/core/src/unix/id-lookups.ts b/packages/core/src/unix/id-lookups.ts index 4a7f16390d..b9b591d4f8 100644 --- a/packages/core/src/unix/id-lookups.ts +++ b/packages/core/src/unix/id-lookups.ts @@ -4,7 +4,7 @@ * Supports both Linux (using getent) and macOS (parsing /etc/group and /etc/passwd) */ -import { execSync } from 'node:child_process'; +import { execFileSync } from 'node:child_process'; import fs from 'node:fs'; /** @@ -26,7 +26,7 @@ export function getGidFromGroupName(groupName: string | undefined | null): numbe try { // Try getent first (Linux, some BSD) try { - const result = execSync(`getent group "${groupName}"`, { + const result = execFileSync('getent', ['group', '--', String(groupName)], { encoding: 'utf-8', stdio: 'pipe', timeout: 2000, @@ -91,7 +91,7 @@ export function getUidFromUsername(username: string | undefined | null): number try { // Try `id -u username` first (most reliable) try { - const result = execSync(`id -u "${username}"`, { + const result = execFileSync('id', ['-u', '--', String(username)], { encoding: 'utf-8', stdio: 'pipe', timeout: 2000, @@ -107,7 +107,7 @@ export function getUidFromUsername(username: string | undefined | null): number // Try getent (Linux, some BSD) try { - const result = execSync(`getent passwd "${username}"`, { + const result = execFileSync('getent', ['passwd', '--', String(username)], { encoding: 'utf-8', stdio: 'pipe', timeout: 2000, @@ -171,7 +171,7 @@ export function getHomedirFromUsername(username: string | undefined | null): str try { // Try getent first (Linux, some BSD) try { - const result = execSync(`getent passwd "${username}"`, { + const result = execFileSync('getent', ['passwd', '--', String(username)], { encoding: 'utf-8', stdio: 'pipe', timeout: 2000, diff --git a/packages/core/src/unix/user-manager.test.ts b/packages/core/src/unix/user-manager.test.ts index ceda62d746..15c20144e9 100644 --- a/packages/core/src/unix/user-manager.test.ts +++ b/packages/core/src/unix/user-manager.test.ts @@ -27,14 +27,16 @@ import { validateResolvedUnixUser, } from './user-manager.js'; -// Mock execSync for system-dependent tests +// Mock execSync and execFileSync for system-dependent tests vi.mock('node:child_process', () => ({ execSync: vi.fn(), + execFileSync: vi.fn(), })); -import { execSync } from 'node:child_process'; +import { execFileSync, execSync } from 'node:child_process'; const mockedExecSync = vi.mocked(execSync); +const mockedExecFileSync = vi.mocked(execFileSync); describe('user-manager', () => { beforeEach(() => { @@ -312,7 +314,7 @@ describe('user-manager', () => { // Note: These tests use mocked execSync since we can't create real users it('returns true when user exists', async () => { - mockedExecSync.mockReturnValueOnce(Buffer.from('')); // Success (no throw) + mockedExecFileSync.mockReturnValueOnce(Buffer.from('')); // Success (no throw) // Import fresh to use mock const { unixUserExists } = await import('./user-manager.js'); @@ -320,7 +322,7 @@ describe('user-manager', () => { }); it('returns false when user does not exist', async () => { - mockedExecSync.mockImplementationOnce(() => { + mockedExecFileSync.mockImplementationOnce(() => { throw new Error('id: nonexistent: no such user'); }); @@ -456,7 +458,7 @@ describe('user-manager', () => { }); it('validates user exists for strict mode', () => { - mockedExecSync.mockImplementationOnce(() => { + mockedExecFileSync.mockImplementationOnce(() => { throw new Error('no such user'); }); @@ -466,7 +468,7 @@ describe('user-manager', () => { }); it('validates user exists for insulated mode', () => { - mockedExecSync.mockImplementationOnce(() => { + mockedExecFileSync.mockImplementationOnce(() => { throw new Error('no such user'); }); @@ -482,13 +484,13 @@ describe('user-manager', () => { }); it('passes when user exists', () => { - mockedExecSync.mockReturnValueOnce(Buffer.from('')); // user exists + mockedExecFileSync.mockReturnValueOnce(Buffer.from('')); // user exists expect(() => validateResolvedUnixUser('strict', 'alice')).not.toThrow(); }); it('error message includes mode context', () => { - mockedExecSync.mockImplementationOnce(() => { + mockedExecFileSync.mockImplementationOnce(() => { throw new Error('no such user'); }); diff --git a/packages/core/src/unix/user-manager.ts b/packages/core/src/unix/user-manager.ts index 1b15c20156..03fb99e881 100644 --- a/packages/core/src/unix/user-manager.ts +++ b/packages/core/src/unix/user-manager.ts @@ -7,7 +7,7 @@ * @see context/guides/rbac-and-unix-isolation.md */ -import { execSync } from 'node:child_process'; +import { execFileSync, execSync } from 'node:child_process'; import type { UnixUserMode } from '../config/types.js'; import { formatShortId } from '../lib/ids.js'; import type { UserID, UUID } from '../types/index.js'; @@ -339,7 +339,7 @@ export class UnixUserNotFoundError extends Error { */ export function unixUserExists(username: string): boolean { try { - execSync(`id "${username}" > /dev/null 2>&1`, { stdio: 'pipe' }); + execFileSync('id', ['--', String(username)], { stdio: 'ignore' }); return true; } catch { return false;