From b478dcda4ec295d24372719b5a28de29e887e4a3 Mon Sep 17 00:00:00 2001 From: Dmitry Prudnikov Date: Sun, 1 Nov 2026 17:59:01 +0200 Subject: [PATCH] fix(pq): clear SHAKE state on drop - Enable shake/zeroize from the experimental-pq feature, so a downstream crate gets the clearing without asking for it - Qualify the feature on an isolated consumer, because dev-dependencies enable it in every workspace build - Build experimental-pq without std, which the feature table promises and a missing import broke - Stop ML-KEM tests from assuming a randomness source a build without signing does not have, and pin the refusal such a build answers with Closes #292 --- .github/workflows/ci.yml | 4 + Cargo.toml | 2 +- docs/ml-dsa-patch.md | 5 ++ src/key_establishment.rs | 8 +- src/provider/key_encapsulation/rustcrypto.rs | 18 ++++ tests/shake_state_clearing.rs | 24 ++++++ tools/xml-sec-tools/src/check_features.rs | 84 ++++++++++++++++++- .../xml-sec-tools/src/check_features/tests.rs | 72 +++++++++++++++- 8 files changed, 210 insertions(+), 7 deletions(-) create mode 100644 tests/shake_state_clearing.rs diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index f9ba90cd..14787deb 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -287,6 +287,10 @@ jobs: cargo clippy -p xml-sec --lib --no-default-features --features cms,xml-backend-xmloxide --target thumbv7em-none-eabihf -- -D warnings + - run: >- + cargo clippy -p xml-sec --lib --no-default-features + --features experimental-pq,xml-backend-roxmltree + --target thumbv7em-none-eabihf -- -D warnings - run: >- cargo clippy -p xml-sec --lib --no-default-features --features dstu4145,cms,xml-backend-xmloxide diff --git a/Cargo.toml b/Cargo.toml index 7be463d3..7406f898 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -328,7 +328,7 @@ common = { package = "crypto-common", version = "0.2", default-features = false, ctutils = { version = "0.4", default-features = false, optional = true } hybrid-array = { version = "0.4", default-features = false, features = ["extra-sizes", "alloc", "zeroize"], optional = true } module-lattice = { version = "0.2.3", default-features = false, features = ["ctutils", "alloc", "zeroize"], optional = true } -shake = { version = "0.1", default-features = false, optional = true } +shake = { version = "0.1", default-features = false, features = ["zeroize"], optional = true } const-oid = { version = "0.10", default-features = false, features = ["db"], optional = true } slh-dsa = { version = "=0.2.0-rc.5", default-features = false, features = ["alloc", "zeroize"], optional = true } ml-kem = { version = "0.3.2", default-features = false, features = ["alloc", "zeroize"], optional = true } diff --git a/docs/ml-dsa-patch.md b/docs/ml-dsa-patch.md index c68a0455..d77941c5 100644 --- a/docs/ml-dsa-patch.md +++ b/docs/ml-dsa-patch.md @@ -28,6 +28,11 @@ Expanded-only keys are exported as expanded keys, never a fabricated seed. Secret staging buffers are zeroized. All public signing and verification still pass through `CryptoProvider`. +`experimental-pq` enables `shake/zeroize`, so the `Shake` and `ShakeReader` values +clear their own state and cursor when dropped. This is dependency-owned clearing of +those values only: it does not reach compiler-generated copies or other temporaries, +and it is no claim of complete secret erasure. + Namespace, feature and Rust 1.92 lint adaptations let the same algorithm live as a private module. They do not change cryptographic behavior. Inspect the generated delta in `scripts/patches/ml-dsa-0.1.1.patch` when updating the donor. diff --git a/src/key_establishment.rs b/src/key_establishment.rs index 40d006cc..98d8c7e3 100644 --- a/src/key_establishment.rs +++ b/src/key_establishment.rs @@ -313,7 +313,11 @@ mod tests { "failed-random" } fn supports(&self, capability: ProviderCapability<'_>) -> bool { - RustCryptoProvider.supports(capability) + // Its own source, which fails, exists in every build. + matches!( + capability, + ProviderCapability::Random | ProviderCapability::Encapsulate(_) + ) || RustCryptoProvider.supports(capability) } fn fill_random(&self, _: &mut [u8]) -> Result<(), ProviderError> { Err(ProviderError::Random("test entropy failure".into())) @@ -431,7 +435,7 @@ mod tests { )); assert_eq!(usage.encapsulation_operations, 1); assert!(matches!( - usage.encapsulate(&policy, &RustCryptoProvider, &key), + usage.encapsulate(&policy, &FailedRandom, &key), Err(KeyEstablishmentError::Policy( PolicyViolation::ResourceLimit { .. } )) diff --git a/src/provider/key_encapsulation/rustcrypto.rs b/src/provider/key_encapsulation/rustcrypto.rs index 739e2e60..c994648b 100644 --- a/src/provider/key_encapsulation/rustcrypto.rs +++ b/src/provider/key_encapsulation/rustcrypto.rs @@ -5,6 +5,7 @@ use super::{ KeyEncapsulationKey, ProviderError, Zeroizing, }; use crate::provider::{ProviderCapability, ProviderInputError}; +use alloc::vec::Vec; use der::{ AnyRef, Decode, Encode, Reader, Tag, TagMode, TagNumber, Tagged, asn1::{BitStringRef, ContextSpecific, OctetStringRef}, @@ -546,6 +547,22 @@ mod tests { KeyEncapsulationAlgorithm::MlKem1024, ]; + #[cfg(not(feature = "xmldsig-sign"))] + #[test] + fn encapsulation_without_a_randomness_source_is_unsupported() { + // FIPS 203 section 7.2 needs fresh m, and a build without the bundled + // source has none: refusing is the only answer that is not a weak key. + // https://doi.org/10.6028/NIST.FIPS.203 + let key = RustCryptoMlKemPrivateKey::from_seed(ALGORITHMS[0], &[42; 64]) + .expect("seed") + .public_key(); + assert!(matches!( + RustCryptoProvider.encapsulate_key(&key), + Err(ProviderError::Unsupported { .. }) + )); + } + + #[cfg(feature = "xmldsig-sign")] #[test] fn all_parameter_sets_roundtrip_every_private_encoding() { // RFC 9935 section 6 permits every CHOICE branch, not just the preferred seed. @@ -616,6 +633,7 @@ mod tests { } } + #[cfg(feature = "xmldsig-sign")] #[test] fn valid_size_invalid_ciphertext_is_implicitly_rejected() { // FIPS 203 section 7.3 returns a deterministic fallback secret, never an oracle bit. diff --git a/tests/shake_state_clearing.rs b/tests/shake_state_clearing.rs new file mode 100644 index 00000000..1c4fb69c --- /dev/null +++ b/tests/shake_state_clearing.rs @@ -0,0 +1,24 @@ +//! The SHAKE state of the embedded ML-DSA code is cleared when it is dropped. +//! +//! A workspace build cannot detect a missing `shake/zeroize`, because +//! dev-dependencies enable it too; `check-features experimental` proves it on an +//! isolated consumer. This target pins the marker the feature must produce. +#![cfg(feature = "experimental-pq")] + +use shake::{Shake128, Shake128Reader, Shake256, Shake256Reader}; +use zeroize::ZeroizeOnDrop; + +const fn clears_on_drop() {} + +const _: () = { + clears_on_drop::(); + clears_on_drop::(); +}; + +#[test] +fn readers_clear_through_their_own_drop() { + // A reader has no marker: its Drop always exists and clears under the same + // feature as the hasher's, so the assertion above stands for both. + assert!(core::mem::needs_drop::()); + assert!(core::mem::needs_drop::()); +} diff --git a/tools/xml-sec-tools/src/check_features.rs b/tools/xml-sec-tools/src/check_features.rs index 93f242ae..d14df940 100644 --- a/tools/xml-sec-tools/src/check_features.rs +++ b/tools/xml-sec-tools/src/check_features.rs @@ -2,7 +2,9 @@ //! feature profile of the core package alone. Workspace feature unification //! could otherwise hide a broken gate. -use crate::repo::{Failure, Run, Step, System, TOOL, require}; +use crate::repo::{Failure, Run, Step, System, TOOL, require, root}; +use serde_json::Value; +use std::path::Path; #[cfg(test)] mod tests; @@ -30,6 +32,9 @@ const CMS_FORBIDDEN: &[&str] = VERIFY_ONLY_FORBIDDEN const BACKEND_DIAGNOSTIC: &str = "compile at least one XML backend"; +/// What the embedded post-quantum code relies on from its dependencies. +const EXPERIMENTAL_CONSUMER: [(&str, &str); 1] = [("shake", "zeroize")]; + #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub(crate) enum Phase { All, @@ -63,6 +68,9 @@ pub(crate) enum Action { VerifyOnlyGraph(Step, &'static [&'static str]), /// A build that must fail with the missing-backend diagnostic. MissingBackend(Step), + /// `cargo metadata` of a throwaway consumer, whose graph must enable each + /// `(package, feature)` pair without asking for it. + ConsumerFeatures(Step, &'static [(&'static str, &'static str)]), } struct Profile { @@ -136,7 +144,7 @@ const PROFILES: [Profile; 17] = [ profile( "experimental", Some("experimental-pq,xmldsig-sign,xml-backend-xmloxide"), - &[], + &["shake_state_clearing"], true, ), profile( @@ -227,6 +235,16 @@ pub(crate) fn plan( forbidden, )); } + if phase.includes(Phase::Lint) && profile == "experimental" { + actions.push(Action::ConsumerFeatures( + Step::new( + "consumer graph", + "cargo", + &["metadata", "--format-version", "1"], + ), + &EXPERIMENTAL_CONSUMER, + )); + } if phase.includes(Phase::Lint) { let clippy = [ &["clippy"][..], @@ -263,6 +281,50 @@ pub(crate) fn forbidden_crate(graph: &str, forbidden: &[&'static str]) -> Option }) } +/// A manifest outside the workspace that depends on `root` like a downstream +/// crate: no dev-dependency or workspace member can unify features into it. +fn consumer_manifest(root: &Path) -> String { + format!( + "[package]\nname = \"feature-consumer\"\nversion = \"0.0.0\"\nedition = \"2024\"\n\n\ + [lib]\npath = \"lib.rs\"\n\n[workspace]\n\n[dependencies]\n\ + xml-sec = {{ path = '{}', default-features = false, \ + features = [\"experimental-pq\", \"xml-backend-roxmltree\"] }}\n", + root.display() + ) +} + +/// The first `(package, feature)` of `required` that `metadata`, the output of +/// `cargo metadata`, leaves off. A package missing from the graph counts: an +/// absent crate must not pass as a clean one. +fn missing_feature( + metadata: &str, + required: &[(&'static str, &'static str)], +) -> Result, String> { + let metadata: Value = + serde_json::from_str(metadata).map_err(|error| format!("parse cargo metadata: {error}"))?; + let list = |value: Option<&Value>| value.and_then(Value::as_array).cloned().unwrap_or_default(); + let packages = list(metadata.get("packages")); + let nodes = list(metadata.pointer("/resolve/nodes")); + Ok(required.iter().copied().find(|(package, feature)| { + let mut identities = packages + .iter() + .filter(|candidate| candidate.get("name").and_then(Value::as_str) == Some(package)) + .filter_map(|candidate| candidate.get("id")) + .peekable(); + identities.peek().is_none() + || identities.any(|identity| { + !nodes + .iter() + .filter(|node| node.get("id") == Some(identity)) + .any(|node| { + list(node.get("features")) + .iter() + .any(|name| name.as_str() == Some(feature)) + }) + }) + })) +} + /// Executes `actions` in order; the first failure stops qualification. pub(crate) fn execute(actions: &[Action], runner: &mut impl Run) -> Result<(), Failure> { for action in actions { @@ -275,6 +337,24 @@ pub(crate) fn execute(actions: &[Action], runner: &mut impl Run) -> Result<(), F return Err(format!("verification-only graph contains {name}").into()); } } + Action::ConsumerFeatures(step, required) => { + let consumer = tempfile::tempdir().map_err(|error| error.to_string())?; + let write = |name: &str, text: String| { + std::fs::write(consumer.path().join(name), text) + .map_err(|error| error.to_string()) + }; + write("Cargo.toml", consumer_manifest(&root()))?; + write("lib.rs", String::new())?; + let (status, metadata) = + runner.stdout(&step.clone().in_directory(consumer.path()))?; + require(status)?; + if let Some((package, feature)) = missing_feature(&metadata, required)? { + return Err(format!( + "a downstream consumer does not enable {package}/{feature}" + ) + .into()); + } + } Action::MissingBackend(step) => { let (status, diagnostic) = runner.combined(step)?; if status.success() { diff --git a/tools/xml-sec-tools/src/check_features/tests.rs b/tools/xml-sec-tools/src/check_features/tests.rs index f78d5fd3..b2506996 100644 --- a/tools/xml-sec-tools/src/check_features/tests.rs +++ b/tools/xml-sec-tools/src/check_features/tests.rs @@ -1,6 +1,10 @@ -use super::{Action, VERIFY_ONLY_FORBIDDEN, execute, forbidden_crate, plan}; +use super::{ + Action, VERIFY_ONLY_FORBIDDEN, consumer_manifest, execute, forbidden_crate, missing_feature, + plan, +}; use crate::repo::{Failure, Run, Step}; use std::os::unix::process::ExitStatusExt as _; +use std::path::Path; use std::process::ExitStatus; /// The planned command lines, as the original runner's Cargo log recorded them. @@ -10,7 +14,8 @@ fn lines(actions: &[Action]) -> Vec { .map(|action| { let (Action::Run(step) | Action::VerifyOnlyGraph(step, _) - | Action::MissingBackend(step)) = action; + | Action::MissingBackend(step) + | Action::ConsumerFeatures(step, _)) = action; step.args.join(" ") }) .collect() @@ -228,6 +233,9 @@ fn each_minimal_profile_selects_only_its_declared_feature_graph() { "tree -p xml-sec --no-default-features --features {features} -e normal --prefix none --format {{p}}" )); } + if profile == "experimental" { + expected.push("metadata --format-version 1".to_owned()); + } expected.push(format!( "clippy -p xml-sec --no-default-features --features {features} --all-targets -- -D warnings" )); @@ -235,6 +243,66 @@ fn each_minimal_profile_selects_only_its_declared_feature_graph() { } } +/// `cargo metadata` output naming `shake` with `features`. +fn shake_metadata(features: &str) -> String { + format!( + r#"{{"packages":[{{"name":"shake","id":"id#shake"}},{{"name":"other","id":"id#other"}}], + "resolve":{{"nodes":[{{"id":"id#shake","features":[{features}]}}, + {{"id":"id#other","features":["zeroize"]}}]}}}}"# + ) +} + +#[test] +fn a_consumer_without_shake_state_clearing_fails_qualification() { + // Another crate's feature of the same name must not pass for shake's; this + // is the graph a downstream crate resolved before the manifest asked for it. + let actions = plan("experimental", "lint", None).unwrap(); + assert!(matches!(actions[0], Action::ConsumerFeatures(..))); + let mut runner = Recorded { + output: shake_metadata(""), + ..Recorded::default() + }; + assert_eq!( + execute(&actions, &mut runner).unwrap_err(), + Failure { + code: 1, + message: "a downstream consumer does not enable shake/zeroize".to_owned(), + } + ); + assert_eq!(runner.calls, 1); +} + +#[test] +fn a_consumer_graph_without_the_package_is_no_proof() { + // A renamed or dropped dependency must fail the check, not satisfy it. + assert_eq!( + missing_feature( + r#"{"packages":[],"resolve":{"nodes":[]}}"#, + &[("shake", "zeroize")] + ), + Ok(Some(("shake", "zeroize"))) + ); + assert!(missing_feature("not json", &[("shake", "zeroize")]).is_err()); +} + +#[test] +fn a_consumer_with_shake_state_clearing_passes() { + assert_eq!( + missing_feature(&shake_metadata(r#""zeroize""#), &[("shake", "zeroize")]), + Ok(None) + ); +} + +#[test] +fn the_consumer_stands_outside_the_workspace() { + // Its own empty workspace table is what stops feature unification, and + // default features stay off as they do for a minimal downstream crate. + let manifest = consumer_manifest(Path::new("/repository")); + assert!(manifest.contains("\n[workspace]\n")); + assert!(manifest.contains("path = '/repository', default-features = false")); + assert!(!manifest.contains("dev-dependencies")); +} + #[test] fn the_dstu_profile_runs_its_interoperability_tests_and_no_signing_facade() { // The feature only verifies, so the signing facade has no place here, and