🛡️ Sentinel: [CRITICAL] Fix command injection in Unix utilities

.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.
+
+## 2024-05-22 - [Command Injection Risk via execSync with String Interpolation]
+**Vulnerability:** A command injection vulnerability existed in `packages/core/src/unix/id-lookups.ts` and `packages/core/src/unix/user-manager.ts` where `execSync` was used with string interpolation for dynamic inputs like usernames (e.g., `execSync(\`id -u "${username}"\`)`).
+**Learning:** Using `execSync` or `exec` with dynamically constructed strings executes them within a shell environment (`/bin/sh`), allowing for shell metacharacter injection if the input is not strictly validated.
+**Prevention:** Use `execFileSync` or `execFile` instead, which bypass the shell entirely and pass arguments directly as an array (e.g., `execFileSync('id', ['-u', '--', String(username)])`). Additionally, prefixing dynamic arguments with `--` prevents argument injection vulnerabilities.

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,

packages/core/src/unix/user-manager.test.ts

@@ -30,11 +30,12 @@ import {
 // Mock execSync for system-dependent tests
 vi.mock('node:child_process', () => ({
   execSync: vi.fn(),
+  execFileSync: vi.fn(),
 }));
 
-import { execSync } from 'node:child_process';
+import { execFileSync } from 'node:child_process';
 
-const mockedExecSync = vi.mocked(execSync);
+const mockedExecFileSync = vi.mocked(execFileSync);
 
 describe('user-manager', () => {
   beforeEach(() => {
@@ -309,18 +310,18 @@ describe('user-manager', () => {
   // =========================================================================
 
   describe('unixUserExists (mocked)', () => {
-    // Note: These tests use mocked execSync since we can't create real users
+    // Note: These tests use mocked execFileSync 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');
       expect(unixUserExists('root')).toBe(true);
     });
 
     it('returns false when user does not exist', async () => {
-      mockedExecSync.mockImplementationOnce(() => {
+      mockedExecFileSync.mockImplementationOnce(() => {
         throw new Error('id: nonexistent: no such user');
       });
 
@@ -456,7 +457,7 @@ describe('user-manager', () => {
     });
 
     it('validates user exists for strict mode', () => {
-      mockedExecSync.mockImplementationOnce(() => {
+      mockedExecFileSync.mockImplementationOnce(() => {
         throw new Error('no such user');
       });
 
@@ -466,7 +467,7 @@ describe('user-manager', () => {
     });
 
     it('validates user exists for insulated mode', () => {
-      mockedExecSync.mockImplementationOnce(() => {
+      mockedExecFileSync.mockImplementationOnce(() => {
         throw new Error('no such user');
       });
 
@@ -482,13 +483,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');
       });
 

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 } 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;