diff --git a/CODE_OF_CONDUCT.md b/CODE_OF_CONDUCT.md index 1c7356c..49068e2 100644 --- a/CODE_OF_CONDUCT.md +++ b/CODE_OF_CONDUCT.md @@ -36,7 +36,7 @@ This Code of Conduct applies within all community spaces, and also applies when ## Enforcement -Instances of abusive, harassing, or otherwise unacceptable behavior may be reported to the community leaders responsible for enforcement at owner@example.invalid. All complaints will be reviewed and investigated promptly and fairly. +Instances of abusive, harassing, or otherwise unacceptable behavior may be reported privately to the project maintainer via GitHub at https://github.com/MattJackson. All complaints will be reviewed and investigated promptly and fairly. All community leaders are obligated to respect the privacy and security of the reporter of any incident. diff --git a/src/keydb_format.rs b/src/keydb_format.rs index 225c0c9..09e203a 100644 --- a/src/keydb_format.rs +++ b/src/keydb_format.rs @@ -265,22 +265,33 @@ impl KeyDb { /// [`KeyDb`] rather than an error — callers needing a non-empty db must /// check the parsed contents. pub fn load(path: &std::path::Path) -> std::io::Result { - // Stat-and-cap before reading so a hostile/corrupt file can't force an - // unbounded allocation. A file strictly over the cap is rejected (a - // file exactly at MAX_KEYDB_BYTES is accepted, matching the `>` guard - // and libfreemkv's original at-cap-is-allowed semantics). - if let Ok(meta) = std::fs::metadata(path) { - if meta.len() > MAX_KEYDB_BYTES { - return Err(std::io::Error::new( - std::io::ErrorKind::InvalidData, - format!( - "keydb.cfg exceeds {MAX_KEYDB_BYTES} byte cap: {}", - path.display() - ), - )); - } + // Read through a hard `take` cap rather than trusting a stat. A + // `metadata` pre-check is only advisory: it is skipped entirely when the + // stat fails, and it reports len 0 for a FIFO or character device, so + // `--keydb /dev/zero` passed the guard and then read forever (NUL is + // valid UTF-8, so it did not even fail the decode — it just grew until + // the process was OOM-killed). Reading one byte past the cap accepts an + // exactly-at-cap file and rejects anything larger, matching + // `read_capped_to_string` on the download path. + use std::io::Read; + let f = std::fs::File::open(path)?; + let mut buf = Vec::new(); + f.take(MAX_KEYDB_BYTES + 1).read_to_end(&mut buf)?; + if buf.len() as u64 > MAX_KEYDB_BYTES { + return Err(std::io::Error::new( + std::io::ErrorKind::InvalidData, + format!( + "keydb.cfg exceeds {MAX_KEYDB_BYTES} byte cap: {}", + path.display() + ), + )); } - let data = std::fs::read_to_string(path)?; + let data = String::from_utf8(buf).map_err(|_| { + std::io::Error::new( + std::io::ErrorKind::InvalidData, + format!("keydb.cfg is not valid UTF-8: {}", path.display()), + ) + })?; Ok(Self::parse(&data)) } @@ -727,6 +738,43 @@ impl KeyDb { mod tests { use super::*; + /// `load` must enforce `MAX_KEYDB_BYTES` through a bounded read, not a stat. + /// + /// Regression: the cap used to be a `std::fs::metadata` pre-check followed by + /// an unbounded `read_to_string`. That is not a cap — it is skipped when the + /// stat fails, and a FIFO or character device reports len 0, so the guard + /// passed and the read never terminated. NUL bytes are valid UTF-8, so + /// `--keydb /dev/zero` did not even fail the decode; it allocated until the + /// process died. Asserts both edges of the boundary so a future cap change + /// cannot quietly re-introduce an off-by-one. + #[test] + fn load_enforces_the_size_cap_at_both_edges() { + let dir = std::env::temp_dir() + .join("fmkv-keydb-cap") + .join(format!("{}", std::process::id())); + std::fs::create_dir_all(&dir).unwrap(); + + // Exactly at the cap is accepted. Use a comment line so `parse` is happy + // and the content is valid UTF-8 of a known length. + let at = dir.join("at_cap.cfg"); + let body = vec![b'#'; MAX_KEYDB_BYTES as usize]; + std::fs::write(&at, &body).unwrap(); + assert!( + KeyDb::load(&at).is_ok(), + "a file exactly at MAX_KEYDB_BYTES must load" + ); + + // One byte over is rejected as InvalidData, not truncated and not OOM. + let over = dir.join("over_cap.cfg"); + let mut body = vec![b'#'; MAX_KEYDB_BYTES as usize]; + body.push(b'#'); + std::fs::write(&over, &body).unwrap(); + let err = KeyDb::load(&over).expect_err("over-cap file must be rejected"); + assert_eq!(err.kind(), std::io::ErrorKind::InvalidData); + + std::fs::remove_dir_all(&dir).ok(); + } + /// `to_keydb_cfg` is the exact inverse of `parse`: parse a known line set, /// serialize it, re-parse, and every field survives — device key, processing /// key, host cert (priv key + cert + revocation), and the per-disc M/I(vid)/V/U diff --git a/src/online.rs b/src/online.rs index b450272..ce7f7f5 100644 --- a/src/online.rs +++ b/src/online.rs @@ -281,7 +281,20 @@ impl OnlineSource { // request (and the bearer token) to an internal/metadata host. let pinned = match resolve_and_guard(&self.base_url) { Ok(addrs) => addrs, - Err(_) => return Vec::new(), + Err(_) => { + // Log THAT the URL was rejected, never WHY: the guard's message + // names the resolved address, and an internal address must not + // reach a log an operator may paste into a bug report. The + // static label is enough to separate "misconfigured/blocked + // key-service URL" from "this disc has no key", which is the + // only distinction the operator needs here. + tracing::warn!( + target: "freemkv::keysource", + phase = "keyserver_post", + "key-service URL failed the address guard; skipping the online source" + ); + return Vec::new(); + } }; let agent = hardened_agent(pinned); let mut req = agent.post(&self.base_url); @@ -298,13 +311,30 @@ impl OnlineSource { let post_t0 = std::time::Instant::now(); let resp = match req.send_json(body) { Ok(r) => r, - Err(_) => { - tracing::warn!( - target: "freemkv::keysource", - phase = "keyserver_post", - elapsed_ms = post_t0.elapsed().as_millis() as u64, - "keyserver request failed (timeout, network, or HTTP error)" - ); + Err(e) => { + // Record the HTTP status when the service actually answered: + // 401/403 (bad or expired token), 429 (rate limited) and + // 5xx (service down) are completely different operator actions, + // and collapsing them into one line is why a 502 was read as + // "this disc has no key". The status is the service's own + // response code — it carries no key material and no address. + // A transport error has no status; report it as such rather + // than inventing one. + match e { + ureq::Error::Status(code, _) => tracing::warn!( + target: "freemkv::keysource", + phase = "keyserver_post", + http_status = code, + elapsed_ms = post_t0.elapsed().as_millis() as u64, + "key service returned an HTTP error; no key from online" + ), + ureq::Error::Transport(_) => tracing::warn!( + target: "freemkv::keysource", + phase = "keyserver_post", + elapsed_ms = post_t0.elapsed().as_millis() as u64, + "key service unreachable (connect/timeout/TLS); no key from online" + ), + } return Vec::new(); } }; @@ -324,11 +354,27 @@ impl OnlineSource { .is_err() || buf.len() > MAX_RESPONSE_BYTES { + // Length only — never the body, which carries base64 key material. + tracing::warn!( + target: "freemkv::keysource", + phase = "keyserver_post", + cap = MAX_RESPONSE_BYTES, + "key-service reply was unreadable or over the size cap; no key from online" + ); return Vec::new(); } let json: serde_json::Value = match serde_json::from_slice(&buf) { Ok(j) => j, - Err(_) => return Vec::new(), + Err(_) => { + // Never log the parse error or the payload: a serde message + // quotes the offending input, which here is key material. + tracing::warn!( + target: "freemkv::keysource", + phase = "keyserver_post", + "key-service reply was not valid JSON; no key from online" + ); + return Vec::new(); + } }; // `UK` is an ARRAY of hex keys (the service always returns an array now, // even of one). A single element is the base Unit Key. A full set (one per @@ -343,11 +389,31 @@ impl OnlineSource { out.push(UnitKey::new(0, k)); } } else if let Some(arr) = uk.as_array() { + // A forensic set is only usable COMPLETE: the mux trusts any + // non-empty result as the whole set and never assumes 32 (see + // `get_fmts_indexes`). Skipping an unparseable element would + // hand back a short set that silently omits an index, so a + // malformed element rejects the whole reply — that is a service + // bug, not a usable answer. + let mut bad = false; for (i, v) in arr.iter().enumerate() { - if let Some(k) = v.as_str().and_then(parse_uk) { - out.push(UnitKey::new(i as u32, k)); + match v.as_str().and_then(parse_uk) { + Some(k) => out.push(UnitKey::new(i as u32, k)), + None => { + bad = true; + break; + } } } + if bad { + tracing::warn!( + target: "freemkv::keysource", + phase = "keyserver_post", + keys = arr.len(), + "key-service returned a malformed key in the UK set; rejecting the whole reply" + ); + return Vec::new(); + } } if !out.is_empty() { return out; @@ -356,10 +422,28 @@ impl OnlineSource { // A VUK is derived to the terminal keys locally, via the disc's // encrypted title keys from the context — the library owns the crypto. if let Some(vuk) = json.get("VUK").and_then(|u| u.as_str()).and_then(parse_uk) { - if let Ok(enc) = ctx.enc_title_keys() { - return uks_from_vuk(&vuk, enc); + match ctx.enc_title_keys() { + Ok(enc) => return uks_from_vuk(&vuk, enc), + Err(_) => { + tracing::warn!( + target: "freemkv::keysource", + phase = "keyserver_post", + "key-service returned a VUK but the disc's encrypted title keys \ + are unreadable; cannot derive unit keys" + ); + return Vec::new(); + } } } + // The genuine miss. Logged distinctly from every failure above so an + // operator can tell "the service has no key for this disc" from "the + // service did not answer" — collapsing the two is what made a 502 look + // like a missing key. + tracing::info!( + target: "freemkv::keysource", + phase = "keyserver_post", + "key service has no key for this disc" + ); Vec::new() } }