Fixes credProtect checking in CTAP1 (#629)

We accidentally lost this check in #516. I refactored some of the
filters for better style.

The actual difference in logic is just one line in CTAP1 authenticate,
everything else is style, a test and the order in which we convert and
filter the credentials:

```
let credential_source = filter_listed_credential(credential_source, false)
            .ok_or(Ctap1StatusCode::SW_WRONG_DATA)?;
```

libraries/opensk/src/ctap/ctap1.rs

@@ -13,7 +13,7 @@
 // limitations under the License.
 
 use super::apdu::{Apdu, ApduStatusCode};
-use super::CtapState;
+use super::{filter_listed_credential, CtapState};
 use crate::api::attestation_store::{self, Attestation, AttestationStore};
 use crate::api::crypto::ecdsa::{self, SecretKey as _, Signature};
 use crate::api::crypto::EC_FIELD_SIZE;
@@ -331,39 +331,37 @@ impl Ctap1Command {
             .key_store()
             .unwrap_credential(&key_handle, &application)
             .map_err(|_| Ctap1StatusCode::SW_WRONG_DATA)?;
-        if let Some(credential_source) = credential_source {
+        let credential_source = filter_listed_credential(credential_source, false)
-            let ecdsa_key = credential_source
+            .ok_or(Ctap1StatusCode::SW_WRONG_DATA)?;
-                .private_key
+        let ecdsa_key = credential_source
-                .ecdsa_key(env)
+            .private_key
-                .map_err(|_| Ctap1StatusCode::SW_WRONG_DATA)?;
+            .ecdsa_key(env)
-            if flags == Ctap1Flags::CheckOnly {
+            .map_err(|_| Ctap1StatusCode::SW_WRONG_DATA)?;
-                return Err(Ctap1StatusCode::SW_COND_USE_NOT_SATISFIED);
+        if flags == Ctap1Flags::CheckOnly {
-            }
+            return Err(Ctap1StatusCode::SW_COND_USE_NOT_SATISFIED);
-            ctap_state
-                .increment_global_signature_counter(env)
-                .map_err(|_| Ctap1StatusCode::SW_WRONG_DATA)?;
-            let mut signature_data = ctap_state
-                .generate_auth_data(
-                    env,
-                    &application,
-                    Ctap1Command::USER_PRESENCE_INDICATOR_BYTE,
-                )
-                .map_err(|_| Ctap1StatusCode::SW_WRONG_DATA)?;
-            signature_data.extend(&challenge);
-            let signature = ecdsa_key.sign(&signature_data);
-
-            let mut response = signature_data[application.len()..application.len() + 5].to_vec();
-            response.extend(signature.to_der());
-            Ok(response)
-        } else {
-            Err(Ctap1StatusCode::SW_WRONG_DATA)
         }
+        ctap_state
+            .increment_global_signature_counter(env)
+            .map_err(|_| Ctap1StatusCode::SW_WRONG_DATA)?;
+        let mut signature_data = ctap_state
+            .generate_auth_data(
+                env,
+                &application,
+                Ctap1Command::USER_PRESENCE_INDICATOR_BYTE,
+            )
+            .map_err(|_| Ctap1StatusCode::SW_WRONG_DATA)?;
+        signature_data.extend(&challenge);
+        let signature = ecdsa_key.sign(&signature_data);
+
+        let mut response = signature_data[application.len()..application.len() + 5].to_vec();
+        response.extend(signature.to_der());
+        Ok(response)
     }
 }
 
 #[cfg(test)]
 mod test {
-    use super::super::data_formats::SignatureAlgorithm;
+    use super::super::data_formats::{CredentialProtectionPolicy, SignatureAlgorithm};
     use super::super::TOUCH_TIMEOUT_MS;
     use super::*;
     use crate::api::crypto::sha256::Sha256;
@@ -712,4 +710,27 @@ mod test {
         let response = Ctap1Command::process_command(&mut env, &message, &mut ctap_state);
         assert_eq!(response, Err(Ctap1StatusCode::SW_COND_USE_NOT_SATISFIED));
     }
+
+    #[test]
+    fn test_process_authenticate_cred_protect() {
+        let mut env = TestEnv::default();
+        env.user_presence()
+            .set(|| panic!("Unexpected user presence check in CTAP1"));
+        let mut ctap_state = CtapState::new(&mut env);
+
+        let private_key = PrivateKey::new(&mut env, SignatureAlgorithm::Es256);
+        let rp_id_hash = Sha::<TestEnv>::digest(b"example.com");
+        let credential_source = CredentialSource {
+            private_key,
+            rp_id_hash,
+            cred_protect_policy: Some(CredentialProtectionPolicy::UserVerificationRequired),
+            cred_blob: None,
+        };
+        let key_handle = env.key_store().wrap_credential(credential_source).unwrap();
+        let message =
+            create_authenticate_message(&rp_id_hash, Ctap1Flags::DontEnforceUpAndSign, &key_handle);
+
+        let response = Ctap1Command::process_command(&mut env, &message, &mut ctap_state);
+        assert_eq!(response, Err(Ctap1StatusCode::SW_WRONG_DATA));
+    }
 }

libraries/opensk/src/ctap/mod.rs

@@ -186,28 +186,53 @@ pub fn cbor_write(value: cbor::Value, encoded_cbor: &mut Vec<u8>) -> Result<(),
         .map_err(|_e| Ctap2StatusCode::CTAP2_ERR_VENDOR_INTERNAL_ERROR)
 }
 
-fn decrypt_credential_id<E: Env>(
+/// Filters the credential from the option if credProtect criteria are not met.
-    env: &mut E,
+pub fn filter_listed_credential(
+    credential: Option<CredentialSource>,
+    has_uv: bool,
+) -> Option<CredentialSource> {
+    credential.filter(|c| {
+        has_uv
+            || !matches!(
+                c.cred_protect_policy,
+                Some(CredentialProtectionPolicy::UserVerificationRequired)
+            )
+    })
+}
+
+/// Filters the resident key from the option if credProtect criteria are not met.
+fn filter_listed_resident_credential(
+    credential: Option<PublicKeyCredentialSource>,
+    has_uv: bool,
+) -> Option<PublicKeyCredentialSource> {
+    credential.filter(|c| {
+        has_uv
+            || !matches!(
+                c.cred_protect_policy,
+                Some(CredentialProtectionPolicy::UserVerificationRequired)
+            )
+    })
+}
+
+/// Populates all matching fields in a `PublicKeyCredentialSource`.
+fn to_public_source(
     credential_id: Vec<u8>,
-    rp_id_hash: &[u8],
+    credential_source: CredentialSource,
-) -> Result<Option<PublicKeyCredentialSource>, Ctap2StatusCode> {
+) -> PublicKeyCredentialSource {
-    let credential_source = env
+    PublicKeyCredentialSource {
-        .key_store()
-        .unwrap_credential(&credential_id, rp_id_hash)?;
-    Ok(credential_source.map(|c| PublicKeyCredentialSource {
         key_type: PublicKeyCredentialType::PublicKey,
         credential_id,
-        private_key: c.private_key,
+        private_key: credential_source.private_key,
         rp_id: String::new(),
         user_handle: Vec::new(),
         user_display_name: None,
-        cred_protect_policy: c.cred_protect_policy,
+        cred_protect_policy: credential_source.cred_protect_policy,
         creation_order: 0,
         user_name: None,
         user_icon: None,
-        cred_blob: c.cred_blob,
+        cred_blob: credential_source.cred_blob,
         large_blob_key: None,
-    }))
+    }
 }
 
 // This function is adapted from https://doc.rust-lang.org/nightly/src/core/str/mod.rs.html#2110
@@ -698,22 +723,6 @@ impl<E: Env> CtapState<E> {
         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 E,
@@ -806,13 +815,18 @@ impl<E: Env> CtapState<E> {
         let rp_id_hash = Sha::<E>::digest(rp_id.as_bytes());
         if let Some(exclude_list) = exclude_list {
             for cred_desc in exclude_list {
-                if self.check_cred_protect_for_listed_credential(
+                if filter_listed_resident_credential(
-                    &storage::find_credential(env, &rp_id, &cred_desc.key_id)?,
+                    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)?,
+                .is_some()
-                    has_uv,
+                    || filter_listed_credential(
-                ) {
+                        env.key_store()
+                            .unwrap_credential(&cred_desc.key_id, &rp_id_hash)?,
+                        has_uv,
+                    )
+                    .is_some()
+                {
                     // Perform this check, so bad actors can't brute force exclude_list
                     // without user interaction.
                     let _ = check_user_presence(env, channel);
@@ -1087,13 +1101,20 @@ impl<E: Env> CtapState<E> {
         has_uv: bool,
     ) -> Result<Option<PublicKeyCredentialSource>, Ctap2StatusCode> {
         for allowed_credential in allow_list {
-            let credential = storage::find_credential(env, rp_id, &allowed_credential.key_id)?;
+            let credential = filter_listed_resident_credential(
-            if self.check_cred_protect_for_listed_credential(&credential, has_uv) {
+                storage::find_credential(env, rp_id, &allowed_credential.key_id)?,
+                has_uv,
+            );
+            if credential.is_some() {
                 return Ok(credential);
             }
-            let credential = decrypt_credential_id(env, allowed_credential.key_id, rp_id_hash)?;
+            let credential = filter_listed_credential(
-            if self.check_cred_protect_for_listed_credential(&credential, has_uv) {
+                env.key_store()
-                return Ok(credential);
+                    .unwrap_credential(&allowed_credential.key_id, &rp_id_hash)?,
+                has_uv,
+            );
+            if credential.is_some() {
+                return Ok(credential.map(|c| to_public_source(allowed_credential.key_id, c)));
             }
         }
         Ok(None)