🛡️ Sentinel: [CRITICAL] Fix command injection in unix user tools

.jules/sentinel.md

@@ -1,4 +1,4 @@
-## 2025-05-13 - [SQL Injection Risk via sql.raw in Bulk Insert Retrieval]
-**Vulnerability:** A potential SQL injection vulnerability in `createMany` inside `packages/core/src/db/repositories/tasks.ts`. The code used `sql.raw` to interpolate task IDs into an `IN` clause manually without parameterization.
-**Learning:** Using `sql.raw` to join dynamically generated strings (like UUID arrays) bypasses Drizzle's parameterization, exposing the app to SQL injection if any ID string is manipulated.
-**Prevention:** Always use Drizzle ORM's built-in parameterization functions like `inArray` instead of manual string concatenation or `sql.raw`.
+## 2025-02-18 - [Command Injection via execSync in id-lookups.ts]
+**Vulnerability:** Found `execSync` being used with unsanitized inputs (`groupName` and `username`) interpolated into shell commands. This could lead to a severe command injection vulnerability if user-controllable input reaches this function.
+**Learning:** Shell interpolation allows special characters to be interpreted maliciously. Relying on simple string concatenation for dynamic commands should be strictly avoided in Node.
+**Prevention:** Use `execFileSync` or `spawn` instead, passing executable path and arguments as an array to avoid executing through a shell interpreter (`/bin/sh`).

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', 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', 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', 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', username], {
         encoding: 'utf-8',
         stdio: 'pipe',
         timeout: 2000,

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

@@ -27,14 +27,15 @@ import {
   validateResolvedUnixUser,
 } from './user-manager.js';
 
-// Mock execSync for system-dependent tests
+// Mock execSync/execFileSync 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', [username], { stdio: 'ignore' });
     return true;
   } catch {
     return false;