From 9963b8f2f52b5041c5968a357a7865c6fcc21dca Mon Sep 17 00:00:00 2001 From: huntedbytheirs Date: Sat, 8 Aug 2026 19:00:00 -0400 Subject: [PATCH] fix(security): reject path traversal in build_script and validate default_jobs range --- .omo/evidence/F2-tofu-core.log | 531 +++++++++++++++++++++++++++ .omo/notepads/tofu-core/learnings.md | 19 + .omo/notepads/tofu-core/problems.md | 21 ++ src/tofu/config.d | 75 +++- src/tofu/fetch.d | 102 +++++ 5 files changed, 747 insertions(+), 1 deletion(-) create mode 100644 .omo/evidence/F2-tofu-core.log diff --git a/.omo/evidence/F2-tofu-core.log b/.omo/evidence/F2-tofu-core.log new file mode 100644 index 0000000..b9de064 --- /dev/null +++ b/.omo/evidence/F2-tofu-core.log @@ -0,0 +1,531 @@ +================================================================================ +F2 TOFU-CORE CODE QUALITY AUDIT — FINAL VERDICT +================================================================================ +Date: 2026-08-08 +Reviewer: Sisyphus-Junior (independent agent) +Target: tofu-core Dlang codebase (24 .d files, 11,018 LOC) +Branch: current HEAD +Build: dmd 2.112 (debug, warningsAsErrors), ldc2 (release) +================================================================================ + +VERDICT: REJECTED +================================================================================ +Reason: 1 BLOCKER (C10 — path traversal in fetch.d), 1 MAJOR (C5 — unsafe + cast(int) in config.d), 2 MINOR findings. + See per-check table below for details. +================================================================================ + +PER-CHECK SUMMARY TABLE +─────────────────────── +Check | Name | Result | Severity (if fail) +───────┼─────────────────────────┼──────────┼─────────────────── + C1 | dub build (warnings=err)| PASS | — + C2 | @safe/@system audit | PASS | — + C3 | Hardcoded paths | PASS | — + C4 | Exception handling | PASS | — + C5 | cast() audit | FAIL | MAJOR (1), MINOR (1) + C6 | Error message quality | PASS* | MINOR (2 non-actionable) + C7 | Unbounded memory growth | PASS | — + C8 | Concurrency (spinner) | PASS | — + C9 | Module structure (<1KL) | PASS | — + C10 | Security (sandbox+path) | FAIL | BLOCKER (1) +─────────────────────────────────────────────────────────────────────────────── + + +================================================================================ +DETAILED EVIDENCE PER CHECK +================================================================================ + +─── C1: dub build with warnings-as-errors ──────────────────────────────────── + + Build 1: dmd debug (default) + Command: dub build + Result: PASS + Evidence: + Warning: only DUB-level deprecation about "warningsAsErrors" buildOption + (DUB recommends "buildRequirements" instead — cosmetic, not a + code defect). Zero compiler warnings, zero errors. + Output: + Starting Performing "debug" build using /usr/bin/dmd for x86_64. + Up-to-date toml 1.0.0: target for configuration [library] is up to date. + Up-to-date tofu ~main: target for configuration [application] is up to date. + Finished + + Build 2: ldc2 release (-b release --compiler=ldc2) + Result: PASS + Evidence: + Starting Performing "release" build using ldc2 for x86_64. + Building toml 1.0.0: building configuration [library] + Building tofu ~main: building configuration [application] + Linking tofu + Zero warnings, zero errors. + + Build 3: dmd --force (clean rebuild) + Command: dub build --force + Result: PASS + Only linker messages, zero compiler warnings: + Building toml 1.0.0: building configuration [library] + Building tofu ~main: building configuration [application] + Linking tofu + + Sub-verdict: PASS + + +─── C2: @safe audit ────────────────────────────────────────────────────────── + + Check: grep -rn '@system' src/ + Result: PASS — zero @system function annotations anywhere in the codebase + + Evidence: + All 5 grep hits are documentation comments explaining WHY a particular + section uses @trusted (e.g. "stdin/stdout are @system in dmd 2.112"). + No actual @system annotation on any function. + + $ grep -rn '@system' src/ + src/tofu/log.d:49:/// `@trusted`: `stdout` itself is `@system` to access in dmd 2.112. + src/tofu/http.d:5:/// `@safe`; the `@system` curl internals are isolated behind `@trusted` + src/tofu/http.d:74:// @trusted helpers — all @system curl calls are isolated here + src/tofu/state.d:36:// ─── @trusted wrappers (fs + json — @system in Phobos) ────── + src/tofu/commands/upgrade.d:36:// ─── @trusted I/O wrappers (stdout/stdin are @system in DMD 2.112) ─── + + Annotations count: + - @safe functions/blocks: 242 + - @trusted functions/blocks: 28 + - Ratio: ~8.6:1 safe-to-trusted + - @system blocks (`@system:`): ZERO + + Public API coverage: + All public API functions are either @safe or @trusted with documented + rationale in module-level doc comments. Filesystem, process, and socket + operations are isolated behind @trusted helpers with explicit comments. + + Sub-verdict: PASS + + +─── C3: Hardcoded paths audit ──────────────────────────────────────────────── + + Check: grep -rn '"/tmp/\|~/\|"/usr/\|home/' src/ --include='*.d' + Result: PASS — all hits are in test code or documentation + + Evidence: + src/tofu/config.d:117: // doc comment: "~/.config/tofu/config.toml" + src/tofu/config.d:130: cfg.cacheDir = expandTilde("~/.cache/tofu"); ← DEFAULT, env-overridable + src/tofu/config.d:279: test assertion — assert(cfg.cacheDir == expandTilde("~/.cache/tofu")) + src/tofu/config.d:300-302: test TOML fixture string content + src/tofu/config.d:308-310: test assertions on TOML fixture + src/tofu/config.d:356-362: test assertions with env override + src/tofu/config.d:424-431: test assertions with env override + src/tofu/build.d:142,181: error messages mentioning config file path (informational) + src/tofu/build.d:500: test assertion — assert(pkgNameFromPath("/tmp/...") == "build") + src/tofu/build.d:585: test fixture — cfg.cacheDir = "/tmp/dummy" + src/tofu/errors.d:287: comment — "On a fresh system ~/.cache/tofu may not exist yet." + + Analysis: + - All hardcoded paths are in test assertions, test fixture strings, or + informational error messages. + - The config module (config.d:130) is the sole owner of path defaults + via `expandTilde("~/.cache/tofu")` — overridable by TOML or env var. + - No production code path uses hardcoded absolute paths. + + Sub-verdict: PASS + + +─── C4: Exception handling ─────────────────────────────────────────────────── + + Check 4a: main.d has try/catch around dispatch + Result: PASS + Evidence: + src/main.d lines 101-127: + try { + final switch (pa.cmd) { + case Command.help: ... return 0; + case Command.install: return installCommand(...); + case Command.search: return searchCommand(...); + case Command.upgrade: return upgradeCommand(...); + case Command.remove_: return removeCommand(...); + case Command.info: return infoCommand(...); + } + } catch (Exception e) { + int ec = exitCodeFor(e); + logError("%s", e.msg); + return ec; + } + Note: catches `Exception`, not `Throwable` — correct D idiom (Errors + like OutOfMemoryError should NOT be caught). PASS. + + Lock management: `scope(exit) releaseLock(cacheDir)` at line 81, + `scope(failure) releaseLock(cacheDir)` at line 82 — correct. + + Check 4b: grep 'catch.*Throwable' + Result: PASS — all occurrences in unittest cleanup only + Evidence: + src/tofu/http.d:230: catch (Throwable) {} ← oneShotResponder test util + src/tofu/index.d:403: catch (Throwable) {} ← oneShotResponder test util + src/tofu/binary.d:252: catch (Throwable) {} ← oneShotResponder test util + src/tofu/fetch.d:368: catch (Throwable) ← waitForPort test util + src/tofu/fetch.d:381: catch (Throwable) {} ← killServer test util + src/tofu/fetch.d:412: catch (Throwable) {} ← removeDir test util + + All 6 occurrences are inside `version(unittest)` blocks — test helpers + where silently discarding errors during cleanup is acceptable (one-shot + TCP responders, wait loops, server teardown). No production code path + swallows Throwable. + + Sub-verdict: PASS + + +─── C5: cast() audit ───────────────────────────────────────────────────────── + + Check: grep -rn 'cast(' src/ + Result: FAIL — one MAJOR finding, one MINOR finding + + Findings: + + [MAJOR] src/tofu/config.d:227 — unsafe cast(int) tJobs + ────────────────────────────────────────────────────────── + Line: cfg.defaultJobs = cast(int) tJobs; + Context: tJobs is `long.min` sentinel or a TOML integer (long). + `cfg.defaultJobs` is `int` (32-bit signed). + Issue: `cast(int)` silently truncates large values. + e.g. TOML `default_jobs = 5000000000` wraps to negative int. + Impact: Low (absurd job counts not realistic), but violates + defensive coding. The parsePositiveInt in cli.d correctly + validates via `to!int` (which throws on overflow); the + config path should similarly validate. + Fix: Replace `cast(int) tJobs` with `to!int(tJobs)` or clamp + the value. (NOT fixing — reporting only.) + + [MINOR] src/tofu/commands/install.d:367 — cast(PackageIndex[])[] in test + ────────────────────────────────────────────────────────── + Line: delegate () @safe { return cast(PackageIndex[])[]; } + Context: Test delegate to inject an empty index for testing + "package not found" (exit code 2). + Issue: Raw cast of `[]` literal. Idiom would be + `(PackageIndex[]).init` or `new PackageIndex[0]`. + Impact: Test-only, zero runtime consequence. + Severity: MINOR + + PASS items (all in @trusted or test context): + - errors.d:80-139: cast(TofuError/HttpException/...) — safe downcast + - errors.d:216: cast(pid_t) — safe integer cast in POSIX-specific code + - http.d:98: cast(string) data — in @trusted getImpl, ubyte[]→string + - http.d:223,242: cast()/cast(shared) listener — test-only TCP server + - vercmp.d:113: cast(string) buf — in @trusted lambda, char[]→string + - index.d:278: cast() entries — in @trusted block, extracting parsed.array + - index.d:396,416: cast()/cast(shared) listener — test-only TCP server + - binary.d:245,265: cast()/cast(shared) listener — test-only TCP server + - fetch.d:350: cast(InternetAddress) — test-only findFreePort + - commands/info.d:83,151: cast(string) — in @trusted lambdas + + Sub-verdict: FAIL (MAJOR: config.d:227) + + +─── C6: Error message quality ──────────────────────────────────────────────── + + Check: spot-check 5 throw sites across modules — each must name WHAT + failed and suggest HOW to fix. + + Site 1: src/tofu/build.d:141-144 + Message: "zeta-makepkg not found. Install zeta-toolchain or set + TOFU_ZETA_TOOLCHAIN_PATH in ~/.config/tofu/config.toml" + WHAT: zeta-makepkg not found ✓ + HOW: install zeta-toolchain OR set config path ✓ + Verdict: PASS — fully actionable + + Site 2: src/tofu/fetch.d:277-279 + Message: "build script '%s' not found for package '%s'" + WHAT: which script, which package ✓ + HOW: no suggestion — user doesn't know HOW to resolve + Verdict: MINOR — names WHAT clearly but lacks a suggestion + (e.g. "check the recipe's build_script field") + + Site 3: src/tofu/http.d:113-114 + Message: "cannot reach ZUUR at : timeout after 120s" + WHAT: which URL timed out ✓ + HOW: no suggestion — user doesn't know HOW to fix network issues + Verdict: MINOR — names WHAT clearly but no suggestion + (e.g. "check your network connection or TOFU_ZUUR_URL") + + Site 4: src/tofu/index.d:223-224 + Message: "ZUUR index is invalid: lua command not found on PATH" + WHAT: lua not found ✓ + HOW: implies "install lua" ✓ + Verdict: PASS — actionable + + Site 5: src/tofu/cli.d:213-214 + Message: "unknown option: -Z (run 'tofu --help')" + WHAT: which option is unknown ✓ + HOW: run --help for valid options ✓ + Verdict: PASS — fully actionable + + Summary: 3 of 5 fully actionable, 2 lack HOW guidance. Minor room for + improvement but not a rejection-level issue. + + Sub-verdict: PASS (with 2 MINOR notes) + + +─── C7: Unbounded memory growth in hot paths ───────────────────────────────── + + Check 7a: http.d onReceive string accumulator + Evidence: http.d:96-99 + http.onReceive = (ubyte[] data) { + content ~= cast(string) data; + return data.length; + }; + Analysis: The `get()` function accumulates the full response body into a + string. Used for fetching the ZUUR index (index.lua) — typically + a few KB. Also used for fetching binary package.lua manifests + (a few hundred bytes). Acceptable for these use cases. + The `downloadFile()` function writes to disk, not memory. + Verdict: PASS + + Check 7b: stderr ring buffer bound + Evidence: build.d:46-63,75 + private final class StderrRing { + string[] _lines; + size_t cap; + ... + void add(string line) { + _lines ~= line; + if (_lines.length > cap) + _lines = _lines[1 .. $]; // drop oldest, keep cap + } + } + auto ring = new StderrRing(20); // ← confirmed: 20-line bound + Analysis: Ring buffer initialized at 20 lines (line 75). `add()` drops + oldest line when exceeding capacity. Reader thread joins before + `ring.get()` is called, ensuring happens-before. + Verdict: PASS + + Check 7c: spinner thread join + Evidence: ui.d:95-106 — `_thread.join()` wrapped in try/catch + Verdict: PASS (deferred to C8) + + Sub-verdict: PASS + + +─── C8: Concurrency — ui.d spinner thread ──────────────────────────────────── + + Check: spinner clean stop — no join hang, shared bool flag + Evidence: ui.d:31-119 + + shared bool _running; // line 35 — shared flag + ... + void threadFn() { + while (_running) { // line 50 — checked every iteration + ... // 100ms sleep per iteration + } + } + void stop() { + _running = false; // line 93 — signal to stop + if (_thread !is null) { + try { + _thread.join(); // line 99 — join, max wait ~100ms + } catch (Exception) { + // double-join or thread failure — harmless + } + _thread = null; // prevent double-join + } + } + + Analysis: + - `_running` is `shared bool` — correct for cross-thread communication + - Thread loop checks `_running` every 100ms (Thread.sleep at line 55) + - `stop()` sets the flag, then joins — max join delay ~100ms + - Double-join prevented by `_thread = null` after join + - Exception during join caught and discarded (benign) + - No hang risk — flag is simple bool, no mutex contention + - Non-TTY mode skips thread entirely (lines 89-90) + + Sub-verdict: PASS + + +─── C9: Module structure ───────────────────────────────────────────────────── + + Check: wc -l src/tofu/*.d src/tofu/commands/*.d + Result: PASS — all modules under 1000 lines + + Lines Module + ───── ────────────────────── + 851 src/tofu/resolve.d ← largest + 708 src/tofu/install.d + 695 src/tofu/build.d + 694 src/tofu/index.d + 644 src/tofu/fetch.d + 600 src/tofu/errors.d + 554 src/tofu/commands/install.d + 545 src/tofu/commands/upgrade.d + 541 src/tofu/types.d + 472 src/tofu/recipeparse.d + 462 src/tofu/commands/info.d + 435 src/tofu/config.d + 435 src/tofu/cache.d + 390 src/tofu/binary.d + 376 src/tofu/commands/search.d + 366 src/tofu/state.d + 363 src/tofu/cli.d + 356 src/tofu/http.d + 346 src/tofu/vercmp.d + 334 src/tofu/deps.d + 330 src/tofu/commands/remove.d + 275 src/tofu/ui.d + 242 src/tofu/log.d + 4 src/tofu/package.d ← module re-export only + ───── + 11018 TOTAL + + Largest module (resolve.d, 851 lines) is well under the 1000-line + ceiling. No file requires an override justification. + + Sub-verdict: PASS + + +─── C10: Security ──────────────────────────────────────────────────────────── + + Check 10a: index.d Lua sandbox whitelist + Result: PASS + Evidence: + - Whitelist defined at index.d:70-91 (Lua sandboxLuaScript constant) + - Stripped globals: io, os, require, dofile, loadfile, loadstring, + package, debug + - Test 2 (line 471): os.execute("rm -rf /") → IndexException caught, + sentinel file survives → sandbox works + - Test 3 (line 514): io.open("/etc/shadow") → IndexException caught + - ZETA lib/sandbox.lua port, Lua 5.1/5.2+ compatible + - Confirmed: hunkered-down whitelist, NOT a blacklist + + Check 10b: fetch.d path traversal + Result: FAIL — BLOCKER + Evidence: + ├─ src/tofu/fetch.d:253-258: + │ + │ auto buildScriptPath = extractBuildScript(recipeContent); + │ if (buildScriptPath.length > 0) { + │ logDetail("custom build system: fetching %s", buildScriptPath); + │ auto bsUrl = cfg.recipesUrl(name) ~ "/" ~ buildScriptPath; + │ auto bsDest = cacheDir ~ "/" ~ buildScriptPath; // ← UNSANITIZED + │ + │ () @trusted { + │ auto bsDir = bsDest.dirName; + │ if (bsDir.length > 0 && !exists(bsDir)) + │ mkdirRecurse(bsDir); + │ }(); + │ ... + │ downloadFile(bsUrl, bsDest); // writes to escaped path + │ } + │ + └─ PROBLEM: No sanitization of `buildScriptPath`. + + ATTACK VECTOR: + 1. Malicious recipe in ZUUR includes: + build_system = "custom" + build_script = "../../../.bashrc" + 2. extractBuildScript returns "../../../.bashrc" + 3. bsDest = "~/.cache/tofu/../../../.bashrc" + → resolves to "$HOME/.bashrc" + 4. downloadFile creates $HOME/.bashrc.part, then renames to $HOME/.bashrc + 5. Result: user's .bashrc is overwritten with attacker-controlled content + + Confirmations: + - Zero occurrences of ".." sanitization anywhere in src/ (confirmed + via grep for indexOf/canFind/contains/startsWith of "..") + - No expandTilde, buildPath, normalizePath, or absolutePath applied + to buildScriptPath or bsDest in the build_script code path + - mkdirRecurse and exists() resolve ".." through the OS kernel + + Severity: BLOCKER + The .recipe file content comes from the remote ZUUR repository. + A malicious maintainer (or compromised repository) can write + arbitrary files anywhere the user has write access. + + Required fix (not implemented — reporting only): + - Reject buildScriptPath containing ".." before constructing bsDest + - OR resolve bsDest to absolute canonical path and verify it starts + with cacheDir + + Sub-verdict: FAIL (BLOCKER: path traversal in fetch.d) + + +================================================================================ +FINDINGS SUMMARY TABLE +================================================================================ + + # | Check | Severity | Location | Description + ───┼───────┼──────────┼────────────────────┼───────────────────────────────── + 1 | C10 | BLOCKER | fetch.d:253-258 | Path traversal: buildScriptPath + | | | | from .recipe used unsanitized in + | | | | bsDest, can escape cache dir via + | | | | "../" in build_script field + ───┼───────┼──────────┼────────────────────┼───────────────────────────────── + 2 | C5 | MAJOR | config.d:227 | cast(int) tJobs silently truncates + | | | | large TOML default_jobs values + ───┼───────┼──────────┼────────────────────┼───────────────────────────────── + 3 | C6 | MINOR | fetch.d:277-279 | Error message lacks HOW guidance + | | | | for missing build script + 4 | C6 | MINOR | http.d:113-114 | Error message lacks HOW guidance + | | | | for timeout/network errors + 5 | C5 | MINOR | install.d:367 | cast(PackageIndex[])[] in test — + | | | | raw cast of empty array literal + ───┴───────┴──────────┴────────────────────┴───────────────────────────────── + +================================================================================ +PASS COUNTS +================================================================================ + 8 of 10 checks PASS: C1, C2, C3, C4, C6*, C7, C8, C9 + 1 of 10 checks FAIL (non-blocker): C5 (MAJOR + MINOR) + 1 of 10 checks FAIL (blocker): C10 (BLOCKER) + ── + *C6 PASS but with 2 MINOR non-actionable error message notes + +================================================================================ +OVERALL VERDICT: REJECTED +================================================================================ +The codebase is well-structured, compiles cleanly, handles concurrency +correctly, and has no hardcoded paths or unsafe exception swallowing. +The Lua sandbox is properly implemented. However, the path traversal +vulnerability in fetch.d (C10) is a BLOCKER that could allow a malicious +ZUUR recipe to write files outside the cache directory — this MUST be +fixed before approval. + +Recommended fix (not applied — audit only): + In fetch.d, after line 254, add: + if (buildScriptPath.indexOf("..") >= 0) + throw new FetchException( + "build_script path contains '..': " ~ buildScriptPath); + +This is a one-line fix. Re-audit of C10 after the fix is applied should +be trivial. +================================================================================ + +================================================================================ +F2 FIX APPLIED — build_script path traversal + default_jobs truncation +================================================================================ +Date: 2026-08-08 + +FINDING 1 (BLOCKER, fetch.d:253-258) — RESOLVED + src/tofu/fetch.d: added private @safe validateBuildScriptPath(string) + - rejects ".." segments, leading '/' (absolute), and '\' (backslash) + with: throw new FetchException("build_script path is unsafe: ") + - wired into fetchRecipe step 4 BEFORE building bsUrl/bsDest + - defense in depth: after joining, throws if + !bsDest.startsWith(cacheDir ~ "/") + - valid relative paths unchanged (build.sh, scripts/build.sh pass) + +FINDING 2 (MAJOR, config.d:227) — RESOLVED + src/tofu/config.d: default_jobs from TOML now range-validated (1..1024) + - tJobs < 1 || tJobs > 1024 -> stderr warning + "Warning: invalid default_jobs in config, using default 1" + and cfg.defaultJobs = 1 (no silent cast(int) truncation) + - else cfg.defaultJobs = cast(int) tJobs + +TESTS ADDED + fetch.d: direct validateBuildScriptPath unittests (evil paths rejected, + valid paths pass) + integration Test 6: recipe with + build_script="../../evil.sh" -> FetchException, nothing escapes cache + config.d: default_jobs = 5000000000 -> 1, 1024 -> 1024, 2048 -> 1, + -3 -> 1 + +VERIFICATION + dub build : PASS (warningsAsErrors, no warnings) + dub test : 23 modules passed unittests + smoketest : PASS 16 checks passed diff --git a/.omo/notepads/tofu-core/learnings.md b/.omo/notepads/tofu-core/learnings.md index 268be22..3a4ee4c 100644 --- a/.omo/notepads/tofu-core/learnings.md +++ b/.omo/notepads/tofu-core/learnings.md @@ -1418,3 +1418,22 @@ The sandbox test (test 2) creates a sentinel file, serves an index containing `o - **Escape in unit tests**: all 8 pre-existing info.d tests used recipes WITH a `deps` field, so the absent-deps path was never exercised. Regression tests must cover the *negative* field, not just happy paths. + +## F2 review — build_script path validation & default_jobs range check + +### Path-traversal guard pattern (fetch.d) +- Any user-influenced path destined for `cacheDir ~ "/" ~ path` must be + validated: reject `..` substrings, leading `/` (absolute), and `\` + (Windows separators). Also verify the joined destination with + `startsWith(cacheDir ~ "/")` as defense in depth. Keep the check in a + small private @safe function so it is directly unit-testable without + spinning up the mock HTTP server. +- Valid relative paths (plain names, `scripts/build.sh`) are untouched — + proven by the existing custom-build integration test and smoketest. + +### Range-check before cast (config.d) +- TOML integers parse as `long`; `cast(int)` silently truncates huge + values (5000000000 → negative). Always range-validate before casting + (1..1024 for jobs), warn to stderr, and fall back to the default. This + mirrors the existing `parseIntOr` warning pattern already used for env + vars. diff --git a/.omo/notepads/tofu-core/problems.md b/.omo/notepads/tofu-core/problems.md index b771301..ac0812f 100644 --- a/.omo/notepads/tofu-core/problems.md +++ b/.omo/notepads/tofu-core/problems.md @@ -36,3 +36,24 @@ everywhere else). Verified: `dub build` ✓, `dub test` — 23 modules passed ✓, smoketest — 16 checks PASS ✓. + +## RESOLVED (F2 review, 2026-08-08) + +**Fixed:** Both F2 findings. + +1. **BLOCKER — path traversal in fetch.d (`build_script`)**: extracted + `validateBuildScriptPath(string)` (private @safe) — rejects `..` + segments, leading `/`, and `\` with `FetchException("build_script path + is unsafe: ")`. Wired into `fetchRecipe` step 4 before building + `bsUrl`/`bsDest`, plus defense-in-depth containment check + (`bsDest.startsWith(cacheDir ~ "/")`). Added direct unit tests and an + integration test (recipe with `build_script = "../../evil.sh"` → + FetchException, no file escapes the cache). + +2. **MAJOR — `cast(int)` truncation in config.d `default_jobs`**: + range-validate before casting. `tJobs < 1 || tJobs > 1024` → stderr + warning + fallback to 1. Added tests for 5000000000→1, 1024→1024, + 2048→1, -3→1. + +Verified: `dub build` ✓, `dub test` — 23 modules passed ✓, +smoketest — 16 checks PASS ✓. diff --git a/src/tofu/config.d b/src/tofu/config.d index 274be55..d90f265 100644 --- a/src/tofu/config.d +++ b/src/tofu/config.d @@ -224,7 +224,24 @@ Config load(string configFile = null, if (tZeta.length > 0) cfg.zetaPath = tZeta; auto tJobs = getInt("default_jobs"); - if (tJobs != long.min) cfg.defaultJobs = cast(int) tJobs; + if (tJobs != long.min) + { + if (tJobs < 1 || tJobs > 1024) + { + // Out of range (negative or absurdly large) — cast(int) + // would silently truncate; fall back to the default. + () @trusted { + stderr.writefln( + "Warning: invalid default_jobs %d in config, " + ~ "using default 1", tJobs); + }(); + cfg.defaultJobs = 1; + } + else + { + cfg.defaultJobs = cast(int) tJobs; + } + } } // ── 5. Override with environment variables (highest priority) ──────── @@ -411,6 +428,62 @@ zuur_url = "https://ignored.example.com/zuur" assert(cfg.defaultJobs == 8); } +// ── Extra: default_jobs = 5000000000 (overflow) → fallback to 1. +@safe unittest { + auto tmp = testTempPath("jobs-overflow"); + scope (exit) removeTestFile(tmp); + + writeTestFile(tmp, q"[ +[core] +default_jobs = 5000000000 +]"); + + auto cfg = load(tmp, emptyEnv()); + assert(cfg.defaultJobs == 1); +} + +// ── Extra: default_jobs = 1024 (max valid) → accepted. +@safe unittest { + auto tmp = testTempPath("jobs-max"); + scope (exit) removeTestFile(tmp); + + writeTestFile(tmp, q"[ +[core] +default_jobs = 1024 +]"); + + auto cfg = load(tmp, emptyEnv()); + assert(cfg.defaultJobs == 1024); +} + +// ── Extra: default_jobs = 2048 (> 1024) → fallback to 1. +@safe unittest { + auto tmp = testTempPath("jobs-too-big"); + scope (exit) removeTestFile(tmp); + + writeTestFile(tmp, q"[ +[core] +default_jobs = 2048 +]"); + + auto cfg = load(tmp, emptyEnv()); + assert(cfg.defaultJobs == 1); +} + +// ── Extra: default_jobs negative → fallback to 1. +@safe unittest { + auto tmp = testTempPath("jobs-negative"); + scope (exit) removeTestFile(tmp); + + writeTestFile(tmp, q"[ +[core] +default_jobs = -3 +]"); + + auto cfg = load(tmp, emptyEnv()); + assert(cfg.defaultJobs == 1); +} + // ── Extra: Missing config file is not an error. @safe unittest { auto cfg = load("/nonexistent/tofu/config.toml", emptyEnv()); diff --git a/src/tofu/fetch.d b/src/tofu/fetch.d index c376a0f..82290e3 100644 --- a/src/tofu/fetch.d +++ b/src/tofu/fetch.d @@ -70,6 +70,24 @@ private @safe string extractBuildScript(string content) return bsVal; } +/// Validate a `build_script` path extracted from a recipe before it is +/// used to build a URL or a local cache destination. +/// +/// Rejects paths that could escape the package cache directory or the +/// ZUUR URL path: `..` segments, absolute paths (leading `/`), and +/// backslashes (Windows-style separators). +/// +/// Throws `FetchException` for unsafe paths. +private @safe void validateBuildScriptPath(string path) +{ + if (path.indexOf("..") >= 0 + || (path.length > 0 && path[0] == '/') + || path.indexOf("\\") >= 0) + { + throw new FetchException("build_script path is unsafe: " ~ path); + } +} + /// Find `key = "..."` in Lua-like content and return the quoted /// string value. Returns `null` when the key is not found or /// the value is not a quoted string. @@ -253,10 +271,20 @@ private void cleanupFiles(string[] paths) @trusted auto buildScriptPath = extractBuildScript(recipeContent); if (buildScriptPath.length > 0) { + // Reject paths that escape the cache dir or the ZUUR URL path + // (.. segments, absolute paths, backslashes). + validateBuildScriptPath(buildScriptPath); + logDetail("custom build system: fetching %s", buildScriptPath); auto bsUrl = cfg.recipesUrl(name) ~ "/" ~ buildScriptPath; auto bsDest = cacheDir ~ "/" ~ buildScriptPath; + // Defense in depth: the resolved destination must stay inside + // the package cache directory. + if (!bsDest.startsWith(cacheDir ~ "/")) + throw new FetchException( + "build_script path is unsafe: " ~ buildScriptPath); + // Ensure parent directories for nested build scripts () @trusted { auto bsDir = bsDest.dirName; @@ -331,6 +359,29 @@ build_script = "build.sh"`).length == 0); // build_system = "custom" but no build_script → null assert(extractBuildScript( `build_system = "custom"`).length == 0); + + // ── validateBuildScriptPath: reject traversal / absolute / backslash ── + foreach (bad; ["../../evil.sh", "/etc/passwd", "a\\b.sh", "..\\win.sh"]) + { + bool threw = false; + try + { + validateBuildScriptPath(bad); + assert(false, "Expected rejection for: " ~ bad); + } + catch (FetchException e) + { + threw = true; + assert(e.msg.indexOf("build_script path is unsafe") >= 0, + "Expected unsafe-path message, got: " ~ e.msg); + } + assert(threw, "validateBuildScriptPath should reject: " ~ bad); + } + + // Valid relative paths pass without throwing + validateBuildScriptPath("build.sh"); + validateBuildScriptPath("scripts/build.sh"); + validateBuildScriptPath("tools/build-1.2.sh"); } version (unittest) @@ -642,3 +693,54 @@ version (unittest) assert(cacheClean, "Cache directory should be clean after failed download"); } + +// ── Test 6: malicious build_script path → FetchException, no escape +@safe unittest +{ + auto serveDir = makeTempDir("evil"); + scope (exit) removeDir(serveDir); + + writeTestFile(serveDir, "recipes/hello/hello.recipe", + `return { + name = "hello", + build_system = "custom", + build_script = "../../evil.sh" +}`); + + auto port = findFreePort(); + auto pid = () @trusted { + return spawnProcess( + ["python3", "-m", "http.server", port.to!string, + "--bind", "127.0.0.1"], + workDir: serveDir); + }(); + scope (exit) killServer(pid); + waitForPort(port); + + auto baseUrl = "http://127.0.0.1:" ~ port.to!string; + auto cacheDir = makeTempDir("cache-evil"); + scope (exit) removeDir(cacheDir); + + auto cfg = testConfig(baseUrl, cacheDir); + + bool caught = false; + try + { + fetchRecipe("hello", cfg); + assert(false, "Expected FetchException for unsafe build_script"); + } + catch (FetchException e) + { + caught = true; + assert(e.msg.indexOf("build_script path is unsafe") >= 0, + "Expected unsafe-path message, got: " ~ e.msg); + } + assert(caught, "Should have thrown FetchException"); + + // Verify nothing escaped the cache dir + bool noEscape = () @trusted { + return !exists(buildPath(serveDir, "evil.sh")) + && !exists(buildPath(cacheDir, "evil.sh")); + }(); + assert(noEscape, "No file should be written outside the cache dir"); +}