From 07ff03b4edb0c6ba3563917d13308ee8d4518843 Mon Sep 17 00:00:00 2001 From: Robin Krahl Date: Thu, 23 May 2024 12:01:39 +0200 Subject: [PATCH] Accept scoped PIN tokens for EnumerateCredentialsBegin As described in #80, we currently require PIN tokens without an RP ID restriction for all credential management operations. For most operations, this is correct. For EnumerateCredentialsBegin, we should also accept a token that matches the requested RP ID hash. For DeleteCredential and UpdateUserInformation, we should also accept a token that matches the requested credential ID. As it is not trivial to compare the RP ID hash or the credential ID against the RP ID set for the PIN token, I did not handle these cases in the initial implementation. This led to an incompatibility with libfido2 because it tries to use a restricted PIN token to enumerate credentials. With this patch, we additionally compute the RP ID hash when restricting a PIN token to an RP ID and use that to validate the PIN token for EnumerateCredentialsBegin operations. For DeleteCredential and UpdateUserInformation, we still require tokens without an RP ID restriction because determining the RP ID from the credential ID is much harder and this is not known to cause incompatibility issues. See also: https://github.com/Nitrokey/fido-authenticator/issues/80 --- src/ctap2.rs | 50 +++++++++-------- src/ctap2/pin.rs | 46 ++++++++++++++-- tests/basic.rs | 138 ++++++++++++++++++++++++++++++++++++----------- 3 files changed, 177 insertions(+), 57 deletions(-) diff --git a/src/ctap2.rs b/src/ctap2.rs index 332f14b..42c25c4 100644 --- a/src/ctap2.rs +++ b/src/ctap2.rs @@ -35,7 +35,7 @@ pub mod credential_management; pub mod large_blobs; pub mod pin; -use pin::{PinProtocol, PinProtocolVersion, SharedSecret}; +use pin::{PinProtocol, PinProtocolVersion, RpScope, SharedSecret}; /// Implement `ctap2::Authenticator` for our Authenticator. impl Authenticator for crate::Authenticator { @@ -924,9 +924,7 @@ impl Authenticator for crate::Authenti use credential_management as cm; use ctap2::credential_management::Subcommand; - // TODO: I see "failed pinauth" output, but then still continuation... - // TODO: determine rp_id - self.verify_pin_auth_using_token(parameters, None)?; + self.verify_credential_management_pin_auth(parameters)?; let mut cred_mgmt = cm::CredentialManagement::new(self); let sub_parameters = ¶meters.sub_command_params; @@ -1315,32 +1313,40 @@ impl crate::Authenticator { Ok(pin) } - // fn verify_pin_auth_using_token(&mut self, data: &[u8], pin_auth: &Bytes<16>) - fn verify_pin_auth_using_token( + fn verify_credential_management_pin_auth( &mut self, parameters: &ctap2::credential_management::Request, - rp_id: Option<&str>, ) -> Result<()> { - // info_now!("CM params: {:?}", parameters); use ctap2::credential_management::Subcommand; + let rp_scope = match parameters.sub_command { + Subcommand::EnumerateCredentialsBegin => { + let rp_id_hash = parameters + .sub_command_params + .as_ref() + .and_then(|subparams| subparams.rp_id_hash.as_deref()) + .ok_or(Error::MissingParameter)?; + RpScope::RpIdHash(rp_id_hash) + } + Subcommand::DeleteCredential | Subcommand::UpdateUserInformation => { + // TODO: determine RP ID from credential ID + RpScope::All + } + _ => RpScope::All, + }; match parameters.sub_command { - // are we Haskell yet lol - sub_command @ Subcommand::GetCredsMetadata - | sub_command @ Subcommand::EnumerateRpsBegin - | sub_command @ Subcommand::EnumerateCredentialsBegin - | sub_command @ Subcommand::DeleteCredential - | sub_command @ Subcommand::UpdateUserInformation => { + Subcommand::GetCredsMetadata + | Subcommand::EnumerateRpsBegin + | Subcommand::EnumerateCredentialsBegin + | Subcommand::DeleteCredential + | Subcommand::UpdateUserInformation => { // check pinProtocol - let pin_protocol = parameters - // .sub_command_params.as_ref().ok_or(Error::MissingParameter)? - .pin_protocol - .ok_or(Error::MissingParameter)?; + let pin_protocol = parameters.pin_protocol.ok_or(Error::MissingParameter)?; let pin_protocol = self.parse_pin_protocol(pin_protocol)?; // check pinAuth let mut data: Bytes<{ sizes::MAX_CREDENTIAL_ID_LENGTH_PLUS_256 }> = - Bytes::from_slice(&[sub_command as u8]).unwrap(); - let len = 1 + match sub_command { + Bytes::from_slice(&[parameters.sub_command as u8]).unwrap(); + let len = 1 + match parameters.sub_command { Subcommand::EnumerateCredentialsBegin | Subcommand::DeleteCredential | Subcommand::UpdateUserInformation => { @@ -1368,7 +1374,7 @@ impl crate::Authenticator { if let Ok(pin_token) = pin_protocol.verify_pin_token(&data[..len], pin_auth) { info_now!("passed pinauth"); pin_token.require_permissions(Permissions::CREDENTIAL_MANAGEMENT)?; - pin_token.require_valid_for_rp_id(rp_id)?; + pin_token.require_valid_for_rp(rp_scope)?; Ok(()) } else { info_now!("failed pinauth!"); @@ -1456,7 +1462,7 @@ impl crate::Authenticator { let mut pin_protocol = self.pin_protocol(pin_protocol); let pin_token = pin_protocol.verify_pin_token(data, pin_auth)?; pin_token.require_permissions(permissions)?; - pin_token.require_valid_for_rp_id(Some(rp_id))?; + pin_token.require_valid_for_rp(RpScope::RpId(rp_id))?; return Ok(true); } else { diff --git a/src/ctap2/pin.rs b/src/ctap2/pin.rs index 4156d0f..bde2417 100644 --- a/src/ctap2/pin.rs +++ b/src/ctap2/pin.rs @@ -29,6 +29,13 @@ impl From for u8 { } } +#[derive(Debug)] +pub enum RpScope<'a> { + All, + RpId(&'a str), + RpIdHash(&'a [u8]), +} + #[derive(Debug)] pub struct PinToken { key_id: KeyId, @@ -61,8 +68,22 @@ impl PinToken { } } - pub fn require_valid_for_rp_id(&self, rp_id: Option<&str>) -> Result<()> { - if self.state.rp_id.is_none() || self.state.rp_id.as_deref() == rp_id { + fn is_valid_for_rp(&self, scope: RpScope<'_>) -> bool { + if let Some(rp) = self.state.rp.as_ref() { + // if an RP id is set, the token is only valid for that scope + match scope { + RpScope::All => false, + RpScope::RpId(rp_id) => rp.id == rp_id, + RpScope::RpIdHash(hash) => rp.hash == hash, + } + } else { + // if no RP ID is set, the token is valid for all scopes + true + } + } + + pub fn require_valid_for_rp(&self, scope: RpScope<'_>) -> Result<()> { + if self.is_valid_for_rp(scope) { Ok(()) } else { Err(Error::PinAuthInvalid) @@ -79,7 +100,7 @@ pub struct PinTokenMut<'a, T: CryptoClient> { impl PinTokenMut<'_, T> { pub fn restrict(&mut self, permissions: Permissions, rp_id: Option>) { self.pin_token.state.permissions = permissions; - self.pin_token.state.rp_id = rp_id; + self.pin_token.state.rp = rp_id.map(|id| Rp::new(self.trussed, id)); } // in spec: encrypt(..., pinUvAuthToken) @@ -89,10 +110,27 @@ impl PinTokenMut<'_, T> { } } +#[derive(Debug)] +struct Rp { + id: String<256>, + hash: Bytes<32>, +} + +impl Rp { + fn new(trussed: &mut T, id: String<256>) -> Self { + let hash = + syscall!(trussed.hash(Mechanism::Sha256, Message::from_slice(id.as_ref()).unwrap())) + .hash + .to_bytes() + .unwrap(); + Self { id, hash } + } +} + #[derive(Debug, Default)] struct PinTokenState { permissions: Permissions, - rp_id: Option>, + rp: Option, is_user_present: bool, is_user_verified: bool, is_in_use: bool, diff --git a/tests/basic.rs b/tests/basic.rs index 322822b..66cea57 100644 --- a/tests/basic.rs +++ b/tests/basic.rs @@ -98,34 +98,116 @@ fn test_get_pin_token() { }) } -#[test] -fn test_make_credential() { - virt::run_ctap2(|device| { - let rp = Rp::new("example.com"); - let user = User::new(b"id123") - .name("john.doe") - .display_name("John Doe"); - let pub_key_cred_params = vec![PubKeyCredParam::new("public-key", -7)]; - let request = MakeCredential::new(b"", rp, user, pub_key_cred_params); - let reply = device.exec(request).unwrap(); - assert_eq!(reply.fmt, "packed"); - assert!(reply.auth_data.is_bytes()); - assert!(reply.att_stmt.is_map()); - }); +#[derive(Clone, Debug)] +struct RequestPinToken { + permissions: u8, + rp_id: Option, +} + +#[derive(Debug)] +struct TestMakeCredential { + pin_token: Option, + pub_key_alg: i32, +} + +impl TestMakeCredential { + fn run(&self) { + let key_agreement_key = KeyAgreementKey::generate(); + let pin = b"123456"; + let rp_id = "example.com"; + // TODO: client data + let client_data_hash = b""; + + virt::run_ctap2(|device| { + let pin_auth = self.pin_token.as_ref().map(|pin_token| { + let shared_secret = get_shared_secret(&device, &key_agreement_key); + set_pin(&device, &key_agreement_key, &shared_secret, pin); + let pin_token = get_pin_token( + &device, + &key_agreement_key, + &shared_secret, + pin, + pin_token.permissions, + pin_token.rp_id.clone(), + ); + pin_token.authenticate(client_data_hash) + }); + + let rp = Rp::new(rp_id); + let user = User::new(b"id123") + .name("john.doe") + .display_name("John Doe"); + let pub_key_cred_params = vec![PubKeyCredParam::new("public-key", self.pub_key_alg)]; + let mut request = MakeCredential::new(client_data_hash, rp, user, pub_key_cred_params); + if let Some(pin_auth) = pin_auth { + request.pin_auth = Some(pin_auth); + request.pin_protocol = Some(2); + } + + let result = device.exec(request); + if let Some(error) = self.expected_error() { + assert_eq!(result, Err(Ctap2Error(error))); + } else { + let reply = result.unwrap(); + assert_eq!(reply.fmt, "packed"); + assert!(reply.auth_data.is_bytes()); + assert!(reply.att_stmt.is_map()); + } + }); + } + + fn expected_error(&self) -> Option { + if let Some(pin_token) = &self.pin_token { + if pin_token.permissions != 0x01 { + return Some(0x33); + } + if let Some(rp_id) = &pin_token.rp_id { + if rp_id != "example.com" { + return Some(0x33); + } + } + } + if self.pub_key_alg != -7 { + return Some(0x26); + } + None + } } #[test] -fn test_make_credential_invalid_params() { - virt::run_ctap2(|device| { - let rp = Rp::new("example.com"); - let user = User::new(b"id123") - .name("john.doe") - .display_name("John Doe"); - let pub_key_cred_params = vec![PubKeyCredParam::new("public-key", -11)]; - let request = MakeCredential::new(b"", rp, user, pub_key_cred_params); - let result = device.exec(request); - assert_eq!(result, Err(Ctap2Error(0x26))); - }); +fn test_make_credential() { + let pin_tokens = [ + None, + Some(RequestPinToken { + permissions: 0x01, + rp_id: None, + }), + Some(RequestPinToken { + permissions: 0x01, + rp_id: Some("example.com".to_owned()), + }), + Some(RequestPinToken { + permissions: 0x01, + rp_id: Some("test.com".to_owned()), + }), + Some(RequestPinToken { + permissions: 0x04, + rp_id: None, + }), + ]; + for pin_token in pin_tokens { + for pub_key_alg in [-7, -11] { + let test = TestMakeCredential { + pin_token: pin_token.clone(), + pub_key_alg, + }; + println!("{}", "=".repeat(80)); + println!("Running test:"); + println!("{test:#?}"); + println!(); + test.run(); + } + } } #[derive(Debug)] @@ -220,12 +302,6 @@ impl TestListCredentials { #[test] fn test_list_credentials() { for pin_token_rp_id in [false, true] { - // true is omitted because it currently fails, see: - // https://github.com/Nitrokey/fido-authenticator/issues/80 - if pin_token_rp_id { - continue; - } - let test = TestListCredentials { pin_token_rp_id }; println!("{}", "=".repeat(80)); println!("Running test:");