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: #80

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<UP: UserPresence, T: TrussedRequirements> Authenticator for crate::Authenticator<UP, T> {
@@ -924,9 +924,7 @@ impl<UP: UserPresence, T: TrussedRequirements> 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 = &parameters.sub_command_params;
@@ -1315,32 +1313,40 @@ impl<UP: UserPresence, T: TrussedRequirements> crate::Authenticator<UP, T> {
         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<UP: UserPresence, T: TrussedRequirements> crate::Authenticator<UP, T> {
                 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<UP: UserPresence, T: TrussedRequirements> crate::Authenticator<UP, T> {
                     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 {

src/ctap2/pin.rs

@@ -29,6 +29,13 @@ impl From<PinProtocolVersion> 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<T: CryptoClient> PinTokenMut<'_, T> {
     pub fn restrict(&mut self, permissions: Permissions, rp_id: Option<String<256>>) {
         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<T: CryptoClient> PinTokenMut<'_, T> {
     }
 }
 
+#[derive(Debug)]
+struct Rp {
+    id: String<256>,
+    hash: Bytes<32>,
+}
+
+impl Rp {
+    fn new<T: CryptoClient>(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<String<256>>,
+    rp: Option<Rp>,
     is_user_present: bool,
     is_user_verified: bool,
     is_in_use: bool,

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<String>,
+}
+
+#[derive(Debug)]
+struct TestMakeCredential {
+    pin_token: Option<RequestPinToken>,
+    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<u8> {
+        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:");