Exclude GITHUB_WORKSPACE and relative entries from PATH walk (#558)
* Exclude GITHUB_WORKSPACE and relative entries from PATH walk resolveExecutable previously walked any directory listed in process.env.PATH, which trusts that nothing earlier in the workflow prepended an attacker-controlled location. A malicious PR could land bin/npx in the repo and add `echo "$GITHUB_WORKSPACE/bin" >> $GITHUB_PATH` to a prior step, causing pullfrog to exec the attacker's binary with our scoped tokens in env. Filter out (a) any non-absolute PATH entry (., bin, .., etc., which resolve against cwd) and (b) any entry equal to or under GITHUB_WORKSPACE. The walk then continues to the next legitimate system tooling dir. * Address PR #558 review: comment typo + Windows case bypass - Drop double space in the threat-model comment. - Lowercase paths on Windows before comparing against GITHUB_WORKSPACE. Without this, an attacker can bypass the filter by varying case in their injected PATH entry (`d:\a\repo\bin` vs `D:\a\repo`) — string compare misses but NTFS still resolves the executable inside the workspace.
This commit is contained in:
committed by
pullfrog[bot]
parent
55c95e6f50
commit
6607112d0b
@@ -1,6 +1,6 @@
|
||||
import { execFileSync } from "node:child_process";
|
||||
import { accessSync, constants, existsSync } from "node:fs";
|
||||
import { delimiter, dirname, join } from "node:path";
|
||||
import { delimiter, dirname, isAbsolute, join, resolve, sep } from "node:path";
|
||||
import { fileURLToPath } from "node:url";
|
||||
import actionPackageJson from "./package.json" with { type: "json" };
|
||||
|
||||
@@ -42,9 +42,45 @@ function canAccessExecutable(path: string): boolean {
|
||||
}
|
||||
}
|
||||
|
||||
// reject PATH entries that an attacker can plausibly write to before pullfrog
|
||||
// runs. specifically: relative entries (., bin, etc., which resolve against
|
||||
// cwd), and anything inside the customer's checkout. an attacker who can land
|
||||
// a malicious `npx` in the repo and prepend `$GITHUB_WORKSPACE/bin` to
|
||||
// `GITHUB_PATH` from a prior workflow step would otherwise get full code
|
||||
// execution under our action token.
|
||||
//
|
||||
// on Windows the filesystem is case-insensitive but `resolve()` preserves
|
||||
// input case, so we lowercase both sides before comparing — otherwise an
|
||||
// attacker can bypass the filter by varying the case of GITHUB_WORKSPACE in
|
||||
// their injected PATH entry (`d:\a\repo` vs `D:\a\repo`).
|
||||
function normalizePathForCompare(path: string): string {
|
||||
return process.platform === "win32" ? resolve(path).toLowerCase() : resolve(path);
|
||||
}
|
||||
|
||||
function isUntrustedPathEntry(entry: string, untrustedRoots: string[]): boolean {
|
||||
if (!isAbsolute(entry)) return true;
|
||||
const normalized = normalizePathForCompare(entry);
|
||||
for (const root of untrustedRoots) {
|
||||
if (normalized === root) return true;
|
||||
if (normalized.startsWith(root + sep)) return true;
|
||||
}
|
||||
return false;
|
||||
}
|
||||
|
||||
function getUntrustedPathRoots(env: NodeJS.ProcessEnv): string[] {
|
||||
const roots: string[] = [];
|
||||
const workspace = env.GITHUB_WORKSPACE;
|
||||
if (workspace && isAbsolute(workspace)) roots.push(normalizePathForCompare(workspace));
|
||||
return roots;
|
||||
}
|
||||
|
||||
function resolveExecutable(params: { command: string; env: NodeJS.ProcessEnv }): string | null {
|
||||
const pathValue = params.env.PATH ?? "";
|
||||
const pathEntries = pathValue.split(delimiter).filter(Boolean);
|
||||
const untrustedRoots = getUntrustedPathRoots(params.env);
|
||||
const pathEntries = pathValue
|
||||
.split(delimiter)
|
||||
.filter(Boolean)
|
||||
.filter((entry) => !isUntrustedPathEntry(entry, untrustedRoots));
|
||||
const extensions =
|
||||
process.platform === "win32"
|
||||
? (params.env.PATHEXT ?? ".COM;.EXE;.BAT;.CMD").split(";").filter(Boolean)
|
||||
|
||||
Reference in New Issue
Block a user