fix(security): reject path traversal in build_script and validate default_jobs range
This commit is contained in:
@@ -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 <url>: 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: <path>")
|
||||
- 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 <n> 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
|
||||
@@ -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.
|
||||
|
||||
@@ -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: <path>")`. 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 ✓.
|
||||
|
||||
+74
-1
@@ -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());
|
||||
|
||||
@@ -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");
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user