diff --git a/CHANGELOG.md b/CHANGELOG.md index 1b9cf82a..ce00cb2d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Security - **ADR-131 HOMECORE-UI BFF gateway — public-PR review fixes (PR #1082).** (1) **HIGH — path-traversal / confused-deputy SSRF closed in the `/api/cal/*` reverse-proxy** (`homecore-server/src/gateway.rs`). The wildcard proxy path was interpolated straight into the upstream URL while `proxy()` attaches the server-side calibration bearer, so `/api/cal/v1/../../x` (and percent-encoded `..%2f`, `%2e%2e`, leading `/`, backslash, double-encoded `%252e`) could escape the `…/api/` scope **with the privileged token**. Now `validate_proxy_path()` decode-then-checks and rejects absolute/backslash/dot-segment/encoded-traversal paths with a typed **400 BEFORE the URL is built** (applies to GET **and** POST); legit `v1/...` paths still pass. Pinned by `cal_proxy_rejects_traversal_with_400_before_upstream` (fails on old code) + `validate_proxy_path_rejects_traversal_variants`. (2) **CORS + request-tracing now cover the gateway routes.** `/api/homecore/*` and `/api/cal/*` were `.merge()`d **outside** the layers `homecore-api::router()` applies, leaving them with no CORS allowlist and untraced; the audited `build_cors_layer()` (HC-05) + `TraceLayer` are now applied to the whole merged surface in `main.rs`. Pinned by `gateway_routes_are_cors_covered_after_merge` (Vite-dev-origin preflight succeeds on a gateway route). (3) **Fabricated-data honesty (§6 invariant 3):** the gateway no longer injects a hardcoded `anomaly.threshold: 0.5` — it passes through the REAL upstream threshold or emits `null` (withheld); the dashboard renders a not-available `—` instead of `"null%"`/`"null°C"` for null appliance metrics; the COG panel's Hailo-worker pill reflects the real appliance probe instead of a hardcoded `"connected"`; `rooms.js` treats a null anomaly threshold as withheld, not a fake `0.8` default. (4) **Robustness:** a forwarded `hef` that is a string (not an array) no longer throws in the COG panel; the calibration wizard guards `frames/target` against `NaN%`/`Infinity%` and clears its baseline poll timer on Restart / panel teardown (leaked `setTimeout` loop fixed). (5) **Perf:** per-bank RoomState fetches and the appliance service probes now run concurrently (`futures::join_all`; async `tokio::net::TcpStream` + `timeout` replaces the blocking `connect_timeout` that parked a worker per probe); the mock fixture module is now a dynamic `import()` gated on demo mode so production never bundles it. **Note (workspace-wide, not fixed here):** `homecore-server` requests `reqwest`'s `rustls-tls` only, but cargo feature-unification means a sibling crate enabling the default `native-tls` re-introduces OpenSSL into the final binary regardless — a true "no OpenSSL on the appliance" guarantee requires aligning every reqwest-pulling crate on rustls-only. **Note (pre-existing, out of scope):** DEV-mode `allow_any_non_empty()` bearer auth when `HOMECORE_TOKENS` is unset on `0.0.0.0` is unchanged; the loud `warn!` at boot is retained — provision real tokens before network exposure. **Verified:** `cargo test -p homecore-server --no-default-features` = **18/18 pass**, `cargo build -p homecore-server` clean, UI suite (`node tests`) all green, Python proof VERDICT PASS (hash unchanged). +- **`wifi-densepose-desktop` (Tauri v2 desktop app) beyond-SOTA security review (needs ADR slot 178) — one real IPC serial-command-injection fix + one over-broad shell-capability removal, each MEASURED on Windows; remaining IPC/path/secret dimensions confirmed clean with evidence.** Beyond-SOTA review of the Tauri desktop crate (the real attack surface is the webview→Rust IPC boundary + the capability allowlist). The crate **builds + tests on this Windows box** (`cargo check`/`cargo test -p wifi-densepose-desktop --no-default-features` — Tauri 2.10 + GTK-less Windows webview2 path), so both findings are **MEASURED**, not source-analysis. **WDP-DESK-01 (serial command injection via `configure_esp32_wifi`, MODERATE) — FIXED.** The `#[tauri::command] configure_esp32_wifi(port, ssid, password)` handler took `ssid`/`password` straight from the webview and concatenated them into newline-terminated serial commands (`format!("wifi_config {} {}\r\n", ssid, password)`, `set ssid {}\r\n`, …) with **zero validation** before writing them to the ESP32 over the line-oriented serial protocol. A `\r\n` embedded in either field lets a malicious/compromised webview **terminate the command line early and inject an arbitrary follow-up firmware command** (`reboot`, `erase_nvs`, etc.) — a command-injection-into-device-protocol crossing the IPC trust boundary. Ironic note: the crate already shipped `test_wifi_credentials_validation` documenting the WPA2 length bounds, but the handler never enforced them. **Fix:** a new `validate_wifi_credentials(ssid, password)` rejects out-of-range lengths (SSID 1-32, password 8-63 — WPA2 PSK bounds) **and any control character** (`char::is_control()` catches `\r`/`\n`/NUL), called at the top of the handler before any serial write — fail-closed (`Err` → no bytes sent). Pinned fails-on-old / passes-on-new by `test_validate_wifi_credentials_rejects_injection` (`"net\r\nreboot"`, `"net\ninjected"`, `"pass\r\nerase_nvs"`, embedded NUL — all rejected; would splice into the command stream pre-fix), `test_validate_wifi_credentials_rejects_out_of_range`, and `test_validate_wifi_credentials_accepts_valid` (boundary 32-char SSID / 8- and 63-char passwords still accepted). **WDP-DESK-02 (over-broad shell capability, MODERATE) — REMOVED.** `capabilities/default.json` granted the webview `shell:allow-execute` + `shell:allow-open`, but the Rust backend spawns every process via `std::process::Command` directly (espflash/which/sensing-server — which **bypasses** the Tauri allowlist entirely) and the React UI only ever calls `dialog.open` (file picker) — verified by grep: `tauri_plugin_shell` is `init()`-ed but its `Command`/`open` API is **never invoked from Rust or TS**. The two `shell:` permissions were therefore unused privilege: a webview compromise (e.g. XSS in a UI dep) would have gained **arbitrary host command execution** via `shell.execute` with no scope restriction (no `shell` scope object was even defined). **Fix:** removed both `shell:` permissions from the capability (kept `core:default` + the two `dialog:` perms the UI actually uses). MEASURED: the build-regenerated `gen/schemas/capabilities.json` now reads `"permissions":["core:default","dialog:allow-open","dialog:allow-save"]` (shell perms gone), and the crate still builds + all tests pass — confirming nothing depended on the granted shell scope. (Plugin `init()` + the npm dep left in place to keep the blast radius minimal and avoid touching the off-limits generated ACL manifests; with no permission granted the plugin is inert.) **Dimensions confirmed clean (with evidence):** (1) **No directory-traversal / arbitrary-file primitive crossing the boundary** — the path-taking commands (`flash_firmware`/`verify_firmware`/`ota_update`/`wasm_upload`/`provision_*`) pass the webview-supplied path to `std::fs`/`espflash` to **read a firmware/wasm blob the local user themselves selected via the `dialog.open` native picker**; there is no command that *writes* to or *reads back* an arbitrary attacker-named path to the webview — `settings` read/write is confined to `app_data_dir().join("settings.json")` (fixed filename, no user path component), so no traversal sink exists. (2) **No shell-string interpolation** — every subprocess uses `Command::new(prog).args([...])` (argv vector, no shell), so the `port`/`source`/`chip`/`baud` args cannot inject a second command even though they are unvalidated (the `source` value flows only as a single `--source ` argv element). (3) **No SSRF-to-secret** — the `node_ip`-built URLs (`http://{ip}:8032/...`) target the local ESP32 mesh and return only device status; no credential is returned to the webview. (4) **Panic-on-input** — handlers use `.map_err(|e| e.to_string())?` throughout; the one `srv.pid.expect(...)` in `server_status` is guarded by an explicit `is_none()` early-return on the line above (unreachable), and the discovery/provision deserializers bounds-check before every slice index (`pos + len > data.len()` guards, NVS size capped at 4096). (5) **No hardcoded secrets** — `ota_psk` is an `Option` supplied per-call/from settings, never embedded; grep for embedded keys/tokens over `src/` is empty. (6) **Tauri config** — `tauri.conf.json` ships no `"all": true` / `"$HOME/**"` FS or HTTP scope (no `fs`/`http` plugin enabled at all); the window set is a single fixed main window. `cargo test -p wifi-densepose-desktop --no-default-features`: lib **18 → 21 passed** (+3 validator pins), integration **21 → 21**, 0 failed. Workspace otherwise unchanged; Python deterministic proof unchanged (`f8e76f21a0f9852b70b6d9dd5318239f6b20cbcb4cdd995863263cecdc446f7a`, bit-exact — the desktop crate is off the signal proof path). Both findings warrant **ADR slot 178**. - **`ruview-swarm` beyond-SOTA security + correctness review (ADR-148 drone swarm control plane; needs ADR slot 176) — 4 real fail-open / DoS bugs fixed in the NaN-state-poisoning class, each pinned fails-on-old / passes-on-new; 5 dimensions confirmed clean with evidence.** The shared theme is **IEEE-754 NaN/Inf silently defeating a safety comparison** on data that crosses the untrusted swarm-comm trust boundary (`SwarmOrchestrator::receive_peer_state` / `receive_peer_detection` accept full `DroneState`/`CsiDetection` whose f64/f32 fields deserialize with no finite-check; the integer-encoded MAVLink wire formats in `mavlink_messages.rs` cannot carry NaN, but the serde struct path can). **(1) HIGH — `failsafe::FailSafeMachine::tick` collision-avoidance + battery fail-open** (`failsafe/mod.rs:51,75`). `nearest_neighbor_dist < collision_dist_m` and `battery_pct <= rth_pct` both evaluate `false` for a NaN operand, so a poisoned peer position (→ NaN `nearest_peer_distance` via `Position3D::distance_to`) **silently disabled collision avoidance** and a NaN battery reading kept a drone Nominal — the worst failure for a physical airframe. Fixed to fail CLOSED (`!is_finite() ||` → `EmergencyDiverge` / `ReturnToHome`). MEASURED fails-on-old: `test_nan_neighbor_distance_fails_closed_to_diverge` / `test_nan_battery_fails_closed_to_rth` both returned `Nominal` pre-fix. **(2) MEDIUM — `security::geofence::Geofence::check` NaN-altitude bypass** (`security/geofence.rs:33`). A NaN `z` (altitude) with valid x/y skipped the altitude breach (`NaN < min || NaN > max` = `false`) and returned **`Safe`** through the point-in-polygon path — a silent geofence bypass. Fixed with a leading non-finite-coordinate → `HardBreach` guard. MEASURED fails-on-old: `test_nan_altitude_fails_closed` returned `Safe` pre-fix. **(3) MEDIUM/DoS — `security::antijamming::FhssRadio` `% 0` panic on empty `channels_mhz`** (`security/antijamming.rs:65,71,102`). `FhssConfig` is `Deserialize`; an empty channel list (malformed/hostile config) made `next_hop`/`current_channel_mhz`/`evasive_hop`/`tick` panic with `remainder with a divisor of zero`, crashing the radio task. Fixed with `len == 0` early-returns (benign `0.0` sentinel). MEASURED fails-on-old: `test_empty_channels_does_not_panic` **panicked** (`divisor of zero`) pre-fix. **(4) LOW — `sensing::multiview::MultiViewFusion::fuse` NaN victim-position propagation** (`sensing/multiview.rs:70`). A NaN `victim_position` passed the `is_some()` filter and propagated through the confidence-weighted average into the fused "confirmed victim" location dispatched to the swarm. Fixed by requiring finite `confidence` + finite position components (fail-closed drop). MEASURED fails-on-old: `test_nan_victim_position_dropped_from_fusion` produced a non-finite fused position pre-fix. **Dimensions confirmed clean (with evidence):** (a) **MAVLink decode panic-safety** — `SwarmNodeState::decode(&[u8;20])` `try_into().unwrap()`s are over fixed const ranges of a fixed-size array (provably infallible; no arbitrary-length `&[u8]` path exists). (b) **UWB GPS anti-spoofing is NaN-safe** — `(gps_dist - uwb_dist).abs() <= tol` already fails CLOSED on a NaN range/position (counts as inconsistent → spoof rejected), verified by reasoning + existing `test_spoofed_gps_invalid`. (c) **Bounded grid / no allocation-from-length-field** — `ProbabilityGrid::update_bayesian`/`mark_scanned` bounds-check `cx >= width || cy >= height`; `pos_to_cell` uses saturating `as u32` (Rust `as` saturates, no UB). (d) **Mesh `nearest_k` NaN-safe sort** — `partial_cmp(..).unwrap_or(Equal)` cannot panic on NaN distances. (e) **No hardcoded secrets** — `MavlinkSigner` key is constructor-injected (`[u8;32]`), nothing embedded. **Documented-not-fixed (for ADR-176, not churned to avoid test-rewrite risk):** (i) **Raft `AppendEntries` lacks the Log-Matching consistency check** (`topology/raft.rs:187`) — a follower appends leader entries on `term >= current_term` without validating `prev_log_index`/`prev_log_term`, so a malformed/byzantine leader can corrupt a follower's log (a genuine consensus-safety gap; vote tallying is also delegated to the caller per the existing `handle_message` comment). (ii) **`MavlinkSigner::verify` uses a non-constant-time tag `==` and has no replay/timestamp-window rejection** (`security/mavlink_signing.rs:64`) — the doc comment already flags the replay limitation as a known demo/test simplification. `cargo test -p ruview-swarm --no-default-features`: **117 → 123 passed, 0 failed** (+6 pins). Workspace green; Python deterministic proof unchanged (`f8e76f21a0f9852b70b6d9dd5318239f6b20cbcb4cdd995863263cecdc446f7a`, bit-exact — `ruview-swarm` is off the signal proof path). - **`nvsim` (ADR-089 NV-diamond magnetometer simulator) beyond-SOTA security review — two real degenerate-input findings fixed (config-induced panic/DoS + NaN-state-poisoning silent-corruption), each pinned by a fails-on-old test; determinism integrity, panic-free deserialisation, and RNG-seeding confirmed clean with evidence. Needs ADR slot 177.** Beyond-SOTA review of the standalone WASM-ready forward-only pipeline (`scene → source → propagation → NV ensemble → digitiser → MagFrame + SHA-256 witness`). The real risk surface is degenerate physical-parameter input (the scene + config are the external boundary, especially via the WASM `config_json`/`scene_json` entry points). **Finding 1 — NVSIM-DT-01 (config-induced panic / DoS, MEDIUM; `pipeline.rs:58,95`).** `dt` was derived as `config.dt_s.unwrap_or(1.0 / f_s_hz)`; an externally-supplied `f_s_hz == 0.0` makes `dt == +Inf`, `(dt*1e6) as u64` saturates to `u64::MAX`, and `(sample as u64) * dt_us` then **panics `attempt to multiply with overflow`** for `sample >= 2` (MEASURED: probe panicked at `pipeline.rs:95:30` under the debug/test profile; in `panic=abort` WASM this aborts the module, in release it silently wraps `t_us` to garbage). **Fixed** by sanitising `dt` (non-finite/non-positive → 1 µs fallback), capping the `u64` cast at `u64::MAX`, and using `saturating_mul` for the timestamp so no config can ever overflow it. **Finding 2 — NVSIM-NAN-01 (NaN-state-poisoning silent corruption, MEDIUM; funnel at `digitiser.rs::adc_quantise`).** A non-finite scene parameter (e.g. a `NaN`/`Inf` dipole **position**, `Inf` **moment**, or `NaN` loop **radius**) flows through `scene_field_at` and **bypasses the near-field clamp** — `NaN < R_MIN_M` is `false`, so the `1/r³` path is taken and produces a `NaN`/`Inf` field (MEASURED: `b=[NaN,NaN,NaN], sat=false`). At the ADC that non-finite value hit the `else` branch and **`NaN as i32 == 0`** (Rust saturating cast), emitting a frame with `b_pt=[0,0,0]` and **the `ADC_SATURATED` flag CLEAR** — a frame *indistinguishable from a legitimate zero-field reading* (MEASURED: `b_pt=[0.0,0.0,0.0] flags=0b0000`). This is the same NaN-poisoning class flagged across the calibration/vitals crates; the `propagation` module already guards its NaN paths, but the source→ADC path did not. **Fixed** at the single funnel point: `adc_quantise` now treats any non-finite input as out-of-range → clamps to code `0` **and raises the saturation flag**, so the corruption is visible downstream (the pipeline's existing `adc_sat` OR-reduction propagates `ADC_SATURATED` onto the frame). **Dimensions confirmed clean (with evidence):** (1) **Determinism integrity — clean.** The only RNG is `ChaCha20Rng::seed_from_u64(seed)` fully seeded from the caller's `u64` (grep: one `seed_from_u64`, zero `thread_rng`/`getrandom`/`SystemTime`/`Instant`/`HashMap`); Cargo.toml pins `rand`/`rand_chacha` with `default-features=false` (no OS-entropy path). Box–Muller draws from `gen_range(f64::EPSILON..=1.0)` (avoids `ln(0) = -Inf` by construction). Frame serialisation is fixed little-endian; source summation order is fixed by `Vec` order. The published cross-machine witness `cc8de9b0…93b4` (`proof::tests::proof_witness_publishes_a_known_value`) **still passes unchanged** after both fixes — the happy-path output is byte-identical, confirming the guards only affect degenerate inputs. (Attested caveat, not a finding: libm `cos`/`ln`/`sqrt` *could* differ x86↔wasm; witness is documented as x86_64-captured.) (2) **Panic-free deserialisation — clean.** `MagFrame::from_bytes` validates `len`/magic/version, then the per-field `buf[a..b].try_into().expect(...)` calls are over **fixed sub-ranges of an already-length-checked 60-byte buffer** → provably infallible, not reachable panic vectors. No `unsafe`, no `panic!`/`unreachable!` in production code; every other `unwrap`/`expect` is `#[cfg(test)]`. (3) **Div-by-zero — clean.** `dipole_field`/`current_loop_field` clamp `r_norm < R_MIN_M` (1 mm) before the `1/r³`/`1/r²` divide (finite inputs); `shot_noise_floor` guards `denom <= 0.0 → f64::INFINITY`; `vec3_normalise` guards `n < 1e-20`. (The only gap was the NaN case that *bypasses* the `r_norm` clamp — fixed at the ADC funnel above.) **Pinning tests (fails-on-old / passes-on-new, MEASURED):** `pipeline::degenerate_zero_sample_rate_does_not_panic` (panicked on old code; now finite frames), `pipeline::non_finite_scene_input_flags_frame_instead_of_silently_zeroing` (old: `flags=0b0000`; now `ADC_SATURATED` set, `b_pt` finite), `digitiser::adc_quantise_flags_non_finite_as_saturated` (old: `(0,false)` for NaN; now `(0,true)`). `cargo test -p nvsim --no-default-features`: **50 → 53 passed, 0 failed**. Workspace green; Python deterministic proof unchanged (`f8e76f21…46f7a`, bit-exact — nvsim is off the signal proof path). Needs ADR slot 177. - **`wifi-densepose-core` + `wifi-densepose-cli` beyond-SOTA security review (ADR-127 note; CLI needs ADR slot) — NaN-state-poisoning bug class does NOT originate in core (verdict: no + evidence); both crates confirmed clean on all reviewed dimensions; 4 regression pins added locking in two real DoS guards.** **Load-bearing question — verdict NO (with evidence, MEASURED).** The NaN-state-poisoning class that hit `wifi-densepose-calibration`/`-vitals`/`-geo` (a non-finite input latching into a persistent IIR/Welford/von-Mises/voxel accumulator → silent permanent feature failure) does **not** live in a shared `wifi-densepose-core` primitive: core exposes **no stateful accumulator at all** — no Welford/running-mean, no von-Mises/circular-mean, no IIR/biquad filter state, no voxel grid. Grep over `core/src` for `welford|von_mises|biquad|y1|y2|running_mean|accumulat|voxel|self.*+=` matched only the `InvalidState` *error* enum, "reset state" doc comments, and a test-only LCG — zero stateful logic (MEASURED). Each downstream crate rolls its own accumulator, so each fix is correctly local; corroborated by `wifi-densepose-calibration::Features::from_series`, which **already** filters non-finite samples and returns `Features::ZERO` (the downstream re-implementation of the fix). The only float math in core's hot path is construction-time projection (`CsiFrame::new` → `amplitude`/`phase` via `mapv`) and pure stateless `utils` functions — none persists state across frames. **Dimensions confirmed clean (with evidence):** (1) **panic-on-adversarial-input = 0** — `CsiFrame::from_canonical_bytes` (the replay/forward deserialisation boundary) returns a typed `CanonicalDecodeError` for every malformed input (truncation, bad discriminant, non-UTF-8 device id, nonzero reserved bytes, shape/payload mismatch, trailing bytes); the CLI UDP parser `parse_csi_packet` (the widest CLI attack surface — bound to `0.0.0.0` by default) returns `None` on any malformed datagram. Both proven panic-free over deterministic-LCG fuzz sweeps (new pins). (2) **`Confidence::new` rejects NaN** (`!(0.0..=1.0).contains(&NaN)` ⇒ `true` ⇒ `Err`); `compute_bounding_box`/`to_flat_array` are NaN-tolerant (f32 `min`/`max` ignore NaN). (3) **`amplitude_variance`/`mean_amplitude` panic-free on empty frames** — ndarray 0.17 `var(0.0)`/`mean()` return finite/`None` (handled), not a panic (MEASURED via throwaway probe). (4) **Unbounded-memory DoS bounded** in both deserialisers: `from_canonical_bytes` guards the `Vec::with_capacity(rows*cols)` with `rows.saturating_mul(cols).saturating_mul(16) <= bytes.len()`; `parse_csi_packet` guards `Array2::zeros` with `buf.len() < 20 + n_pairs*2` — a header lying about an enormous `rows×cols` / `n_antennas×n_subcarriers` is rejected before allocation. (5) **CLI path-traversal** in `calibrate-serve` already defended by `sanitize_room_id` (keeps `[A-Za-z0-9_-]`, caps 64 chars, with tests) on every client-supplied `room_id`/`bank`/`baseline` name that reaches a file path; bearer-auth gate + non-loopback-bind warning present. (6) **No hardcoded secrets** (`--token` read from `CALIBRATE_TOKEN` env, never embedded). **Regression pins added (fails-on-old / passes-on-new):** core `canonical_decode_oversized_shape_is_bounded_not_allocated` (MEASURED: with the saturating guard removed it panics `capacity overflow` at `types.rs:801`; passes with the guard) and `canonical_decode_never_panics_on_arbitrary_bytes`; CLI `test_parse_csi_packet_oversized_claim_is_rejected_not_allocated` (a 255×65535-pair claim ≈ 33 MB in a 2 KB datagram → `None`, never OOMs) and `test_parse_csi_packet_never_panics_on_arbitrary_bytes`. `cargo test -p wifi-densepose-core`: **35 → 37** lib tests, 0 failed; `cargo test -p wifi-densepose-cli --no-default-features`: **24 → 26**, 0 failed. Workspace green; Python deterministic proof unchanged (`f8e76f21…46f7a`, bit-exact — core/cli are off the signal proof path). No production code changed — review is clean-with-evidence plus pins. diff --git a/docs/adr/ADR-178-desktop-ipc-injection-and-capability-least-privilege.md b/docs/adr/ADR-178-desktop-ipc-injection-and-capability-least-privilege.md new file mode 100644 index 00000000..d57e3286 --- /dev/null +++ b/docs/adr/ADR-178-desktop-ipc-injection-and-capability-least-privilege.md @@ -0,0 +1,87 @@ +# ADR-178: `wifi-densepose-desktop` IPC Injection Fix + Capability Least-Privilege + +| Field | Value | +|-------|-------| +| **Status** | Accepted — 2 real MODERATE bugs fixed + pinned (MEASURED on Windows) | +| **Date** | 2026-06-15 | +| **Deciders** | ruv | +| **Codename** | **DESK-LOCKDOWN** | +| **Reviews** | `wifi-densepose-desktop` (Tauri v2 desktop app) | +| **Milestone** | #9 (ungated-crate security sweep) — crate 3 of 4 | + +## Context + +`wifi-densepose-desktop` is the Tauri v2 desktop app (ESP32 discovery, firmware +flashing, OTA, provisioning, server control). The real attack surface is the +**Tauri IPC boundary** — `#[tauri::command]` handlers that take arguments from the +webview/JS — and the **capability/allowlist scope**. The crate **builds and tests +on Windows** (Tauri 2.10.3, webview2 path, no GTK), so both findings are MEASURED, +not source-analysis-only. + +## Decision + +Fix the two real findings; attest the rest of the surface clean with evidence. + +### Findings fixed (both MEASURED) + +| # | Severity | Location | Issue | Fix | +|---|----------|----------|-------|-----| +| WDP-DESK-01 | MODERATE | `src/commands/discovery.rs:438` (`configure_esp32_wifi`) | Webview-supplied `ssid`/`password` are concatenated into newline-terminated serial commands (`wifi_config {} {}\r\n`, `set ssid {}\r\n`) with **no validation** → a `\r\n` in either field **injects an arbitrary follow-up firmware command** (`reboot`, `erase_nvs`) across the IPC trust boundary. | `validate_wifi_credentials()` — WPA2 length bounds (SSID 1–32, password 8–63) **+ reject all control chars** (`char::is_control()`), called fail-closed before any serial write. | +| WDP-DESK-02 | MODERATE | `capabilities/default.json:7-8` | `shell:allow-execute` + `shell:allow-open` granted to the webview but **unused** (Rust spawns via `std::process::Command`; the UI uses only `dialog.open`). A webview compromise (a UI-dependency XSS) → arbitrary **unscoped host command execution**. | Removed both `shell:` permissions (kept `core:default` + the two in-use `dialog:` perms); regenerated `gen/schemas/capabilities.json` now asserts `["core:default","dialog:allow-open","dialog:allow-save"]`. | + +Both are MODERATE (not HIGH): each requires a webview compromise or a malicious +local caller to weaponize. The unifying lesson is **least privilege at the IPC +boundary** — validate every webview-supplied argument that reaches a serial/FS/ +process sink, and grant only the capabilities actually exercised. + +### Tauri-command + capability audit (every handler) + +All 30+ command handlers were mapped. Only `configure_esp32_wifi` lacked input +validation on a string that reached a command sink (WDP-DESK-01). Every +subprocess uses `Command::new(prog).args([...])` (argv vector — no shell-string +interpolation), so `port`/`source`/`chip`/`baud` cannot inject a second command +even unvalidated. `tauri.conf.json` ships **no** `fs`/`http` plugin and **no** +`"all":true`/`"$HOME/**"` scope; after WDP-DESK-02 the allowlist is minimal. + +### Dimensions confirmed clean (with evidence) + +1. **Directory traversal / arbitrary file** — path args (`firmware_path`/`wasm_path`) + are blobs the local user selects via the native `dialog.open` picker; settings + I/O is a fixed filename under `app_data_dir`. No attacker-named path sink. +2. **Shell-string injection** — every subprocess is an argv vector; grep found no + shell-string interpolation anywhere. +3. **SSRF-to-secret** — `node_ip`-built URLs target the local ESP32 mesh and return + only device status JSON; no credential returned to the webview. +4. **Panic-on-input** — handlers use `.map_err(|e| e.to_string())?`; the one + `expect` is guarded by an `is_none()` early-return; provision/discovery + deserializers bounds-check every slice index (NVS size capped ≤ 4096). +5. **Hardcoded secrets** — `ota_psk` is a per-call `Option`, never embedded; + grep for embedded keys/tokens over `src/` is empty. +6. **Shell plugin genuinely unused** — `tauri_plugin_shell` is `init()`-ed but its + `Command`/`open` API is never invoked from Rust or the TS UI (which imports only + `@tauri-apps/plugin-dialog`) — confirming WDP-DESK-02 is safe to remove. + +## Validation + +- `cargo check -p wifi-densepose-desktop --no-default-features` → `Finished` (Windows, MEASURED). +- `cargo test -p wifi-densepose-desktop --no-default-features` → lib **18 → 21** (+3 validator pins: + `test_validate_wifi_credentials_rejects_injection` / `_rejects_out_of_range` / `_accepts_valid`), + integration 21/21, **0 failed**. +- Capability narrowing MEASURED: regenerated `capabilities.json` permission set verified. +- `python archive/v1/data/proof/verify.py` → **VERDICT: PASS**, hash `f8e76f21…46f7a` + unchanged (desktop off the signal proof path). + +## Consequences + +### Positive +- An IPC serial-command-injection path and an over-broad shell capability are + closed in the desktop app, each pinned / verified, with the rest of the + 30-command IPC surface attested clean. + +### Negative / Neutral +- None. The removed shell capability was unused; the validator rejects only + malformed/hostile credentials. + +## Links +- ADR-176 / ADR-177 — sibling Milestone-#9 reviews (ruview-swarm, nvsim) +- ADR-172 — core/cli review diff --git a/v2/crates/wifi-densepose-desktop/capabilities/default.json b/v2/crates/wifi-densepose-desktop/capabilities/default.json index 787161b1..f45107f9 100644 --- a/v2/crates/wifi-densepose-desktop/capabilities/default.json +++ b/v2/crates/wifi-densepose-desktop/capabilities/default.json @@ -4,8 +4,6 @@ "windows": ["main"], "permissions": [ "core:default", - "shell:allow-execute", - "shell:allow-open", "dialog:allow-open", "dialog:allow-save" ] diff --git a/v2/crates/wifi-densepose-desktop/gen/schemas/capabilities.json b/v2/crates/wifi-densepose-desktop/gen/schemas/capabilities.json index af972373..2ce561a8 100644 --- a/v2/crates/wifi-densepose-desktop/gen/schemas/capabilities.json +++ b/v2/crates/wifi-densepose-desktop/gen/schemas/capabilities.json @@ -1 +1 @@ -{"default":{"identifier":"default","description":"RuView default capability set","local":true,"windows":["main"],"permissions":["core:default","shell:allow-execute","shell:allow-open","dialog:allow-open","dialog:allow-save"]}} \ No newline at end of file +{"default":{"identifier":"default","description":"RuView default capability set","local":true,"windows":["main"],"permissions":["core:default","dialog:allow-open","dialog:allow-save"]}} \ No newline at end of file diff --git a/v2/crates/wifi-densepose-desktop/src/commands/discovery.rs b/v2/crates/wifi-densepose-desktop/src/commands/discovery.rs index 4608590e..616aa659 100644 --- a/v2/crates/wifi-densepose-desktop/src/commands/discovery.rs +++ b/v2/crates/wifi-densepose-desktop/src/commands/discovery.rs @@ -430,6 +430,35 @@ fn is_esp32_compatible(vid: u16, pid: u16) -> bool { false } +/// Validate WiFi credentials before they are interpolated into a +/// newline-delimited serial command protocol. +/// +/// The ESP32 firmware accepts line-oriented commands such as +/// `wifi_config \r\n`. Because the SSID and password +/// arrive from the webview (untrusted) and are concatenated directly into +/// those command strings, a control character (`\r`, `\n`, or NUL) embedded +/// in either field would let a malicious caller terminate the current line +/// early and inject an arbitrary follow-up command (e.g. `reboot`, `erase`). +/// +/// Enforce the IEEE 802.11 / WPA2 bounds and reject any control characters: +/// - SSID: 1-32 bytes, no control characters +/// - Password: 8-63 bytes (WPA2 PSK ASCII range), no control characters +fn validate_wifi_credentials(ssid: &str, password: &str) -> Result<(), String> { + if ssid.is_empty() || ssid.len() > 32 { + return Err("SSID must be 1-32 characters".into()); + } + if password.len() < 8 || password.len() > 63 { + return Err("WiFi password must be 8-63 characters".into()); + } + if ssid.chars().any(|c| c.is_control()) { + return Err("SSID must not contain control characters".into()); + } + if password.chars().any(|c| c.is_control()) { + return Err("WiFi password must not contain control characters".into()); + } + Ok(()) +} + /// Configure WiFi credentials on an ESP32 via serial port. /// /// Sends WiFi credentials to the ESP32 using a simple serial protocol. @@ -443,6 +472,10 @@ pub async fn configure_esp32_wifi( use std::io::{Read, Write}; use std::time::Duration; + // Reject control characters / out-of-range lengths before the credentials + // are spliced into the line-oriented serial command protocol below. + validate_wifi_credentials(&ssid, &password)?; + tracing::info!("Configuring WiFi on port: {}", port); // Open serial port @@ -549,6 +582,37 @@ mod tests { assert_eq!(node.tdm_total, Some(4)); } + #[test] + fn test_validate_wifi_credentials_accepts_valid() { + assert!(validate_wifi_credentials("MyNetwork", "password123").is_ok()); + // Boundary: 32-char SSID, 63-char password are allowed. + assert!(validate_wifi_credentials(&"A".repeat(32), &"B".repeat(63)).is_ok()); + // Boundary: 8-char password (WPA2 minimum) is allowed. + assert!(validate_wifi_credentials("net", "12345678").is_ok()); + } + + #[test] + fn test_validate_wifi_credentials_rejects_injection() { + // Newline/CR in SSID would terminate the serial command line early and + // let the caller inject a follow-up firmware command. Must be rejected. + assert!(validate_wifi_credentials("net\r\nreboot", "password123").is_err()); + assert!(validate_wifi_credentials("net\ninjected", "password123").is_err()); + // Same vector via the password field. + assert!(validate_wifi_credentials("net", "pass\r\nerase_nvs").is_err()); + // Embedded NUL. + assert!(validate_wifi_credentials("net", "pass\0word1").is_err()); + } + + #[test] + fn test_validate_wifi_credentials_rejects_out_of_range() { + // Empty / over-length SSID. + assert!(validate_wifi_credentials("", "password123").is_err()); + assert!(validate_wifi_credentials(&"A".repeat(33), "password123").is_err()); + // Too-short / too-long password (WPA2 PSK bounds). + assert!(validate_wifi_credentials("net", "short").is_err()); + assert!(validate_wifi_credentials("net", &"B".repeat(64)).is_err()); + } + #[test] fn test_is_esp32_compatible() { // CP2102