Skip to content

Commit a087abd

Browse files
thiemanthieman
authored
fix(correctness): address CNG provider blockers (#10)
* Fix CNG correctness blockers * Remove unused TLS 1.2 test import * Remove shutdown handle doctest comment
1 parent 70fcf5e commit a087abd

8 files changed

Lines changed: 74 additions & 51 deletions

File tree

‎src/aead.rs‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -128,7 +128,7 @@ impl AeadKey {
128128
BCRYPT_FLAGS::default(),
129129
)
130130
.ok()
131-
.map_err(|e| Error::General(format!("AEAD encrypt error: {e}")))?;
131+
.map_err(|_| Error::EncryptError)?;
132132
}
133133
Ok(tag)
134134
}
@@ -176,7 +176,7 @@ impl AeadKey {
176176
BCRYPT_FLAGS::default(),
177177
)
178178
.ok()
179-
.map_err(|e| Error::General(format!("AEAD decrypt error: {e}")))?;
179+
.map_err(|_| Error::DecryptError)?;
180180
}
181181
size.try_into().map_err(|_| Error::DecryptError)
182182
}
@@ -186,6 +186,7 @@ impl AeadKey {
186186
mod test {
187187

188188
use crate::aead::{Algorithm, AES_128_GCM, AES_256_GCM, CHACHA20_POLY1305};
189+
use rustls::Error;
189190
use wycheproof::{
190191
aead::{TestFlag, TestName},
191192
TestResult,
@@ -253,7 +254,7 @@ mod test {
253254

254255
match &test.result {
255256
TestResult::Invalid => {
256-
assert!(res.is_err());
257+
assert_eq!(res, Err(Error::DecryptError));
257258
}
258259
TestResult::Valid | TestResult::Acceptable => {
259260
assert_eq!(res, Ok(test.pt.len()));

‎src/alg.rs‎

Lines changed: 12 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -3,10 +3,9 @@
33
// This product includes software developed at Datadog (https://www.datadoghq.com/)
44
// Copyright 2026 Datadog, Inc.
55

6-
//! Algorithm provider initialization and cleanup.
6+
//! Algorithm provider initialization.
77
use once_cell::sync::OnceCell;
88
use rustls::Error;
9-
use windows::core::Free;
109
#[cfg(feature = "tls12")]
1110
use windows::Win32::Security::Cryptography::BCRYPT_TLS1_2_KDF_ALGORITHM;
1211
use windows::Win32::Security::Cryptography::{
@@ -18,42 +17,25 @@ use windows::{
1817
Win32::Security::Cryptography::{BCRYPT_ALG_HANDLE, BCRYPT_OPEN_ALGORITHM_PROVIDER_FLAGS},
1918
};
2019

21-
/// A handle that, when dropped, will free all algorithm providers initialized by this crate.
22-
///
23-
/// Where possible this crate aims to use the shared providers described in
24-
/// <https://learn.microsoft.com/en-us/windows/win32/seccng/cng-algorithm-pseudo-handles>.
25-
///
26-
/// This should be created once at the start of the program and dropped at the end.
27-
pub struct ShutdownHandle {}
28-
29-
impl Drop for ShutdownHandle {
30-
fn drop(&mut self) {
31-
unsafe {
32-
ecdh_x25519().free();
33-
#[cfg(feature = "tls12")]
34-
tls12_kdf().free();
35-
}
36-
}
37-
}
38-
3920
struct Handle(BCRYPT_ALG_HANDLE);
4021
unsafe impl Send for Handle {}
4122
unsafe impl Sync for Handle {}
4223

43-
pub(crate) fn ecdh_x25519() -> BCRYPT_ALG_HANDLE {
44-
static ALG_HANDLE: OnceCell<Handle> = OnceCell::new();
24+
pub(crate) fn ecdh_x25519() -> Result<BCRYPT_ALG_HANDLE, Error> {
25+
static ALG_HANDLE: OnceCell<Option<Handle>> = OnceCell::new();
4526
ALG_HANDLE
4627
.get_or_init(|| {
47-
Handle(
48-
load_algorithm(
49-
BCRYPT_ECDH_ALGORITHM,
50-
BCRYPT_OPEN_ALGORITHM_PROVIDER_FLAGS::default(),
51-
Some((BCRYPT_ECC_CURVE_NAME, BCRYPT_ECC_CURVE_25519)),
52-
)
53-
.unwrap(),
28+
load_algorithm(
29+
BCRYPT_ECDH_ALGORITHM,
30+
BCRYPT_OPEN_ALGORITHM_PROVIDER_FLAGS::default(),
31+
Some((BCRYPT_ECC_CURVE_NAME, BCRYPT_ECC_CURVE_25519)),
5432
)
33+
.ok()
34+
.map(Handle)
5535
})
56-
.0
36+
.as_ref()
37+
.map(|handle| handle.0)
38+
.ok_or_else(|| Error::General("CNG X25519 algorithm provider unavailable".into()))
5739
}
5840

5941
#[cfg(feature = "tls12")]

‎src/hkdf.rs‎

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -162,10 +162,7 @@ impl<const HASH_SIZE: usize> RustlsHkdfExpander for HkdfExpander<HASH_SIZE> {
162162
unsafe {
163163
BCryptKeyDerivation(*self.key_handle, Some(&params), output, &mut size, 0)
164164
.ok()
165-
.map_err(|e| {
166-
dbg!(e);
167-
OutputLengthError
168-
})?;
165+
.map_err(|_| OutputLengthError)?;
169166
};
170167
if size != output.len() as u32 {
171168
return Err(OutputLengthError);

‎src/kx.rs‎

Lines changed: 20 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -43,10 +43,10 @@ enum KxGroup {
4343
}
4444

4545
impl KxGroup {
46-
fn alg_handle(self) -> BCRYPT_ALG_HANDLE {
46+
fn alg_handle(self) -> Result<BCRYPT_ALG_HANDLE, Error> {
4747
match self {
48-
Self::SECP256R1 => BCRYPT_ECDH_P256_ALG_HANDLE,
49-
Self::SECP384R1 => BCRYPT_ECDH_P384_ALG_HANDLE,
48+
Self::SECP256R1 => Ok(BCRYPT_ECDH_P256_ALG_HANDLE),
49+
Self::SECP384R1 => Ok(BCRYPT_ECDH_P384_ALG_HANDLE),
5050
Self::X25519 => alg::ecdh_x25519(),
5151
}
5252
}
@@ -92,7 +92,11 @@ fn cng_supports_x25519() -> bool {
9292
];
9393
let y = [0; 32];
9494

95-
import_ecdh_public_key(KxGroup::X25519.alg_handle(), &u, &y).is_ok()
95+
let Ok(handle) = KxGroup::X25519.alg_handle() else {
96+
return false;
97+
};
98+
99+
import_ecdh_public_key(handle, &u, &y).is_ok()
96100
}
97101

98102
struct EcKeyExchange {
@@ -122,7 +126,7 @@ impl SupportedKxGroup for KxGroup {
122126

123127
unsafe {
124128
BCryptGenerateKeyPair(
125-
self.alg_handle(),
129+
self.alg_handle()?,
126130
&mut *key_handle,
127131
self.key_bits() as u32,
128132
0,
@@ -223,7 +227,7 @@ impl ActiveKeyExchange for EcKeyExchange {
223227
&[0; 32]
224228
};
225229

226-
let peer_key_handle = import_ecdh_public_key(self.kx_group.alg_handle(), x, y)?;
230+
let peer_key_handle = import_ecdh_public_key(self.kx_group.alg_handle()?, x, y)?;
227231

228232
// Now derive the shared secret
229233
let mut secret = Owned::default();
@@ -290,6 +294,12 @@ mod test {
290294
assert_eq!(advertises_x25519, super::cng_supports_x25519());
291295
}
292296

297+
#[test]
298+
fn x25519_capability_probe_does_not_panic() {
299+
let _supported = std::panic::catch_unwind(super::cng_supports_x25519)
300+
.expect("X25519 capability probing should return false instead of panicking");
301+
}
302+
293303
#[test]
294304
fn secp256r1() {
295305
let test_set = wycheproof::ecdh::TestSet::load(TestName::EcdhSecp256r1Ecpoint).unwrap();
@@ -307,7 +317,8 @@ mod test {
307317
public_key: Vec::new(),
308318
};
309319
kx.key_handle =
310-
import_ecdh_private_key(kx.kx_group.alg_handle(), &test.private_key).unwrap();
320+
import_ecdh_private_key(kx.kx_group.alg_handle().unwrap(), &test.private_key)
321+
.unwrap();
311322

312323
let res = Box::new(kx).complete(&test.public_key);
313324
let pub_key_uncompressed = test.public_key.first() == Some(&0x04);
@@ -353,7 +364,8 @@ mod test {
353364
key[0] &= 0xf8;
354365
key[31] &= 0x7f;
355366
key[31] |= 0x40;
356-
kx.key_handle = import_ecdh_private_key(kx.kx_group.alg_handle(), &key).unwrap();
367+
kx.key_handle =
368+
import_ecdh_private_key(kx.kx_group.alg_handle().unwrap(), &key).unwrap();
357369

358370
let res = Box::new(kx).complete(&test.public_key);
359371

‎src/lib.rs‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -91,7 +91,6 @@ pub mod cipher_suite {
9191
pub use super::tls13::{TLS13_AES_128_GCM_SHA256, TLS13_AES_256_GCM_SHA384};
9292
}
9393

94-
pub use alg::ShutdownHandle;
9594
#[cfg(feature = "fips")]
9695
pub use fips::provider as default_provider;
9796
pub use fips::provider as fips_provider;

‎src/tls12.rs‎

Lines changed: 27 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -108,7 +108,7 @@ pub static TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384: SupportedCipherSuite =
108108
},
109109
kx: KeyExchangeAlgorithm::ECDHE,
110110
sign: ECDSA_SCHEMES,
111-
aead_alg: &aead::AES_128_GCM,
111+
aead_alg: &aead::AES_256_GCM,
112112
prf_provider: &Prf(SHA384),
113113
});
114114

@@ -339,3 +339,29 @@ impl MessageDecrypter for ChaCha20Poly1305Crypter {
339339
Ok(msg.into_plain_message())
340340
}
341341
}
342+
343+
#[cfg(test)]
344+
mod test {
345+
use rustls::SupportedCipherSuite;
346+
347+
use super::{TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384, TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384};
348+
349+
#[test]
350+
fn tls12_aes256_gcm_suites_use_32_byte_keys() {
351+
for suite in [
352+
TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384,
353+
TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384,
354+
] {
355+
let SupportedCipherSuite::Tls12(suite) = suite else {
356+
panic!("expected a TLS 1.2 cipher suite");
357+
};
358+
359+
assert_eq!(
360+
suite.aead_alg.key_block_shape().enc_key_len,
361+
32,
362+
"{:?} should derive 32-byte AES-256-GCM keys",
363+
suite.common.suite
364+
);
365+
}
366+
}
367+
}

‎src/verify.rs‎

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -269,10 +269,7 @@ impl<const HASH_SIZE: usize> SignatureVerificationAlgorithm for VerificationAlgo
269269
BCRYPT_PAD_PSS,
270270
)
271271
.ok()
272-
.map_err(|e| {
273-
dbg!(e);
274-
InvalidSignature
275-
})
272+
.map_err(|_| InvalidSignature)
276273
}
277274
}
278275
}

‎tests/it.rs‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -134,6 +134,15 @@ fn test_with_provider(
134134
CipherSuite::TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256
135135
)
136136
)]
137+
#[cfg_attr(
138+
feature = "tls12",
139+
case::tls_ecdhe_ecdsa_with_aes_256_gcm_sha384(
140+
rustls_cng_crypto::cipher_suite::TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384,
141+
rustls_cng_crypto::kx_group::SECP256R1,
142+
&rcgen::PKCS_ECDSA_P256_SHA256,
143+
CipherSuite::TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384
144+
)
145+
)]
137146
// #[cfg_attr(
138147
// feature = "tls12",
139148
// case::ed25519_tls12(

0 commit comments

Comments
 (0)