diff --git a/CHANGELOG.md b/CHANGELOG.md index a1db3cf..fc42bc7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,31 @@ and this project adheres to [Semantic Versioning](https://semver.org/). Targeted at 0.4.0. This release is a behaviour change for anyone who set `require_provenance`: see the entry below before upgrading. +### Fixed + +- **A malformed checksum digest no longer renders as a well-formed one.** + `Checksum::to_sri` decoded the stored hex through a helper that mapped an + unparseable byte pair to `0`, so a digest that was not clean hex still + produced a valid-looking `sha512-` string. The only place that string + is shown is the mismatch message naming the *expected* value, which meant a + corrupt or hand-edited digest was illustrated back to the operator with bytes + silently zeroed, and the message read as a confident answer about the wrong + bytes. The helper is fallible now and `to_sri` returns `Result`, so an + undecodable digest is a refusal naming the offending input rather than a + plausible digest. + +- **The baseline store no longer discards three permission errors.** Three + filesystem hardening steps in `store.rs` were written as `let _ = ...`, so a + failure of any of them was invisible: the data directory chmod to `0o700`, + the pre-create of the database at `0o600`, and the chmod of the database to + `0o600`. The pre-create was the load-bearing one. `Connection::open` creates + the file at whatever the umask allows, so when the pre-create failed the + database came up at a default mode and a step whose whole purpose was + narrowing it had failed in silence. All three propagate now. + + No change to behaviour on the success path: the modes were always being set, + and `enforces_unix_file_and_directory_permissions` still pins that. + ### Changed - `recall sync` follows a bounded redirect chain instead of refusing every diff --git a/references/drift.md b/references/drift.md index 99e5ba5..d4b6a56 100644 --- a/references/drift.md +++ b/references/drift.md @@ -20,8 +20,8 @@ Severity is this file's own judgement, not the project's. | 3 | Integrity mismatch surfaces as `Err` + exit 1, not `Verdict::Block` | Medium — a consumer keying on band never sees BLOCK | | 4 | No cargo `-`/`_` name canonicalization, contrary to `CHANGELOG` 0.3.0 | Medium — `serde_json` and `serde-json` are distinct crates | | 10 | `diff_single_file` coerces invalid UTF-8 to `""`, so a real change reads as all-add or all-delete | Medium — undocumented, silently mis-scores | -| 12 | `store.rs` discards the `0o600` pre-create error (`let _ =`) | Low — the DB may be more permissive than intended | -| 9 | `hex_to_bytes` uses `.unwrap_or(0)`, mapping invalid hex to byte `0` | Low — fails toward "valid-looking" | +| 12 | ~~`store.rs` discards the `0o600` pre-create error (`let _ =`)~~ **resolved** — all three discarded errors propagate. See below | Low | +| 9 | ~~`hex_to_bytes` uses `.unwrap_or(0)`, mapping invalid hex to byte `0`~~ **resolved** — the helper is fallible and `to_sri` returns `Result` | Low | | 5 | ~~`registry/mod.rs` doc comments are stale~~ **resolved** — both were deleted; the four adapters are all real. See below | Low | | 6 | Manager count is six in `CHANGELOG` 0.3.1, eleven in `ARCHITECTURE.md` §5; `--help` says eleven | Low — `--help` is authoritative | | 7 | ~~Dead code: `Policy::is_maintainer_blocked`, `blocklist.maintainers`, `allowlist.packages[].max_risk` / `.integrity`~~ **resolved** — the three policy keys are now refused at load. Still dead: `BaselineStore::clear_advisory_cache` | Low — test-only helper, `#[allow(dead_code)]` | @@ -141,8 +141,12 @@ Each finding is cited in its lane file with the exact source location. a name that resolved publicly for the check and privately for the connection no longer gets through. Only the configured base is exempt. See [registry](features/registry.md). -9. **`registry/mod.rs::hex_to_bytes` uses `.unwrap_or(0)`**, silently mapping - invalid hex to byte `0`. +9. **~~`registry/mod.rs::hex_to_bytes` uses `.unwrap_or(0)`~~ — resolved.** The + helper maps an unparseable byte pair to `0`, silently mapping invalid hex to + byte `0`. It is fallible now and `hex_to_bytes` is the only caller of + `to_sri`, so an undecodable digest is a refusal that names the offending + input. Tests `a_malformed_digest_fails_loud_instead_of_zeroing` and + `a_well_formed_digest_still_renders`. See [registry](features/registry.md). 10. **`diff_single_file` silently converts invalid UTF-8 to `""`**, so a real change reads as all-add or all-delete. `ambiguous`, not documented. See [diff](features/diff.md). @@ -154,4 +158,12 @@ Each finding is cited in its lane file with the exact source location. refusals name which of those failed. Test `a_pypi_archive_whose_metadata_cannot_be_read_is_refused`. See [review](features/review.md). -12. **`store.rs` silently discards the `0o600` pre-create error** (`let _ =`). \ No newline at end of file +12. **~~`store.rs` silently discards the `0o600` pre-create error~~ — resolved.** + Three hardening steps were `let _ = ...`: the data-directory chmod to + `0o700`, the pre-create of the database at `0o600`, and the database chmod + to `0o600`. All three propagate now. The pre-create was the load-bearing + one, because `Connection::open` creates the file at whatever the umask + allows, so a failed pre-create meant a database at a default mode and a + narrowing step that had failed in silence. Test + `an_unopenable_database_path_refuses_at_the_pre_create`. See + [store](features/store.md). \ No newline at end of file diff --git a/references/features/registry.md b/references/features/registry.md index 4797d4c..6e9a668 100644 --- a/references/features/registry.md +++ b/references/features/registry.md @@ -73,9 +73,14 @@ RegistryLimits::default() max_redirects = 5 ``` -**`hex_to_bytes` uses `.unwrap_or(0)`** — an invalid hex pair silently becomes -byte `0` instead of erroring. The only non-fail-closed spot in `mod.rs`, -reachable only via `to_sri()` on an already-validated `value_hex`. +**`hex_to_bytes` is fallible** — an invalid hex pair is a refusal naming the +offending digest, not byte `0`. `to_sri` returns `Result` for this reason: the +decoder cannot produce a well-formed SRI string from a digest it could not read, +and the only place `to_sri` renders is the mismatch message naming the expected +value, where a silently zeroed byte put the wrong bytes in a confident sentence. +Reachable only through direct `Checksum` construction, since `Checksum::parse` +validates its hex before storing it. Tests `a_malformed_digest_fails_loud_ +instead_of_zeroing` and `a_well_formed_digest_still_renders`. **`Checksum::parse` takes the FIRST recognized token of a whitespace-separated list**, so npm's multi-algorithm `dist.integrity` resolves deterministically. diff --git a/references/features/store.md b/references/features/store.md index d9871b9..2406053 100644 --- a/references/features/store.md +++ b/references/features/store.md @@ -71,10 +71,14 @@ Audit `action` values actually written (free-form `&str`, not an enum): `_new` copies** (PKs change to composites) and only `ALTER`s `audit_log`. The advisory expiry index is dropped with the old table and must be recreated after the rename. -- **Filesystem hardening is `#[cfg(unix)]` and mostly best-effort.** Parent dir - `0o700`, DB `0o600`, symlink re-checked *before* and *after* - `Connection::open`; `set_permissions` and the pre-create `OpenOptions` both - use `let _ =`, so they are not fail-closed. +- **Filesystem hardening is `#[cfg(unix)` and fail-closed on every step.** Parent + dir `0o700`, DB `0o600`, symlink re-checked *before* and *after* + `Connection::open`; `set_permissions` and the pre-create `OpenOptions` all + propagate their errors. The pre-create is the one that matters, because + `Connection::open` creates the file at whatever the umask allows, so a + swallowed pre-create failure meant a database at a default mode and a + narrowing step that failed in silence. Test + `an_unopenable_database_path_refuses_at_the_pre_create`. - **`AuditEntry` has `row.get::<_, i64>(5)? as u32`** — an unchecked narrowing cast. And `hit_count`/`slsa_level` use `.max(0) as u*`, which coerces a negative corrupted value to `0` rather than erroring. diff --git a/src/registry/mod.rs b/src/registry/mod.rs index 8094c64..e577d00 100644 --- a/src/registry/mod.rs +++ b/src/registry/mod.rs @@ -117,13 +117,19 @@ impl Checksum { /// npm SRI form (`sha512-`), used when talking to npm-shaped /// registries and preserved for backwards-compatible display. - pub fn to_sri(&self) -> String { + /// + /// Fallible because decoding the digest is. The old form mapped an + /// unparseable hex pair to `0`, so a malformed digest rendered as a + /// well-formed SRI string and the only place an operator ever saw it was a + /// mismatch message naming the expected value, which then illustrated the + /// wrong bytes without saying so. + pub fn to_sri(&self) -> Result { match self.alg { - ChecksumAlg::Sha512 => format!( + ChecksumAlg::Sha512 => Ok(format!( "sha512-{}", - base64::engine::general_purpose::STANDARD.encode(hex_to_bytes(&self.value_hex)) - ), - ChecksumAlg::Sha256 => self.to_display(), + base64::engine::general_purpose::STANDARD.encode(hex_to_bytes(&self.value_hex)?) + )), + ChecksumAlg::Sha256 => Ok(self.to_display()), } } @@ -137,9 +143,23 @@ pub fn hex_encode(bytes: &[u8]) -> String { bytes.iter().map(|b| format!("{b:02x}")).collect() } -fn hex_to_bytes(hex: &str) -> Vec { +/// Decode a lowercase hex digest into bytes. Fail closed on anything that is +/// not clean hex, rather than mapping an unparseable pair to `0`, which would +/// produce a digest that still decodes and still compares as a mismatch later. +fn hex_to_bytes(hex: &str) -> Result, BluelineError> { + if !hex.len().is_multiple_of(2) { + return Err(BluelineError::Verification(format!( + "digest `{hex}` has an odd number of hex digits" + ))); + } (0..hex.len() / 2) - .map(|i| u8::from_str_radix(&hex[2 * i..2 * i + 2], 16).unwrap_or(0)) + .map(|i| { + u8::from_str_radix(&hex[2 * i..2 * i + 2], 16).map_err(|_| { + BluelineError::Verification(format!( + "digest `{hex}` contains a non-hexadecimal pair at byte {i}" + )) + }) + }) .collect() } @@ -321,11 +341,53 @@ mod tests { #[test] fn sri_round_trips_through_display() { let c = Checksum::parse(TEST_SRI).unwrap(); - assert_eq!(c.to_sri(), TEST_SRI); + assert_eq!(c.to_sri().unwrap(), TEST_SRI); let again = Checksum::parse(&c.to_display()).unwrap(); assert_eq!(again, c); } + /// The old `hex_to_bytes` mapped an unparseable hex pair to `0`, so a + /// malformed digest rendered as a well-formed SRI string. The only place it + /// surfaced was a mismatch message naming the expected value, where the + /// zeroed bytes illustrated the wrong answer without saying so. + #[test] + fn a_malformed_digest_fails_loud_instead_of_zeroing() { + let mut bad = Checksum::parse(TEST_SRI).unwrap(); + bad.value_hex = TEST_HEX[..126].to_string(); + bad.value_hex.push_str("zz"); + + let err = bad + .to_sri() + .expect_err("a non-hexadecimal digest must not render as an SRI string"); + let msg = err.to_string(); + assert!( + msg.contains("zz") && msg.contains("non-hexadecimal"), + "the refusal must name the offending digest, got: {msg}" + ); + + // And an odd-length one, which the old `len() / 2` truncated silently. + let mut odd = Checksum::parse(TEST_SRI).unwrap(); + odd.value_hex.pop(); + let err = odd + .to_sri() + .expect_err("an odd-length digest must not be silently truncated"); + assert!( + err.to_string().contains("odd number of hex digits"), + "got: {}", + err + ); + } + + /// The honest path still round-trips: this is what would have caught a fix + /// that made every digest refuse. + #[test] + fn a_well_formed_digest_still_renders() { + let c = Checksum::parse(TEST_HEX).unwrap(); + let sri = c.to_sri().unwrap(); + assert!(sri.starts_with("sha512-"), "got {sri}"); + assert_eq!(Checksum::parse(&sri).unwrap(), c); + } + #[test] fn picks_sha512_from_multi_algorithm_list() { let c = Checksum::parse(&format!("sha1-YWJjZA== {TEST_SRI}")).unwrap(); diff --git a/src/registry/npm.rs b/src/registry/npm.rs index 26e712d..cff7928 100644 --- a/src/registry/npm.rs +++ b/src/registry/npm.rs @@ -128,12 +128,16 @@ impl NpmRegistry { match &pkg.integrity { Some(expected) if expected.alg == ChecksumAlg::Sha512 => { if !expected.value_hex.eq_ignore_ascii_case(&computed.value_hex) { + // Both digests are produced by `Checksum::parse` or + // `hex_encode`, so neither can fail here; `?` keeps it loud + // if that ever stops being true rather than rendering a + // digest with bytes silently zeroed. return Err(BluelineError::Verification(format!( "{}@{}: tarball sha512 mismatch (expected {}, got {})", pkg.name, pkg.version, - expected.to_sri(), - computed.to_sri() + expected.to_sri()?, + computed.to_sri()? ))); } } diff --git a/src/store.rs b/src/store.rs index 3f7d7d4..aa2fe3a 100644 --- a/src/store.rs +++ b/src/store.rs @@ -355,7 +355,12 @@ impl BaselineStore { use std::os::unix::fs::PermissionsExt; let mut perms = meta.permissions(); perms.set_mode(0o700); - let _ = fs::set_permissions(parent, perms); + fs::set_permissions(parent, perms).map_err(|e| { + BluelineError::Store(format!( + "setting permissions on {}: {e}", + parent.display() + )) + })?; } } #[cfg(not(unix))] @@ -376,12 +381,22 @@ impl BaselineStore { )); } use std::os::unix::fs::OpenOptionsExt; - let _ = fs::OpenOptions::new() + // Pre-create the file at 0o600 so it is never briefly world-readable + // before SQLite opens it: `Connection::open` would otherwise create + // it at whatever the umask allows, and the chmod after open cannot + // close a window it did not measure. Failure is fatal rather than + // ignored, because the step below creates the file anyway, so an + // ignored error here is exactly the permission the mode was chosen + // to avoid. + fs::OpenOptions::new() .write(true) .create(true) .truncate(false) .mode(0o600) - .open(path); + .open(path) + .map_err(|e| { + BluelineError::Store(format!("creating {} at mode 0o600: {e}", path.display())) + })?; } let mut conn = rusqlite::Connection::open(path) @@ -1003,7 +1018,7 @@ mod tests { Ecosystem::Npm, "pkg", "1.0.0", - &Checksum::parse(&ck("one").to_sri()).unwrap(), + &Checksum::parse(&ck("one").to_sri().unwrap()).unwrap(), ) .unwrap(); assert_eq!( @@ -1334,7 +1349,7 @@ mod tests { migrations.to_latest(&mut conn).unwrap(); conn.execute( "INSERT INTO known_clean (name, version, integrity, clean) VALUES ('legacy', '1.0.0', ?1, 1)", - rusqlite::params![legacy_checksum.to_sri()], + rusqlite::params![legacy_checksum.to_sri().unwrap()], ) .unwrap(); conn.execute( @@ -1365,7 +1380,7 @@ mod tests { assert_eq!(clean.len(), 1); assert_eq!(clean[0].0.to_string(), "1.0.0"); // The legacy SRI value survives verbatim and normalizes to the same content. - assert_eq!(clean[0].1, legacy_checksum.to_sri()); + assert_eq!(clean[0].1, legacy_checksum.to_sri().unwrap()); assert_eq!(Checksum::parse(&clean[0].1).unwrap(), legacy_checksum); assert!( @@ -1495,6 +1510,35 @@ mod tests { assert_eq!(file_meta.permissions().mode() & 0o777, 0o600); } + /// The pre-create at 0o600 is load-bearing, and its error used to be + /// discarded with `let _ =`. Dropping it silently was the defect: the step + /// below creates the file anyway, so an ignored failure simply meant SQLite + /// made it at the umask default, which is what the mode exists to prevent. + #[test] + #[cfg(unix)] + fn an_unopenable_database_path_refuses_at_the_pre_create() { + let dir = tempfile::tempdir().unwrap(); + // A directory at the path the database should occupy: everything up to + // the pre-create succeeds, which is what isolates that arm. The old + // `let _ =` swallowed this and let SQLite fail later with a message that + // named no mode and no reason to suspect the permissions. + let db_path = dir.path().join("t.db"); + fs::create_dir(&db_path).unwrap(); + + let err = BaselineStore::open_at(&db_path).err().expect( + "a database path that cannot be opened must refuse, not fall through to SQLite", + ); + let msg = err.to_string(); + assert!( + msg.contains("at mode 0o600"), + "the refusal must come from the pre-create and name the mode it wanted, got: {msg}" + ); + assert!( + msg.contains("t.db"), + "the refusal must name the path it could not use, got: {msg}" + ); + } + #[test] fn advisory_cache_roundtrip_and_clear() { let dir = tempfile::tempdir().unwrap(); diff --git a/tests/cli.rs b/tests/cli.rs index ca83329..a178471 100644 --- a/tests/cli.rs +++ b/tests/cli.rs @@ -247,7 +247,8 @@ fn full_flow_json_output() { assert_eq!( blueline::registry::Checksum::parse(displayed) .unwrap() - .to_sri(), + .to_sri() + .unwrap(), integrity ); assert_eq!(json["ecosystem"], "npm"); @@ -272,7 +273,7 @@ fn full_flow_json_output() { // The witness is stored in the canonical display form; it must normalize // to the same digest content as the SRI the fixture served. let stored = blueline::registry::Checksum::parse(&integrity_row).unwrap(); - assert_eq!(stored.to_sri(), integrity); + assert_eq!(stored.to_sri().unwrap(), integrity); assert_eq!(clean, 0, "review records evidence, never a clean blessing"); }