From 3d1d8279847ef0dd8407b0a74e6c963a2eef4ce0 Mon Sep 17 00:00:00 2001
From: Jean-Michel Picod <jmichel@google.com>
Date: Thu, 26 Nov 2020 16:29:14 +0100
Subject: [PATCH] Address PR comments

---
 src/ctap/ctap1.rs   | 28 +++++++++++++++++-----------
 src/ctap/storage.rs |  8 ++++----
 2 files changed, 21 insertions(+), 15 deletions(-)

diff --git a/src/ctap/ctap1.rs b/src/ctap/ctap1.rs
index 0fa4745..41df364 100644
--- a/src/ctap/ctap1.rs
+++ b/src/ctap/ctap1.rs
@@ -35,6 +35,8 @@ pub enum Ctap1StatusCode {
     SW_WRONG_LENGTH = 0x6700,
     SW_CLA_NOT_SUPPORTED = 0x6E00,
     SW_INS_NOT_SUPPORTED = 0x6D00,
+    SW_MEMERR = 0x6501,
+    SW_COMMAND_ABORTED = 0x6F00,
     SW_VENDOR_KEY_HANDLE_TOO_LONG = 0xF000,
 }
 
@@ -49,6 +51,8 @@ impl TryFrom<u16> for Ctap1StatusCode {
             0x6700 => Ok(Ctap1StatusCode::SW_WRONG_LENGTH),
             0x6E00 => Ok(Ctap1StatusCode::SW_CLA_NOT_SUPPORTED),
             0x6D00 => Ok(Ctap1StatusCode::SW_INS_NOT_SUPPORTED),
+            0x6501 => Ok(Ctap1StatusCode::SW_MEMERR),
+            0x6F00 => Ok(Ctap1StatusCode::SW_COMMAND_ABORTED),
             0xF000 => Ok(Ctap1StatusCode::SW_VENDOR_KEY_HANDLE_TOO_LONG),
             _ => Err(()),
         }
@@ -288,20 +292,22 @@ impl Ctap1Command {
         let pk = sk.genpk();
         let key_handle = ctap_state
             .encrypt_key_handle(sk, &application, None)
-            .map_err(|_| Ctap1StatusCode::SW_VENDOR_KEY_HANDLE_TOO_LONG)?;
+            .map_err(|_| Ctap1StatusCode::SW_COMMAND_ABORTED)?;
         if key_handle.len() > 0xFF {
             // This is just being defensive with unreachable code.
             return Err(Ctap1StatusCode::SW_VENDOR_KEY_HANDLE_TOO_LONG);
         }
 
-        let certificate = match ctap_state.persistent_store.attestation_certificate() {
-            Ok(Some(value)) => value,
-            _ => return Err(Ctap1StatusCode::SW_INS_NOT_SUPPORTED),
-        };
-        let private_key = match ctap_state.persistent_store.attestation_private_key() {
-            Ok(Some(value)) => value,
-            _ => return Err(Ctap1StatusCode::SW_INS_NOT_SUPPORTED),
-        };
+        let certificate = ctap_state
+            .persistent_store
+            .attestation_certificate()
+            .map_err(|_| Ctap1StatusCode::SW_MEMERR)?
+            .ok_or(Ctap1StatusCode::SW_COMMAND_ABORTED)?;
+        let private_key = ctap_state
+            .persistent_store
+            .attestation_private_key()
+            .map_err(|_| Ctap1StatusCode::SW_MEMERR)?
+            .ok_or(Ctap1StatusCode::SW_COMMAND_ABORTED)?;
 
         let mut response = Vec::with_capacity(105 + key_handle.len() + certificate.len());
         response.push(Ctap1Command::LEGACY_BYTE);
@@ -442,7 +448,7 @@ mod test {
         ctap_state.u2f_up_state.grant_up(START_CLOCK_VALUE);
         let response = Ctap1Command::process_command(&message, &mut ctap_state, START_CLOCK_VALUE);
         // Certificate and private key are missing
-        assert_eq!(response, Err(Ctap1StatusCode::SW_INS_NOT_SUPPORTED));
+        assert_eq!(response, Err(Ctap1StatusCode::SW_COMMAND_ABORTED));
 
         let fake_key = [0x41u8; key_material::ATTESTATION_PRIVATE_KEY_LENGTH];
         assert!(ctap_state
@@ -453,7 +459,7 @@ mod test {
         ctap_state.u2f_up_state.grant_up(START_CLOCK_VALUE);
         let response = Ctap1Command::process_command(&message, &mut ctap_state, START_CLOCK_VALUE);
         // Certificate is still missing
-        assert_eq!(response, Err(Ctap1StatusCode::SW_INS_NOT_SUPPORTED));
+        assert_eq!(response, Err(Ctap1StatusCode::SW_COMMAND_ABORTED));
 
         let fake_cert = [0x99u8; 100]; // Arbitrary length
         assert!(ctap_state
diff --git a/src/ctap/storage.rs b/src/ctap/storage.rs
index 8e396b6..5ec6511 100644
--- a/src/ctap/storage.rs
+++ b/src/ctap/storage.rs
@@ -230,8 +230,8 @@ impl PersistentStore {
                 .unwrap();
         }
         // TODO(jmichel): remove this when vendor command is in place
-        #[cfg(not(any(test, feature = "ram_storage")))]
-        self.load_attestation_from_firmware();
+        #[cfg(not(test))]
+        self.load_attestation_data_from_firmware();
 
         if self.store.find_one(&Key::Aaguid).is_none() {
             self.set_aaguid(key_material::AAGUID).unwrap();
@@ -239,8 +239,8 @@ impl PersistentStore {
     }
 
     // TODO(jmichel): remove this function when vendor command is in place.
-    #[cfg(not(any(test, feature = "ram_storage")))]
-    fn load_attestation_from_firmware(&mut self) {
+    #[cfg(not(test))]
+    fn load_attestation_data_from_firmware(&mut self) {
         // The following 2 entries are meant to be written by vendor-specific commands.
         if self.store.find_one(&Key::AttestationPrivateKey).is_none() {
             self.set_attestation_private_key(key_material::ATTESTATION_PRIVATE_KEY)