Enforce native WSL filesystem trust boundary - #88
Conversation
CherylSnowVeil
left a comment
There was a problem hiding this comment.
Initial review — head 35643e6
Reviewed the full current diff (new internal/wslfs package plus docs updates) against main. CI is green. Overall this is a careful preflight implementation: strict Lstat-based checks, correct parent-before-child creation order, rollback limited to directories created by the failed attempt, and a fail-closed non-Linux stub. The roadmap/doc edits accurately reflect merged PRs #80–#84 and #86.
I also assessed the three inline comments already left by the automated reviewer; where they overlap with real issues I note that below.
Findings
[important] Device check stops at the home directory — submounts escape the root-filesystem boundary
internal/wslfs/wslfs_linux.go (Prepare, inspectDirectory, validateManagedFile)
Prepare compares st_dev of layout.Home against /, but none of the managed directories (~/.config, ~/.config/container-bin, ~/.local/state/container-bin, ~/.local/lib/container-bin, ~/.local/bin) or managed-file parents get the same check. A bind mount or separate filesystem at any of those paths bypasses the boundary: with DrvFs metadata enabled, a Windows-backed mount can carry the correct uid and mode 0700 and pass every ownership/mode/symlink check while the registry, lockfile and state actually live on a Windows filesystem — precisely what the home device check exists to prevent. docs/wsl.md also claims preparation rejects "any other separate filesystem rather than guessing its trust semantics", which is only enforced at the home level. (Concurs with the existing automated inline comment on lines 93–101.)
Remediation: keep rootDevice and require st_dev == rootDevice in inspectDirectory and for the managed endpoints, or narrow the documented guarantee to the home mount itself.
[nit] Tests assume os.TempDir() is on the root filesystem device
internal/wslfs/wslfs_linux_test.go (testLayout)
t.TempDir() lands under /tmp/$TMPDIR. On hosts where /tmp is tmpfs — common in containers and hardened systemd setups, and plausible in WSL dev environments — every Prepare-calling test fails with "not distribution root device" for environmental reasons. TestPrepareRejectsHomeOnDifferentFilesystem already demonstrates the device-comparison + t.Skip pattern; applying the same guard in testLayout would keep the suite meaningful on such machines.
[nit] Freshly created directories are not chmodded, so a restrictive umask produces a false failure
internal/wslfs/wslfs_linux.go (ensureDirectory)
os.Mkdir(path, createMode) is subject to the process umask. An umask that strips owner bits (e.g. 0277) makes Prepare create ~/.config as 0500, fail its own inspectDirectory check, roll the directory back, and error out — even though the object was created by this call and chmodding it would not violate the "never repair existing objects" rule, which is meant for pre-existing paths. os.Chmod(createMode) on the created branch after a successful Mkdir avoids the spurious failure. Rare umask values, hence nit.
[nit] Mode().Perm() drops special bits, so mode equality is not exact
internal/wslfs/wslfs_linux.go (inspectDirectory, validateManagedFile)
Perm() masks to 0777, so a managed binary at 04755 or a private directory at 01700/02700 passes the "must have mode 0755/0700" checks. Practical impact today is negligible — requireOwner pins uid == current user, so setuid/setgid grant nothing the caller lacks — but for a strict trust-boundary check the full raw mode (stat.Mode & 07777) is cheap to compare and matches the stated "exactly this mode" contract. (Matches the automated comment on lines 212–213.)
[suggestion] StateNamespace check is syntactic, not identity-bound
internal/wslfs/wslfs_linux.go (validateLayout, lines 150–159)
The wsl2- + 32-hex shape check cannot detect a well-formed but foreign namespace, and Prepare has no way to recompute the digest — the inputs (/etc/machine-id, hostenv's unexported namespace domain) aren't available to it. No in-repo caller can hit this today since NativeWSLLayout is the only producer, so actionability is low; but if the wiring slice wants layout identity assurance, the natural fix is a hostenv-side verify/recompute API rather than shape checks here. (Re: the automated comment on lines 150–159.)
[suggestion] Note the point-in-time nature of the preflight for the wiring slice
All checks are Lstat/Readlink at a single instant; the consumer that acts on these paths afterwards can still be redirected (e.g. a Windows-side writer via \\wsl$ swapping a validated directory for a symlink between Prepare and file creation). That's inherent to a path-based preflight, but worth stating in docs/wsl.md so the wiring slice either re-validates per operation or uses openat-style traversal (O_NOFOLLOW/RESOLVE_BENEATH) on managed paths.
Summary
No blocking issues. The one substantive concern is the device-boundary gap below the home mount — worth fixing or explicitly scoping before this preflight is wired into an enabled frontend. Test coverage of the fail-closed paths is strong.
|
Addressed the completed review in signed commit 1c874c3:\n\n- bind the state namespace to the live WSL distro, /etc/machine-id, and current UID instead of accepting a syntactically valid foreign namespace;\n- enforce the distribution-root filesystem device on every managed directory, file, and management shim;\n- reject special mode bits using the raw Unix mode;\n- chmod only directories created by the current call so restrictive umasks cannot cause a false failure;\n- make temporary-filesystem tests skip safely when the fixture is not on the root device; and\n- document the preflight's point-in-time nature for the later wiring slice.\n\nValidation: gofmt clean, go vet ./..., go test -race ./..., focused internal/wslfs tests, and both Python wrapper suites all pass. |
|
CI follow-up: commit 16104fe fixes the Linux-only fixture assumption. The nested-filesystem test now creates a current-user-owned directory inside /dev/shm before exercising the device check, instead of testing the root-owned /dev/shm directory itself. Focused tests and go test -race ./... pass locally. |
CherylSnowVeil
left a comment
There was a problem hiding this comment.
Re-review — head 16104fe
Validation of previous findings
All findings from the initial review (head 35643e6) were verified against the current implementation, not just the fix description in 1c874c3/16104fe:
- [important] Device check stopped at home — fixed.
preparenow threadsrootDevicethroughensureDirectory/inspectDirectoryfor all eight managed directories,validateManagedFilefor the registry/lockfile/binary endpoints, andvalidateManagementShimfor the shim link itself (requireDevice,internal/wslfs/wslfs_linux.go:296). A nested DrvFs or other separate mount anywhere in the layout now fails closed.TestInspectDirectoryRejectsNestedDifferentFilesystemcovers the unit-level rejection with a current-user-owned/dev/shmfixture — the16104fecorrection is right: the prior test would have tripped the owner check on root-owned/dev/shmbefore reaching the device check. - [nit]
os.TempDir()device assumption in tests — fixed.testLayoutnow stats the temp home and/andt.Skipfs on a device mismatch (wslfs_linux_test.go:23-41). - [nit] Umask false-failure on created dirs — fixed.
ensureDirectorycallsos.Chmod(path, createMode)only on theMkdir-succeeded branch (wslfs_linux.go:176-180); existing objects are still never repaired, and a chmod failure reportscreated=trueso rollback removes the new directory.TestPrepareCreatesExactModesDespiteRestrictiveUmaskcovers umask0277. - [nit]
Perm()dropped special bits — fixed.exactModecompares the rawstat.Mode & 0o7777; private dirs and managed files require exact equality, and non-private dirs rejectperm & 0o7022 != 0(special bits plus group/other write). Sticky-bit coverage added in the "private directory special mode" subtest. - [suggestion] StateNamespace was syntactic only — fixed.
preparerecomputesruntime.NativeWSLLayout(layout.Home, layout.UID, machineID)(machine-id read inPreparefrom/etc/machine-id) and rejects whenlayout.Distroorlayout.StateNamespacediverges — before any filesystem mutation.TestPrepareRejectsForeignStateNamespaceBeforeMutationverifies no directories are created on rejection. - [suggestion] Point-in-time nature undocumented — fixed.
docs/wsl.mdnow explicitly states the preflight is not a durable path handle and directs the wiring slice to revalidate at each mutation boundary or use descriptor-relative no-follow traversal.
Fresh review of the current PR
Re-reviewed the complete current diff (internal/wslfs/* plus the four docs files) rather than only the delta since 35643e6. Checked:
- Ordering and rollback: identity checks (layout shape, UID, distro/namespace recompute) all run before any
Mkdir;createdis appended before the error return so chmod/inspect failures roll back; removal runs in reverse creation order andos.Removeonly deletes empty directories. - Symlink handling: home requires both
Lstat-is-dir andEvalSymlinksidentity (catches symlinked path components); managed files reject non-regular objects including dangling symlinks (Lstatsucceeds on them); the management shim must be a uid-owned on-device symlink whoseCleaned target is exactlyBinaryPath— chains and hardlinks fail closed. - Boundary completeness: every path the preflight touches — home, all intermediate shared dirs (
.config,.local,.local/state,.local/lib), private dirs, shim dir, and all file/shim endpoints — is bound to the/device. The doc claim about rejecting "any other separate filesystem" is now accurate. - Fail-closed surfaces: non-Linux stub always errors; non-WSL2 classification, unreadable/invalid machine-id, and foreign layouts all error before mutation.
- Docs: roadmap queue renumbering is internally consistent; merged-PR attributions (#80–#84, #86) match the git history;
docs/architecture.mdcorrectly listswslfs -> hostenvand the package has no callers outside itself, matching the "unexposed" claim.
Result: no actionable findings on the current revision. The important device-boundary gap and all lower-severity items are resolved, and nothing new was introduced. CI is fully green (format/vet/test on Linux and Windows, Windows ARM64, CodeQL, govulncheck, zizmor, dependency review).
Non-blocking observation (no action needed): unit tests now exercise prepare with an injected runtime/machine-id rather than the exported Prepare, so the /etc/machine-id read path is untested — that's inherent to testing WSL behavior on non-WSL CI, and the wrapper is thin. Splitting it this way was the right call.
Summary
The WSL frontend remains gated; this PR does not publish a Linux artifact or claim WSL support.
Validation
gofmt -l .go vet ./...go test -race ./...python3 -m unittest -v internal/registry/pipx_wrapper_test.py internal/cli/pipx_discovery_test.py./internal/wslfsRoadmap: #2