fix(cuda): resolve nvidia-smi via absolute path to block Windows CWD planting (BE-5357) - #641
fix(cuda): resolve nvidia-smi via absolute path to block Windows CWD planting (BE-5357)#641mattmillerai wants to merge 8 commits into
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)
…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>
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.
`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>
…planting (BE-5357) Promote hardware.py's _resolve_binary()/_is_planted_in_cwd() into a shared leaf module, comfy_cli/_safe_exec.py, and route cuda_detect's nvidia-smi fallback probe through it. cuda_detect invoked the probe by bare name, so on Windows CreateProcess searched the current working directory before $PATH and a planted nvidia-smi.exe would run - the same vector #567 closed in hardware.py. The helper lives in its own module rather than being imported from hardware because hardware already imports cuda_detect. Resolution returning None means "skip the probe", which lands on _detect_via_nvidia_smi's existing degrade-to-None contract, so no new error handling is required. hardware._run now delegates to the shared helper; its behaviour is unchanged.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
There was a problem hiding this comment.
🔍 Cursor Review — Consolidated panel
Triggered by @mattmillerai.
Found 8 finding(s).
| Severity | Count |
|---|---|
| 🟡 Medium | 4 |
| 🟢 Low | 4 |
Panel: 8/8 reviewers contributed findings.
Four review findings on the promoted _safe_exec helper:
- is_planted_in_cwd failed open: an OSError/ValueError from getcwd()/realpath()
returned "not planted", so resolve_binary handed the unvetted path back and it
got spawned. It now fails closed - we cannot prove the binary sits outside the
CWD, so the probe is skipped, which is the same degradation as the binary being
absent. This was the module's only error path that did not degrade safely.
- os.path.isabs is not "fully qualified" on Windows: ntpath.isabs accepts a
drive-less rooted path (\tools\nvidia-smi.exe) on the 3.10-3.12 interpreters we
support, and CreateProcess then re-resolves it against the process's current
drive. _is_fully_qualified additionally requires a drive (or UNC share) on
Windows so the "trusted absolute path" assumption actually holds.
- shutil.which short-circuits its PATH search when the name carries a directory
component, returning the caller's string after only an isfile+X_OK check - so
resolve_binary("/tmp/attacker/evil") passed both CWD guards. The helper is
documented as shared and lost its leading underscore in the move, so a name
with a path part (separator or Windows drive prefix) is now refused up front.
- _detect_via_nvidia_smi caught only (FileNotFoundError, SubprocessError), but
spawning a resolved absolute path reaches other OSError variants:
PermissionError on a noexec/SELinux-restricted mount, or Errno 8 Exec format
error for a +x file that is not a valid executable. Those escaped and aborted
comfy install with a traceback instead of degrading to None as the docstring
promises; the except now covers OSError, matching hardware._run.
Docstrings also stop overclaiming: the CWD guard's one known false positive
(running comfy from a directory that is itself an absolute $PATH entry) is now
stated rather than asserted away.
|
STACKED — merging lands on Review resolution — all 8 Cursor panel threads addressed (980039e)Fixed (4):
Replied, no code change (3): the Deferred (1): the remaining bare-name Tests: |
|
🤖 The reviews loop filed Linear follow-up ticket(s) for review thread(s) deferred as out of scope for this PR:
|
|
Needs a rebase — mechanical, and everything I could check without merging already checks out, so this should be quick. Why it conflicts: its base #567 just landed as a squash merge, so the stacked commits no longer match What I verified while reviewing #567:
One deliberate behavior change worth calling out in the description: this flips Good call scoping out Ping me once it's rebased. |
ELI-5
comfyasks the machine "what CUDA driver do you have?" and, when the direct driver-library check comes up empty, falls back to running thenvidia-smiprogram. It used to ask for that program by name. On Windows, asking by name means "look in the folder I'm standing in first" — so if you ran acomfycommand from a folder a bad actor had prepared, their fakenvidia-smi.exewould run instead of the real one. This PR makes it look up the program's real, full path first, and simply skip the check if the only thing it finds is sitting in the current folder. Nothing else changes: a skipped check degrades to "unknown CUDA version", exactly like a failed check already did.What changed
comfy_cli/_safe_exec.py—resolve_binary()/is_planted_in_cwd(), moved verbatim (docstrings adapted from "hardware probe" to the general case) out ofcomfy_cli/hardware.py. It is a deliberate leaf module — it imports nothing fromcomfy_cli— becausehardwarealready importscuda_detect, so importing the helper fromhardwarewould create a cycle.comfy_cli/cuda_detect.py—_detect_via_nvidia_smi()resolvesnvidia-smithrough_safe_exec.resolve_binary()and spawns the resolved absolute path; an unresolvable / CWD-anchored match skips the probe. No new error handling:Nonelands on the function's existing degrade-to-Nonecontract, same as the(FileNotFoundError, subprocess.SubprocessError)path.comfy_cli/hardware.py—_run()now delegates to_safe_exec.resolve_binary(). Behaviour is unchanged; this is the mechanical half of the promotion.Tests
All 13 resolver tests added by #567 are preserved, just relocated to follow the code:
tests/comfy_cli/test_safe_exec.py(new) — the 9 CWD-guard / relative-match tests, now against_safe_execdirectly, plus 3 never-raises tests.tests/comfy_cli/test_hardware.py— keeps the 5_run-level tests, repointed fromhardware.shutilto_safe_exec.shutil. The now-irrelevantplatform.systempatches were dropped from the moved tests (the guard has never branched on platform; the docstrings say so).tests/comfy_cli/test_cuda_detect.py— 5 new tests mirroring fix(hardware): resolve probe binaries via absolute path to block Windows CWD planting (BE-3434) #567 on the cuda_detect call site: the resolved absolute path is what gets spawned, a CWD-planted binary is rejected without spawning, a relativewhichresult is never executed, an absent binary skips the probe, and a broken lookup still degrades toNone. The existing parse-level tests get an autouse fixture stubbing resolution, so they run identically on a machine with no NVIDIA driver.ruff check .,ruff format --check ., and the fullpytestsuite are green (3668 passed, 37 skipped).Falsification of the "skip the probe" path
This diff adds a deny/skip path, so the premise "anything the old bare-name call could have run,
resolve_binarystill returns" was checked empirically rather than only unit-tested:$PATH: every one thatshutil.whichfinds,resolve_binaryalso returns — 0 denials.$PATH) is correctly denied:which→<tmpdir>/nvidia-smi,resolve_binary→None.subprocess' own exec-path search andshutil.whichread the same$PATH, so the two cannot disagree on a normal installation.The residual false negative is the trade-off #567 already made and reviewed: if the CWD is the directory holding the real binary, the probe is skipped. Impact here is bounded —
_detect_via_nvidia_smiis only the fallback;_detect_via_ctypesruns first and succeeds on any machine with a loadable CUDA driver library, and a fully-failed detection already falls back toDEFAULT_CUDA_TAGwith a--cuda-versionoverride available.Out of scope (noted, not fixed)
_load_libcuda()in the same module loadsnvcuda.dllby bare name on Windows. That is a DLL search-order question (LoadLibraryEx, mitigated by SafeDllSearchMode putting the CWD after the system directory), not theCreateProcessvector this ticket scopes to, so it is left alone. Worth a separate look.gitingit_utils.py/cmdline.py,pip). Same class of vector, different blast radius — deliberately not widened into here.