adds code style improvements, including a new enum for permissions

src/ctap/command.rs

@@ -332,7 +332,9 @@ impl TryFrom<cbor::Value> for AuthenticatorClientPinParameters {
         let min_pin_length = min_pin_length
             .map(extract_unsigned)
             .transpose()?
-            .map(|m| m as u8);
+            .map(u8::try_from)
+            .transpose()
+            .map_err(|_| Ctap2StatusCode::CTAP2_ERR_PIN_POLICY_VIOLATION)?;
         #[cfg(feature = "with_ctap2_1")]
         let min_pin_length_rp_ids = match min_pin_length_rp_ids {
             Some(entry) => Some(

src/ctap/mod.rs

@@ -38,6 +38,8 @@ use self::data_formats::{
     PublicKeyCredentialUserEntity, SignatureAlgorithm,
 };
 use self::hid::ChannelID;
+#[cfg(feature = "with_ctap2_1")]
+use self::pin_protocol_v1::PinPermission;
 use self::pin_protocol_v1::PinProtocolV1;
 use self::response::{
     AuthenticatorGetAssertionResponse, AuthenticatorGetInfoResponse,
@@ -414,8 +416,11 @@ where
                     return Err(Ctap2StatusCode::CTAP2_ERR_PIN_AUTH_INVALID);
                 }
                 #[cfg(feature = "with_ctap2_1")]
-                self.pin_protocol_v1
+                {
-                    .has_make_credential_permission(&rp_id)?;
+                    self.pin_protocol_v1
+                        .has_permission(PinPermission::MakeCredential)?;
+                    self.pin_protocol_v1.has_permission_for_rp_id(&rp_id)?;
+                }
                 UP_FLAG | UV_FLAG | AT_FLAG | ed_flag
             }
             None => {
@@ -595,7 +600,11 @@ where
                     return Err(Ctap2StatusCode::CTAP2_ERR_PIN_AUTH_INVALID);
                 }
                 #[cfg(feature = "with_ctap2_1")]
-                self.pin_protocol_v1.has_get_assertion_permission(&rp_id)?;
+                {
+                    self.pin_protocol_v1
+                        .has_permission(PinPermission::GetAssertion)?;
+                    self.pin_protocol_v1.has_permission_for_rp_id(&rp_id)?;
+                }
                 UV_FLAG
             }
             None => {

src/ctap/pin_protocol_v1.rs

@@ -1,4 +1,4 @@
-// Copyright 2019 Google LLC
+// Copyright 2020 Google LLC
 //
 // Licensed under the Apache License, Version 2.0 (the "License");
 // you may not use this file except in compliance with the License.
@@ -63,9 +63,9 @@ fn encrypt_hmac_secret_output(
     let aes_enc_key = crypto::aes256::EncryptionKey::new(shared_secret);
     let aes_dec_key = crypto::aes256::DecryptionKey::new(&aes_enc_key);
     // The specification specifically asks for a zero IV.
-    let iv = [0; 16];
+    let iv = [0u8; 16];
 
-    let mut cred_random_secret = [0; 32];
+    let mut cred_random_secret = [0u8; 32];
     cred_random_secret.clone_from_slice(cred_random);
 
     // Initialization of 4 blocks in any case makes this function more readable.
@@ -76,7 +76,7 @@ fn encrypt_hmac_secret_output(
     }
     cbc_decrypt(&aes_dec_key, iv, &mut blocks[..block_len]);
 
-    let mut decrypted_salt1 = [0; 32];
+    let mut decrypted_salt1 = [0u8; 32];
     decrypted_salt1[..16].clone_from_slice(&blocks[0]);
     let output1 = hmac_256::<Sha256>(&cred_random_secret, &decrypted_salt1[..]);
     decrypted_salt1[16..].clone_from_slice(&blocks[1]);
@@ -85,7 +85,7 @@ fn encrypt_hmac_secret_output(
     }
 
     if block_len == 4 {
-        let mut decrypted_salt2 = [0; 32];
+        let mut decrypted_salt2 = [0u8; 32];
         decrypted_salt2[..16].clone_from_slice(&blocks[2]);
         decrypted_salt2[16..].clone_from_slice(&blocks[3]);
         let output2 = hmac_256::<Sha256>(&cred_random_secret, &decrypted_salt2[..]);
@@ -110,7 +110,7 @@ fn check_and_store_new_pin(
     if new_pin_enc.len() != PIN_PADDED_LENGTH {
         return false;
     }
-    let iv = [0; 16];
+    let iv = [0u8; 16];
     // Assuming PIN_PADDED_LENGTH % block_size == 0 here.
     let mut blocks = [[0u8; 16]; PIN_PADDED_LENGTH / 16];
     for i in 0..PIN_PADDED_LENGTH / 16 {
@@ -136,12 +136,31 @@ fn check_and_store_new_pin(
         // TODO(kaczmarczyck) check last byte == 0x00
         return false;
     }
-    let mut pin_hash = [0; 16];
+    let mut pin_hash = [0u8; 16];
     pin_hash.copy_from_slice(&Sha256::hash(&pin[..])[..16]);
     persistent_store.set_pin_hash(&pin_hash);
     true
 }
 
+#[cfg(feature = "with_ctap2_1")]
+// TODO remove when all variants are used
+#[allow(dead_code)]
+pub enum PinPermission {
+    MakeCredential = 0x01,
+    GetAssertion = 0x02,
+    CredentialManagement = 0x04,
+    BioEnrollment = 0x08,
+    PlatformConfiguration = 0x10,
+    AuthenticatorConfiguration = 0x20,
+}
+
+#[cfg(feature = "with_ctap2_1")]
+impl PinPermission {
+    pub fn check(self, stored_bits: u8) -> bool {
+        self as u8 & stored_bits != 0
+    }
+}
+
 pub struct PinProtocolV1 {
     key_agreement_key: crypto::ecdh::SecKey,
     pin_uv_auth_token: [u8; PIN_TOKEN_LENGTH],
@@ -187,7 +206,7 @@ impl PinProtocolV1 {
                     return Err(Ctap2StatusCode::CTAP2_ERR_PIN_INVALID);
                 }
 
-                let iv = [0; 16];
+                let iv = [0u8; 16];
                 let mut blocks = [[0u8; 16]; 1];
                 blocks[0].copy_from_slice(&pin_hash_enc[0..PIN_AUTH_LENGTH]);
                 cbc_decrypt(aes_dec_key, iv, &mut blocks);
@@ -309,7 +328,7 @@ impl PinProtocolV1 {
         self.check_pin_hash_enc(rng, persistent_store, &aes_dec_key, pin_hash_enc)?;
 
         // Assuming PIN_TOKEN_LENGTH % block_size == 0 here.
-        let iv = [0; 16];
+        let iv = [0u8; 16];
         let mut blocks = [[0u8; 16]; PIN_TOKEN_LENGTH / 16];
         for (i, item) in blocks.iter_mut().take(PIN_TOKEN_LENGTH / 16).enumerate() {
             item.copy_from_slice(&self.pin_uv_auth_token[i * 16..(i + 1) * 16]);
@@ -569,19 +588,16 @@ impl PinProtocolV1 {
     }
 
     #[cfg(feature = "with_ctap2_1")]
-    pub fn has_make_credential_permission(&mut self, rp_id: &str) -> Result<(), Ctap2StatusCode> {
+    pub fn has_permission(&self, permission: PinPermission) -> Result<(), Ctap2StatusCode> {
-        self.has_permission(0x01, rp_id)
+        if permission.check(self.permissions) {
+            Ok(())
+        } else {
+            Err(Ctap2StatusCode::CTAP2_ERR_PIN_AUTH_INVALID)
+        }
     }
 
     #[cfg(feature = "with_ctap2_1")]
-    pub fn has_get_assertion_permission(&mut self, rp_id: &str) -> Result<(), Ctap2StatusCode> {
+    pub fn has_permission_for_rp_id(&mut self, rp_id: &str) -> Result<(), Ctap2StatusCode> {
-        self.has_permission(0x02, rp_id)
-    }
-
-    // TODO(kaczmarczyck) use permissons for new commands
-
-    #[cfg(feature = "with_ctap2_1")]
-    fn has_permission(&mut self, bitmask: u8, rp_id: &str) -> Result<(), Ctap2StatusCode> {
         if let Some(permissions_rp_id) = &self.permissions_rp_id {
             if rp_id != permissions_rp_id {
                 return Err(Ctap2StatusCode::CTAP2_ERR_PIN_AUTH_INVALID);
@@ -589,11 +605,7 @@ impl PinProtocolV1 {
         } else {
             self.permissions_rp_id = Some(String::from(rp_id));
         }
-        if self.permissions & bitmask != 0 {
+        Ok(())
-            Ok(())
-        } else {
-            Err(Ctap2StatusCode::CTAP2_ERR_PIN_AUTH_INVALID)
-        }
     }
 }
 
@@ -603,12 +615,12 @@ mod test {
     use crypto::rng256::ThreadRng256;
 
     fn set_standard_pin(persistent_store: &mut PersistentStore) {
-        let mut pin = [0x00; 64];
+        let mut pin = [0u8; 64];
         pin[0] = 0x31;
         pin[1] = 0x32;
         pin[2] = 0x33;
         pin[3] = 0x34;
-        let mut pin_hash = [0; 16];
+        let mut pin_hash = [0u8; 16];
         pin_hash.copy_from_slice(&Sha256::hash(&pin[..])[..16]);
         persistent_store.set_pin_hash(&pin_hash);
     }
@@ -620,7 +632,7 @@ mod test {
         blocks[0][1] = 0x32;
         blocks[0][2] = 0x33;
         blocks[0][3] = 0x34;
-        let iv = [0; 16];
+        let iv = [0u8; 16];
         cbc_encrypt(&aes_enc_key, iv, &mut blocks);
 
         let mut encrypted_pin = Vec::with_capacity(64);
@@ -632,7 +644,7 @@ mod test {
 
     fn encrypt_standard_pin_hash(shared_secret: &[u8; 32]) -> Vec<u8> {
         let aes_enc_key = crypto::aes256::EncryptionKey::new(shared_secret);
-        let mut pin = [0x00; 64];
+        let mut pin = [0u8; 64];
         pin[0] = 0x31;
         pin[1] = 0x32;
         pin[2] = 0x33;
@@ -641,7 +653,7 @@ mod test {
 
         let mut blocks = [[0u8; 16]; 1];
         blocks[0].copy_from_slice(&pin_hash[..16]);
-        let iv = [0; 16];
+        let iv = [0u8; 16];
         cbc_encrypt(&aes_enc_key, iv, &mut blocks);
 
         let mut encrypted_pin_hash = Vec::with_capacity(16);
@@ -699,7 +711,7 @@ mod test {
                 &mut rng,
                 &mut persistent_store,
                 &aes_dec_key,
-                pin_hash_enc.clone()
+                pin_hash_enc
             ),
             Err(Ctap2StatusCode::CTAP2_ERR_PIN_AUTH_BLOCKED)
         );
@@ -1089,19 +1101,77 @@ mod test {
     fn test_has_permission() {
         let mut rng = ThreadRng256 {};
         let mut pin_protocol_v1 = PinProtocolV1::new(&mut rng);
-        pin_protocol_v1.permissions = 0x03;
+        pin_protocol_v1.permissions = 0x7F;
-        assert_eq!(pin_protocol_v1.has_permission(0x01, "example.com"), Ok(()));
+        assert_eq!(
+            pin_protocol_v1.has_permission(PinPermission::MakeCredential),
+            Ok(())
+        );
+        assert_eq!(
+            pin_protocol_v1.has_permission(PinPermission::GetAssertion),
+            Ok(())
+        );
+        assert_eq!(
+            pin_protocol_v1.has_permission(PinPermission::CredentialManagement),
+            Ok(())
+        );
+        assert_eq!(
+            pin_protocol_v1.has_permission(PinPermission::BioEnrollment),
+            Ok(())
+        );
+        assert_eq!(
+            pin_protocol_v1.has_permission(PinPermission::PlatformConfiguration),
+            Ok(())
+        );
+        assert_eq!(
+            pin_protocol_v1.has_permission(PinPermission::AuthenticatorConfiguration),
+            Ok(())
+        );
+        pin_protocol_v1.permissions = 0x00;
+        assert_eq!(
+            pin_protocol_v1.has_permission(PinPermission::MakeCredential),
+            Err(Ctap2StatusCode::CTAP2_ERR_PIN_AUTH_INVALID)
+        );
+        assert_eq!(
+            pin_protocol_v1.has_permission(PinPermission::GetAssertion),
+            Err(Ctap2StatusCode::CTAP2_ERR_PIN_AUTH_INVALID)
+        );
+        assert_eq!(
+            pin_protocol_v1.has_permission(PinPermission::CredentialManagement),
+            Err(Ctap2StatusCode::CTAP2_ERR_PIN_AUTH_INVALID)
+        );
+        assert_eq!(
+            pin_protocol_v1.has_permission(PinPermission::BioEnrollment),
+            Err(Ctap2StatusCode::CTAP2_ERR_PIN_AUTH_INVALID)
+        );
+        assert_eq!(
+            pin_protocol_v1.has_permission(PinPermission::PlatformConfiguration),
+            Err(Ctap2StatusCode::CTAP2_ERR_PIN_AUTH_INVALID)
+        );
+        assert_eq!(
+            pin_protocol_v1.has_permission(PinPermission::AuthenticatorConfiguration),
+            Err(Ctap2StatusCode::CTAP2_ERR_PIN_AUTH_INVALID)
+        );
+    }
+
+    #[cfg(feature = "with_ctap2_1")]
+    #[test]
+    fn test_has_permission_for_rp_id() {
+        let mut rng = ThreadRng256 {};
+        let mut pin_protocol_v1 = PinProtocolV1::new(&mut rng);
+        assert_eq!(
+            pin_protocol_v1.has_permission_for_rp_id("example.com"),
+            Ok(())
+        );
         assert_eq!(
             pin_protocol_v1.permissions_rp_id,
             Some(String::from("example.com"))
         );
-        assert_eq!(pin_protocol_v1.has_permission(0x01, "example.com"), Ok(()));
         assert_eq!(
-            pin_protocol_v1.has_permission(0x01, "counter-example.com"),
+            pin_protocol_v1.has_permission_for_rp_id("example.com"),
-            Err(Ctap2StatusCode::CTAP2_ERR_PIN_AUTH_INVALID)
+            Ok(())
         );
         assert_eq!(
-            pin_protocol_v1.has_permission(0x04, "example.com"),
+            pin_protocol_v1.has_permission_for_rp_id("counter-example.com"),
             Err(Ctap2StatusCode::CTAP2_ERR_PIN_AUTH_INVALID)
         );
     }

src/ctap/response.rs

@@ -288,8 +288,9 @@ mod test {
 
     #[test]
     fn test_get_info_into_cbor() {
+        let versions = vec!["FIDO_2_0".to_string()];
         let get_info_response = AuthenticatorGetInfoResponse {
-            versions: vec!["FIDO_2_0".to_string()],
+            versions: versions.clone(),
             extensions: None,
             aaguid: [0x00; 16],
             options: None,
@@ -313,12 +314,12 @@ mod test {
             ResponseData::AuthenticatorGetInfo(get_info_response).into();
         #[cfg(not(feature = "with_ctap2_1"))]
         let expected_cbor = cbor_map_options! {
-            0x01 => cbor_array_vec![vec!["FIDO_2_0"]],
+            0x01 => cbor_array_vec![versions],
             0x03 => vec![0x00; 16],
         };
         #[cfg(feature = "with_ctap2_1")]
         let expected_cbor = cbor_map_options! {
-            0x01 => cbor_array_vec![vec!["FIDO_2_0"]],
+            0x01 => cbor_array_vec![versions],
             0x03 => vec![0x00; 16],
             0x0D => 4,
         };

src/ctap/storage.rs

@@ -66,7 +66,8 @@ const AAGUID: usize = 7;
 const MIN_PIN_LENGTH: usize = 8;
 #[cfg(feature = "with_ctap2_1")]
 const MIN_PIN_LENGTH_RP_IDS: usize = 9;
-// Different NUM_TAGS make the storage incompatible, so we use max(8,10).
+// Different NUM_TAGS depending on the CTAP version make the storage incompatible,
+// so we use the maximum.
 const NUM_TAGS: usize = 10;
 
 const MAX_PIN_RETRIES: u8 = 6;
@@ -417,19 +418,20 @@ impl PersistentStore {
             }
             Some((index, entry)) => {
                 debug_assert_eq!(entry.data.len(), 1);
-                if entry.data[0] > 0 {
+                if entry.data[0] == 0 {
-                    let new_value = entry.data[0].saturating_sub(1);
+                    return;
-                    self.store
-                        .replace(
-                            index,
-                            StoreEntry {
-                                tag: PIN_RETRIES,
-                                data: &[new_value],
-                                sensitive: false,
-                            },
-                        )
-                        .unwrap();
                 }
+                let new_value = entry.data[0].saturating_sub(1);
+                self.store
+                    .replace(
+                        index,
+                        StoreEntry {
+                            tag: PIN_RETRIES,
+                            data: &[new_value],
+                            sensitive: false,
+                        },
+                    )
+                    .unwrap();
             }
         }
     }