Move out check credProtectPolicy logic (#516)

* Move out check credProtectPolicy logic

Move the credProtectPolicy check outside credential ID decryption &
discoverable credential finding. Modify the unit tests, and add unit
tests for credProtectPolicy checking in non resident flows that were
originally missing.

src/ctap/credential_id.rs

@@ -206,7 +206,6 @@ pub fn decrypt_credential_id(
     env: &mut impl Env,
     credential_id: Vec<u8>,
     rp_id_hash: &[u8],
-    check_cred_protect: bool,
 ) -> Result<Option<PublicKeyCredentialSource>, Ctap2StatusCode> {
     if credential_id.len() < MIN_CREDENTIAL_ID_SIZE {
         return Ok(None);
@@ -240,9 +239,7 @@ pub fn decrypt_credential_id(
         return Ok(None);
     };
 
-    let is_protected = credential_source.cred_protect_policy
+    if rp_id_hash != credential_source.rp_id_hash {
-        == Some(CredentialProtectionPolicy::UserVerificationRequired);
-    if rp_id_hash != credential_source.rp_id_hash || (check_cred_protect && is_protected) {
         return Ok(None);
     }
 
@@ -279,7 +276,7 @@ mod test {
         let rp_id_hash = [0x55; 32];
         let encrypted_id =
             encrypt_to_credential_id(&mut env, &private_key, &rp_id_hash, None).unwrap();
-        let decrypted_source = decrypt_credential_id(&mut env, encrypted_id, &rp_id_hash, false)
+        let decrypted_source = decrypt_credential_id(&mut env, encrypted_id, &rp_id_hash)
             .unwrap()
             .unwrap();
 
@@ -313,7 +310,7 @@ mod test {
         encrypted_id.extend(&id_hmac);
 
         assert_eq!(
-            decrypt_credential_id(&mut env, encrypted_id, &rp_id_hash, false),
+            decrypt_credential_id(&mut env, encrypted_id, &rp_id_hash),
             Ok(None)
         );
     }
@@ -329,7 +326,7 @@ mod test {
             let mut modified_id = encrypted_id.clone();
             modified_id[i] ^= 0x01;
             assert_eq!(
-                decrypt_credential_id(&mut env, modified_id, &rp_id_hash, false),
+                decrypt_credential_id(&mut env, modified_id, &rp_id_hash),
                 Ok(None)
             );
         }
@@ -356,12 +353,7 @@ mod test {
 
         for length in (1..CBOR_CREDENTIAL_ID_SIZE).step_by(16) {
             assert_eq!(
-                decrypt_credential_id(
+                decrypt_credential_id(&mut env, encrypted_id[..length].to_vec(), &rp_id_hash),
-                    &mut env,
-                    encrypted_id[..length].to_vec(),
-                    &rp_id_hash,
-                    false
-                ),
                 Ok(None)
             );
         }
@@ -408,13 +400,13 @@ mod test {
         let rp_id_hash = [0x55; 32];
         let encrypted_id =
             legacy_encrypt_to_credential_id(&mut env, ecdsa_key, &rp_id_hash).unwrap();
-        // When checking credProtect for legacy credentials the check will always pass because we didn't persist credProtect
+        let decrypted_source = decrypt_credential_id(&mut env, encrypted_id, &rp_id_hash)
-        // policy info in it.
-        let decrypted_source = decrypt_credential_id(&mut env, encrypted_id, &rp_id_hash, true)
             .unwrap()
             .unwrap();
 
         assert_eq!(private_key, decrypted_source.private_key);
+        // Legacy credentials didn't persist credProtectPolicy info, so it should be treated as None.
+        assert!(decrypted_source.cred_protect_policy.is_none());
     }
 
     #[test]
@@ -429,7 +421,7 @@ mod test {
     }
 
     #[test]
-    fn test_check_cred_protect_fail() {
+    fn test_cred_protect_persisted() {
         let mut env = TestEnv::new();
         let private_key = PrivateKey::new(&mut env, SignatureAlgorithm::Es256);
 
@@ -442,34 +434,13 @@ mod test {
         )
         .unwrap();
 
-        assert_eq!(
+        let decrypted_source = decrypt_credential_id(&mut env, encrypted_id, &rp_id_hash)
-            decrypt_credential_id(&mut env, encrypted_id, &rp_id_hash, true),
-            Ok(None)
-        );
-    }
-
-    #[test]
-    fn test_check_cred_protect_success() {
-        let mut env = TestEnv::new();
-        let private_key = PrivateKey::new(&mut env, SignatureAlgorithm::Es256);
-
-        let rp_id_hash = [0x55; 32];
-        let encrypted_id = encrypt_to_credential_id(
-            &mut env,
-            &private_key,
-            &rp_id_hash,
-            Some(CredentialProtectionPolicy::UserVerificationOptionalWithCredentialIdList),
-        )
-        .unwrap();
-
-        let decrypted_source = decrypt_credential_id(&mut env, encrypted_id, &rp_id_hash, true)
             .unwrap()
             .unwrap();
-
         assert_eq!(decrypted_source.private_key, private_key);
         assert_eq!(
             decrypted_source.cred_protect_policy,
-            Some(CredentialProtectionPolicy::UserVerificationOptionalWithCredentialIdList)
+            Some(CredentialProtectionPolicy::UserVerificationRequired)
         );
     }
 }

src/ctap/credential_management.rs

@@ -872,10 +872,9 @@ mod test {
             Ok(ResponseData::AuthenticatorCredentialManagement(None))
         );
 
-        let updated_credential =
+        let updated_credential = storage::find_credential(&mut env, "example.com", &[0x1D; 32])
-            storage::find_credential(&mut env, "example.com", &[0x1D; 32], false)
+            .unwrap()
-                .unwrap()
+            .unwrap();
-                .unwrap();
         assert_eq!(updated_credential.user_handle, vec![0x01]);
         assert_eq!(&updated_credential.user_name.unwrap(), "new_name");
         assert_eq!(

src/ctap/ctap1.rs

@@ -309,7 +309,7 @@ impl Ctap1Command {
         flags: Ctap1Flags,
         ctap_state: &mut CtapState,
     ) -> Result<Vec<u8>, Ctap1StatusCode> {
-        let credential_source = decrypt_credential_id(env, key_handle, &application, false)
+        let credential_source = decrypt_credential_id(env, key_handle, &application)
             .map_err(|_| Ctap1StatusCode::SW_WRONG_DATA)?;
         if let Some(credential_source) = credential_source {
             let ecdsa_key = credential_source
@@ -440,7 +440,6 @@ mod test {
             &mut env,
             response[67..67 + CBOR_CREDENTIAL_ID_SIZE].to_vec(),
             &application,
-            false
         )
         .unwrap()
         .is_some());

src/ctap/mod.rs

@@ -717,6 +717,22 @@ impl CtapState {
         Ok(())
     }
 
+    fn check_cred_protect_for_listed_credential(
+        &mut self,
+        credential: &Option<PublicKeyCredentialSource>,
+        has_uv: bool,
+    ) -> bool {
+        if let Some(credential) = credential {
+            has_uv
+                || !matches!(
+                    credential.cred_protect_policy,
+                    Some(CredentialProtectionPolicy::UserVerificationRequired),
+                )
+        } else {
+            false
+        }
+    }
+
     fn process_make_credential(
         &mut self,
         env: &mut impl Env,
@@ -809,9 +825,13 @@ impl CtapState {
         let rp_id_hash = Sha256::hash(rp_id.as_bytes());
         if let Some(exclude_list) = exclude_list {
             for cred_desc in exclude_list {
-                if storage::find_credential(env, &rp_id, &cred_desc.key_id, !has_uv)?.is_some()
+                if self.check_cred_protect_for_listed_credential(
-                    || decrypt_credential_id(env, cred_desc.key_id, &rp_id_hash, !has_uv)?.is_some()
+                    &storage::find_credential(env, &rp_id, &cred_desc.key_id)?,
-                {
+                    has_uv,
+                ) || self.check_cred_protect_for_listed_credential(
+                    &decrypt_credential_id(env, cred_desc.key_id, &rp_id_hash)?,
+                    has_uv,
+                ) {
                     // Perform this check, so bad actors can't brute force exclude_list
                     // without user interaction.
                     let _ = check_user_presence(env, channel);
@@ -1078,14 +1098,12 @@ impl CtapState {
         has_uv: bool,
     ) -> Result<Option<PublicKeyCredentialSource>, Ctap2StatusCode> {
         for allowed_credential in allow_list {
-            let credential =
+            let credential = storage::find_credential(env, rp_id, &allowed_credential.key_id)?;
-                storage::find_credential(env, rp_id, &allowed_credential.key_id, !has_uv)?;
+            if self.check_cred_protect_for_listed_credential(&credential, has_uv) {
-            if credential.is_some() {
                 return Ok(credential);
             }
-            let credential =
+            let credential = decrypt_credential_id(env, allowed_credential.key_id, rp_id_hash)?;
-                decrypt_credential_id(env, allowed_credential.key_id, rp_id_hash, !has_uv)?;
+            if self.check_cred_protect_for_listed_credential(&credential, has_uv) {
-            if credential.is_some() {
                 return Ok(credential);
             }
         }
@@ -1802,6 +1820,61 @@ mod test {
         assert!(make_credential_response.is_ok());
     }
 
+    #[test]
+    fn test_non_resident_process_make_credential_credential_with_cred_protect() {
+        let mut env = TestEnv::new();
+        let mut ctap_state = CtapState::new(&mut env, CtapInstant::new(0));
+
+        let test_policy = CredentialProtectionPolicy::UserVerificationOptionalWithCredentialIdList;
+        let mut make_credential_params =
+            create_make_credential_parameters_with_cred_protect_policy(test_policy);
+        make_credential_params.options.rk = false;
+        let make_credential_response =
+            ctap_state.process_make_credential(&mut env, make_credential_params, DUMMY_CHANNEL);
+        assert!(make_credential_response.is_ok());
+        let credential_id = match make_credential_response.unwrap() {
+            ResponseData::AuthenticatorMakeCredential(make_credential_response) => {
+                let auth_data = make_credential_response.auth_data;
+                let offset = 37 + storage::aaguid(&mut env).unwrap().len();
+                assert_eq!(auth_data[offset], 0x00);
+                assert_eq!(auth_data[offset + 1] as usize, CBOR_CREDENTIAL_ID_SIZE);
+                auth_data[offset + 2..offset + 2 + CBOR_CREDENTIAL_ID_SIZE].to_vec()
+            }
+            _ => panic!("Invalid response type"),
+        };
+        let make_credential_params =
+            create_make_credential_parameters_with_exclude_list(&credential_id);
+        let make_credential_response =
+            ctap_state.process_make_credential(&mut env, make_credential_params, DUMMY_CHANNEL);
+        assert_eq!(
+            make_credential_response,
+            Err(Ctap2StatusCode::CTAP2_ERR_CREDENTIAL_EXCLUDED)
+        );
+
+        let test_policy = CredentialProtectionPolicy::UserVerificationRequired;
+        let mut make_credential_params =
+            create_make_credential_parameters_with_cred_protect_policy(test_policy);
+        make_credential_params.options.rk = false;
+        let make_credential_response =
+            ctap_state.process_make_credential(&mut env, make_credential_params, DUMMY_CHANNEL);
+        assert!(make_credential_response.is_ok());
+        let credential_id = match make_credential_response.unwrap() {
+            ResponseData::AuthenticatorMakeCredential(make_credential_response) => {
+                let auth_data = make_credential_response.auth_data;
+                let offset = 37 + storage::aaguid(&mut env).unwrap().len();
+                assert_eq!(auth_data[offset], 0x00);
+                assert_eq!(auth_data[offset + 1] as usize, CBOR_CREDENTIAL_ID_SIZE);
+                auth_data[offset + 2..offset + 2 + CBOR_CREDENTIAL_ID_SIZE].to_vec()
+            }
+            _ => panic!("Invalid response type"),
+        };
+        let make_credential_params =
+            create_make_credential_parameters_with_exclude_list(&credential_id);
+        let make_credential_response =
+            ctap_state.process_make_credential(&mut env, make_credential_params, DUMMY_CHANNEL);
+        assert!(make_credential_response.is_ok());
+    }
+
     #[test]
     fn test_process_make_credential_hmac_secret() {
         let mut env = TestEnv::new();
@@ -2650,6 +2723,99 @@ mod test {
         );
     }
 
+    #[test]
+    fn test_non_resident_process_get_assertion_with_cred_protect() {
+        let mut env = TestEnv::new();
+        let mut ctap_state = CtapState::new(&mut env, CtapInstant::new(0));
+
+        let test_policy = CredentialProtectionPolicy::UserVerificationOptionalWithCredentialIdList;
+        let mut make_credential_params =
+            create_make_credential_parameters_with_cred_protect_policy(test_policy);
+        make_credential_params.options.rk = false;
+        let make_credential_response =
+            ctap_state.process_make_credential(&mut env, make_credential_params, DUMMY_CHANNEL);
+        assert!(make_credential_response.is_ok());
+        let credential_id = match make_credential_response.unwrap() {
+            ResponseData::AuthenticatorMakeCredential(make_credential_response) => {
+                let auth_data = make_credential_response.auth_data;
+                let offset = 37 + storage::aaguid(&mut env).unwrap().len();
+                assert_eq!(auth_data[offset], 0x00);
+                assert_eq!(auth_data[offset + 1] as usize, CBOR_CREDENTIAL_ID_SIZE);
+                auth_data[offset + 2..offset + 2 + CBOR_CREDENTIAL_ID_SIZE].to_vec()
+            }
+            _ => panic!("Invalid response type"),
+        };
+        let cred_desc = PublicKeyCredentialDescriptor {
+            key_type: PublicKeyCredentialType::PublicKey,
+            key_id: credential_id,
+            transports: None,
+        };
+        let get_assertion_params = AuthenticatorGetAssertionParameters {
+            rp_id: String::from("example.com"),
+            client_data_hash: vec![0xCD],
+            allow_list: Some(vec![cred_desc]),
+            extensions: GetAssertionExtensions::default(),
+            options: GetAssertionOptions {
+                up: false,
+                uv: false,
+            },
+            pin_uv_auth_param: None,
+            pin_uv_auth_protocol: None,
+        };
+        let get_assertion_response = ctap_state.process_get_assertion(
+            &mut env,
+            get_assertion_params,
+            DUMMY_CHANNEL,
+            CtapInstant::new(0),
+        );
+        assert!(get_assertion_response.is_ok());
+
+        let test_policy = CredentialProtectionPolicy::UserVerificationRequired;
+        let mut make_credential_params =
+            create_make_credential_parameters_with_cred_protect_policy(test_policy);
+        make_credential_params.options.rk = false;
+        let make_credential_response =
+            ctap_state.process_make_credential(&mut env, make_credential_params, DUMMY_CHANNEL);
+        assert!(make_credential_response.is_ok());
+        let credential_id = match make_credential_response.unwrap() {
+            ResponseData::AuthenticatorMakeCredential(make_credential_response) => {
+                let auth_data = make_credential_response.auth_data;
+                let offset = 37 + storage::aaguid(&mut env).unwrap().len();
+                assert_eq!(auth_data[offset], 0x00);
+                assert_eq!(auth_data[offset + 1] as usize, CBOR_CREDENTIAL_ID_SIZE);
+                auth_data[offset + 2..offset + 2 + CBOR_CREDENTIAL_ID_SIZE].to_vec()
+            }
+            _ => panic!("Invalid response type"),
+        };
+        let cred_desc = PublicKeyCredentialDescriptor {
+            key_type: PublicKeyCredentialType::PublicKey,
+            key_id: credential_id,
+            transports: None,
+        };
+        let get_assertion_params = AuthenticatorGetAssertionParameters {
+            rp_id: String::from("example.com"),
+            client_data_hash: vec![0xCD],
+            allow_list: Some(vec![cred_desc]),
+            extensions: GetAssertionExtensions::default(),
+            options: GetAssertionOptions {
+                up: false,
+                uv: false,
+            },
+            pin_uv_auth_param: None,
+            pin_uv_auth_protocol: None,
+        };
+        let get_assertion_response = ctap_state.process_get_assertion(
+            &mut env,
+            get_assertion_params,
+            DUMMY_CHANNEL,
+            CtapInstant::new(0),
+        );
+        assert_eq!(
+            get_assertion_response,
+            Err(Ctap2StatusCode::CTAP2_ERR_NO_CREDENTIALS),
+        );
+    }
+
     #[test]
     fn test_process_get_assertion_with_cred_blob() {
         let mut env = TestEnv::new();

src/ctap/storage.rs

@@ -19,8 +19,7 @@ use crate::api::customization::Customization;
 use crate::api::key_store::KeyStore;
 use crate::ctap::client_pin::PIN_AUTH_LENGTH;
 use crate::ctap::data_formats::{
-    extract_array, extract_text_string, CredentialProtectionPolicy, PublicKeyCredentialSource,
+    extract_array, extract_text_string, PublicKeyCredentialSource, PublicKeyCredentialUserEntity,
-    PublicKeyCredentialUserEntity,
 };
 use crate::ctap::status_code::Ctap2StatusCode;
 use crate::ctap::{key_material, INITIAL_SIGNATURE_COUNTER};
@@ -114,16 +113,13 @@ pub fn find_credential(
     env: &mut impl Env,
     rp_id: &str,
     credential_id: &[u8],
-    check_cred_protect: bool,
 ) -> Result<Option<PublicKeyCredentialSource>, Ctap2StatusCode> {
     let credential = match find_credential_item(env, credential_id) {
         Err(Ctap2StatusCode::CTAP2_ERR_NO_CREDENTIALS) => return Ok(None),
         Err(e) => return Err(e),
         Ok((_key, credential)) => credential,
     };
-    let is_protected = credential.cred_protect_policy
+    if credential.rp_id != rp_id {
-        == Some(CredentialProtectionPolicy::UserVerificationRequired);
-    if credential.rp_id != rp_id || (check_cred_protect && is_protected) {
         return Ok(None);
     }
     Ok(Some(credential))
@@ -651,7 +647,9 @@ mod test {
     use super::*;
     use crate::api::attestation_store::{self, Attestation, AttestationStore};
     use crate::ctap::crypto_wrapper::PrivateKey;
-    use crate::ctap::data_formats::{PublicKeyCredentialSource, PublicKeyCredentialType};
+    use crate::ctap::data_formats::{
+        CredentialProtectionPolicy, PublicKeyCredentialSource, PublicKeyCredentialType,
+    };
     use crate::env::test::TestEnv;
     use rng256::Rng256;
 
@@ -725,14 +723,14 @@ mod test {
         let credential_source = create_credential_source(&mut env, "example.com", vec![0x1D]);
         let credential_id = credential_source.credential_id.clone();
         assert!(store_credential(&mut env, credential_source).is_ok());
-        let stored_credential = find_credential(&mut env, "example.com", &credential_id, false)
+        let stored_credential = find_credential(&mut env, "example.com", &credential_id)
             .unwrap()
             .unwrap();
         assert_eq!(stored_credential.user_name, None);
         assert_eq!(stored_credential.user_display_name, None);
         assert_eq!(stored_credential.user_icon, None);
         assert!(update_credential(&mut env, &credential_id, user.clone()).is_ok());
-        let stored_credential = find_credential(&mut env, "example.com", &credential_id, false)
+        let stored_credential = find_credential(&mut env, "example.com", &credential_id)
             .unwrap()
             .unwrap();
         assert_eq!(stored_credential.user_name, user.user_name);
@@ -796,16 +794,12 @@ mod test {
         assert!(store_credential(&mut env, credential_source0).is_ok());
         assert!(store_credential(&mut env, credential_source1).is_ok());
         assert_eq!(count_credentials(&mut env).unwrap(), 1);
-        assert!(
+        assert!(find_credential(&mut env, "example.com", &credential_id0)
-            find_credential(&mut env, "example.com", &credential_id0, false)
+            .unwrap()
-                .unwrap()
+            .is_none());
-                .is_none()
+        assert!(find_credential(&mut env, "example.com", &credential_id1)
-        );
+            .unwrap()
-        assert!(
+            .is_some());
-            find_credential(&mut env, "example.com", &credential_id1, false)
-                .unwrap()
-                .is_some()
-        );
 
         reset(&mut env).unwrap();
         let max_supported_resident_keys = env.customization().max_supported_resident_keys();
@@ -858,9 +852,9 @@ mod test {
         assert!(store_credential(&mut env, credential_source0).is_ok());
         assert!(store_credential(&mut env, credential_source1).is_ok());
 
-        let no_credential = find_credential(&mut env, "another.example.com", &id0, false).unwrap();
+        let no_credential = find_credential(&mut env, "another.example.com", &id0).unwrap();
         assert_eq!(no_credential, None);
-        let found_credential = find_credential(&mut env, "example.com", &id0, false).unwrap();
+        let found_credential = find_credential(&mut env, "example.com", &id0).unwrap();
         let expected_credential = PublicKeyCredentialSource {
             key_type: PublicKeyCredentialType::PublicKey,
             credential_id: id0,
@@ -878,31 +872,6 @@ mod test {
         assert_eq!(found_credential, Some(expected_credential));
     }
 
-    #[test]
-    fn test_find_with_cred_protect() {
-        let mut env = TestEnv::new();
-        assert_eq!(count_credentials(&mut env).unwrap(), 0);
-        let private_key = PrivateKey::new_ecdsa(&mut env);
-        let credential = PublicKeyCredentialSource {
-            key_type: PublicKeyCredentialType::PublicKey,
-            credential_id: env.rng().gen_uniform_u8x32().to_vec(),
-            private_key,
-            rp_id: String::from("example.com"),
-            user_handle: vec![0x00],
-            user_display_name: None,
-            cred_protect_policy: Some(CredentialProtectionPolicy::UserVerificationRequired),
-            creation_order: 0,
-            user_name: None,
-            user_icon: None,
-            cred_blob: None,
-            large_blob_key: None,
-        };
-        assert!(store_credential(&mut env, credential).is_ok());
-
-        let no_credential = find_credential(&mut env, "example.com", &[0x00], true).unwrap();
-        assert_eq!(no_credential, None);
-    }
-
     #[test]
     fn test_cred_random_secret() {
         let mut env = TestEnv::new();