Skip to content

[WRONG BRANCH] fix(config): avoid untrusted process lookup in PID validation #174

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
luvs01 wants to merge 1 commit into from
Closed

[WRONG BRANCH] fix(config): avoid untrusted process lookup in PID validation #174

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 11 additions & 3 deletions src/config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2832,10 +2832,17 @@ export function verifyPidIdentity(candidatePid: number): number | null {

function readProcessCommandLine(pid: number): string | undefined {
try {
if (process.platform === "linux") {
const output = readFileSync(`/proc/${pid}/cmdline`, "utf-8");
return output.replace(/\0/g, " ").trim() || undefined;
}
if (process.platform === "win32") {
const systemRoot = /^[a-z]:\\windows$/i.test(process.env.SystemRoot ?? "")
? process.env.SystemRoot!
: "C:\\Windows";
// Prefer WMIC over PowerShell: much faster cold start, and windowsHide avoids console flash.
// Fall back to PowerShell when WMIC is absent (newer Windows images).
const wmic = `${process.env.SystemRoot ?? "C:\\Windows"}\\System32\\wbem\\WMIC.exe`;
const wmic = `${systemRoot}\\System32\\wbem\\WMIC.exe`;
try {
const output = execFileSync(wmic, [
"process", "where", `ProcessId=${pid}`, "get", "CommandLine", "/VALUE",
Expand All @@ -2846,7 +2853,8 @@ function readProcessCommandLine(pid: number): string | undefined {
} catch {
/* WMIC missing or failed — fall through */
}
const output = execFileSync("powershell.exe", [
const powershell = `${systemRoot}\\System32\\WindowsPowerShell\\v1.0\\powershell.exe`;
const output = execFileSync(powershell, [
"-NoProfile",
"-NoLogo",
"-NonInteractive",
Expand All @@ -2857,7 +2865,7 @@ function readProcessCommandLine(pid: number): string | undefined {
], { encoding: "utf-8", stdio: ["ignore", "pipe", "ignore"], timeout: 3000, windowsHide: true });
return output.trim() || undefined;
}
const output = execFileSync("ps", ["-p", String(pid), "-o", "command="], {
const output = execFileSync("/bin/ps", ["-p", String(pid), "-o", "command="], {
encoding: "utf-8",
stdio: ["ignore", "pipe", "ignore"],
timeout: 1000,
Expand Down
20 changes: 20 additions & 0 deletions tests/config.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import {
positiveIntegerConfigError,
positiveIntegerRecordConfigError,
readConfigDiagnostics,
readPid,
readRuntimePort,
removePid,
removeRuntimePort,
Expand Down Expand Up @@ -1703,6 +1704,25 @@ describe("opencodex config defaults", () => {
expect(readFileSync(getPidPath(), "utf-8")).toBe(String(process.pid));
});

test.if(process.platform === "linux")("pid validation does not execute ps from PATH", () => {
const attackerDir = join(testDir, "attacker-bin");
const markerPath = join(testDir, "executed");
mkdirSync(attackerDir);
const fakePs = join(attackerDir, "ps");
writeFileSync(fakePs, `#!/bin/sh\ntouch '${markerPath}'\necho 'ocx start'\n`, { mode: 0o755 });
const previousPath = process.env.PATH;
process.env.PATH = `${attackerDir}:${previousPath ?? ""}`;
writePid(process.pid);

try {
Comment on lines +1713 to +1717

@coderabbitai coderabbitai Bot Aug 8, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

Keep PATH cleanup around all setup that follows the mutation.

Line [1714] changes the process-wide PATH, but Line [1715] calls writePid before the try at Line [1717]. If writePid throws, the finally block does not run and later Bun tests inherit the attacker directory in PATH. Move writePid before the PATH assignment or place both operations inside the try.

Proposed fix 
 const previousPath = process.env.PATH;
-process.env.PATH = `${attackerDir}:${previousPath ?? ""}`;
-writePid(process.pid);
-
 try {
+  process.env.PATH = `${attackerDir}:${previousPath ?? ""}`;
+  writePid(process.pid);
   expect(readPid()).toBeNull();

As per path instructions, tests are flat Bun tests under tests/, so process-wide PATH state must be restored on every setup path.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const previousPath = process.env.PATH;
process.env.PATH = `${attackerDir}:${previousPath ?? ""}`;
writePid(process.pid);
try {
const previousPath = process.env.PATH;
try {
process.env.PATH = `${attackerDir}:${previousPath ?? ""}`;
writePid(process.pid);
🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/config.test.ts` around lines 1713 - 1717, Ensure the PATH mutation in
the test setup is always covered by the existing try/finally cleanup: move
writePid(process.pid) before assigning process.env.PATH, or move the PATH
assignment and writePid call together inside the try block. Preserve restoration
of the previous PATH on every setup failure path.

Source: Path instructions

expect(readPid()).toBeNull();
expect(existsSync(markerPath)).toBe(false);
} finally {
if (previousPath === undefined) delete process.env.PATH;
else process.env.PATH = previousPath;
}
});

test("removes pid file only when the expected pid still matches", () => {
writeFileSync(getPidPath(), "111", "utf-8");
removePid(222);
Expand Down
Loading