Windows port — Phase 3: disk health & SMART#9
Conversation
…in elevated reader (no PATH planting EoP)
…OCTOU + report-injection) The elevated apply/smart IPC used the PID in predictable %TEMP% file names (fyd-apply-<pid>-*, fyd-smart-<pid>-*). A local same-user attacker could pre-create a junction at the predictable path to redirect the admin write (CWE-377 TOCTOU) or substitute a forged SmartInfo report (spoof/XSS into the frontend). Replace the PID with a CSPRNG nonce (getrandom, 20 digits, digit-only to satisfy the child's token guard) so the path is unguessable — closing both vectors at the root. Also: read_smart_elevated surfaces a report-write failure (exit 1) instead of masking it; doc comments updated (pid -> token).
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ab74c308f3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let Some(name) = dev.get("name").and_then(|n| n.as_str()) else { | ||
| continue; | ||
| }; | ||
| results.push(read_one_smart(&smartctl, name)); |
There was a problem hiding this comment.
On Windows this returns each SmartInfo keyed by the raw smartctl --scan-open name, but the UI builds smart[info.device] and renders only smart[disk.device], while the Windows disk list keys cards from sysinfo volume names/mount points (health.rs:208-213). When those identifiers differ, as with normal Windows drive-letter volumes versus smartctl device tokens, a successful SMART read is never attached to any disk card and the health tab hides the SMART data.
Useful? React with 👍 / 👎.
| dirs.iter().any(|d| d.join(bin).is_file()) | ||
| // On Windows, executables carry a .exe suffix. | ||
| #[cfg(target_os = "windows")] | ||
| let found = dirs.iter().any(|d| d.join(format!("{bin}.exe")).is_file()); |
There was a problem hiding this comment.
For bin == "smartctl", this still searches every PATH directory, but the elevated reader intentionally refuses PATH and only accepts the two Program Files smartmontools locations (headless.rs:191-199). If a user has a portable/custom/stale smartctl.exe on PATH but not in those trusted locations, smart_deps_status() reports it installed and leaves missing empty, so the install prompt disappears while disk_smart() can only return no SMART data.
Useful? React with 👍 / 👎.
| let Some(name) = dev.get("name").and_then(|n| n.as_str()) else { | ||
| continue; | ||
| }; | ||
| results.push(read_one_smart(&smartctl, name)); |
There was a problem hiding this comment.
When --scan-open finds a device that needs an explicit type, this drops the reported type and later calls only smartctl -a -j <name>. The smartctl man page says --scan reports device type/protocol, --scan-open may change that type through autodetection, and -d TYPE is how callers specify it, so USB/SAT/RAID/NVMe devices that scan successfully can still fail the subsequent read and show no Windows SMART data.
Useful? React with 👍 / 👎.
| // The elevated child self-discovers devices via `smartctl --scan-open`, so | ||
| // the caller's device list is unused. One UAC prompt; report read from file. | ||
| let token = elevation_token(); | ||
| let report_path = std::env::temp_dir().join(format!("fyd-smart-{token}-report.json")); |
There was a problem hiding this comment.
This report path is derived from the unelevated user's %TEMP%, but read_smart_elevated derives its own path with std::env::temp_dir() after Start-Process -Verb RunAs. When UAC prompts for different admin credentials, the child runs under that admin account and writes to the admin temp directory, so the parent never sees the report and returns empty SMART despite a successful read; this affects standard Windows accounts.
Useful? React with 👍 / 👎.
…, disks comment Doc/comment-only follow-ups from the Phase 3 final review (no logic change): - correct stale 'parent PID' comments (token is now a random nonce) in execute.rs/headless.rs (the value is security-relevant — avoid misleading readers) - add a SUPERSEDED security banner to the plan's Task 3 so its pre-hardening snippet (PATH fallback + PID token) is never copied verbatim (would reintroduce the EoP) - health.rs: 'volumes (not physical disks)' instead of 'physical-ish'
The Windows clippy gate (-D warnings) failed: since Task 2 routes Windows SMART installs through winget (not pkexec_install_deps), HELPER_PATH, resolve_helper_path and pkexec_install_deps became dead code on Windows. Gate the first two to cfg(not(windows)) (Linux+macOS still use them) and pkexec_install_deps to cfg(linux) (macOS=brew, Windows=winget). Cross-platform execute_plan/user_zones keep Path/PathBuf imports alive on Windows. (Local clippy runs on Linux where these ARE used — only the Windows CI catches this, which is why it exists.)
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 70c1151d4c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| device, | ||
| model: None, | ||
| size_bytes: disk.total_space(), | ||
| rotational: false, |
There was a problem hiding this comment.
On Windows machines with an HDD, this forces rotational to false, and the health UI renders false as SSD, so every Windows HDD is mislabeled. sysinfo exposes disk kind information on Windows; otherwise this needs an unknown state instead of mapping unavailable data to SSD.
Useful? React with 👍 / 👎.
| let mut buf = [0u8; 8]; | ||
| match getrandom::getrandom(&mut buf) { | ||
| Ok(()) => format!("{:020}", u64::from_le_bytes(buf)), | ||
| Err(_) => std::process::id().to_string(), |
There was a problem hiding this comment.
If the OS RNG returns an error, this falls back to the process ID for the temp-file token used by the Windows elevated --apply/--smart IPC. In that environment the path is predictable again, so a same-user attacker can pre-create the report path/junction or forge a report before the elevated process writes it; avoid launching elevation or return an error instead of continuing with a PID token.
Useful? React with 👍 / 👎.
| let name = disk.name().to_string_lossy().into_owned(); | ||
| let device = if name.is_empty() { | ||
| disk.mount_point().to_string_lossy().into_owned() | ||
| } else { | ||
| name |
There was a problem hiding this comment.
When two Windows volumes have the same non-empty label, this uses that label from disk.name() as the device; the seen set below then treats them as the same disk and drops later volumes, so health_overview and SMART dependency status omit real drives. Use the mount point or volume GUID as the stable key and keep the label separate for display.
Useful? React with 👍 / 👎.
| let out = Command::new("winget") | ||
| .args([ | ||
| "install", | ||
| "--id", | ||
| "smartmontools.smartmontools", | ||
| "--accept-source-agreements", | ||
| "--accept-package-agreements", | ||
| "--silent", | ||
| ]) |
There was a problem hiding this comment.
Microsoft's winget install docs say --id only restricts which field is searched and --exact is needed to require an exact match. On systems with an added source or another ID containing this string, the one-click install can become ambiguous or select the wrong package despite the intended allowlist; add --exact/-e and preferably pin the source.
Useful? React with 👍 / 👎.
1 participant
Phase 3 of the Windows port. Disk-health tab on Windows: disk list + uptime via sysinfo; SMART via smartctl read by the Phase-2 UAC-elevated executor (
--smartmode); one-clickwinget install smartmontoolswhen missing. Throughput stays 0 (PDH deferred, like macOS).Security hardening (from adversarial review): trusted-absolute smartctl only (no PATH-planting EoP); CSPRNG random-nonce tokens for all elevated-IPC temp paths (closes TOCTOU + forged-report injection at the root; also hardens Phase-2 apply). Linux/macOS unchanged (cfg-gated). SMART runtime is manual-smoke (no smartctl/disk on CI).