From a03fd5ab5060a91f38986ee8537a978ac77c1aa5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Burak=20Emre=20Kabakc=C4=B1?= Date: Wed, 13 May 2026 06:08:51 +0100 Subject: [PATCH 1/2] fix(orchestrator): block Nix-expression injection in nix-shell -p MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Skill-declared nixPackages were passed directly to `nix-shell -p `, which evaluates each as a Nix expression. A skill could smuggle a side-effecting expression — e.g. `pkgs.fetchurl; builtins.exec ...`, `import ./evil.nix`, or `a && b` — past the previous charset-only validator and run code at evaluation time, before the worker process even started. Replace the raw pass-through with strict validation: each entry must be either a leaf identifier matching `^[a-z0-9][a-z0-9-]*$` or a `.` attr path (only known per-language sets like `python3Packages`, `nodePackages`, …; matches the shapes real skills already use). Validated names are re-emitted as explicit `pkgs.<...>` references inside a single `nix-shell -E` expression (`let pkgs = import {}; in pkgs.mkShell { buildInputs = [ … ]; }`), so nix-shell never parses caller-controlled text. --- .../unit/nix-package-attr-ref.test.ts | 53 +++++++++++ .../orchestration/impl/embedded-deployment.ts | 91 ++++++++++++++++--- 2 files changed, 129 insertions(+), 15 deletions(-) create mode 100644 packages/server/src/__tests__/unit/nix-package-attr-ref.test.ts diff --git a/packages/server/src/__tests__/unit/nix-package-attr-ref.test.ts b/packages/server/src/__tests__/unit/nix-package-attr-ref.test.ts new file mode 100644 index 000000000..1ab89abe3 --- /dev/null +++ b/packages/server/src/__tests__/unit/nix-package-attr-ref.test.ts @@ -0,0 +1,53 @@ +import { describe, expect, it } from "bun:test"; +import { OrchestratorError } from "@lobu/core"; +import { nixPackageAttrRef } from "../../gateway/orchestration/impl/embedded-deployment"; + +describe("nixPackageAttrRef", () => { + it("accepts plain leaf package names", () => { + expect(nixPackageAttrRef("ripgrep")).toBe("pkgs.ripgrep"); + expect(nixPackageAttrRef("jq")).toBe("pkgs.jq"); + expect(nixPackageAttrRef("python3")).toBe("pkgs.python3"); + expect(nixPackageAttrRef("chromium")).toBe("pkgs.chromium"); + }); + + it("accepts known-namespace attr paths", () => { + expect(nixPackageAttrRef("python3Packages.requests")).toBe( + "pkgs.python3Packages.requests" + ); + expect(nixPackageAttrRef("nodePackages.typescript")).toBe( + "pkgs.nodePackages.typescript" + ); + }); + + it("rejects Nix-expression injection via shell metacharacters", () => { + expect(() => nixPackageAttrRef("pkgs.x; touch /tmp/pwn")).toThrow( + OrchestratorError + ); + }); + + it("rejects boolean/operator expressions", () => { + expect(() => nixPackageAttrRef("a && b")).toThrow(OrchestratorError); + }); + + it("rejects import expressions", () => { + expect(() => nixPackageAttrRef("import ./evil.nix")).toThrow( + OrchestratorError + ); + }); + + it("rejects dotted attr paths outside the known namespace allowlist", () => { + expect(() => nixPackageAttrRef("pkgs.fetchurl")).toThrow(OrchestratorError); + expect(() => nixPackageAttrRef("builtins.exec")).toThrow(OrchestratorError); + }); + + it("rejects nested attr paths even within a known namespace", () => { + expect(() => + nixPackageAttrRef("python3Packages.foo.bar") + ).toThrow(OrchestratorError); + }); + + it("rejects empty and uppercase-only leaf names", () => { + expect(() => nixPackageAttrRef("")).toThrow(OrchestratorError); + expect(() => nixPackageAttrRef("RipGrep")).toThrow(OrchestratorError); + }); +}); diff --git a/packages/server/src/gateway/orchestration/impl/embedded-deployment.ts b/packages/server/src/gateway/orchestration/impl/embedded-deployment.ts index 4ac06d5b8..cf035722e 100644 --- a/packages/server/src/gateway/orchestration/impl/embedded-deployment.ts +++ b/packages/server/src/gateway/orchestration/impl/embedded-deployment.ts @@ -174,6 +174,70 @@ function buildShellCommand(command: string, args: string[]): string { return [command, ...args].map(shellQuote).join(" "); } +/** + * Nix attribute namespaces that hold per-language package sets. A skill may + * reference a leaf inside one of these (e.g. `python3Packages.requests`); both + * the namespace and the leaf are validated and the result is re-emitted as an + * explicit `pkgs.<...>` reference — the raw string is never handed to nix. + */ +const NIX_PACKAGE_NAMESPACES = new Set([ + "python3Packages", + "python311Packages", + "python312Packages", + "nodePackages", + "perlPackages", + "rubyPackages", + "haskellPackages", + "rPackages", + "ocamlPackages", + "luaPackages", +]); + +const NIX_LEAF_RE = /^[a-z0-9][a-z0-9-]*$/; +const NIX_ATTR_LEAF_RE = /^[a-zA-Z0-9_][a-zA-Z0-9_-]*$/; + +/** + * Validate a skill-declared Nix package name and return a safe Nix attribute + * reference (`pkgs.`). `nix-shell -p` evaluates each argument as a Nix + * *expression*, so a bare string like `pkgs.fetchurl; builtins.exec ...` or + * `import ./evil.nix` would run code at evaluation time. We never forward the + * raw string: it must be a strict leaf identifier (`^[a-z0-9][a-z0-9-]*$`) or a + * `.` attr path, and it is re-emitted as an explicit + * `pkgs.<...>` attribute reference. + */ +export function nixPackageAttrRef(pkg: string): string { + // Defence in depth: reject obvious shell/Nix metacharacters up front. + if (/[\s;&|`$(){}<>'"\\!*?#]/.test(pkg)) { + throw new OrchestratorError( + ErrorCode.DEPLOYMENT_CREATE_FAILED, + `Invalid nix package name: ${pkg}` + ); + } + const dot = pkg.indexOf("."); + if (dot === -1) { + if (!NIX_LEAF_RE.test(pkg)) { + throw new OrchestratorError( + ErrorCode.DEPLOYMENT_CREATE_FAILED, + `Invalid nix package name: ${pkg}` + ); + } + return `pkgs.${pkg}`; + } + const namespace = pkg.slice(0, dot); + const leaf = pkg.slice(dot + 1); + if ( + !NIX_PACKAGE_NAMESPACES.has(namespace) || + leaf.includes(".") || + !NIX_ATTR_LEAF_RE.test(leaf) + ) { + throw new OrchestratorError( + ErrorCode.DEPLOYMENT_CREATE_FAILED, + `Invalid nix package name: ${pkg}` + ); + } + return `pkgs.${namespace}.${leaf}`; +} + export class EmbeddedDeploymentManager extends BaseDeploymentManager { private workers: Map = new Map(); @@ -294,23 +358,20 @@ export class EmbeddedDeploymentManager extends BaseDeploymentManager { let spawnArgs: string[]; if (nixPackages.length > 0) { - // Each entry is passed to `nix-shell -p`, which evaluates it as a Nix - // expression — `pkgs.foo; rm -rf /` would execute as a shell side - // effect. Restrict to bare attribute names; operators that need a - // richer expression should pre-build the shell. - for (const pkg of nixPackages) { - if (!/^[A-Za-z0-9._-]+$/.test(pkg)) { - throw new OrchestratorError( - ErrorCode.DEPLOYMENT_CREATE_FAILED, - `Invalid nix package name: ${pkg}` - ); - } - } - // Wrap in nix-shell so nix binaries are on PATH. + // `nix-shell -p ` evaluates each as a Nix *expression*, so a + // bare package string like `pkgs.fetchurl; builtins.exec …` or + // `import ./evil.nix` would run code at evaluation time. Never forward + // the raw skill string: validate it to a strict leaf (or known + // `.`) identifier and re-emit an explicit `pkgs.` + // attribute reference instead. + const packageRefs = nixPackages.map(nixPackageAttrRef); + // Wrap in nix-shell so nix binaries are on PATH. `-E` takes a single + // expression that resolves to the build inputs; `pkgs` is bound to the + // nixpkgs set via a `let` and every ref was validated above. command = "nix-shell"; spawnArgs = [ - "-p", - ...nixPackages, + "-E", + `let pkgs = import {}; in pkgs.mkShell { buildInputs = [ ${packageRefs.join(" ")} ]; }`, "--run", buildShellCommand(workerInvocation.command, workerInvocation.args), ]; From 59879fbdf4b5f78ce965a1d2a337802292fb3f31 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Burak=20Emre=20Kabakc=C4=B1?= Date: Wed, 13 May 2026 14:25:33 +0100 Subject: [PATCH 2/2] fix(orchestrator): allow underscores in Nix package leaf names MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit NIX_LEAF_RE was /^[a-z0-9][a-z0-9-]*$/ which rejected valid nixpkgs attributes like `poppler_utils` (used in examples/personal-finance/lobu.toml). Underscore is a legal character in Nix attribute names and is safe here because the injection-guard metacharacter regex already blocks ; | & etc., and the attr is re-emitted as pkgs. — the raw string is never handed to nix. Updated regex: /^[a-z0-9_][a-z0-9_-]*$/ (mirrors NIX_ATTR_LEAF_RE which was already correct for namespace-qualified leaves). Added positive test cases: poppler_utils, csvtk, cairo_2. Added negative cases confirming semicolons are still blocked even when combined with underscore-containing names (pkgs;builtins.exec, foo_bar;builtins.exec). Fixes Codex P2 finding on PR #682 (sec/3-nix-injection). --- .../src/__tests__/unit/nix-package-attr-ref.test.ts | 12 ++++++++++++ .../orchestration/impl/embedded-deployment.ts | 4 ++-- 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/packages/server/src/__tests__/unit/nix-package-attr-ref.test.ts b/packages/server/src/__tests__/unit/nix-package-attr-ref.test.ts index 1ab89abe3..93d5b8999 100644 --- a/packages/server/src/__tests__/unit/nix-package-attr-ref.test.ts +++ b/packages/server/src/__tests__/unit/nix-package-attr-ref.test.ts @@ -10,6 +10,12 @@ describe("nixPackageAttrRef", () => { expect(nixPackageAttrRef("chromium")).toBe("pkgs.chromium"); }); + it("accepts leaf package names containing underscores", () => { + expect(nixPackageAttrRef("poppler_utils")).toBe("pkgs.poppler_utils"); + expect(nixPackageAttrRef("csvtk")).toBe("pkgs.csvtk"); + expect(nixPackageAttrRef("cairo_2")).toBe("pkgs.cairo_2"); + }); + it("accepts known-namespace attr paths", () => { expect(nixPackageAttrRef("python3Packages.requests")).toBe( "pkgs.python3Packages.requests" @@ -23,6 +29,12 @@ describe("nixPackageAttrRef", () => { expect(() => nixPackageAttrRef("pkgs.x; touch /tmp/pwn")).toThrow( OrchestratorError ); + expect(() => nixPackageAttrRef("pkgs;builtins.exec")).toThrow( + OrchestratorError + ); + expect(() => nixPackageAttrRef("foo_bar;builtins.exec")).toThrow( + OrchestratorError + ); }); it("rejects boolean/operator expressions", () => { diff --git a/packages/server/src/gateway/orchestration/impl/embedded-deployment.ts b/packages/server/src/gateway/orchestration/impl/embedded-deployment.ts index cf035722e..fce7c36b1 100644 --- a/packages/server/src/gateway/orchestration/impl/embedded-deployment.ts +++ b/packages/server/src/gateway/orchestration/impl/embedded-deployment.ts @@ -193,7 +193,7 @@ const NIX_PACKAGE_NAMESPACES = new Set([ "luaPackages", ]); -const NIX_LEAF_RE = /^[a-z0-9][a-z0-9-]*$/; +const NIX_LEAF_RE = /^[a-z0-9_][a-z0-9_-]*$/; const NIX_ATTR_LEAF_RE = /^[a-zA-Z0-9_][a-zA-Z0-9_-]*$/; /** @@ -201,7 +201,7 @@ const NIX_ATTR_LEAF_RE = /^[a-zA-Z0-9_][a-zA-Z0-9_-]*$/; * reference (`pkgs.`). `nix-shell -p` evaluates each argument as a Nix * *expression*, so a bare string like `pkgs.fetchurl; builtins.exec ...` or * `import ./evil.nix` would run code at evaluation time. We never forward the - * raw string: it must be a strict leaf identifier (`^[a-z0-9][a-z0-9-]*$`) or a + * raw string: it must be a strict leaf identifier (`^[a-z0-9_][a-z0-9_-]*$`) or a * `.` attr path, and it is re-emitted as an explicit * `pkgs.<...>` attribute reference. */