diff --git a/src/crypto/mod.rs b/src/crypto/mod.rs index fd74030e..a0c2ba0f 100644 --- a/src/crypto/mod.rs +++ b/src/crypto/mod.rs @@ -39,7 +39,7 @@ mod der; pub use backend::ecdsa_p256_sha256_sign_raw; -pub struct PinUvAuthProtocol(Box); +pub struct PinUvAuthProtocol(pub(crate) Box); impl PinUvAuthProtocol { pub fn id(&self) -> u64 { self.0.protocol_id() @@ -53,7 +53,7 @@ impl PinUvAuthProtocol { /// PinProtocolImpl. So we stash a copy of the calling PinUvAuthProtocol in the output SharedSecret. /// We need a trick here to tell the compiler that every PinProtocolImpl we define will implement /// Clone. -trait ClonablePinProtocolImpl { +pub(crate) trait ClonablePinProtocolImpl { fn clone_box(&self) -> Box; } @@ -73,7 +73,7 @@ impl Clone for PinUvAuthProtocol { } /// CTAP 2.1, Section 6.5.4. PIN/UV Auth Protocol Abstract Definition -trait PinProtocolImpl: ClonablePinProtocolImpl { +pub(crate) trait PinProtocolImpl: ClonablePinProtocolImpl { fn protocol_id(&self) -> u64; fn initialize(&self); fn encrypt(&self, key: &[u8], plaintext: &[u8]) -> Result, CryptoError>; @@ -364,10 +364,10 @@ impl PinUvAuthToken { #[derive(Clone, Debug)] pub struct PinUvAuthParam { - pin_auth: Vec, + pub(crate) pin_auth: Vec, pub pin_protocol: PinUvAuthProtocol, #[allow(dead_code)] // Not yet used - permissions: PinUvAuthTokenPermission, + pub(crate) permissions: PinUvAuthTokenPermission, } impl PinUvAuthParam { diff --git a/src/ctap2/commands/authenticator_config.rs b/src/ctap2/commands/authenticator_config.rs index f72640d7..3454766e 100644 --- a/src/ctap2/commands/authenticator_config.rs +++ b/src/ctap2/commands/authenticator_config.rs @@ -223,3 +223,38 @@ impl PinUvAuthCommand for AuthenticatorConfig { None } } + +#[cfg(test)] +mod test { + use super::*; + use crate::ctap2::commands::assert_canonical_cbor_encoding; + + #[test] + fn test_cbor_canonical() { + for subcommand in [ + AuthConfigCommand::EnableEnterpriseAttestation, + AuthConfigCommand::ToggleAlwaysUv, + AuthConfigCommand::SetMinPINLength(SetMinPINLength { + /// Minimum PIN length in code points + new_min_pin_length: Some(42), + /// RP IDs which are allowed to get this information via the minPinLength extension. + /// This parameter MUST NOT be used unless the minPinLength extension is supported. + min_pin_length_rpids: Some(vec!["foobar".to_string()]), + /// The authenticator returns CTAP2_ERR_PIN_POLICY_VIOLATION until changePIN is successful. + force_change_pin: Some(true), + }), + ] { + let request = AuthenticatorConfig { + subcommand, + pin_uv_auth_param: Some(PinUvAuthParam { + pin_auth: vec![0xDE, 0xAD, 0xBE, 0xEF], + pin_protocol: crate::crypto::PinUvAuthProtocol(Box::new( + crate::crypto::PinUvAuth2 {}, + )), + permissions: crate::ctap2::PinUvAuthTokenPermission::MakeCredential, + }), + }; + assert_canonical_cbor_encoding(&request); + } + } +} diff --git a/src/ctap2/commands/bio_enrollment.rs b/src/ctap2/commands/bio_enrollment.rs index 817bb5ac..6ebc1e34 100644 --- a/src/ctap2/commands/bio_enrollment.rs +++ b/src/ctap2/commands/bio_enrollment.rs @@ -658,3 +658,39 @@ pub enum BioEnrollmentResult { FingerprintSensorInfo(FingerprintSensorInfo), SampleStatus(LastEnrollmentSampleStatus, u64), } + +#[cfg(test)] +mod test { + use super::*; + use crate::ctap2::commands::assert_canonical_cbor_encoding; + + #[test] + fn test_cbor_canonical() { + for subcommand in [ + BioEnrollmentCommand::EnrollBegin(Some(42)), + BioEnrollmentCommand::EnrollCaptureNextSample((vec![0xDE, 0xAD, 0xBE, 0xEF], Some(42))), + BioEnrollmentCommand::CancelCurrentEnrollment, + BioEnrollmentCommand::EnumerateEnrollments, + BioEnrollmentCommand::SetFriendlyName(( + vec![0xDE, 0xAD, 0xBE, 0xEF], + "foobar".to_string(), + )), + BioEnrollmentCommand::RemoveEnrollment(vec![0xDE, 0xAD, 0xBE, 0xEF]), + BioEnrollmentCommand::GetFingerprintSensorInfo, + ] { + let request = BioEnrollment { + modality: BioEnrollmentModality::Fingerprint, + subcommand, + pin_uv_auth_param: Some(PinUvAuthParam { + pin_auth: vec![0xDE, 0xAD, 0xBE, 0xEF], + pin_protocol: crate::crypto::PinUvAuthProtocol(Box::new( + crate::crypto::PinUvAuth2 {}, + )), + permissions: crate::ctap2::PinUvAuthTokenPermission::MakeCredential, + }), + use_legacy_preview: false, + }; + assert_canonical_cbor_encoding(&request); + } + } +} diff --git a/src/ctap2/commands/client_pin.rs b/src/ctap2/commands/client_pin.rs index eb3b7136..ea0971eb 100644 --- a/src/ctap2/commands/client_pin.rs +++ b/src/ctap2/commands/client_pin.rs @@ -713,9 +713,16 @@ impl From for PinError { #[cfg(test)] mod test { - use super::ClientPinResponse; + use super::{ClientPIN, ClientPinResponse, PINSubcommand, PinUvAuthProtocol}; use crate::crypto::{COSEAlgorithm, COSEEC2Key, COSEKey, COSEKeyType, Curve}; + use crate::ctap2::attestation::AAGuid; + use crate::ctap2::commands::assert_canonical_cbor_encoding; + use crate::ctap2::commands::get_info::AuthenticatorOptions; + use crate::ctap2::AuthenticatorVersion; + use crate::util::decode_hex; + use crate::AuthenticatorInfo; use serde_cbor::de::from_slice; + use std::convert::TryFrom; #[test] fn test_get_key_agreement() { @@ -848,4 +855,50 @@ mod test { from_slice(&reference).expect("could not deserialize reference"); assert_eq!(expected, result); } + + #[test] + #[allow(non_snake_case)] + fn test_cbor_canonical() { + let pin_protocol = PinUvAuthProtocol::try_from(&AuthenticatorInfo { + versions: vec![AuthenticatorVersion::U2F_V2, AuthenticatorVersion::FIDO_2_0], + extensions: vec![], + aaguid: AAGuid([0u8; 16]), + options: AuthenticatorOptions { + platform_device: false, + resident_key: true, + client_pin: Some(false), + user_presence: true, + ..Default::default() + }, + max_msg_size: Some(1200), + pin_protocols: None, + ..Default::default() + }) + .unwrap(); + + // from tests for crate::crypto + let DEV_PUB_X = + decode_hex("0501D5BC78DA9252560A26CB08FCC60CBE0B6D3B8E1D1FCEE514FAC0AF675168"); + let DEV_PUB_Y = + decode_hex("D551B3ED46F665731F95B4532939C25D91DB7EB844BD96D4ABD4083785F8DF47"); + let key = COSEKey { + alg: COSEAlgorithm::ES256, + key: COSEKeyType::EC2(COSEEC2Key { + curve: Curve::SECP256R1, + x: DEV_PUB_X, + y: DEV_PUB_Y, + }), + }; + let request = ClientPIN { + pin_protocol: Some(pin_protocol), + subcommand: PINSubcommand::GetPinRetries, + key_agreement: Some(key), + pin_auth: Some(vec![0xDE, 0xAD, 0xBE, 0xEF]), + new_pin_enc: Some(vec![0xDE, 0xAD, 0xBE, 0xEF]), + pin_hash_enc: Some(vec![0xDE, 0xAD, 0xBE, 0xEF]), + permissions: Some(42), + rp_id: Some("foobar".to_string()), + }; + assert_canonical_cbor_encoding(&request); + } } diff --git a/src/ctap2/commands/credential_management.rs b/src/ctap2/commands/credential_management.rs index 2a58bb17..924e1688 100644 --- a/src/ctap2/commands/credential_management.rs +++ b/src/ctap2/commands/credential_management.rs @@ -455,3 +455,49 @@ impl PinUvAuthCommand for CredentialManagement { self.pin_uv_auth_param.as_ref() } } + +#[cfg(test)] +mod test { + use super::*; + use crate::ctap2::commands::assert_canonical_cbor_encoding; + use crate::ctap2::server::Transport; + + #[test] + fn test_cbor_canonical() { + for subcommand in [ + CredManagementCommand::GetCredsMetadata, + CredManagementCommand::EnumerateRPsBegin, + CredManagementCommand::EnumerateRPsGetNextRP, + CredManagementCommand::EnumerateCredentialsBegin(RpIdHash([0u8; 32])), + CredManagementCommand::EnumerateCredentialsGetNextCredential, + CredManagementCommand::DeleteCredential(PublicKeyCredentialDescriptor { + id: vec![0xDE, 0xAD, 0xBE, 0xEF], + transports: vec![Transport::NFC], + }), + CredManagementCommand::UpdateUserInformation(( + PublicKeyCredentialDescriptor { + id: vec![0xDE, 0xAD, 0xBE, 0xEF], + transports: vec![Transport::NFC], + }, + PublicKeyCredentialUserEntity { + id: vec![0xDE, 0xAD, 0xBE, 0xEF], + name: Some("foobar".to_string()), + display_name: Some("foobar".to_string()), + }, + )), + ] { + let request = CredentialManagement { + subcommand, // subCommand currently being requested + pin_uv_auth_param: Some(PinUvAuthParam { + pin_auth: vec![0xDE, 0xAD, 0xBE, 0xEF], + pin_protocol: crate::crypto::PinUvAuthProtocol(Box::new( + crate::crypto::PinUvAuth2 {}, + )), + permissions: crate::ctap2::PinUvAuthTokenPermission::MakeCredential, + }), + use_legacy_preview: false, + }; + assert_canonical_cbor_encoding(&request); + } + } +} diff --git a/src/ctap2/commands/get_assertion.rs b/src/ctap2/commands/get_assertion.rs index 54d7d491..b845e384 100644 --- a/src/ctap2/commands/get_assertion.rs +++ b/src/ctap2/commands/get_assertion.rs @@ -618,7 +618,7 @@ pub mod test { use crate::ctap2::commands::get_info::{ AuthenticatorInfo, AuthenticatorOptions, AuthenticatorVersion, }; - use crate::ctap2::commands::RequestCtap1; + use crate::ctap2::commands::{assert_canonical_cbor_encoding, RequestCtap1}; use crate::ctap2::preflight::{ do_credential_list_filtering_ctap1, do_credential_list_filtering_ctap2, }; @@ -660,6 +660,7 @@ pub mod test { }, Default::default(), ); + assert_canonical_cbor_encoding(&assertion); let mut device = Device::new("commands/get_assertion").unwrap(); assert_eq!(device.get_protocol(), FidoProtocol::CTAP2); let mut cid = [0u8; 4]; @@ -859,6 +860,7 @@ pub mod test { }, Default::default(), ); + assert_canonical_cbor_encoding(&assertion); let mut device = Device::new("commands/get_assertion").unwrap(); // not really used (all functions ignore it) // channel id device.downgrade_to_ctap1(); @@ -948,6 +950,7 @@ pub mod test { }, Default::default(), ); + assert_canonical_cbor_encoding(&assertion); let mut device = Device::new("commands/get_assertion").unwrap(); // not really used (all functions ignore it) // channel id @@ -1128,6 +1131,7 @@ pub mod test { }, Default::default(), ); + assert_canonical_cbor_encoding(&assertion); let mut device = Device::new("commands/get_assertion").unwrap(); assert_eq!(device.get_protocol(), FidoProtocol::CTAP2); let mut cid = [0u8; 4]; diff --git a/src/ctap2/commands/make_credentials.rs b/src/ctap2/commands/make_credentials.rs index 1de10484..15903280 100644 --- a/src/ctap2/commands/make_credentials.rs +++ b/src/ctap2/commands/make_credentials.rs @@ -605,7 +605,7 @@ pub mod test { AuthenticatorDataFlags, Signature, }; use crate::ctap2::client_data::{Challenge, CollectedClientData, TokenBinding, WebauthnType}; - use crate::ctap2::commands::{RequestCtap1, RequestCtap2}; + use crate::ctap2::commands::{assert_canonical_cbor_encoding, RequestCtap1, RequestCtap2}; use crate::ctap2::server::RpIdHash; use crate::ctap2::server::{ AuthenticatorAttachment, PublicKeyCredentialParameters, PublicKeyCredentialUserEntity, @@ -654,6 +654,7 @@ pub mod test { }, Default::default(), ); + assert_canonical_cbor_encoding(&req); let mut device = Device::new("commands/make_credentials").unwrap(); // not really used (all functions ignore it) assert_eq!(device.get_protocol(), FidoProtocol::CTAP2); @@ -709,6 +710,7 @@ pub mod test { }, Default::default(), ); + assert_canonical_cbor_encoding(&req); let (req_serialized, _) = req .ctap1_format() diff --git a/src/ctap2/commands/mod.rs b/src/ctap2/commands/mod.rs index 12990122..34584946 100644 --- a/src/ctap2/commands/mod.rs +++ b/src/ctap2/commands/mod.rs @@ -5,6 +5,7 @@ use crate::ctap2::server::UserVerificationRequirement; use crate::errors::AuthenticatorError; use crate::transport::errors::{ApduErrorStatus, HIDError}; use crate::transport::{FidoDevice, VirtualFidoDevice}; +use serde::Serialize; use serde_cbor::{error::Error as CborError, Value}; use serde_json as json; use std::error::Error as StdErrorT; @@ -475,3 +476,66 @@ impl fmt::Display for CommandError { } impl StdErrorT for CommandError {} + +/// Asserts that a value encodes to canonical CBOR encoding according to CTAP2.1 spec (https://fidoalliance.org/specs/fido-v2.1-ps-20210615/fido-client-to-authenticator-protocol-v2.1-ps-errata-20220621.html#ctap2-canonical-cbor-encoding-form). This function is used by submodules to verify the requests they define. +fn assert_canonical_cbor_encoding(value: &T) { + let bytes = serde_cbor::to_vec(value) + .unwrap_or_else(|e| panic!("Failed to serialize value {:?}: {}", value, e)); + let opaque_value: serde_cbor::Value = serde_cbor::from_slice(&bytes) + .unwrap_or_else(|e| panic!("Failed to deserialize value {:?}: {}", value, e)); + + // Below, each requirement is first quoted from the spec and then tested (or explained why testing is not necessary): + + // > Integers MUST be encoded as small as possible. + // > - 0 to 23 and -1 to -24 MUST be expressed in the same byte as the major type; + // > - 24 to 255 and -25 to -256 MUST be expressed only with an additional uint8_t; + // > - 256 to 65535 and -257 to -65536 MUST be expressed only with an additional uint16_t; + // > - 65536 to 4294967295 and -65537 to -4294967296 MUST be expressed only with an additional uint32_t. + // serde_cbor handles this as required, see https://github.com/pyfisch/cbor/blob/master/src/ser.rs#L140-L182 + + // > The representations of any floating-point values are not changed. + // > Note: The size of a floating point value—16-, 32-, or 64-bits—is considered part of the value for the purpose of CTAP2. E.g., a 16-bit value of 1.5, say, has different semantic meaning than a 32-bit value of 1.5, and both can be canonical for their own meanings. + // serde_cbor won't change u32 to u64 by itself, so this is fine, too + + // > The expression of lengths in major types 2 through 5 MUST be as short as possible. The rules for these lengths follow the above rule for integers. + // See above -- serde_cbor handles this for us. + + // > Indefinite-length items MUST be made into definite-length items. + // serde_cbor might serialize with indefinite length if serde doesn't know the length when beginning to serialize an array/a map. + // We use a trick here: serde_cbor::Value manages arrays/maps as `Array`/`Map`. Both of these have iterators implementing `ExactIterator`, so serde_cbor does not serialize them with indefinite length (unless the length is larger than usize max value, which shouldn't happen in practice). So, just re-serialize and compare with the result of the first serialization. + + let bytes_after_reserializing = serde_cbor::to_vec(&opaque_value) + .unwrap_or_else(|e| panic!("Failed to reserialize value {:?}: {}", opaque_value, e)); + assert_eq!(bytes, bytes_after_reserializing, "Bytes aren't equal before and after reserializing, value is probably not in CTAP2 canonical CBOR encoding form"); + + // > The keys in every map MUST be sorted lowest value to highest. The sorting rules are: + // > - If the major types are different, the one with the lower value in numerical order sorts earlier. + // > - If two keys have different lengths, the shorter one sorts earlier; + // > - If two keys have different lengths, the shorter one sorts earlier; + // > - If two keys have the same length, the one with the lower value in (byte-wise) lexical order sorts earlier. + // This is equivalent to the CBOR core canonicalization requirements (https://datatracker.ietf.org/doc/html/draft-ietf-cbor-7049bis-04#section-4.10), which is how the BTReeMap is sorted. Therefore, the reserialization trick from above also handles this case. + + // > Tags as defined in Section 2.4 in [RFC8949] MUST NOT be present. + fn assert_has_no_tags(value: &serde_cbor::Value) { + match value { + serde_cbor::Value::Array(vec) => { + for entry in vec { + assert_has_no_tags(entry); + } + } + serde_cbor::Value::Map(map) => { + for (key, entry) in map { + assert_has_no_tags(key); + assert_has_no_tags(entry); + } + } + serde_cbor::Value::Tag(tag, value) => panic!( + "Tags are not allowed inside canonical CBOR encoding, but found tag {} on value {:?}", + tag, value + ), + _ => {}, + } + } + + assert_has_no_tags(&opaque_value); +}