final changes to CTAP2.1 algorithm (#299)

src/ctap/client_pin.rs

@@ -198,7 +198,7 @@ impl ClientPin {
     ) -> Result<AuthenticatorClientPinResponse, Ctap2StatusCode> {
         Ok(AuthenticatorClientPinResponse {
             key_agreement: None,
-            pin_token: None,
+            pin_uv_auth_token: None,
             retries: Some(persistent_store.pin_retries()? as u64),
             power_cycle_state: Some(self.consecutive_pin_mismatches >= 3),
         })
@@ -214,7 +214,7 @@ impl ClientPin {
         );
         Ok(AuthenticatorClientPinResponse {
             key_agreement,
-            pin_token: None,
+            pin_uv_auth_token: None,
             retries: None,
             power_cycle_state: None,
         })
@@ -298,10 +298,15 @@ impl ClientPin {
             pin_uv_auth_protocol,
             key_agreement,
             pin_hash_enc,
+            permissions,
+            permissions_rp_id,
             ..
         } = client_pin_params;
         let key_agreement = ok_or_missing(key_agreement)?;
         let pin_hash_enc = ok_or_missing(pin_hash_enc)?;
+        if permissions.is_some() || permissions_rp_id.is_some() {
+            return Err(Ctap2StatusCode::CTAP1_ERR_INVALID_PARAMETER);
+        }
 
         if persistent_store.pin_retries()? == 0 {
             return Err(Ctap2StatusCode::CTAP2_ERR_PIN_BLOCKED);
@@ -317,21 +322,21 @@ impl ClientPin {
         if persistent_store.has_force_pin_change()? {
             return Err(Ctap2StatusCode::CTAP2_ERR_PIN_INVALID);
         }
-        let pin_token = shared_secret.encrypt(
-            rng,
-            self.get_pin_protocol(pin_uv_auth_protocol)
-                .get_pin_uv_auth_token(),
-        )?;
 
         self.pin_protocol_v1.reset_pin_uv_auth_token(rng);
         self.pin_protocol_v2.reset_pin_uv_auth_token(rng);
         self.pin_uv_auth_token_state
             .begin_using_pin_uv_auth_token(now);
         self.pin_uv_auth_token_state.set_default_permissions();
+        let pin_uv_auth_token = shared_secret.encrypt(
+            rng,
+            self.get_pin_protocol(pin_uv_auth_protocol)
+                .get_pin_uv_auth_token(),
+        )?;
 
         Ok(AuthenticatorClientPinResponse {
             key_agreement: None,
-            pin_token: Some(pin_token),
+            pin_uv_auth_token: Some(pin_uv_auth_token),
             retries: None,
             power_cycle_state: None,
         })
@@ -359,9 +364,10 @@ impl ClientPin {
         mut client_pin_params: AuthenticatorClientPinParameters,
         now: ClockValue,
     ) -> Result<AuthenticatorClientPinResponse, Ctap2StatusCode> {
-        let permissions = ok_or_missing(client_pin_params.permissions)?;
         // Mutating client_pin_params is just an optimization to move it into
         // process_get_pin_token, without cloning permissions_rp_id here.
+        // getPinToken requires permissions* to be None.
+        let permissions = ok_or_missing(client_pin_params.permissions.take())?;
         let permissions_rp_id = client_pin_params.permissions_rp_id.take();
 
         if permissions == 0 {
@@ -657,6 +663,13 @@ mod test {
             .as_ref()
             .encrypt(&mut rng, &pin_hash[..16])
             .unwrap();
+        let (permissions, permissions_rp_id) = match sub_command {
+            ClientPinSubCommand::GetPinUvAuthTokenUsingUvWithPermissions
+            | ClientPinSubCommand::GetPinUvAuthTokenUsingPinWithPermissions => {
+                (Some(0x03), Some("example.com".to_string()))
+            }
+            _ => (None, None),
+        };
         let params = AuthenticatorClientPinParameters {
             pin_uv_auth_protocol,
             sub_command,
@@ -668,8 +681,8 @@ mod test {
             pin_uv_auth_param: Some(pin_uv_auth_param),
             new_pin_enc: Some(new_pin_enc),
             pin_hash_enc: Some(pin_hash_enc),
-            permissions: Some(0x03),
+            permissions,
-            permissions_rp_id: Some("example.com".to_string()),
+            permissions_rp_id,
         };
         (client_pin, params)
     }
@@ -813,7 +826,7 @@ mod test {
         let mut persistent_store = PersistentStore::new(&mut rng);
         let expected_response = Some(AuthenticatorClientPinResponse {
             key_agreement: None,
-            pin_token: None,
+            pin_uv_auth_token: None,
             retries: Some(persistent_store.pin_retries().unwrap() as u64),
             power_cycle_state: Some(false),
         });
@@ -830,7 +843,7 @@ mod test {
         client_pin.consecutive_pin_mismatches = 3;
         let expected_response = Some(AuthenticatorClientPinResponse {
             key_agreement: None,
-            pin_token: None,
+            pin_uv_auth_token: None,
             retries: Some(persistent_store.pin_retries().unwrap() as u64),
             power_cycle_state: Some(true),
         });
@@ -859,7 +872,7 @@ mod test {
         let mut persistent_store = PersistentStore::new(&mut rng);
         let expected_response = Some(AuthenticatorClientPinResponse {
             key_agreement: params.key_agreement.clone(),
-            pin_token: None,
+            pin_uv_auth_token: None,
             retries: None,
             power_cycle_state: None,
         });
@@ -964,18 +977,37 @@ mod test {
             pin_uv_auth_protocol,
             ClientPinSubCommand::GetPinToken,
         );
+        let shared_secret = client_pin
+            .get_pin_protocol(pin_uv_auth_protocol)
+            .decapsulate(
+                params.key_agreement.clone().unwrap(),
+                params.pin_uv_auth_protocol,
+            )
+            .unwrap();
         let mut rng = ThreadRng256 {};
         let mut persistent_store = PersistentStore::new(&mut rng);
         set_standard_pin(&mut persistent_store);
 
-        assert!(client_pin
+        let response = client_pin
             .process_command(
                 &mut rng,
                 &mut persistent_store,
                 params.clone(),
-                DUMMY_CLOCK_VALUE
+                DUMMY_CLOCK_VALUE,
             )
-            .is_ok());
+            .unwrap();
+        let encrypted_token = match response {
+            ResponseData::AuthenticatorClientPin(Some(response)) => {
+                response.pin_uv_auth_token.unwrap()
+            }
+            _ => panic!("Invalid response type"),
+        };
+        assert_eq!(
+            &shared_secret.decrypt(&encrypted_token).unwrap(),
+            client_pin
+                .get_pin_protocol(pin_uv_auth_protocol)
+                .get_pin_uv_auth_token()
+        );
         assert_eq!(
             client_pin
                 .pin_uv_auth_token_state
@@ -1051,18 +1083,37 @@ mod test {
             pin_uv_auth_protocol,
             ClientPinSubCommand::GetPinUvAuthTokenUsingPinWithPermissions,
         );
+        let shared_secret = client_pin
+            .get_pin_protocol(pin_uv_auth_protocol)
+            .decapsulate(
+                params.key_agreement.clone().unwrap(),
+                params.pin_uv_auth_protocol,
+            )
+            .unwrap();
         let mut rng = ThreadRng256 {};
         let mut persistent_store = PersistentStore::new(&mut rng);
         set_standard_pin(&mut persistent_store);
 
-        assert!(client_pin
+        let response = client_pin
             .process_command(
                 &mut rng,
                 &mut persistent_store,
                 params.clone(),
-                DUMMY_CLOCK_VALUE
+                DUMMY_CLOCK_VALUE,
             )
-            .is_ok());
+            .unwrap();
+        let encrypted_token = match response {
+            ResponseData::AuthenticatorClientPin(Some(response)) => {
+                response.pin_uv_auth_token.unwrap()
+            }
+            _ => panic!("Invalid response type"),
+        };
+        assert_eq!(
+            &shared_secret.decrypt(&encrypted_token).unwrap(),
+            client_pin
+                .get_pin_protocol(pin_uv_auth_protocol)
+                .get_pin_uv_auth_token()
+        );
         assert_eq!(
             client_pin
                 .pin_uv_auth_token_state

src/ctap/data_formats.rs

@@ -409,8 +409,11 @@ impl TryFrom<cbor::Value> for MakeCredentialOptions {
             Some(options_entry) => extract_bool(options_entry)?,
             None => false,
         };
-        if up.is_some() {
+        // In CTAP2.0, the up option is supposed to always fail when present.
-            return Err(Ctap2StatusCode::CTAP2_ERR_INVALID_OPTION);
+        if let Some(options_entry) = up {
+            if !extract_bool(options_entry)? {
+                return Err(Ctap2StatusCode::CTAP2_ERR_INVALID_OPTION);
+            }
         }
         let uv = match uv {
             Some(options_entry) => extract_bool(options_entry)?,

src/ctap/mod.rs

@@ -554,6 +554,68 @@ where
             false
         };
 
+        // MakeCredential always requires user presence.
+        // User verification depends on the PIN auth inputs, which are checked here.
+        // The ED flag is added later, if applicable.
+        let has_uv = pin_uv_auth_param.is_some();
+        let mut flags = match pin_uv_auth_param {
+            Some(pin_uv_auth_param) => {
+                // This case is not mentioned in CTAP2.1, so we keep 2.0 logic.
+                if self.persistent_store.pin_hash()?.is_none() {
+                    return Err(Ctap2StatusCode::CTAP2_ERR_PIN_NOT_SET);
+                }
+                self.client_pin.verify_pin_uv_auth_token(
+                    &client_data_hash,
+                    &pin_uv_auth_param,
+                    pin_uv_auth_protocol.ok_or(Ctap2StatusCode::CTAP2_ERR_MISSING_PARAMETER)?,
+                )?;
+                self.client_pin
+                    .has_permission(PinPermission::MakeCredential)?;
+                self.client_pin.check_user_verified_flag()?;
+                // Checking for the correct permissions_rp_id is specified earlier.
+                // Error codes are identical though, so the implementation can be identical with
+                // GetAssertion.
+                self.client_pin.ensure_rp_id_permission(&rp_id)?;
+                UV_FLAG
+            }
+            None => {
+                if options.uv {
+                    return Err(Ctap2StatusCode::CTAP2_ERR_INVALID_OPTION);
+                }
+                if self.persistent_store.has_always_uv()? {
+                    return Err(Ctap2StatusCode::CTAP2_ERR_PUAT_REQUIRED);
+                }
+                // Corresponds to makeCredUvNotRqd set to true.
+                if options.rk && self.persistent_store.pin_hash()?.is_some() {
+                    return Err(Ctap2StatusCode::CTAP2_ERR_PUAT_REQUIRED);
+                }
+                0x00
+            }
+        };
+        flags |= UP_FLAG | AT_FLAG;
+
+        let rp_id_hash = Sha256::hash(rp_id.as_bytes());
+        if let Some(exclude_list) = exclude_list {
+            for cred_desc in exclude_list {
+                if self
+                    .persistent_store
+                    .find_credential(&rp_id, &cred_desc.key_id, !has_uv)?
+                    .is_some()
+                    || self
+                        .decrypt_credential_source(cred_desc.key_id, &rp_id_hash)?
+                        .is_some()
+                {
+                    // Perform this check, so bad actors can't brute force exclude_list
+                    // without user interaction.
+                    let _ = (self.check_user_presence)(cid);
+                    return Err(Ctap2StatusCode::CTAP2_ERR_CREDENTIAL_EXCLUDED);
+                }
+            }
+        }
+
+        (self.check_user_presence)(cid)?;
+        self.client_pin.clear_token_flags();
+
         let mut cred_protect_policy = extensions.cred_protect;
         if cred_protect_policy.unwrap_or(CredentialProtectionPolicy::UserVerificationOptional)
             < DEFAULT_CRED_PROTECT.unwrap_or(CredentialProtectionPolicy::UserVerificationOptional)
@@ -576,74 +638,17 @@ where
             None
         };
         let has_extension_output = extensions.hmac_secret
-            || cred_protect_policy.is_some()
+            || extensions.cred_protect.is_some()
             || min_pin_length
             || has_cred_blob_output;
+        if has_extension_output {
+            flags |= ED_FLAG
+        };
         let large_blob_key = match (options.rk, extensions.large_blob_key) {
             (true, Some(true)) => Some(self.rng.gen_uniform_u8x32().to_vec()),
             _ => None,
         };
 
-        let rp_id_hash = Sha256::hash(rp_id.as_bytes());
-        if let Some(exclude_list) = exclude_list {
-            for cred_desc in exclude_list {
-                if self
-                    .persistent_store
-                    .find_credential(&rp_id, &cred_desc.key_id, pin_uv_auth_param.is_none())?
-                    .is_some()
-                    || self
-                        .decrypt_credential_source(cred_desc.key_id, &rp_id_hash)?
-                        .is_some()
-                {
-                    // Perform this check, so bad actors can't brute force exclude_list
-                    // without user interaction.
-                    (self.check_user_presence)(cid)?;
-                    return Err(Ctap2StatusCode::CTAP2_ERR_CREDENTIAL_EXCLUDED);
-                }
-            }
-        }
-
-        // MakeCredential always requires user presence.
-        // User verification depends on the PIN auth inputs, which are checked here.
-        let ed_flag = if has_extension_output { ED_FLAG } else { 0 };
-        let flags = match pin_uv_auth_param {
-            Some(pin_uv_auth_param) => {
-                if self.persistent_store.pin_hash()?.is_none() {
-                    // Specification is unclear, could be CTAP2_ERR_INVALID_OPTION.
-                    return Err(Ctap2StatusCode::CTAP2_ERR_PIN_NOT_SET);
-                }
-                self.client_pin.verify_pin_uv_auth_token(
-                    &client_data_hash,
-                    &pin_uv_auth_param,
-                    pin_uv_auth_protocol.ok_or(Ctap2StatusCode::CTAP2_ERR_MISSING_PARAMETER)?,
-                )?;
-                self.client_pin
-                    .has_permission(PinPermission::MakeCredential)?;
-                self.client_pin.check_user_verified_flag()?;
-                // Checking for the correct permissions_rp_id is specified earlier.
-                // Error codes are identical though, so the implementation can be identical with
-                // GetAssertion.
-                self.client_pin.ensure_rp_id_permission(&rp_id)?;
-                UP_FLAG | UV_FLAG | AT_FLAG | ed_flag
-            }
-            None => {
-                if self.persistent_store.has_always_uv()? {
-                    return Err(Ctap2StatusCode::CTAP2_ERR_PUAT_REQUIRED);
-                }
-                // Corresponds to makeCredUvNotRqd set to true.
-                if options.rk && self.persistent_store.pin_hash()?.is_some() {
-                    return Err(Ctap2StatusCode::CTAP2_ERR_PUAT_REQUIRED);
-                }
-                if options.uv {
-                    return Err(Ctap2StatusCode::CTAP2_ERR_INVALID_OPTION);
-                }
-                UP_FLAG | AT_FLAG | ed_flag
-            }
-        };
-
-        (self.check_user_presence)(cid)?;
-        self.client_pin.clear_token_flags();
-
         let sk = crypto::ecdsa::SecKey::gensk(self.rng);
         let pk = sk.genpk();
 
@@ -699,9 +704,10 @@ where
             } else {
                 None
             };
+            let cred_protect_output = extensions.cred_protect.and(cred_protect_policy);
             let extensions_output = cbor_map_options! {
                 "hmac-secret" => hmac_secret_output,
-                "credProtect" => cred_protect_policy,
+                "credProtect" => cred_protect_output,
                 "minPinLength" => min_pin_length_output,
                 "credBlob" => cred_blob_output,
             };
@@ -904,8 +910,8 @@ where
         let has_uv = pin_uv_auth_param.is_some();
         let mut flags = match pin_uv_auth_param {
             Some(pin_uv_auth_param) => {
+                // This case is not mentioned in CTAP2.1, so we keep 2.0 logic.
                 if self.persistent_store.pin_hash()?.is_none() {
-                    // Specification is unclear, could be CTAP2_ERR_UNSUPPORTED_OPTION.
                     return Err(Ctap2StatusCode::CTAP2_ERR_PIN_NOT_SET);
                 }
                 self.client_pin.verify_pin_uv_auth_token(
@@ -923,12 +929,12 @@ where
                 UV_FLAG
             }
             None => {
-                if self.persistent_store.has_always_uv()? {
-                    return Err(Ctap2StatusCode::CTAP2_ERR_PUAT_REQUIRED);
-                }
                 if options.uv {
                     return Err(Ctap2StatusCode::CTAP2_ERR_INVALID_OPTION);
                 }
+                if options.up && self.persistent_store.has_always_uv()? {
+                    return Err(Ctap2StatusCode::CTAP2_ERR_PUAT_REQUIRED);
+                }
                 0x00
             }
         };
@@ -970,15 +976,14 @@ where
             (credential, stored_credentials)
         };
 
+        let credential = credential.ok_or(Ctap2StatusCode::CTAP2_ERR_NO_CREDENTIALS)?;
+
         // This check comes before CTAP2_ERR_NO_CREDENTIALS in CTAP 2.0.
-        // For CTAP 2.1, it was moved to a later protocol step.
         if options.up {
             (self.check_user_presence)(cid)?;
             self.client_pin.clear_token_flags();
         }
 
-        let credential = credential.ok_or(Ctap2StatusCode::CTAP2_ERR_NO_CREDENTIALS)?;
-
         self.increment_global_signature_counter()?;
 
         let assertion_input = AssertionInput {

src/ctap/response.rs

@@ -211,7 +211,7 @@ impl From<AuthenticatorGetInfoResponse> for cbor::Value {
 #[derive(Debug, PartialEq)]
 pub struct AuthenticatorClientPinResponse {
     pub key_agreement: Option<CoseKey>,
-    pub pin_token: Option<Vec<u8>>,
+    pub pin_uv_auth_token: Option<Vec<u8>>,
     pub retries: Option<u64>,
     pub power_cycle_state: Option<bool>,
     // - 0x05: uvRetries missing as we don't support internal UV.
@@ -221,14 +221,14 @@ impl From<AuthenticatorClientPinResponse> for cbor::Value {
     fn from(client_pin_response: AuthenticatorClientPinResponse) -> Self {
         let AuthenticatorClientPinResponse {
             key_agreement,
-            pin_token,
+            pin_uv_auth_token,
             retries,
             power_cycle_state,
         } = client_pin_response;
 
         cbor_map_options! {
             0x01 => key_agreement.map(cbor::Value::from),
-            0x02 => pin_token,
+            0x02 => pin_uv_auth_token,
             0x03 => retries,
             0x04 => power_cycle_state,
         }
@@ -495,7 +495,7 @@ mod test {
         let cose_key = CoseKey::from(pk);
         let client_pin_response = AuthenticatorClientPinResponse {
             key_agreement: Some(cose_key.clone()),
-            pin_token: Some(vec![70]),
+            pin_uv_auth_token: Some(vec![70]),
             retries: Some(8),
             power_cycle_state: Some(false),
         };