fix(hardware): resolve probe binaries via absolute path to block Windows CWD planting (BE-3434) - #567
Conversation
Add comfy_cli/hardware.py with a single detect_hardware() entry point that reports OS, CPU, RAM, and a GPU sub-block (vendor/model/VRAM/unified-memory) so agent surfaces can route weak machines away from local diffusion. - macOS: cpu via sysctl machdep.cpu.brand_string; Apple Silicon -> apple unified-memory GPU; Intel Mac -> unknown non-unified GPU (no system_profiler). - NVIDIA (any OS): nvidia-smi CSV, then ctypes libcuda fallback (reuses cuda_detect._load_libcuda). - AMD (Linux): best-effort rocm-smi --json. - Never raises, never blocks long: every probe wrapped, subprocesses timeout=5. Wire hardware into EnvChecker.fill_data() (JSON) and a Hardware row in fill_print_table() (pretty). Extend schemas/env.json with an OPTIONAL, fully nullable hardware property (not in required) so older/failed-probe payloads stay valid. Envelope schema unchanged (envelope/1).
…(BE-3399) - env_checker: escape untrusted cpu/gpu-model strings in the Rich-rendered hardware summary so markup-like content (e.g. "[/]") can't raise MarkupError and crash `comfy env` in pretty mode. - hardware(_detect_gpu_amd): return None (no phantom all-None AMD block) when nothing parses, matching the NVIDIA probes; gate the card loop on the "card" key prefix so a non-card metadata block isn't parsed as the GPU; exclude the "VRAM Total Used Memory" usage key so total capacity isn't understated. - hardware(_detect_gpu_nvidia_ctypes): pop/restore CUDA_VISIBLE_DEVICES around the ctypes probe (mirrors cuda_detect) so an exported ""/"-1" doesn't hide a present GPU. - tests: cover AMD total-vs-used key, metadata-block skip, all-None->None, and markup escaping. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ows CWD planting (BE-3434)
mattmillerai
added
agent-coded
dosubot
Bot
added
size:M
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 47 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughHardware probing now resolves executables to absolute paths, rejects binaries planted directly in the working directory, handles resolution failures and empty commands, and uses the configured subprocess timeout. Tests cover POSIX and Windows path behavior. ChangesHardware Probe Security
Sequence Diagram(s)sequenceDiagram
participant Caller
participant HardwareProbe
participant BinaryLookup
participant Subprocess
Caller->>HardwareProbe: request hardware probe
HardwareProbe->>BinaryLookup: resolve executable
BinaryLookup-->>HardwareProbe: absolute path or None
HardwareProbe->>Subprocess: execute resolved command with timeout
Subprocess-->>HardwareProbe: probe output
Possibly related PRs
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔍 Cursor Review — Consolidated panel
Triggered by @mattmillerai.
Found 4 finding(s).
| Severity | Count |
|---|---|
| 🟡 Medium | 1 |
| 🟢 Low | 3 |
Panel: 6/8 reviewers contributed findings.
Reviewers that did not contribute: kimi-k2.5:adversarial (empty), kimi-k2.5:edge-case (empty)
…3434) Address cursor-review findings on the probe binary CWD guard: - Medium (5/8): the old subtree check rejected any binary whose CWD is an ancestor, so running `comfy env` from `C:\Windows` (or a drive root) silently disabled GPU/CPU detection for `C:\Windows\System32\nvidia-smi.exe`. Reject only a binary sitting *directly* in the CWD (the actual planting signature); a legitimate system binary in a subdirectory is untouched. - Low (1/8): apply the guard on every platform, not just Windows — a `.`/empty entry in POSIX `$PATH` lets `shutil.which` return a CWD match too. - Low (1/8): normalize both paths with `os.path.normcase` so Windows' case-insensitivity can't fail the guard open. - Low (4/8): guard `_run` against an empty `cmd` so it degrades to None instead of raising IndexError, honoring the never-raise contract. Renames `_is_within_cwd` -> `_is_planted_in_cwd`; updates and extends tests (subdirectory-of-CWD allowed, POSIX plant rejected, empty-cmd). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
dosubot
Bot
added
size:L
|
This PR currently conflicts with Please rebase (or merge |
Resolves the add/add conflict in comfy_cli/hardware.py: this branch's CWD-planting absolute-path resolution (_resolve_binary/_is_planted_in_cwd) combined with main's independently-merged AMD multi-card fallback fix (BE-3399 follow-up). env_checker.py keeps main's IPv6-bracketing / _resolved_local_address addition alongside this branch's hardware summary.
dosubot
Bot
added
size:M
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@comfy_cli/hardware.py`:
- Around line 74-81: Normalize the path returned by shutil.which to an absolute
path before checking or returning it in the hardware probe helper. Apply
os.path.abspath to path before _is_planted_in_cwd and ensure the returned value
passed to _run is absolute, while preserving the existing missing-command and
CWD-planted rejection behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 936c1d1c-e4f9-42f1-9860-819b5cc5de4a
📒 Files selected for processing (2)
comfy_cli/hardware.pytests/comfy_cli/test_hardware.py
`shutil.which` returns `os.path.join(entry, name)`, so a relative `$PATH` entry (`.`, an empty entry, `subdir`, or Windows' implicitly prepended `os.curdir`) yields a RELATIVE match. `_is_planted_in_cwd` correctly lets `subdir/nvidia-smi` through — its parent is not the CWD — but `_run` then handed that relative string to `subprocess.check_output`, which re-resolves it against the attacker-controlled CWD: the exact hijack this PR closes. `_resolve_binary` now skips any non-absolute `which` result. A relative result means the matching `$PATH` entry was itself relative, so the binary is anchored under the CWD; a binary found through a normal absolute `$PATH` entry always comes back absolute and is unaffected. This is strictly stronger than normalising with `abspath`, which would still spawn `<cwd>/subdir/nvidia-smi`. Adds four tests: the relative-subdirectory, `./`-relative and bare-name `which` results are all rejected, and `_run` never spawns a relative path. Addresses CodeRabbit review thread r3684167296. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
@bigcat88 the conflict is cleared — Since your comment I also pushed dd3b9fa, which resolves the one outstanding CodeRabbit thread (r3684167296): |
|
🤖 The reviews loop filed Linear follow-up ticket(s) for review thread(s) deferred as out of scope for this PR:
|
bigcat88
left a comment
bigcat88
left a comment
There was a problem hiding this comment.
Approving. Your tests all mock shutil.which, so nothing in the suite had actually executed a planted binary — I built one and ran the real attack.
The attack, reproduced and blocked
A real executable nvidia-smi dropped in a scratch directory, run with PATH=.:$PATH (the POSIX equivalent of Windows' implicit CWD search), CWD set to that directory:
main: RESULT: '550.100' PLANTED BINARY EXECUTED: YES <-- hijacked
this branch: RESULT: None PLANTED BINARY EXECUTED: no <-- blocked
On main the planted script ran (it wrote its marker file) and its fabricated 550.100 was returned as the driver version — so the hijack isn't just code execution, it also feeds a lie into the hardware probe. Blocked here.
The false-positive guarantee holds — checked against real binaries, not mocks
This is the half that would hurt if it were wrong, since a silently-skipped probe degrades to "unknown" with no error:
-
Real system binary:
/usr/bin/nvidia-smiexists on this box.comfy --json envreturns byte-identical hardware on both branches —NVIDIA GeForce RTX 5090,vram_bytes: 34190917632,AMD Ryzen 9 9950X. No regression. -
Swept every executable on this box's
$PATH— 821 thatshutil.whichresolves, 0 denied by_resolve_binary. Corroborates the 1003/0 sweep in #641's description on different hardware. -
Binary in a subdirectory of the CWD (your
System32case) — a real executable at<cwd>/System32/rocm-smireached via an absolute$PATHentry, CWD set to its parent:resolved: <cwd>/System32/rocm-smi run : 'LEGIT-SYSTEM-BINARY'Allowed, as documented. Rejecting only the immediate parent is the right line — the alternative (
is_relative_to) would breakcomfy envfor anyone whose CWD happens to be an ancestor of a system directory.
Design notes
The two rejection rules are both necessary and neither subsumes the other, which your test_rejects_relative_subdirectory_match makes explicit by asserting not _is_planted_in_cwd(relative) as a precondition before the absolute check catches it. That's the kind of test that documents why a second guard exists — worth keeping.
Routing everything through _run as the single choke point is what makes this auditable: every probe (sysctl, nvidia-smi, rocm-smi) inherits the fix, and there's no second invocation path to miss.
normcase on both sides of the comparison is right for Windows.
One thing your own follow-up improves on: here, an unresolvable path or a cross-drive ValueError is treated as not planted (fail-open), documented as "so a legitimate binary is never rejected". #641 reverses that to fail-closed, with the better argument — "failing open here would be the module's only error path that hands an unvetted string to subprocess." I agree with #641: for a security guard, an unprovable location should skip the probe, and skipping degrades exactly like the binary being absent, which callers already handle. Since #641 is stacked on this and lands right behind it, I'm not blocking — just noting the final state is the stricter one, and that's the right end state.
The empty-cmd guard returning None rather than raising IndexError keeps the module's never-raise contract intact.
Full suite green on this branch merged with current main; ruff check + ruff format --diff clean under the CI-pinned 0.15.15.
Merge sequencing: #641 is stacked on this branch and promotes these helpers into comfy_cli/_safe_exec.py for cuda_detect. This lands first; I'll take #641 once GitHub retargets it.
3 participants
ELI-5
comfy envpeeks at your machine's hardware by running little programs likenvidia-smiandsysctl. Before this change it asked for them by bare name — and on Windows, "runnvidia-smi" means "look in the folder I'm standing in first." So if youcdinto a folder a bad actor prepared and it contained a fakenvidia-smi.exe,comfy envwould run their program. This change looks up the real program on your system PATH and runs it by its full absolute path instead, so the folder you happen to be in can't hijack the probe.What & why
Follow-up to #562 (deferred review thread r3607162549).
hardware.py's_runinvoked probe binaries (nvidia-smi,rocm-smi,sysctl) by bare name. On WindowsCreateProcesssearches the current working directory, so runningcomfy envfrom an attacker-controlled directory could execute a plantednvidia-smi.exe(low severity, binary-planting).Fix (
comfy_cli/hardware.py):_resolve_binary(name)resolvescmd[0]viashutil.which()(a trusted PATH lookup) and returns the absolute path;_runnow invokes[resolved, *cmd[1:]]. IfwhichreturnsNone(binary absent) the probe is skipped, exactly matching the module's existing degrade-to-Nonecontract._runis the single choke point — every probe (sysctl,nvidia-smi,rocm-smi) goes through it.shutil.whichcan itself resolve against the current directory (always on Windows, which prependsos.curdir; on any platform when$PATHholds.or an empty entry). Two such results are rejected on every platform:whichreturnsos.path.join(entry, name), so a relative result means the matching$PATHentry was relative and the binary lives under the attacker-controlled CWD; handing that string tosubprocesswould re-resolve it against the CWD. A binary found via a normal absolute$PATHentry always comes back absolute and is unaffected._is_planted_in_cwd) — the signature of a plant. The check is deliberately narrow (parent directory is the CWD, compared vianormcase(realpath(...))) so running from an ancestor likeC:\Windowsnever rejects a legitimateSystem32\nvidia-smi.exe. Ambiguity (different drive, unresolvable path) is treated as not planted.timeout=5bound and the never-raise contract are preserved (_resolve_binary/_is_planted_in_cwdare fully wrapped and degrade toNone/False; an emptycmdreturnsNonerather than raisingIndexError).POSIX is behaviour-neutral in practice:
subprocesswithoutshell=Truealready usesexecvp(PATH search, never the CWD), andshutil.whichresolves the same binary — the guards only fire when$PATHitself contains a CWD-anchored entry, which is the hijack case.Tests
tests/comfy_cli/test_hardware.py— three classes (11 tests):_runinvokes the resolved absolute path / skips when absent / never raises on resolve failure / degrades on an emptycmd; the CWD guard rejects a CWD-planted binary on Windows and POSIX, and allows aSystem32binary and a binary in a subdirectory of the CWD; relativewhichresults (subdir/…,./…, bare name) are rejected and never spawned. Full module suite: 33 passed.ruff check+ruff format --checkclean (CI's ruff 0.15.15).Notes / judgment calls
maindirectly (it was originally stacked onmatt/be-3399-hardware-block).cuda_detect.pyhas a similar bare-namenvidia-smiinvocation, but it predates feat: add cross-platform hardware block to comfy env --json (BE-3399) #562 and this ticket scopes specifically tohardware.py:_run. Flagging for a possible separate follow-up.shutil.whichon the default PATH (rather than restricting to hardcoded system dirs) — the CWD-exclusion guard is the ticket's primary suggested mitigation and is more robust than an allowlist that could miss the NVIDIA driver's install location.