diff --git a/docs/adr/ADR-050-quality-engineering-security-hardening.md b/docs/adr/ADR-050-quality-engineering-security-hardening.md new file mode 100644 index 00000000..c37145eb --- /dev/null +++ b/docs/adr/ADR-050-quality-engineering-security-hardening.md @@ -0,0 +1,100 @@ +# ADR-050: Quality Engineering Response — Security Hardening & Code Quality + +| Field | Value | +|-------|-------| +| Status | Accepted | +| Date | 2026-03-06 | +| Deciders | ruv | +| Depends on | ADR-032 (Multistatic Mesh Security) | +| Issue | [#170](https://github.com/ruvnet/wifi-densepose/issues/170) | + +## Context + +An independent quality engineering analysis ([issue #170](https://github.com/ruvnet/wifi-densepose/issues/170)) identified 7 critical findings across the Rust codebase. After verification against the source code, the following findings are confirmed and require action: + +### Confirmed Critical Findings + +| # | Finding | Location | Verified | +|---|---------|----------|----------| +| 1 | Fake HMAC in `secure_tdm.rs` — XOR fold with hardcoded key | `hardware/src/esp32/secure_tdm.rs:253` | YES — comments say "sufficient for testing" | +| 2 | `sensing-server/main.rs` is 3,741 lines — CC=65, god object | `sensing-server/src/main.rs` | YES — confirmed 3,741 lines | +| 3 | WebSocket server has zero authentication | Rust WS codebase | YES — no auth/token checks found | +| 4 | Zero security tests in Rust codebase | Entire workspace | YES — no auth/injection/tampering tests | +| 5 | 54K fps claim has no supporting benchmark | No criterion benchmarks | YES — no benchmarks exist | + +### Findings Requiring Further Investigation + +| # | Finding | Status | +|---|---------|--------| +| 6 | Unauthenticated OTA firmware endpoint | Not found in Rust code — may be ESP32 C firmware level | +| 7 | WASM upload without mandatory signatures | Needs review of WASM loader | +| 8 | O(n^2) autocorrelation in heart rate detection | Needs profiling to confirm impact | + +## Decision + +Address findings in 3 priority sprints as recommended by the report. + +### Sprint 1: Security (Blocks Deployment) + +1. **Replace fake HMAC with real HMAC-SHA256** in `secure_tdm.rs` + - Use the `hmac` + `sha2` crates (already in `Cargo.lock`) + - Remove XOR fold implementation + - Add key derivation (no more hardcoded keys) + +2. **Add WebSocket authentication** + - Token-based auth on WS upgrade handshake + - Optional API key for local-network deployments + - Configurable via environment variable + +3. **Add security test suite** + - Auth bypass attempts + - Malformed CSI frame injection + - Protocol tampering (TDM beacon replay, nonce reuse) + +### Sprint 2: Code Quality & Testability + +4. **Decompose `main.rs`** (3,741 lines -> ~14 focused modules) + - Extract HTTP routes, WebSocket handler, CSI pipeline, config, state + - Target: no file over 500 lines + +5. **Add criterion benchmarks** + - CSI frame parsing throughput + - Signal processing pipeline latency + - WebSocket broadcast fanout + +### Sprint 3: Functional Verification + +6. **Vital sign accuracy verification** + - Reference signal tests with known BPM + - False-negative rate measurement + +7. **Fix O(n^2) autocorrelation** (if confirmed by profiling) + - Replace brute-force lag with FFT-based autocorrelation + +## Consequences + +### Positive + +- Addresses all critical security findings before any production deployment +- `main.rs` decomposition enables unit testing of server components +- Criterion benchmarks provide verifiable performance claims +- Security test suite prevents regression + +### Negative + +- Sprint 1 security changes are breaking for any existing TDM mesh deployments (fake HMAC -> real HMAC requires firmware update) +- `main.rs` decomposition is a large refactor with merge conflict risk + +### Neutral + +- The report correctly identifies that life-safety claims (disaster detection, vital signs) require rigorous verification — this is an ongoing process, not a single sprint + +## Acknowledgment + +Thanks to [@proffesor-for-testing](https://github.com/proffesor-for-testing) for the thorough 10-report analysis. The full report is archived at the [original gist](https://gist.github.com/proffesor-for-testing/02321e3f272720aa94484fffec6ab19b). + +## References + +- Issue #170: Quality Engineering Analysis +- ADR-032: Multistatic Mesh Security Hardening +- ADR-028: ESP32 Capability Audit diff --git a/firmware/esp32-csi-node/main/ota_update.c b/firmware/esp32-csi-node/main/ota_update.c index 9f486f0c..5b920154 100644 --- a/firmware/esp32-csi-node/main/ota_update.c +++ b/firmware/esp32-csi-node/main/ota_update.c @@ -15,6 +15,8 @@ #include "esp_ota_ops.h" #include "esp_http_server.h" #include "esp_app_desc.h" +#include "nvs_flash.h" +#include "nvs.h" static const char *TAG = "ota_update"; @@ -24,6 +26,52 @@ static const char *TAG = "ota_update"; /** Maximum firmware size (900 KB — matches CI binary size gate). */ #define OTA_MAX_SIZE (900 * 1024) +/** NVS namespace and key for the OTA pre-shared key. */ +#define OTA_NVS_NAMESPACE "security" +#define OTA_NVS_KEY "ota_psk" + +/** Maximum PSK length (hex-encoded SHA-256). */ +#define OTA_PSK_MAX_LEN 65 + +/** Cached PSK loaded from NVS at init time. Empty = auth disabled. */ +static char s_ota_psk[OTA_PSK_MAX_LEN] = {0}; + +/** + * ADR-050: Verify the Authorization header contains the correct PSK. + * Returns true if auth is disabled (no PSK provisioned) or if the + * Bearer token matches the stored PSK. + */ +static bool ota_check_auth(httpd_req_t *req) +{ + if (s_ota_psk[0] == '\0') { + /* No PSK provisioned — auth disabled (permissive for dev). */ + return true; + } + + char auth_header[128] = {0}; + if (httpd_req_get_hdr_value_str(req, "Authorization", auth_header, + sizeof(auth_header)) != ESP_OK) { + return false; + } + + /* Expect "Bearer " */ + const char *prefix = "Bearer "; + if (strncmp(auth_header, prefix, strlen(prefix)) != 0) { + return false; + } + + const char *token = auth_header + strlen(prefix); + /* Constant-time comparison to prevent timing attacks. */ + size_t psk_len = strlen(s_ota_psk); + size_t tok_len = strlen(token); + if (psk_len != tok_len) return false; + volatile uint8_t result = 0; + for (size_t i = 0; i < psk_len; i++) { + result |= (uint8_t)(s_ota_psk[i] ^ token[i]); + } + return result == 0; +} + /** * GET /ota/status — return firmware version and partition info. */ @@ -53,6 +101,14 @@ static esp_err_t ota_status_handler(httpd_req_t *req) */ static esp_err_t ota_upload_handler(httpd_req_t *req) { + /* ADR-050: Authenticate before accepting firmware upload. */ + if (!ota_check_auth(req)) { + ESP_LOGW(TAG, "OTA upload rejected: authentication failed"); + httpd_resp_send_err(req, HTTPD_403_FORBIDDEN, + "Authentication required. Use: Authorization: Bearer "); + return ESP_FAIL; + } + ESP_LOGI(TAG, "OTA update started, content_length=%d", req->content_len); if (req->content_len <= 0 || req->content_len > OTA_MAX_SIZE) { @@ -187,6 +243,20 @@ static esp_err_t ota_start_server(httpd_handle_t *out_handle) esp_err_t ota_update_init(void) { + /* ADR-050: Load OTA PSK from NVS if provisioned. */ + nvs_handle_t nvs; + if (nvs_open(OTA_NVS_NAMESPACE, NVS_READONLY, &nvs) == ESP_OK) { + size_t len = sizeof(s_ota_psk); + if (nvs_get_str(nvs, OTA_NVS_KEY, s_ota_psk, &len) == ESP_OK) { + ESP_LOGI(TAG, "OTA PSK loaded from NVS (%d chars) — authentication enabled", (int)len - 1); + } else { + ESP_LOGW(TAG, "No OTA PSK in NVS — OTA authentication DISABLED (provision with nvs_set)"); + } + nvs_close(nvs); + } else { + ESP_LOGW(TAG, "NVS namespace '%s' not found — OTA authentication DISABLED", OTA_NVS_NAMESPACE); + } + return ota_start_server(NULL); } diff --git a/firmware/esp32-csi-node/main/wasm_upload.c b/firmware/esp32-csi-node/main/wasm_upload.c index 2387279a..66a7ec2f 100644 --- a/firmware/esp32-csi-node/main/wasm_upload.c +++ b/firmware/esp32-csi-node/main/wasm_upload.c @@ -107,8 +107,9 @@ static esp_err_t wasm_upload_handler(httpd_req_t *req) return ESP_FAIL; } - /* Verify signature if wasm_verify is enabled. */ -#ifdef CONFIG_WASM_VERIFY_SIGNATURE + /* ADR-050: Verify signature (default-on; skip only if + * CONFIG_WASM_SKIP_SIGNATURE is explicitly set for dev/lab). */ +#ifndef CONFIG_WASM_SKIP_SIGNATURE { /* Load pubkey from NVS config (set via provision.py --wasm-pubkey). */ extern nvs_config_t g_nvs_config; @@ -173,11 +174,11 @@ static esp_err_t wasm_upload_handler(httpd_req_t *req) } else if (rvf_is_raw_wasm(buf, (uint32_t)total)) { /* ── Raw WASM path (dev/lab only) ── */ -#ifdef CONFIG_WASM_VERIFY_SIGNATURE +#ifndef CONFIG_WASM_SKIP_SIGNATURE free(buf); httpd_resp_send_err(req, HTTPD_403_FORBIDDEN, - "Raw WASM upload rejected (wasm_verify enabled). " - "Use RVF container with signature."); + "Raw WASM upload rejected (signature verification enabled). " + "Use RVF container with signature, or set CONFIG_WASM_SKIP_SIGNATURE for dev."); return ESP_FAIL; #else format = "raw"; diff --git a/rust-port/wifi-densepose-rs/Cargo.lock b/rust-port/wifi-densepose-rs/Cargo.lock index 0cc03b95..ff83c0c1 100644 --- a/rust-port/wifi-densepose-rs/Cargo.lock +++ b/rust-port/wifi-densepose-rs/Cargo.lock @@ -4579,10 +4579,12 @@ dependencies = [ "chrono", "clap", "criterion", + "hmac", "midstreamer-quic", "midstreamer-scheduler", "serde", "serde_json", + "sha2", "thiserror 1.0.69", "tokio", "tracing", diff --git a/rust-port/wifi-densepose-rs/Cargo.toml b/rust-port/wifi-densepose-rs/Cargo.toml index 24ab2e7e..4e196293 100644 --- a/rust-port/wifi-densepose-rs/Cargo.toml +++ b/rust-port/wifi-densepose-rs/Cargo.toml @@ -101,7 +101,7 @@ csv = "1.3" indicatif = "0.17" # CLI -clap = { version = "4.4", features = ["derive"] } +clap = { version = "4.4", features = ["derive", "env"] } # Testing criterion = { version = "0.5", features = ["html_reports"] } diff --git a/rust-port/wifi-densepose-rs/crates/wifi-densepose-hardware/Cargo.toml b/rust-port/wifi-densepose-rs/crates/wifi-densepose-hardware/Cargo.toml index a198b777..7f5617ea 100644 --- a/rust-port/wifi-densepose-rs/crates/wifi-densepose-hardware/Cargo.toml +++ b/rust-port/wifi-densepose-rs/crates/wifi-densepose-hardware/Cargo.toml @@ -24,6 +24,9 @@ linux-wifi = [] [dependencies] # CLI argument parsing (for bin/aggregator) clap = { version = "4.4", features = ["derive"] } +# Cryptographic HMAC (ADR-050: replace fake XOR-fold HMAC) +hmac = "0.12" +sha2 = "0.10" # Byte parsing byteorder = "1.5" # Time diff --git a/rust-port/wifi-densepose-rs/crates/wifi-densepose-hardware/src/esp32/secure_tdm.rs b/rust-port/wifi-densepose-rs/crates/wifi-densepose-hardware/src/esp32/secure_tdm.rs index afbedc03..3a605d1a 100644 --- a/rust-port/wifi-densepose-rs/crates/wifi-densepose-hardware/src/esp32/secure_tdm.rs +++ b/rust-port/wifi-densepose-rs/crates/wifi-densepose-hardware/src/esp32/secure_tdm.rs @@ -33,9 +33,13 @@ use super::quic_transport::{ QuicTransportHandle, QuicTransportError, SecurityMode, }; use super::tdm::{SyncBeacon, TdmCoordinator, TdmSchedule, TdmSlotCompleted}; +use hmac::{Hmac, Mac}; +use sha2::Sha256; use std::collections::VecDeque; use std::fmt; +type HmacSha256 = Hmac; + // --------------------------------------------------------------------------- // Constants // --------------------------------------------------------------------------- @@ -245,19 +249,17 @@ impl AuthenticatedBeacon { }) } - /// Compute the expected HMAC tag for this beacon using the given key. + /// Compute the HMAC-SHA256 tag for this beacon, truncated to 8 bytes. /// - /// Uses a simplified HMAC approximation for testing. In production, - /// this calls mbedtls HMAC-SHA256 via the ESP-IDF hardware accelerator - /// or the `sha2` crate on aggregator nodes. + /// Uses the `hmac` + `sha2` crates for cryptographically secure + /// message authentication (ADR-050, Sprint 1). pub fn compute_tag(payload_and_nonce: &[u8], key: &[u8; 16]) -> [u8; HMAC_TAG_SIZE] { - // Simplified HMAC: XOR key into payload hash. In production, use - // real HMAC-SHA256 from sha2 crate. This is sufficient for - // testing the protocol structure. + let mut mac = HmacSha256::new_from_slice(key) + .expect("HMAC-SHA256 accepts any key length"); + mac.update(payload_and_nonce); + let result = mac.finalize().into_bytes(); let mut tag = [0u8; HMAC_TAG_SIZE]; - for (i, byte) in payload_and_nonce.iter().enumerate() { - tag[i % HMAC_TAG_SIZE] ^= byte ^ key[i % 16]; - } + tag.copy_from_slice(&result[..HMAC_TAG_SIZE]); tag } @@ -975,6 +977,97 @@ mod tests { assert_eq!(SecLevel::Enforcing as u8, 2); } + // ---- Security tests (ADR-050) ---- + + #[test] + fn test_hmac_different_keys_produce_different_tags() { + let msg = b"test payload with nonce"; + let key1: [u8; 16] = [0x01; 16]; + let key2: [u8; 16] = [0x02; 16]; + let tag1 = AuthenticatedBeacon::compute_tag(msg, &key1); + let tag2 = AuthenticatedBeacon::compute_tag(msg, &key2); + assert_ne!(tag1, tag2, "Different keys must produce different HMAC tags"); + } + + #[test] + fn test_hmac_different_messages_produce_different_tags() { + let key: [u8; 16] = DEFAULT_TEST_KEY; + let tag1 = AuthenticatedBeacon::compute_tag(b"message one", &key); + let tag2 = AuthenticatedBeacon::compute_tag(b"message two", &key); + assert_ne!(tag1, tag2, "Different messages must produce different HMAC tags"); + } + + #[test] + fn test_hmac_is_deterministic() { + let key: [u8; 16] = DEFAULT_TEST_KEY; + let msg = b"determinism test"; + let tag1 = AuthenticatedBeacon::compute_tag(msg, &key); + let tag2 = AuthenticatedBeacon::compute_tag(msg, &key); + assert_eq!(tag1, tag2, "Same key + message must produce identical tags"); + } + + #[test] + fn test_wrong_key_fails_verification() { + let beacon = SyncBeacon { + cycle_id: 42, + cycle_period: Duration::from_millis(50), + drift_correction_us: 0, + generated_at: std::time::Instant::now(), + }; + let correct_key: [u8; 16] = DEFAULT_TEST_KEY; + let wrong_key: [u8; 16] = [0xFF; 16]; + let nonce = 1u32; + + let mut msg = [0u8; 20]; + msg[..16].copy_from_slice(&beacon.to_bytes()); + msg[16..20].copy_from_slice(&nonce.to_le_bytes()); + let tag = AuthenticatedBeacon::compute_tag(&msg, &correct_key); + + let auth = AuthenticatedBeacon { beacon, nonce, hmac_tag: tag }; + assert!(auth.verify(&wrong_key).is_err(), "Wrong key must fail verification"); + } + + #[test] + fn test_single_bit_flip_in_payload_fails_verification() { + let beacon = SyncBeacon { + cycle_id: 42, + cycle_period: Duration::from_millis(50), + drift_correction_us: 0, + generated_at: std::time::Instant::now(), + }; + let key: [u8; 16] = DEFAULT_TEST_KEY; + let nonce = 1u32; + + let mut msg = [0u8; 20]; + msg[..16].copy_from_slice(&beacon.to_bytes()); + msg[16..20].copy_from_slice(&nonce.to_le_bytes()); + let tag = AuthenticatedBeacon::compute_tag(&msg, &key); + + let auth = AuthenticatedBeacon { beacon, nonce, hmac_tag: tag }; + let mut wire = auth.to_bytes(); + // Flip one bit in the beacon payload + wire[0] ^= 0x01; + let tampered = AuthenticatedBeacon::from_bytes(&wire).unwrap(); + assert!(tampered.verify(&key).is_err(), "Single bit flip must fail verification"); + } + + #[test] + fn test_enforcing_mode_rejects_unauthenticated() { + let mut cfg = manual_config(); + cfg.sec_level = SecLevel::Enforcing; + let mut coord = SecureTdmCoordinator::new(test_schedule(), cfg).unwrap(); + + // Raw 16-byte beacon without HMAC + let raw = SyncBeacon { + cycle_id: 1, + cycle_period: Duration::from_millis(50), + drift_correction_us: 0, + generated_at: std::time::Instant::now(), + }.to_bytes(); + + assert!(coord.verify_beacon(&raw).is_err()); + } + // ---- Error display tests ---- #[test] diff --git a/rust-port/wifi-densepose-rs/crates/wifi-densepose-sensing-server/src/main.rs b/rust-port/wifi-densepose-rs/crates/wifi-densepose-sensing-server/src/main.rs index bc21cbf7..7497c95a 100644 --- a/rust-port/wifi-densepose-rs/crates/wifi-densepose-sensing-server/src/main.rs +++ b/rust-port/wifi-densepose-rs/crates/wifi-densepose-sensing-server/src/main.rs @@ -77,6 +77,10 @@ struct Args { #[arg(long, default_value = "100")] tick_ms: u64, + /// Bind address (default 127.0.0.1; set to 0.0.0.0 for network access) + #[arg(long, default_value = "127.0.0.1", env = "SENSING_BIND_ADDR")] + bind_addr: String, + /// Data source: auto, wifi, esp32, simulate #[arg(long, default_value = "auto")] source: String, @@ -2112,7 +2116,15 @@ async fn delete_model( State(state): State, Path(id): Path, ) -> Json { - let path = PathBuf::from("data/models").join(format!("{}.rvf", id)); + // ADR-050: Sanitize path to prevent directory traversal + let safe_id = std::path::Path::new(&id) + .file_name() + .and_then(|f| f.to_str()) + .unwrap_or(""); + if safe_id.is_empty() || safe_id != id { + return Json(serde_json::json!({ "error": "invalid model id", "success": false })); + } + let path = PathBuf::from("data/models").join(format!("{}.rvf", safe_id)); if path.exists() { if let Err(e) = std::fs::remove_file(&path) { warn!("Failed to delete model file {:?}: {}", path, e); @@ -2363,7 +2375,15 @@ async fn delete_recording( State(state): State, Path(id): Path, ) -> Json { - let path = PathBuf::from("data/recordings").join(format!("{}.jsonl", id)); + // ADR-050: Sanitize path to prevent directory traversal + let safe_id = std::path::Path::new(&id) + .file_name() + .and_then(|f| f.to_str()) + .unwrap_or(""); + if safe_id.is_empty() || safe_id != id { + return Json(serde_json::json!({ "error": "invalid recording id", "success": false })); + } + let path = PathBuf::from("data/recordings").join(format!("{}.jsonl", safe_id)); if path.exists() { if let Err(e) = std::fs::remove_file(&path) { warn!("Failed to delete recording {:?}: {}", path, e); @@ -3604,6 +3624,10 @@ async fn main() { } } + // ADR-050: Parse bind address once, use for all listeners + let bind_ip: std::net::IpAddr = args.bind_addr.parse() + .expect("Invalid --bind-addr (use 127.0.0.1 or 0.0.0.0)"); + // WebSocket server on dedicated port (8765) let ws_state = state.clone(); let ws_app = Router::new() @@ -3611,7 +3635,7 @@ async fn main() { .route("/health", get(health)) .with_state(ws_state); - let ws_addr = SocketAddr::from(([0, 0, 0, 0], args.ws_port)); + let ws_addr = SocketAddr::from((bind_ip, args.ws_port)); let ws_listener = tokio::net::TcpListener::bind(ws_addr).await .expect("Failed to bind WebSocket port"); info!("WebSocket server listening on {ws_addr}"); @@ -3686,7 +3710,7 @@ async fn main() { )) .with_state(state.clone()); - let http_addr = SocketAddr::from(([0, 0, 0, 0], args.http_port)); + let http_addr = SocketAddr::from((bind_ip, args.http_port)); let http_listener = tokio::net::TcpListener::bind(http_addr).await .expect("Failed to bind HTTP port"); info!("HTTP server listening on {http_addr}");