Add tests to check that all CBOR requests are in canonicalized form.

Closes mozilla#225.

Author: EliasHolzmann

src/crypto/mod.rs

@@ -39,7 +39,7 @@ mod der;
 
 pub use backend::ecdsa_p256_sha256_sign_raw;
 
-pub struct PinUvAuthProtocol(Box<dyn PinProtocolImpl + Send + Sync>);
+pub struct PinUvAuthProtocol(pub(crate) Box<dyn PinProtocolImpl + Send + Sync>);
 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<dyn PinProtocolImpl + Send + Sync>;
 }
 
@@ -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<Vec<u8>, CryptoError>;
@@ -364,10 +364,10 @@ impl PinUvAuthToken {
 
 #[derive(Clone, Debug)]
 pub struct PinUvAuthParam {
-    pin_auth: Vec<u8>,
+    pub(crate) pin_auth: Vec<u8>,
     pub pin_protocol: PinUvAuthProtocol,
     #[allow(dead_code)] // Not yet used
-    permissions: PinUvAuthTokenPermission,
+    pub(crate) permissions: PinUvAuthTokenPermission,
 }
 
 impl PinUvAuthParam {

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);
+        }
+    }
+}

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);
+        }
+    }
+}

src/ctap2/commands/client_pin.rs

@@ -713,9 +713,16 @@ impl From<CryptoError> 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);
+    }
 }

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);
+        }
+    }
+}

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];

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()

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<T: Serialize + fmt::Debug>(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);
+}

src/transport/hid.rs

@@ -91,6 +91,7 @@ pub trait HIDDevice: FidoDevice + Read + Write {
     ) -> io::Result<(HIDCmd, Vec<u8>)> {
         self.u2f_write(cmd.into(), send)?;
         debug!("sent to Device {:?} cmd={:?}: {:?}", self.id(), cmd, send);
+        dbg!();
         loop {
             let (cmd, data) = self.u2f_read()?;
             if cmd != HIDCmd::Keepalive {
@@ -180,6 +181,7 @@ impl<T: HIDDevice + TestDevice> FidoDeviceIO for T {
         msg: &Req,
         keep_alive: &dyn Fn() -> bool,
     ) -> Result<Req::Output, HIDError> {
+        dbg!();
         debug!("sending {:?} to {:?}", msg, self);
         #[cfg(test)]
         {