From c4f62fa064ce41f28d51bbd692e73ea10f3264bf Mon Sep 17 00:00:00 2001 From: Fabian Kaczmarczyck Date: Wed, 9 Aug 2023 17:27:01 +0200 Subject: [PATCH] Cleans up legacy credential parsing This is a backwards incompatible change. This PR already introduces backwards incompatible new credential parsing, and therefore we can also remove all other legacy parsing. --- libraries/opensk/src/api/key_store.rs | 94 +++------------------- libraries/opensk/src/api/private_key.rs | 7 +- libraries/opensk/src/ctap/ctap1.rs | 11 ++- libraries/opensk/src/ctap/data_formats.rs | 95 +---------------------- 4 files changed, 20 insertions(+), 187 deletions(-) diff --git a/libraries/opensk/src/api/key_store.rs b/libraries/opensk/src/api/key_store.rs index ede4901b..f3290102 100644 --- a/libraries/opensk/src/api/key_store.rs +++ b/libraries/opensk/src/api/key_store.rs @@ -212,22 +212,18 @@ impl KeyStore for T { return Ok(None); } - let credential_source = if bytes.len() == LEGACY_CREDENTIAL_ID_SIZE { - decrypt_legacy_credential_id::(&master_keys.encryption, &bytes[..hmac_message_size])? - } else { - match bytes[0] { - CBOR_CREDENTIAL_ID_VERSION => { - if bytes.len() != CBOR_CREDENTIAL_ID_SIZE { - return Ok(None); - } - decrypt_cbor_credential_id::( - self, - &master_keys.encryption, - &bytes[1..hmac_message_size], - )? + let credential_source = match bytes[0] { + CBOR_CREDENTIAL_ID_VERSION => { + if bytes.len() != CBOR_CREDENTIAL_ID_SIZE { + return Ok(None); } - _ => return Ok(None), + decrypt_cbor_credential_id::( + self, + &master_keys.encryption, + &bytes[1..hmac_message_size], + )? } + _ => return Ok(None), }; if let Some(credential_source) = &credential_source { @@ -326,30 +322,6 @@ fn remove_padding(data: &[u8]) -> Result<&[u8], Error> { Ok(&data[..data.len() - pad_length as usize]) } -fn decrypt_legacy_credential_id( - encryption_key_bytes: &[u8; 32], - bytes: &[u8], -) -> Result, Error> { - let aes_key = AesKey::::new(encryption_key_bytes); - let plaintext = aes256_cbc_decrypt::(&aes_key, bytes, true) - .map_err(|_| Error)? - .expose_secret_to_vec(); - if plaintext.len() != 64 { - return Ok(None); - } - let private_key = if let Some(key) = PrivateKey::new_ecdsa_from_bytes(&plaintext[..32]) { - key - } else { - return Ok(None); - }; - Ok(Some(CredentialSource { - private_key, - rp_id_hash: plaintext[32..64].try_into().unwrap(), - cred_protect_policy: None, - cred_blob: None, - })) -} - fn decrypt_cbor_credential_id( env: &mut E, encryption_key_bytes: &[u8; 32], @@ -591,52 +563,6 @@ mod test { test_wrap_unwrap_credential_missing_blocks(SignatureAlgorithm::Eddsa); } - /// This is a copy of the function that genereated deprecated key handles. - fn legacy_encrypt_to_credential_id( - env: &mut TestEnv, - private_key: EcdsaSk, - application: &[u8; 32], - ) -> Result, Error> { - let master_keys = get_master_keys(env).unwrap(); - let aes_key = AesKey::::new(&master_keys.encryption); - let hmac_key = master_keys.authentication; - let mut plaintext = [0; 64]; - private_key.to_slice(array_mut_ref!(plaintext, 0, 32)); - plaintext[32..64].copy_from_slice(application); - - let mut encrypted_id = aes256_cbc_encrypt::(env.rng(), &aes_key, &plaintext, true) - .map_err(|_| Error)?; - let mut id_hmac = [0; HASH_SIZE]; - Hmac::::mac(&hmac_key, &encrypted_id[..], &mut id_hmac); - encrypted_id.extend(&id_hmac); - Ok(encrypted_id) - } - - #[test] - fn test_encrypt_decrypt_credential_legacy() { - let mut env = TestEnv::default(); - let private_key = PrivateKey::new(&mut env, SignatureAlgorithm::Es256); - let rp_id_hash = [0x55; 32]; - let credential_source = CredentialSource { - private_key, - rp_id_hash, - cred_protect_policy: None, - cred_blob: None, - }; - let ecdsa_key = credential_source - .private_key - .ecdsa_key::() - .unwrap(); - let credential_id = - legacy_encrypt_to_credential_id(&mut env, ecdsa_key, &rp_id_hash).unwrap(); - let unwrapped = env - .key_store() - .unwrap_credential(&credential_id, &rp_id_hash) - .unwrap() - .unwrap(); - assert_eq!(credential_source, unwrapped); - } - #[test] fn test_wrap_credential_size() { let mut env = TestEnv::default(); diff --git a/libraries/opensk/src/api/private_key.rs b/libraries/opensk/src/api/private_key.rs index b4e5789e..1125b31e 100644 --- a/libraries/opensk/src/api/private_key.rs +++ b/libraries/opensk/src/api/private_key.rs @@ -69,9 +69,7 @@ impl PrivateKey { } /// Helper function that creates a private key of type ECDSA. - /// - /// This function is public for legacy credential source parsing only. - pub fn new_ecdsa_from_bytes(bytes: &[u8]) -> Option { + fn new_ecdsa_from_bytes(bytes: &[u8]) -> Option { if bytes.len() != 32 { return None; } @@ -80,8 +78,9 @@ impl PrivateKey { Some(PrivateKey::Ecdsa(seed)) } + /// Helper function that creates a private key of type Ed25519. #[cfg(feature = "ed25519")] - pub fn new_ed25519_from_bytes(bytes: &[u8]) -> Option { + fn new_ed25519_from_bytes(bytes: &[u8]) -> Option { if bytes.len() != 32 { return None; } diff --git a/libraries/opensk/src/ctap/ctap1.rs b/libraries/opensk/src/ctap/ctap1.rs index 0d81535b..bb9d326e 100644 --- a/libraries/opensk/src/ctap/ctap1.rs +++ b/libraries/opensk/src/ctap/ctap1.rs @@ -333,10 +333,6 @@ impl Ctap1Command { .map_err(|_| Ctap1StatusCode::SW_WRONG_DATA)?; let credential_source = filter_listed_credential(credential_source, false) .ok_or(Ctap1StatusCode::SW_WRONG_DATA)?; - let ecdsa_key = credential_source - .private_key - .ecdsa_key::() - .map_err(|_| Ctap1StatusCode::SW_WRONG_DATA)?; if flags == Ctap1Flags::CheckOnly { return Err(Ctap1StatusCode::SW_COND_USE_NOT_SATISFIED); } @@ -351,10 +347,13 @@ impl Ctap1Command { ) .map_err(|_| Ctap1StatusCode::SW_WRONG_DATA)?; signature_data.extend(&challenge); - let signature = ecdsa_key.sign(&signature_data); + let signature = credential_source + .private_key + .sign_and_encode::(&signature_data) + .map_err(|_| Ctap1StatusCode::SW_INTERNAL_EXCEPTION)?; let mut response = signature_data[application.len()..application.len() + 5].to_vec(); - response.extend(signature.to_der()); + response.extend(&signature); Ok(response) } } diff --git a/libraries/opensk/src/ctap/data_formats.rs b/libraries/opensk/src/ctap/data_formats.rs index 5620a7e5..c7ccb6d4 100644 --- a/libraries/opensk/src/ctap/data_formats.rs +++ b/libraries/opensk/src/ctap/data_formats.rs @@ -602,8 +602,6 @@ pub struct PublicKeyCredentialSource { // is associated with a unique tag, implemented with a CBOR unsigned key. enum PublicKeyCredentialSourceField { CredentialId = 0, - // Deprecated, we still read this field for backwards compatibility. - EcdsaPrivateKey = 1, RpId = 2, UserHandle = 3, UserDisplayName = 4, @@ -617,6 +615,7 @@ enum PublicKeyCredentialSourceField { // When a field is removed, its tag should be reserved and not used for new fields. We document // those reserved tags below. // Reserved tags: + // - EcdsaPrivateKey = 1, // - CredRandom = 5, } @@ -661,7 +660,6 @@ impl PublicKeyCredentialSource { destructure_cbor_map! { let { PublicKeyCredentialSourceField::CredentialId => credential_id, - PublicKeyCredentialSourceField::EcdsaPrivateKey => ecdsa_private_key, PublicKeyCredentialSourceField::RpId => rp_id, PublicKeyCredentialSourceField::UserHandle => user_handle, PublicKeyCredentialSourceField::UserDisplayName => user_display_name, @@ -687,19 +685,7 @@ impl PublicKeyCredentialSource { let user_icon = user_icon.map(extract_text_string).transpose()?; let cred_blob = cred_blob.map(extract_byte_string).transpose()?; let large_blob_key = large_blob_key.map(extract_byte_string).transpose()?; - - // Parse the private key from the deprecated field if necessary. - let ecdsa_private_key = ecdsa_private_key.map(extract_byte_string).transpose()?; - let private_key = private_key - .map(|v| PrivateKey::from_cbor::(wrap_key, v)) - .transpose()?; - let private_key = match (ecdsa_private_key, private_key) { - (None, None) => return Err(Ctap2StatusCode::CTAP2_ERR_MISSING_PARAMETER), - (Some(_), Some(_)) => return Err(Ctap2StatusCode::CTAP2_ERR_VENDOR_INTERNAL_ERROR), - (Some(k), None) => PrivateKey::new_ecdsa_from_bytes(&k) - .ok_or(Ctap2StatusCode::CTAP2_ERR_VENDOR_INTERNAL_ERROR)?, - (None, Some(k)) => k, - }; + let private_key = PrivateKey::from_cbor::(wrap_key, ok_or_missing(private_key)?)?; // We don't return whether there were unknown fields in the CBOR value. This means that // deserialization is not injective. In particular deserialization is only an inverse of @@ -2221,83 +2207,6 @@ mod test { ); } - #[test] - fn test_credential_source_cbor_read_legacy() { - let mut env = TestEnv::default(); - let wrap_key = env.key_store().wrap_key::().unwrap(); - let private_key = PrivateKey::new_ecdsa(&mut env); - let key_bytes = private_key.to_bytes(); - let credential = PublicKeyCredentialSource { - key_type: PublicKeyCredentialType::PublicKey, - credential_id: env.rng().gen_uniform_u8x32().to_vec(), - private_key, - rp_id: "example.com".to_string(), - user_handle: b"foo".to_vec(), - user_display_name: None, - cred_protect_policy: None, - creation_order: 0, - user_name: None, - user_icon: None, - cred_blob: None, - large_blob_key: None, - }; - - let source_cbor = cbor_map! { - PublicKeyCredentialSourceField::CredentialId => credential.credential_id.clone(), - PublicKeyCredentialSourceField::EcdsaPrivateKey => key_bytes, - PublicKeyCredentialSourceField::RpId => credential.rp_id.clone(), - PublicKeyCredentialSourceField::UserHandle => credential.user_handle.clone(), - }; - assert_eq!( - PublicKeyCredentialSource::from_cbor::(&wrap_key, source_cbor), - Ok(credential) - ); - } - - #[test] - fn test_credential_source_cbor_legacy_error() { - let mut env = TestEnv::default(); - let wrap_key = env.key_store().wrap_key::().unwrap(); - let private_key = PrivateKey::new_ecdsa(&mut env); - let key_bytes = private_key.to_bytes(); - let credential = PublicKeyCredentialSource { - key_type: PublicKeyCredentialType::PublicKey, - credential_id: env.rng().gen_uniform_u8x32().to_vec(), - private_key: private_key.clone(), - rp_id: "example.com".to_string(), - user_handle: b"foo".to_vec(), - user_display_name: None, - cred_protect_policy: None, - creation_order: 0, - user_name: None, - user_icon: None, - cred_blob: None, - large_blob_key: None, - }; - - let source_cbor = cbor_map! { - PublicKeyCredentialSourceField::CredentialId => credential.credential_id.clone(), - PublicKeyCredentialSourceField::RpId => credential.rp_id.clone(), - PublicKeyCredentialSourceField::UserHandle => credential.user_handle.clone(), - }; - assert_eq!( - PublicKeyCredentialSource::from_cbor::(&wrap_key, source_cbor), - Err(Ctap2StatusCode::CTAP2_ERR_MISSING_PARAMETER) - ); - - let source_cbor = cbor_map! { - PublicKeyCredentialSourceField::CredentialId => credential.credential_id, - PublicKeyCredentialSourceField::EcdsaPrivateKey => key_bytes, - PublicKeyCredentialSourceField::RpId => credential.rp_id, - PublicKeyCredentialSourceField::UserHandle => credential.user_handle, - PublicKeyCredentialSourceField::PrivateKey => private_key.to_cbor::(env.rng(), &wrap_key).unwrap(), - }; - assert_eq!( - PublicKeyCredentialSource::from_cbor::(&wrap_key, source_cbor), - Err(Ctap2StatusCode::CTAP2_ERR_VENDOR_INTERNAL_ERROR) - ); - } - #[test] fn test_credential_source_invalid_cbor() { let mut env = TestEnv::default();