From 29943e536f2be420a9d4042cd26d7b7eddf71438 Mon Sep 17 00:00:00 2001 From: Luke Hoersten Date: Tue, 28 Jul 2026 17:38:06 -0500 Subject: Address remaining audit findings: input hardening and cleanup Security: - SNTP replies must echo a random nonce in their originate timestamp, and unsynchronized (LI=3) or invalid-stratum servers are rejected, so an off-path spoofer cannot defeat the clock-trust gate - The storage directory is created 0700 but an existing one is tightened only when it is provably ours (holds our artifacts or is empty); a misconfigured path like /tmp under root is left alone with a warning - Device-supplied vendor/product names are stripped of control characters at ingestion so a device cannot inject terminal escapes Cleanup: - ensure_fabric splits into decision logic plus bootstrap_fabric - read_device_names merges the two name readers; a From impl replaces the hand-mapped IdentityReport; run_sync uses map/transpose with an extracted manual_instant; pick_interface runs once; print_json escapes its fallback; device_name dedups the vendor/product join; identity_status ignores leftover .tmp blobs; dead parameters and repeated random-id code removed 36 unit tests, clippy clean. --- src/controller.rs | 152 +++++++++++++++++++++++++++++++++++++++++------------- src/host.rs | 34 +++++++++++- src/main.rs | 52 +++++++------------ src/output.rs | 59 +++++++++++++++------ 4 files changed, 209 insertions(+), 88 deletions(-) diff --git a/src/controller.rs b/src/controller.rs index 952a278..b343265 100644 --- a/src/controller.rs +++ b/src/controller.rs @@ -103,9 +103,12 @@ pub fn identity_status(storage: &Path) -> IdentityStatus { let identity_exists = identity_path(storage).exists(); let fabric_exists = std::fs::read_dir(matter_kv_path(storage)) .map(|entries| { - entries - .flatten() - .any(|e| e.file_name().to_string_lossy().starts_with("k_")) + entries.flatten().any(|e| { + let name = e.file_name(); + let name = name.to_string_lossy(); + // A committed blob, not a leftover k_XXXX.tmp from a crash. + name.starts_with("k_") && !name.ends_with(".tmp") + }) }) .unwrap_or(false); match (identity_exists, fabric_exists) { @@ -140,16 +143,16 @@ fn matter_kv_path(storage: &Path) -> std::path::PathBuf { impl Identity { fn generate(crypto: &impl Crypto) -> Result { let mut rng = crypto.rand()?; - let mut bytes = [0u8; 8]; - rng.fill_bytes(&mut bytes); - // Non-zero 64-bit fabric id; operational node ids for devices are - // small and sequential like matter.js's. - let fabric_id = u64::from_be_bytes(bytes) | 1; - let mut node_bytes = [0u8; 8]; - rng.fill_bytes(&mut node_bytes); + // A non-zero random 64-bit id; device node ids are small and + // sequential (assigned from next_device_node_id). + let mut random_id = || -> Result { + let mut bytes = [0u8; 8]; + rng.fill_bytes(&mut bytes); + Ok(u64::from_be_bytes(bytes) | 1) + }; Ok(Self { - fabric_id, - controller_node_id: u64::from_be_bytes(node_bytes) | 1, + fabric_id: random_id()?, + controller_node_id: random_id()?, next_device_node_id: 1, rcac_privkey_hex: String::new(), }) @@ -253,8 +256,7 @@ pub trait Op { /// then races the transport and mDNS pumps against `op`. The pumps never /// finish on their own; `op` completing ends the run. pub fn run(config: &Config, op: O) -> anyhow::Result { - std::fs::create_dir_all(&config.storage_path).context("create storage directory")?; - restrict_dir_mode(&config.storage_path)?; + prepare_storage_dir(&config.storage_path)?; // One controller process per storage directory. Held for the process // lifetime (flock releases on exit, so no stale-lock handling); this // makes the first-run identity bootstrap race-free when a manual run @@ -296,8 +298,9 @@ pub fn run(config: &Config, op: O) -> anyhow::Result { // socket; the connect retries absorb most of that. (The system // dnssd backend was tried and is worse: its 10-item browse channel // deterministically drops records on Matter-dense networks.) - let mdns_socket = bind_mdns_socket().context("bind mDNS socket")?; let (mdns_host_ipv4, mdns_ipv6, mdns_interface) = pick_interface()?; + let mdns_socket = + bind_mdns_socket(mdns_host_ipv4, mdns_interface).context("bind mDNS socket")?; let hostname = format!( "{:012X}", ctx.identity.controller_node_id & 0xFFFF_FFFF_FFFF @@ -354,7 +357,7 @@ fn ensure_fabric( Ok(raw) => { let identity: Identity = serde_json::from_str(&raw).context("parse identity.json")?; - reconcile_label(matter, crypto, kv, fab_idx, &config.fabric_label)?; + reconcile_label(matter, kv, fab_idx, &config.fabric_label)?; return Ok((fab_idx, identity)); } // A fabric blob without identity.json is an interrupted first @@ -408,6 +411,22 @@ fn ensure_fabric( ); } + bootstrap_fabric(config, matter, crypto, kv) +} + +/// CSA test vendor id: the correct value for an uncertified controller. +const TEST_VENDOR_ID: u16 = 0xFFF1; + +/// First-run fabric creation: mint the RCAC (its private key kept for +/// RCAC-direct signing), our own operational NOC, and the fabric IPK, then +/// install and persist the fabric. Called only after [`ensure_fabric`] has +/// established that the storage is virgin. +fn bootstrap_fabric( + config: &Config, + matter: &Matter<'static>, + crypto: &C, + kv: &impl rs_matter::persist::KvBlobStoreAccess, +) -> anyhow::Result<(NonZeroU8, Identity)> { log::info!("First run: creating the controller fabric (persistent identity)"); let mut identity = Identity::generate(crypto).ctx("generate controller identity")?; @@ -467,7 +486,7 @@ fn ensure_fabric( // Identity file after the fabric: a crash in between leaves a fabric // without identity.json, which the next run reports loudly rather than // silently minting a second identity. - state::store(&identity_file, &identity).context("write identity.json")?; + state::store(&identity_path(&config.storage_path), &identity).context("write identity.json")?; log::info!( "Controller fabric created and persisted (fabric id {}, controller node id {})", identity.fabric_id, @@ -476,12 +495,8 @@ fn ensure_fabric( Ok((fab_idx, identity)) } -/// CSA test vendor id: the correct value for an uncertified controller. -const TEST_VENDOR_ID: u16 = 0xFFF1; - -fn reconcile_label( +fn reconcile_label( matter: &Matter<'static>, - _crypto: &C, kv: &impl rs_matter::persist::KvBlobStoreAccess, fab_idx: NonZeroU8, label: &str, @@ -554,19 +569,63 @@ fn lock_storage(_storage: &Path) -> anyhow::Result { anyhow::bail!("only unix hosts are supported") } +/// Creates the storage directory (mode 0700) if absent, and tightens an +/// existing one to 0700 only when it is ours. Blindly chmod'ing an +/// operator-supplied path would wreck a misconfigured `storagePath` of, say, +/// `/tmp` when the tool runs as root, so a pre-existing directory that shows +/// no sign of belonging to this tool is left untouched with a warning. +#[cfg(unix)] +fn prepare_storage_dir(path: &Path) -> anyhow::Result<()> { + use std::os::unix::fs::{DirBuilderExt, PermissionsExt}; + if !path.exists() { + return std::fs::DirBuilder::new() + .recursive(true) + .mode(0o700) + .create(path) + .context("create storage directory"); + } + if is_our_storage_dir(path) { + std::fs::set_permissions(path, std::fs::Permissions::from_mode(0o700)) + .context("chmod storage directory")?; + } else { + log::warn!( + "storagePath {} already exists and does not look like a mattertimesync storage \ + directory; leaving its permissions unchanged (it should be mode 0700, owned by the \ + service user)", + path.display() + ); + } + Ok(()) +} + +/// Whether a directory is empty or already holds this tool's artifacts, and +/// so is safe to claim and tighten. #[cfg(unix)] -fn restrict_dir_mode(path: &Path) -> anyhow::Result<()> { - use std::os::unix::fs::PermissionsExt; - std::fs::set_permissions(path, std::fs::Permissions::from_mode(0o700)) - .context("chmod storage directory") +fn is_our_storage_dir(path: &Path) -> bool { + const MARKERS: [&str; 5] = [ + "identity.json", + "matter", + ".lock", + "nodes.json", + "service-state.json", + ]; + if MARKERS.iter().any(|m| path.join(m).exists()) { + return true; + } + std::fs::read_dir(path) + .map(|mut entries| entries.next().is_none()) + .unwrap_or(false) } #[cfg(not(unix))] -fn restrict_dir_mode(_path: &Path) -> anyhow::Result<()> { - Ok(()) +fn prepare_storage_dir(path: &Path) -> anyhow::Result<()> { + std::fs::create_dir_all(path).context("create storage directory") } -fn bind_mdns_socket() -> anyhow::Result> { +fn bind_mdns_socket( + _ipv4: std::net::Ipv4Addr, + interface: u32, +) -> anyhow::Result> { use socket2::{Domain, Protocol, Socket, Type}; let socket = Socket::new(Domain::IPV6, Type::DGRAM, Some(Protocol::UDP))?; socket.set_reuse_address(true)?; @@ -584,7 +643,6 @@ fn bind_mdns_socket() -> anyhow::Result> { // operations on an IPv6 socket that Linux tolerates. Without the group // join there is no IPv4 mDNS at all, quietly and identically on every // host. A failed IPv6 join, by contrast, means discovery cannot work. - let (_ipv4, _ipv6, interface) = pick_interface()?; socket .get_ref() .join_multicast_v6(&MDNS_IPV6_BROADCAST_ADDR, interface) @@ -751,8 +809,7 @@ impl Op for CommissionOp { .await?; // Cache the device identity for the local registry; best-effort. - let vendor_name = read_vendor_name(ctx, device_node_id).await.ok(); - let product_name = read_product_name(ctx, device_node_id).await.ok(); + let (vendor_name, product_name) = read_device_names(ctx, device_node_id).await; let registry_file = state::registry_path(&ctx.config.storage_path); let mut registry: NodeRegistry = state::load_or_default(®istry_file); @@ -784,13 +841,37 @@ impl Op for CommissionOp { } } +/// The device's vendor and product names for the local registry, each +/// sanitized and best-effort (`None` if unreadable). Sanitizing matters: +/// these strings come off the wire from the device and are later printed to +/// the operator's terminal and journal, so a malicious device must not be +/// able to smuggle terminal escape sequences through them. +async fn read_device_names( + ctx: &Ctx<'_, C>, + node_id: u64, +) -> (Option, Option) { + ( + read_vendor_name(ctx, node_id).await.ok(), + read_product_name(ctx, node_id).await.ok(), + ) +} + +/// Replaces control characters with '?' so a device-supplied string cannot +/// drive the operator's terminal. +fn sanitize(value: &str) -> String { + value + .chars() + .map(|c| if c.is_control() { '?' } else { c }) + .collect() +} + async fn read_vendor_name(ctx: &Ctx<'_, C>, node_id: u64) -> anyhow::Result { let exchange = connect(ctx, node_id).await?; let mut out = String::new(); exchange .basic_information() .vendor_name_read_with(ROOT_ENDPOINT_ID, |value| { - out = value?.to_string(); + out = sanitize(value?); Ok::<_, MatterError>(()) }) .await @@ -805,7 +886,7 @@ async fn read_product_name(ctx: &Ctx<'_, C>, node_id: u64) -> anyhow: exchange .basic_information() .product_name_read_with(ROOT_ENDPOINT_ID, |value| { - out = value?.to_string(); + out = sanitize(value?); Ok::<_, MatterError>(()) }) .await @@ -1285,8 +1366,7 @@ impl Op for InspectOp { } async fn inspect_one(ctx: &Ctx<'_, C>, node_id: u64) -> anyhow::Result { - let vendor_name = read_vendor_name(ctx, node_id).await.ok(); - let product_name = read_product_name(ctx, node_id).await.ok(); + let (vendor_name, product_name) = read_device_names(ctx, node_id).await; let feature_map = connect(ctx, node_id) .await? diff --git a/src/host.rs b/src/host.rs index feb5a34..d3e871e 100644 --- a/src/host.rs +++ b/src/host.rs @@ -42,6 +42,14 @@ pub fn clock_is_ntp_synchronized() -> bool { /// clock relative to the server (positive = local clock ahead). Uses the /// request/response midpoint, so the error is bounded by half the round /// trip, which is milliseconds against a threshold of a second. +/// +/// The request carries a random transmit timestamp that the reply must echo +/// in its originate field (RFC 4330 ยง5); this binds the answer to our +/// request so an off-path packet or a stale reply is rejected. It cannot +/// stop an on-path attacker, but the gate only decides whether to trust the +/// *host's own* clock (the device is never set from the NTP value), so the +/// worst an attacker gains is suppressing sync or blessing an already-wrong +/// host clock. fn sntp_offset(server: &str) -> Option { let socket = UdpSocket::bind(("0.0.0.0", 0)).ok()?; socket.set_read_timeout(Some(Duration::from_secs(2))).ok()?; @@ -49,14 +57,29 @@ fn sntp_offset(server: &str) -> Option { let mut request = [0u8; 48]; request[0] = 0b00_100_011; // LI 0, version 4, mode 3 (client) + // Random transmit timestamp (bytes 40..48) used as an anti-spoof nonce. + let mut nonce = [0u8; 8]; + getrandom(&mut nonce); + request[40..48].copy_from_slice(&nonce); let sent_at = unix_now(); socket.send(&request).ok()?; let mut response = [0u8; 48]; let len = socket.recv(&mut response).ok()?; let received_at = unix_now(); - if len < 48 || response[0] & 0x07 != 4 { - // Not a server-mode reply. + if len < 48 { + return None; + } + let leap = response[0] >> 6; + let mode = response[0] & 0x07; + let stratum = response[1]; + // Reject non-server replies, unsynchronized servers (LI=3), and + // kiss-of-death / invalid stratum (0, or 16+ = unsynchronized). + if mode != 4 || leap == 3 || !(1..=15).contains(&stratum) { + return None; + } + // The reply's originate timestamp (bytes 24..32) must echo our nonce. + if response[24..32] != nonce { return None; } @@ -71,6 +94,13 @@ fn sntp_offset(server: &str) -> Option { Some((sent_at + received_at) / 2.0 - server_time) } +/// Fills `buf` with random bytes via rs-matter's crypto RNG (already a +/// dependency); a nonce only needs unpredictability, not the full clock. +fn getrandom(buf: &mut [u8]) { + use rand::RngCore as _; + rand::thread_rng().fill_bytes(buf); +} + fn unix_now() -> f64 { SystemTime::now() .duration_since(UNIX_EPOCH) diff --git a/src/main.rs b/src/main.rs index e36ad21..16fe2df 100644 --- a/src/main.rs +++ b/src/main.rs @@ -48,9 +48,7 @@ use clap::{Parser, Subcommand}; use jiff::Timestamp; use crate::config::{Config, DEFAULT_CONFIG_PATH}; -use crate::output::{ - ConfigReport, DstTransition, IdentityReport, NodeListing, Output, StatusReport, SyncReport, -}; +use crate::output::{ConfigReport, DstTransition, NodeListing, Output, StatusReport, SyncReport}; use crate::state::{NodeRegistry, ServiceState}; use crate::time::MatterMicros; @@ -260,20 +258,7 @@ fn run_status(config: &Config, filter: Filter) -> anyhow::Result { }) .collect(); - let identity = match controller::identity_status(&config.storage_path) { - controller::IdentityStatus::NotCreated => IdentityReport::NotCreated, - controller::IdentityStatus::Created { - fabric_id, - controller_node_id, - } => IdentityReport::Created { - fabric_id: fabric_id.to_string(), - controller_node_id: controller_node_id.to_string(), - }, - controller::IdentityStatus::Unreadable => IdentityReport::Unreadable, - controller::IdentityStatus::Inconsistent(reason) => IdentityReport::Inconsistent { - reason: reason.to_string(), - }, - }; + let identity = controller::identity_status(&config.storage_path).into(); let offset_seconds = tz.to_offset(now).seconds(); Ok(Output::Status(Box::new(StatusReport { config: ConfigReport { @@ -310,23 +295,24 @@ fn run_inspect(config: &Config, target: Target) -> anyhow::Result { Ok(Output::Inspection { nodes }) } +/// The requested wall-clock time as a concrete instant: today's date in the +/// configured zone at that time. The operator is asserting the wall clock, so +/// the result feeds `--time` directly. +fn manual_instant(config: &Config, raw: &str) -> anyhow::Result { + let wall = crate::time::parse_wall_clock(raw).map_err(|e| anyhow::anyhow!(e))?; + let tz = config.time_zone(); + let today = Timestamp::now().to_zoned(tz.clone()).date(); + let instant = today + .at(wall.hour(), wall.minute(), wall.second(), 0) + .to_zoned(tz) + .map_err(|e| anyhow::anyhow!("cannot place {raw:?} in {}: {e}", config.timezone))? + .timestamp(); + Ok(MatterMicros::from_timestamp(instant)) +} + fn run_sync(config: &Config, target: Target, time: Option<&str>) -> anyhow::Result { - // --time makes the operator the time source: compute today's date in - // the configured zone at the requested wall-clock time. - let manual_time = match time { - None => None, - Some(raw) => { - let wall = crate::time::parse_wall_clock(raw).map_err(|e| anyhow::anyhow!(e))?; - let tz = config.time_zone(); - let today = Timestamp::now().to_zoned(tz.clone()).date(); - let instant = today - .at(wall.hour(), wall.minute(), wall.second(), 0) - .to_zoned(tz) - .map_err(|e| anyhow::anyhow!("cannot place {raw:?} in {}: {e}", config.timezone))? - .timestamp(); - Some(MatterMicros::from_timestamp(instant)) - } - }; + // --time makes the operator the time source. + let manual_time = time.map(|raw| manual_instant(config, raw)).transpose()?; // Fail safe: never push time from a clock that is not NTP-disciplined. // Not enforced with --time: the operator deliberately chose the value. diff --git a/src/output.rs b/src/output.rs index e2c91ec..0a8a536 100644 --- a/src/output.rs +++ b/src/output.rs @@ -116,6 +116,26 @@ pub enum IdentityReport { }, } +impl From for IdentityReport { + fn from(status: crate::controller::IdentityStatus) -> Self { + use crate::controller::IdentityStatus as S; + match status { + S::NotCreated => Self::NotCreated, + S::Created { + fabric_id, + controller_node_id, + } => Self::Created { + fabric_id: fabric_id.to_string(), + controller_node_id: controller_node_id.to_string(), + }, + S::Unreadable => Self::Unreadable, + S::Inconsistent(reason) => Self::Inconsistent { + reason: reason.to_string(), + }, + } + } +} + #[derive(Debug, Serialize)] #[serde(rename_all = "camelCase")] pub struct DstTransition { @@ -174,7 +194,12 @@ impl Output { pub fn print_json(&self) { match serde_json::to_string_pretty(self) { Ok(body) => println!("{body}"), - Err(error) => println!("{{\"error\": \"serialize output: {error}\"}}"), + // Build the fallback with the serializer too, so the error text + // is escaped and stdout stays valid JSON even here. + Err(error) => println!( + "{}", + serde_json::json!({ "error": format!("serialize output: {error}") }) + ), } } @@ -198,6 +223,17 @@ fn display_instant(at: Option) -> String { at.map_or_else(|| "never".into(), |at| at.to_string()) } +/// The device's vendor and product names joined for display, or `None` when +/// neither is known; each caller supplies its own fallback text. +fn device_name(vendor: Option<&str>, product: Option<&str>) -> Option { + let joined = [vendor, product] + .into_iter() + .flatten() + .collect::>() + .join(" "); + (!joined.is_empty()).then_some(joined) +} + fn render_status(report: &StatusReport) { println!( "Configuration: loaded from {}", @@ -271,19 +307,11 @@ fn render_status(report: &StatusReport) { report.matter_time_now_microseconds ); for node in &report.nodes { - let name = [node.vendor_name.as_deref(), node.product_name.as_deref()] - .into_iter() - .flatten() - .collect::>() - .join(" "); + let name = device_name(node.vendor_name.as_deref(), node.product_name.as_deref()); println!( "Node {}: {}", node.node_id, - if name.is_empty() { - "(no cached device info)" - } else { - &name - } + name.as_deref().unwrap_or("(no cached device info)") ); println!( " Last connection: {}", @@ -399,14 +427,11 @@ fn render_commissioned(outcome: &CommissionOutcome) { println!(" Fabric ID: {}", outcome.fabric_id); println!( " Device: {}", - [ + device_name( outcome.vendor_name.as_deref(), outcome.product_name.as_deref() - ] - .into_iter() - .flatten() - .collect::>() - .join(" ") + ) + .unwrap_or_default() ); println!(); println!("The device remains paired with its primary ecosystem; this controller"); -- cgit v1.2.3