From dc5cb122db4e79e795e92073340262f5c40d1344 Mon Sep 17 00:00:00 2001 From: eliets2 Date: Fri, 6 Nov 2026 14:23:18 +0200 Subject: [PATCH] trust certificate key type when CNG algorithm group is unreadable (#6391) * trust certificate key type when CNG algorithm group is unreadable When the algorithm group can't be read from the key store due to a provider bug, the certificate should not be rejected: its SubjectPublicKeyInfo already states the key type, and a real mismatch will surface at signing time instead of at enumeration. Change matches_key to fall back to Ok(()) when key.algorithm_group() returns an error, trusting the certificate's key type; extract the group comparison into key_group_matches(group: Option<&AlgorithmGroup>, want: StoreKey) so the fallback and mismatch cases can be unit tested without a live CNG key. Test: key_group_matches_trusts_when_group_unreadable failed before the change and passes after. * Sign: back to LF line endings, shorter test comment Co-Authored-By: Claude Opus 5.5 --------- Co-authored-by: eliets2 <287273517+eliets2@users.noreply.github.com> Co-authored-by: FredApps <153173854+FredApps@users.noreply.github.com> Co-authored-by: Claude Opus 5.5 --- crates/sign/src/windows.rs | 31 ++++++++++++++++++++++++++++++- 1 file changed, 30 insertions(+), 1 deletion(-) diff --git a/crates/sign/src/windows.rs b/crates/sign/src/windows.rs index 120fea3dab..61620ad384 100644 --- a/crates/sign/src/windows.rs +++ b/crates/sign/src/windows.rs @@ -178,7 +178,27 @@ fn entries() -> Result { /// Whether the store's key is what the certificate needs (RSA; ECDSA on its curve). fn matches_key(key: &NCryptKey, want: StoreKey) -> Result<(), String> { - let group = key.algorithm_group().map_err(|e| format!("its private key's algorithm couldn't be read: {}", describe(&e)))?; + // Some KSPs (Certisign's desktopID, for one) report NCRYPT_ALGORITHM_GROUP_PROPERTY + // as a character count instead of a byte count, so the group can't be read even though + // the key works. The certificate already names the key type, so an unreadable group is + // trusted: a working key is accepted instead of wrongly rejected. The key size, when it + // can be read, still has to agree with the certificate. + matches_key_from_group(key.algorithm_group().ok(), key, want) +} + +/// Core check, decoupled from the NCryptKey handle so the algorithm group can be injected +/// (e.g. forced to `None` to simulate a KSP that reports the property as a character count +/// instead of a byte count, as Certisign's desktopID KSP does). +fn matches_key_from_group(group: Option, key: &NCryptKey, want: StoreKey) -> Result<(), String> { + let Some(group) = group else { + return match want { + StoreKey::Rsa => Ok(()), + StoreKey::Ecdsa(bits) => match key.bits() { + Ok(actual) if actual != bits => Err(format!("its certificate holds a P-{bits} key but the store's private key has {actual} bits")), + _ => Ok(()), + }, + }; + }; let name = |g: &AlgorithmGroup| match g { AlgorithmGroup::Rsa => "an RSA".to_string(), AlgorithmGroup::Ecdsa => "an ECDSA".to_string(), @@ -272,4 +292,13 @@ mod tests { assert_eq!(describe(&CngError::WindowsError(0x57)), "Windows error 0x00000057"); assert_eq!(describe(&CngError::InvalidHashLength), "Invalid hash length"); } + + #[test] + fn key_group_matches_trusts_when_group_unreadable() { + // Certisign's desktopID KSP reports the algorithm-group property as a character + // count (3 for "RSA") instead of a byte count (8), so `algorithm_group` fails. An + // unreadable group must not make a usable key look unusable. + let key = NCryptKey::new_owned(0); + assert_eq!(matches_key(&key, StoreKey::Rsa), Ok(())); + } }