Preserve AAGUID in AttestationObject::anonymize

.github/workflows/ci.yml

@@ -42,13 +42,9 @@ jobs:
           - OS: ubuntu-latest
             TARGET: x86_64-unknown-linux-gnu
             NATIVE_BUILD: true
-            # XXX: This version downgrade is required because 1.8.x has too high of an MSRV
-            # and the lockfile isn't checked in for tests to use, so the latest dependency version
-            # is always used instead.
             ADD_INSTALL: |
               sudo apt-get update
               sudo apt-get install -y libudev-dev
-              cargo update -p base64ct --precise 1.6.0
             BUILD_OPTIONS: --features crypto_rust --no-default-features
 
           # Mac dummy crypto

Cargo.toml

@@ -81,7 +81,7 @@ env_logger = "^0.6"
 getopts = "^0.2"
 assert_matches = "1.2"
 rpassword = "5.0"
-flate3 = "1"
+flate3 = "~1.1"
 aes-gcm = "0.10"
 # Workaround for 'broken' generic-array 0.14.9, see ctap2_discoverable_creds.rs for details
 generic-array = { version = "1.3", features = ["compat-0_14"] }

rust-toolchain.toml

@@ -1,5 +1,5 @@
 [toolchain]
 # Current rust version for mozilla-central
 # https://firefox-source-docs.mozilla.org/writing-rust-code/update-policy.html
-channel = "1.82.0"
+channel = "1.90.0"
 components = ["rustfmt", "clippy", "rust-analyzer"]

src/crypto/mod.rs

@@ -1132,7 +1132,7 @@ pub struct U2FRegisterAnswer<'a> {
 }
 
 // We will only return MalformedInput here
-pub fn parse_u2f_der_certificate(data: &[u8]) -> Result<U2FRegisterAnswer, CryptoError> {
+pub fn parse_u2f_der_certificate(data: &[u8]) -> Result<U2FRegisterAnswer<'_>, CryptoError> {
     // So we don't panic below, when accessing individual bytes
     if data.len() < 4 {
         return Err(CryptoError::MalformedInput);

src/crypto/nss.rs

@@ -195,7 +195,7 @@ pub fn encrypt_aes_256_cbc_no_pad(key: &[u8], iv: Option<&[u8]>, data: &[u8]) ->
         _ => return Err(CryptoError::LibraryFailure),
     };
 
-    if data.len() % AES_BLOCK_SIZE != 0 {
+    if !data.len().is_multiple_of(AES_BLOCK_SIZE) {
         return Err(CryptoError::LibraryFailure);
     }
 
@@ -256,7 +256,7 @@ pub fn decrypt_aes_256_cbc_no_pad(key: &[u8], iv: Option<&[u8]>, data: &[u8]) ->
         _ => return Err(CryptoError::LibraryFailure),
     };
 
-    if data.len() % AES_BLOCK_SIZE != 0 {
+    if !data.len().is_multiple_of(AES_BLOCK_SIZE) {
         return Err(CryptoError::LibraryFailure);
     }
 

src/crypto/openssl.rs

@@ -77,7 +77,7 @@ pub fn encrypt_aes_256_cbc_no_pad(key: &[u8], iv: Option<&[u8]>, data: &[u8]) ->
     encrypter.pad(false);
 
     let in_len = data.len();
-    if in_len % AES_BLOCK_SIZE != 0 {
+    if !in_len.is_multiple_of(AES_BLOCK_SIZE) {
         return Err(CryptoError::LibraryFailure);
     }
 
@@ -101,7 +101,7 @@ pub fn decrypt_aes_256_cbc_no_pad(key: &[u8], iv: Option<&[u8]>, data: &[u8]) ->
     encrypter.pad(false);
 
     let in_len = data.len();
-    if in_len % AES_BLOCK_SIZE != 0 {
+    if !in_len.is_multiple_of(AES_BLOCK_SIZE) {
         return Err(CryptoError::LibraryFailure);
     }
 

src/ctap2/attestation.rs

@@ -569,12 +569,27 @@ pub struct AttestationObject {
 }
 
 impl AttestationObject {
+    // Apply the AttestationConveyancePreference "none" transform from
+    // https://w3c.github.io/webauthn/#sctn-createCredential. If the
+    // attestation is self attestation (zero AAGUID, "packed" format, no x5c)
+    // the spec says no action is needed. Otherwise the attestation statement
+    // is replaced with the empty "none" statement, but the
+    // attestedCredentialData (including the AAGUID, which identifies the
+    // make/model of the authenticator but no individual user/device) is left
+    // intact.
     pub fn anonymize(&mut self) {
-        // Remove the attestation statement and the AAGUID from the authenticator data.
-        self.att_stmt = AttestationStatement::None;
-        if let Some(credential_data) = self.auth_data.credential_data.as_mut() {
-            credential_data.aaguid = AAGuid::default();
+        if let AttestationStatement::Packed(ref packed) = self.att_stmt {
+            if packed.attestation_cert.is_empty()
+                && self
+                    .auth_data
+                    .credential_data
+                    .as_ref()
+                    .is_some_and(|d| d.aaguid == AAGuid::default())
+            {
+                return;
+            }
         }
+        self.att_stmt = AttestationStatement::None;
     }
 }
 
@@ -1057,30 +1072,71 @@ pub mod test {
     #[test]
     fn test_anonymize_att_obj() {
         // Anonymize should prevent identifying data in the attestation statement from being
-        // serialized.
+        // serialized, but should preserve the AAGUID per the WebAuthn spec's
+        // AttestationConveyancePreference "none" transform.
         let mut att_obj = create_attestation_obj();
 
         // This test assumes that the sample attestation object contains identifying information
         assert_ne!(att_obj.att_stmt, AttestationStatement::None);
-        assert_ne!(
+        let original_aaguid = att_obj
+            .auth_data
+            .credential_data
+            .as_ref()
+            .expect("credential_data should be Some")
+            .aaguid
+            .clone();
+        assert_ne!(original_aaguid, AAGuid::default());
+
+        att_obj.anonymize();
+
+        // Write the attestation object out to bytes and read it back. The result should not
+        // have an attestation statement, but the AAGUID should be preserved.
+        let encoded_att_obj = to_vec(&att_obj).expect("could not serialize anonymized att_obj");
+        let att_obj: AttestationObject =
+            from_slice(&encoded_att_obj).expect("could not deserialize anonymized att_obj");
+
+        assert_eq!(att_obj.att_stmt, AttestationStatement::None);
+        assert_eq!(
             att_obj
                 .auth_data
                 .credential_data
                 .as_ref()
                 .expect("credential_data should be Some")
                 .aaguid,
-            AAGuid::default()
+            original_aaguid
         );
+    }
 
-        att_obj.anonymize();
+    #[test]
+    fn test_anonymize_self_attestation_is_noop() {
+        // Per the WebAuthn spec, if the attestation is self attestation (zero
+        // AAGUID, "packed" format, no x5c) the "none" transform is a no-op.
+        let mut att_obj = create_attestation_obj();
 
-        // Write the attestation object out to bytes and read it back. The result should not
-        // have an attestation statement, and it should have the default AAGUID.
-        let encoded_att_obj = to_vec(&att_obj).expect("could not serialize anonymized att_obj");
-        let att_obj: AttestationObject =
-            from_slice(&encoded_att_obj).expect("could not deserialize anonymized att_obj");
+        // Convert to a self attestation: clear the AAGUID and drop x5c.
+        att_obj
+            .auth_data
+            .credential_data
+            .as_mut()
+            .expect("credential_data should be Some")
+            .aaguid = AAGuid::default();
+        let self_att = AttestationStatement::Packed(AttestationStatementPacked {
+            alg: COSEAlgorithm::ES256,
+            sig: Signature(vec![0x01, 0x02, 0x03, 0x04]),
+            attestation_cert: vec![],
+        });
+        att_obj.att_stmt = self_att;
 
-        assert_eq!(att_obj.att_stmt, AttestationStatement::None);
+        let expected_att_stmt = AttestationStatement::Packed(AttestationStatementPacked {
+            alg: COSEAlgorithm::ES256,
+            sig: Signature(vec![0x01, 0x02, 0x03, 0x04]),
+            attestation_cert: vec![],
+        });
+
+        att_obj.anonymize();
+
+        // The self attestation should be preserved unchanged.
+        assert_eq!(att_obj.att_stmt, expected_att_stmt);
         assert_eq!(
             att_obj
                 .auth_data

src/ctap2/commands/make_credentials.rs

@@ -398,7 +398,7 @@ impl MakeCredentials {
         //      Note: a CTAP 2.0 authenticator is allowed to create a discoverable credential even
         //      if one was not requested, so there is a case in which we cannot confidently
         //      return `rk=false` here. We omit the response entirely in this case.
-        let dev_supports_rk = maybe_info.map_or(false, |info| info.options.resident_key);
+        let dev_supports_rk = maybe_info.is_some_and(|info| info.options.resident_key);
         let requested_rk = self.options.resident_key.unwrap_or(false);
         let max_supported_version = maybe_info.map_or(AuthenticatorVersion::U2F_V2, |info| {
             info.max_supported_version()

src/ctap2/mod.rs

@@ -476,7 +476,7 @@ pub fn register<Dev: FidoDevice>(
     // that does not support this extension.)"
     let dev_supports_cred_protect = dev
         .get_authenticator_info()
-        .map_or(false, |info| info.supports_cred_protect());
+        .is_some_and(|info| info.supports_cred_protect());
     if args.extensions.enforce_credential_protection_policy == Some(true)
         && args.extensions.credential_protection_policy
             != Some(CredentialProtectionPolicy::UserVerificationOptional)

src/ctap2/utils.rs

@@ -32,8 +32,8 @@ pub fn read_be_u16<R: Read, E: de::Error>(data: &mut R) -> Result<u16, E> {
 }
 
 pub fn read_byte<R: Read, E: de::Error>(data: &mut R) -> Result<u8, E> {
-    match data.bytes().next() {
-        Some(Ok(s)) => Ok(s),
-        _ => Err(serde_parse_err("u8")),
-    }
+    let mut buf = [0; 1];
+    data.read_exact(&mut buf)
+        .map_err(|_| serde_parse_err("u8"))?;
+    Ok(buf[0])
 }

src/transport/freebsd/device.rs

@@ -169,7 +169,7 @@ impl HIDDevice for Device {
     }
 
     fn get_property(&self, _prop_name: &str) -> io::Result<String> {
-        Err(io::Error::new(io::ErrorKind::Other, "Not implemented"))
+        Err(io::Error::other("Not implemented"))
     }
 
     fn get_device_info(&self) -> U2FDeviceInfo {

src/transport/linux/monitor.rs

@@ -187,8 +187,7 @@ pub fn get_property_linux(path: &PathBuf, prop_name: &str) -> io::Result<String>
         }
     }
 
-    Err(io::Error::new(
-        io::ErrorKind::Other,
+    Err(io::Error::other(
         "Unable to find device",
     ))
 }

src/transport/mod.rs

@@ -187,7 +187,7 @@ where
 
     fn block_and_blink(&mut self, keep_alive: &dyn Fn() -> bool) -> BlinkResult {
         let supports_select_cmd = self.get_protocol() == FidoProtocol::CTAP2
-            && self.get_authenticator_info().map_or(false, |i| {
+            && self.get_authenticator_info().is_some_and(|i| {
                 i.versions.contains(&AuthenticatorVersion::FIDO_2_1)
             });
         let resp = if supports_select_cmd {

src/transport/netbsd/device.rs

@@ -171,7 +171,7 @@ impl HIDDevice for Device {
     }
 
     fn get_property(&self, _prop_name: &str) -> io::Result<String> {
-        Err(io::Error::new(io::ErrorKind::Other, "Not implemented"))
+        Err(io::Error::other("Not implemented"))
     }
 
     fn get_device_info(&self) -> U2FDeviceInfo {

src/transport/openbsd/device.rs

@@ -152,7 +152,7 @@ impl HIDDevice for Device {
     }
 
     fn get_property(&self, _prop_name: &str) -> io::Result<String> {
-        Err(io::Error::new(io::ErrorKind::Other, "Not implemented"))
+        Err(io::Error::other("Not implemented"))
     }
 
     fn get_device_info(&self) -> U2FDeviceInfo {

src/transport/windows/device.rs

@@ -110,7 +110,7 @@ impl HIDDevice for Device {
     }
 
     fn get_property(&self, _prop_name: &str) -> io::Result<String> {
-        Err(io::Error::new(io::ErrorKind::Other, "Not implemented"))
+        Err(io::Error::other("Not implemented"))
     }
 
     fn get_device_info(&self) -> U2FDeviceInfo {

src/transport/windows/winapi.rs

@@ -64,7 +64,7 @@ extern "system" {
 }
 
 fn from_wide_ptr(ptr: *const u16, len: usize) -> String {
-    assert!(!ptr.is_null() && len % 2 == 0);
+    assert!(!ptr.is_null() && len.is_multiple_of(2));
     let slice = unsafe { slice::from_raw_parts(ptr, len / 2) };
     OsString::from_wide(slice).to_string_lossy().into_owned()
 }
@@ -95,7 +95,7 @@ impl DeviceInfoSet {
         self.set
     }
 
-    pub fn devices(&self) -> DeviceInfoSetIter {
+    pub fn devices(&self) -> DeviceInfoSetIter<'_> {
         DeviceInfoSetIter::new(self)
     }
 }

src/util.rs

@@ -67,7 +67,7 @@ pub fn from_unix_result<T: Signed>(rv: T) -> io::Result<T> {
 }
 
 pub fn io_err(msg: &str) -> io::Error {
-    io::Error::new(io::ErrorKind::Other, msg)
+    io::Error::other(msg)
 }
 
 #[cfg(test)]