From 148cb7c5468fbb94f5666240d1601dc803c06288 Mon Sep 17 00:00:00 2001 From: Szczepan Zalega Date: Wed, 18 Jan 2023 19:00:21 +0100 Subject: [PATCH 1/9] Use smaller buffers for the EncryptedDataContainer crypto details --- src/encrypted_container.rs | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/src/encrypted_container.rs b/src/encrypted_container.rs index 2a5a14e65..b630d8ae3 100644 --- a/src/encrypted_container.rs +++ b/src/encrypted_container.rs @@ -1,3 +1,4 @@ +use heapless_bytes::Bytes; use serde::{Deserialize, Serialize}; use trussed::types::{KeyId, Message}; use trussed::{cbor_deserialize, cbor_serialize, try_syscall}; @@ -88,14 +89,18 @@ impl From for trussed::error::Error { /// - Investigate postcard structure extensibility, as a means for smaller overhead for serialization #[derive(Debug, serde::Deserialize, serde::Serialize)] pub struct EncryptedDataContainer { + /// The ciphertext. 1024 bytes maximum. Reusing trussed::types::Message. #[serde(rename = "D")] data: trussed::types::Message, #[serde(rename = "T")] - tag: trussed::types::ShortData, + tag: ContainerTag, #[serde(rename = "N")] - nonce: trussed::types::ShortData, + nonce: ContainerNonce, } +type ContainerTag = Bytes<16>; +type ContainerNonce = Bytes<12>; + impl TryFrom<&[u8]> for EncryptedDataContainer { type Error = Error; @@ -189,8 +194,8 @@ impl EncryptedDataContainer { let encrypted_serialized_credential = EncryptedDataContainer { data: ciphertext, - nonce: encryption_results.nonce, - tag: encryption_results.tag, + nonce: encryption_results.nonce.try_convert_into().unwrap(), // should always be 12 bytes + tag: encryption_results.tag.try_convert_into().unwrap(), // should always be 16 bytes }; Ok(encrypted_serialized_credential) } From c26067b7a6b6084d7d3f814473868ed09fab5099 Mon Sep 17 00:00:00 2001 From: Szczepan Zalega Date: Fri, 20 Jan 2023 15:16:27 +0100 Subject: [PATCH 2/9] Use read-only access to the persistent storage where possible Load tests counters: 425 RW before, 421 RO / 4 RW now --- src/authenticator.rs | 10 ++--- src/state.rs | 105 ++++++++++++++++++++++++------------------- 2 files changed, 64 insertions(+), 51 deletions(-) diff --git a/src/authenticator.rs b/src/authenticator.rs index 6274ca84e..55a761456 100644 --- a/src/authenticator.rs +++ b/src/authenticator.rs @@ -150,7 +150,7 @@ where ) -> Result { let no_authorization_needed = self .state - .persistent(&mut self.trussed, |_, state| !state.password_set()); + .persistent_read_only(&mut self.trussed, |_, state| !state.password_set()); // TODO: abstract out this idea to make it usable for all the PIV security indicators @@ -236,7 +236,7 @@ where let state = self .state - .persistent(&mut self.trussed, |_, state| state.clone()); + .persistent_read_only(&mut self.trussed, |_, state| state.clone()); let answer_to_select = AnswerToSelect::new(state.salt); let data: heapless::Vec = if state.password_set() { @@ -576,7 +576,7 @@ where if let Some(key) = self .state - .persistent(&mut self.trussed, |_, state| state.authorization_key) + .persistent_read_only(&mut self.trussed, |_, state| state.authorization_key) { debug_now!("key set: {:?}", key); @@ -638,7 +638,7 @@ where return Err(Status::ConditionsOfUseNotSatisfied); } debug_now!("clearing password/key"); - if let Some(key) = self.state.persistent(&mut self.trussed, |_, state| { + if let Some(key) = self.state.persistent_read_write(&mut self.trussed, |_, state| { let existing_key = state.authorization_key; state.authorization_key = None; existing_key @@ -744,7 +744,7 @@ where .key; debug_now!("storing password/key"); - self.state.persistent(&mut self.trussed, |_, state| { + self.state.persistent_read_write(&mut self.trussed, |_, state| { state.authorization_key = Some(key) }); diff --git a/src/state.rs b/src/state.rs index d274199f4..824c5d9af 100644 --- a/src/state.rs +++ b/src/state.rs @@ -21,6 +21,8 @@ pub struct State { // temporary "state", to be removed again // pub hack: Hack, // trussed: RefCell>, + counter_read_write: u32, + counter_read_only: u32, } #[derive(Clone, Debug, Eq, PartialEq, serde::Deserialize, serde::Serialize)] @@ -109,9 +111,18 @@ impl State { where T: trussed::Client + trussed::client::Chacha8Poly1305, { - let encryption_key = self.persistent(trussed, |trussed, state| { - state.get_or_generate_encryption_key(trussed) - })?; + // Try to read it + let maybe_encryption_key = self.persistent_read_only(trussed, |_, state| { + state.encryption_key + }); + + // Generate encryption key + let encryption_key = match maybe_encryption_key { + Some(e) => e, + None => self.persistent_read_write(trussed, |trussed, state| { + state.get_or_generate_encryption_key(trussed) + })? + }; Ok(encryption_key) } @@ -156,27 +167,7 @@ impl State { where T: trussed::Client + trussed::client::Chacha8Poly1305, { - // 1. If there is serialized, persistent state (i.e., the try_syscall! to `read_file` does - // not fail), then assume it is valid and deserialize it. If the reading fails, assume - // that this is the first run, and set defaults. - // - // NB: This is an attack vector. If the state can be corrupted, this clears the password. - // Consider resetting the device in this situation - let mut state: Persistent = - try_syscall!(trussed.read_file(Location::Internal, PathBuf::from(Self::FILENAME))) - .map(|response| postcard_deserialize(&response.data).unwrap()) - .unwrap_or_else(|_| { - let salt: [u8; 8] = syscall!(trussed.random_bytes(8)) - .bytes - .as_ref() - .try_into() - .unwrap(); - Persistent { - salt, - authorization_key: None, - encryption_key: None, - } - }); + let mut state: Persistent = Self::get_persistent_or_default(trussed); // 2. Let the app read or modify the state let result = f(trussed, &mut state); @@ -194,7 +185,7 @@ impl State { result } - pub fn persistent( + pub fn persistent_read_write( &mut self, trussed: &mut T, f: impl FnOnce(&mut T, &mut Persistent) -> X, @@ -202,28 +193,10 @@ impl State { where T: trussed::Client + trussed::client::Chacha8Poly1305, { - // 1. If there is serialized, persistent state (i.e., the try_syscall! to `read_file` does - // not fail), then assume it is valid and deserialize it. If the reading fails, assume - // that this is the first run, and set defaults. - // - // NB: This is an attack vector. If the state can be corrupted, this clears the password. - // Consider resetting the device in this situation - let mut state: Persistent = - try_syscall!(trussed.read_file(Location::Internal, PathBuf::from(Self::FILENAME))) - .map(|response| postcard_deserialize(&response.data).unwrap()) - .unwrap_or_else(|_| { - let salt: [u8; 8] = syscall!(trussed.random_bytes(8)) - .bytes - .as_ref() - .try_into() - .unwrap(); - Persistent { - salt, - authorization_key: None, - encryption_key: None, - } - }); + let mut state: Persistent = Self::get_persistent_or_default(trussed); + self.counter_read_write += 1; + debug_now!("Getting the state RW {}", self.counter_read_write); // 2. Let the app read or modify the state let x = f(trussed, &mut state); @@ -237,6 +210,46 @@ impl State { x } + + pub fn persistent_read_only( + &mut self, + trussed: &mut T, + f: impl FnOnce(&mut T, &Persistent) -> X, + ) -> X + where + T: trussed::Client + trussed::client::Chacha8Poly1305, + { + let state: Persistent = Self::get_persistent_or_default(trussed); + + self.counter_read_only += 1; + debug_now!("Getting the state RO {}", self.counter_read_only); + // 2. Let the app read the state + let x = f(trussed, &state); + x + } + + fn get_persistent_or_default(trussed: &mut impl trussed::Client) -> Persistent { + // 1. If there is serialized, persistent state (i.e., the try_syscall! to `read_file` does + // not fail), then assume it is valid and deserialize it. If the reading fails, assume + // that this is the first run, and set defaults. + // + // NB: This is an attack vector. If the state can be corrupted, this clears the password. + // Consider resetting the device in this situation + try_syscall!(trussed.read_file(Location::Internal, PathBuf::from(Self::FILENAME))) + .map(|response| postcard_deserialize(&response.data).unwrap()) + .unwrap_or_else(|_| { + let salt: [u8; 8] = syscall!(trussed.random_bytes(8)) + .bytes + .as_ref() + .try_into() + .unwrap(); + Persistent { + salt, + authorization_key: None, + encryption_key: None, + } + }) + } } #[derive(Clone, Debug, Eq, PartialEq)] From a8ea3d96dd7b990fb565b9c8b6cdc3b228efc76a Mon Sep 17 00:00:00 2001 From: Szczepan Zalega Date: Fri, 20 Jan 2023 15:26:29 +0100 Subject: [PATCH 3/9] Use state read/write counters only in devel --- src/state.rs | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/src/state.rs b/src/state.rs index 824c5d9af..9107e3d95 100644 --- a/src/state.rs +++ b/src/state.rs @@ -21,7 +21,9 @@ pub struct State { // temporary "state", to be removed again // pub hack: Hack, // trussed: RefCell>, + #[cfg(feature = "devel")] counter_read_write: u32, + #[cfg(feature = "devel")] counter_read_only: u32, } @@ -195,8 +197,10 @@ impl State { { let mut state: Persistent = Self::get_persistent_or_default(trussed); - self.counter_read_write += 1; - debug_now!("Getting the state RW {}", self.counter_read_write); + #[cfg(feature = "devel")]{ + self.counter_read_write += 1; + debug_now!("Getting the state RW {}", self.counter_read_write); + } // 2. Let the app read or modify the state let x = f(trussed, &mut state); @@ -221,8 +225,10 @@ impl State { { let state: Persistent = Self::get_persistent_or_default(trussed); - self.counter_read_only += 1; - debug_now!("Getting the state RO {}", self.counter_read_only); + #[cfg(feature = "devel")]{ + self.counter_read_only += 1; + debug_now!("Getting the state RO {}", self.counter_read_only); + } // 2. Let the app read the state let x = f(trussed, &state); x From de2c934bce9dac3471fb51c332e522689682502c Mon Sep 17 00:00:00 2001 From: Szczepan Zalega Date: Fri, 20 Jan 2023 15:54:35 +0100 Subject: [PATCH 4/9] Handle state writing error --- src/authenticator.rs | 18 ++++++++++-------- src/state.rs | 38 ++++++-------------------------------- 2 files changed, 16 insertions(+), 40 deletions(-) diff --git a/src/authenticator.rs b/src/authenticator.rs index 55a761456..c13259816 100644 --- a/src/authenticator.rs +++ b/src/authenticator.rs @@ -638,13 +638,14 @@ where return Err(Status::ConditionsOfUseNotSatisfied); } debug_now!("clearing password/key"); - if let Some(key) = self.state.persistent_read_write(&mut self.trussed, |_, state| { + if let Some(key) = self.state.try_persistent_read_write(&mut self.trussed, |_, state| { let existing_key = state.authorization_key; state.authorization_key = None; - existing_key - }) { - syscall!(self.trussed.delete(key)); - } + Ok(existing_key) + }).map_err(|_| Status::NotEnoughMemory)? + { + syscall!(self.trussed.delete(key)); + } Ok(()) } @@ -744,9 +745,10 @@ where .key; debug_now!("storing password/key"); - self.state.persistent_read_write(&mut self.trussed, |_, state| { - state.authorization_key = Some(key) - }); + self.state.try_persistent_read_write(&mut self.trussed, |_, state| { + state.authorization_key = Some(key); + Ok(()) + }).map_err(|_| Status::NotEnoughMemory)?; // pub struct SetPassword<'l> { // pub kind: oath::Kind, diff --git a/src/state.rs b/src/state.rs index 9107e3d95..14f471a38 100644 --- a/src/state.rs +++ b/src/state.rs @@ -121,7 +121,7 @@ impl State { // Generate encryption key let encryption_key = match maybe_encryption_key { Some(e) => e, - None => self.persistent_read_write(trussed, |trussed, state| { + None => self.try_persistent_read_write(trussed, |trussed, state| { state.get_or_generate_encryption_key(trussed) })? }; @@ -161,37 +161,11 @@ impl State { .map_err(|e| e.into()) } - pub fn try_persistent( + pub fn try_persistent_read_write( &mut self, trussed: &mut T, - f: impl FnOnce(&mut T, &mut Persistent) -> Result<(), Status>, - ) -> Result<(), Status> - where - T: trussed::Client + trussed::client::Chacha8Poly1305, - { - let mut state: Persistent = Self::get_persistent_or_default(trussed); - - // 2. Let the app read or modify the state - let result = f(trussed, &mut state); - - // 3. Always write it back - try_syscall!(trussed.write_file( - Location::Internal, - PathBuf::from(Self::FILENAME), - postcard_serialize_bytes(&state).unwrap(), - None, - )) - .map_err(|_| Status::NotEnoughMemory)?; - - // 4. Return whatever - result - } - - pub fn persistent_read_write( - &mut self, - trussed: &mut T, - f: impl FnOnce(&mut T, &mut Persistent) -> X, - ) -> X + f: impl FnOnce(&mut T, &mut Persistent) -> Result, + ) -> Result where T: trussed::Client + trussed::client::Chacha8Poly1305, { @@ -205,12 +179,12 @@ impl State { let x = f(trussed, &mut state); // 3. Always write it back - syscall!(trussed.write_file( + try_syscall!(trussed.write_file( Location::Internal, PathBuf::from(Self::FILENAME), postcard_serialize_bytes(&state).unwrap(), None, - )); + ))?; x } From 368fdd527fddc8e8f1ed75057828e93ec2a1487f Mon Sep 17 00:00:00 2001 From: Szczepan Zalega Date: Fri, 20 Jan 2023 15:57:04 +0100 Subject: [PATCH 5/9] Cargo fmt --- src/authenticator.rs | 29 +++++++++++++++++------------ src/encrypted_container.rs | 2 +- src/state.rs | 17 +++++++++-------- 3 files changed, 27 insertions(+), 21 deletions(-) diff --git a/src/authenticator.rs b/src/authenticator.rs index c13259816..ec5c56d59 100644 --- a/src/authenticator.rs +++ b/src/authenticator.rs @@ -638,14 +638,17 @@ where return Err(Status::ConditionsOfUseNotSatisfied); } debug_now!("clearing password/key"); - if let Some(key) = self.state.try_persistent_read_write(&mut self.trussed, |_, state| { - let existing_key = state.authorization_key; - state.authorization_key = None; - Ok(existing_key) - }).map_err(|_| Status::NotEnoughMemory)? - { - syscall!(self.trussed.delete(key)); - } + if let Some(key) = self + .state + .try_persistent_read_write(&mut self.trussed, |_, state| { + let existing_key = state.authorization_key; + state.authorization_key = None; + Ok(existing_key) + }) + .map_err(|_| Status::NotEnoughMemory)? + { + syscall!(self.trussed.delete(key)); + } Ok(()) } @@ -745,10 +748,12 @@ where .key; debug_now!("storing password/key"); - self.state.try_persistent_read_write(&mut self.trussed, |_, state| { - state.authorization_key = Some(key); - Ok(()) - }).map_err(|_| Status::NotEnoughMemory)?; + self.state + .try_persistent_read_write(&mut self.trussed, |_, state| { + state.authorization_key = Some(key); + Ok(()) + }) + .map_err(|_| Status::NotEnoughMemory)?; // pub struct SetPassword<'l> { // pub kind: oath::Kind, diff --git a/src/encrypted_container.rs b/src/encrypted_container.rs index b630d8ae3..9e55dbc4b 100644 --- a/src/encrypted_container.rs +++ b/src/encrypted_container.rs @@ -195,7 +195,7 @@ impl EncryptedDataContainer { let encrypted_serialized_credential = EncryptedDataContainer { data: ciphertext, nonce: encryption_results.nonce.try_convert_into().unwrap(), // should always be 12 bytes - tag: encryption_results.tag.try_convert_into().unwrap(), // should always be 16 bytes + tag: encryption_results.tag.try_convert_into().unwrap(), // should always be 16 bytes }; Ok(encrypted_serialized_credential) } diff --git a/src/state.rs b/src/state.rs index 14f471a38..724d64c74 100644 --- a/src/state.rs +++ b/src/state.rs @@ -114,16 +114,15 @@ impl State { T: trussed::Client + trussed::client::Chacha8Poly1305, { // Try to read it - let maybe_encryption_key = self.persistent_read_only(trussed, |_, state| { - state.encryption_key - }); + let maybe_encryption_key = + self.persistent_read_only(trussed, |_, state| state.encryption_key); // Generate encryption key let encryption_key = match maybe_encryption_key { Some(e) => e, None => self.try_persistent_read_write(trussed, |trussed, state| { state.get_or_generate_encryption_key(trussed) - })? + })?, }; Ok(encryption_key) } @@ -171,7 +170,8 @@ impl State { { let mut state: Persistent = Self::get_persistent_or_default(trussed); - #[cfg(feature = "devel")]{ + #[cfg(feature = "devel")] + { self.counter_read_write += 1; debug_now!("Getting the state RW {}", self.counter_read_write); } @@ -194,12 +194,13 @@ impl State { trussed: &mut T, f: impl FnOnce(&mut T, &Persistent) -> X, ) -> X - where - T: trussed::Client + trussed::client::Chacha8Poly1305, + where + T: trussed::Client + trussed::client::Chacha8Poly1305, { let state: Persistent = Self::get_persistent_or_default(trussed); - #[cfg(feature = "devel")]{ + #[cfg(feature = "devel")] + { self.counter_read_only += 1; debug_now!("Getting the state RO {}", self.counter_read_only); } From 45b2f9540d4495c931a4cd93e678b639dbd9abc7 Mon Sep 17 00:00:00 2001 From: Szczepan Zalega Date: Fri, 20 Jan 2023 16:00:28 +0100 Subject: [PATCH 6/9] Add some error handling Handle injected key writing error Show error on failed write --- src/authenticator.rs | 3 ++- src/state.rs | 6 +++++- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/src/authenticator.rs b/src/authenticator.rs index ec5c56d59..04e44671f 100644 --- a/src/authenticator.rs +++ b/src/authenticator.rs @@ -727,9 +727,10 @@ where } info_now!("injecting the key"); - let tmp_key = syscall!(self + let tmp_key = try_syscall!(self .trussed .unsafe_inject_shared_key(key, Location::Volatile,)) + .map_err(|_| Status::NotEnoughMemory)? .key; let verification = syscall!(self.trussed.sign_hmacsha1(tmp_key, challenge)).signature; diff --git a/src/state.rs b/src/state.rs index 724d64c74..50a4435a9 100644 --- a/src/state.rs +++ b/src/state.rs @@ -105,7 +105,10 @@ impl State { .map_err(|_| Status::UnspecifiedPersistentExecutionError)?; debug_now!("Container size: {}", data_serialized.len()); try_syscall!(trussed.write_file(Location::Internal, filename, data_serialized, None)) - .map_err(|_| iso7816::Status::NotEnoughMemory)?; + .map_err(|_| { + debug_now!("Failed to write the file"); + iso7816::Status::NotEnoughMemory + })?; Ok(()) } @@ -219,6 +222,7 @@ impl State { try_syscall!(trussed.read_file(Location::Internal, PathBuf::from(Self::FILENAME))) .map(|response| postcard_deserialize(&response.data).unwrap()) .unwrap_or_else(|_| { + // TODO check if this can fail let salt: [u8; 8] = syscall!(trussed.random_bytes(8)) .bytes .as_ref() From 2b72d0fb9b6beeb040d58974f2ff0cb727f0adc4 Mon Sep 17 00:00:00 2001 From: Szczepan Zalega Date: Fri, 20 Jan 2023 17:21:07 +0100 Subject: [PATCH 7/9] Handle multipacket responses Abort if the previous state is not set. Try to clean up after Credential's write failure. Using 0x61FF, as we do not know the count of the remaining bytes. --- src/authenticator.rs | 79 +++++++++++++++++++++++++++++++++++--------- src/command.rs | 3 ++ src/ctaphid.rs | 17 +++++++--- 3 files changed, 78 insertions(+), 21 deletions(-) diff --git a/src/authenticator.rs b/src/authenticator.rs index 04e44671f..6009ff9aa 100644 --- a/src/authenticator.rs +++ b/src/authenticator.rs @@ -219,6 +219,7 @@ where Command::SetPassword(set_password) => self.set_password(set_password), Command::ClearPassword => self.clear_password(), Command::VerifyCode(verify_code) => self.verify_code(verify_code, reply), + Command::SendRemaining => self.send_remaining(reply), _ => Err(Status::ConditionsOfUseNotSatisfied), } } @@ -328,6 +329,7 @@ where /// The YK5 can store a Grande Totale of 32 OATH credentials. pub fn list_credentials(&mut self, reply: &mut Data) -> Result { + // TODO check if this one conflicts with send remaining if !self.state.runtime.client_authorized { return Err(Status::ConditionsOfUseNotSatisfied); } @@ -339,18 +341,39 @@ where // 79 75 62 69 63 6F // 90 00 - let maybe_credential_enc = syscall!(self.trussed.read_dir_files_first( - Location::Internal, - Self::credential_directory(), - None - )) - .data; - let mut maybe_credential: Option = match maybe_credential_enc { - None => None, - Some(c) => self.state.decrypt_content(&mut self.trussed, c).ok(), + let mut file_index = if let Some(CommandState::ListCredentials(s_file_index)) = + self.state.runtime.previously + { + s_file_index + } else { + 0 + }; + + let mut maybe_credential = match &self.state.runtime.previously { + Some(s) => { + info_now!("found continuation state: {:?}", s); + let maybe_credential: Option = + match syscall!(self.trussed.read_dir_files_next()).data { + None => None, + Some(c) => self.state.decrypt_content(&mut self.trussed, c).ok(), + }; + maybe_credential + } + None => { + let maybe_credential_enc = syscall!(self.trussed.read_dir_files_first( + Location::Internal, + Self::credential_directory(), + None + )) + .data; + let maybe_credential: Option = match maybe_credential_enc { + None => None, + Some(c) => self.state.decrypt_content(&mut self.trussed, c).ok(), + }; + maybe_credential + } }; - let mut file_index = 0; while let Some(credential) = maybe_credential { // keep track, in case we need continuation file_index += 1; @@ -364,15 +387,19 @@ where .unwrap(); reply.extend_from_slice(&credential.label).unwrap(); + // TODO check output buffer load and react accordingly + // instead of the raw counter - establish maximum label length or introduce additional + // buffer to keep data that had not fit into the reply, to be retrieved later + if file_index % 2 == 0 { + // split response + return Err(Status::MoreAvailable(0xFF)); + } + // check if there's more maybe_credential = match syscall!(self.trussed.read_dir_files_next()).data { None => None, Some(c) => self.state.decrypt_content(&mut self.trussed, c).ok(), }; - - if file_index % 8 == 0 { - // TODO: split response - } } // ran to completion @@ -381,6 +408,13 @@ where Ok(()) } + fn send_remaining(&mut self, reply: &mut Data<{ R }>) -> Result { + if self.state.runtime.previously.is_none() { + return Err(Status::ConditionsOfUseNotSatisfied); + } + self.list_credentials(reply) + } + pub fn register(&mut self, register: command::Register<'_>) -> Result { self.user_present()?; @@ -414,8 +448,21 @@ where let filename = self.filename_for_label(&credential.label); // 4. Serialize the credential (implicitly) and store it - self.state - .try_write_file(&mut self.trussed, filename, &credential)?; + let write_res = self + .state + .try_write_file(&mut self.trussed, filename, &credential); + + if write_res.is_err() { + // TODO reuse delete() call + // 1. Try to delete the key from Trussed, ignore errors + try_syscall!(self.trussed.delete(credential.secret)).ok(); + // 2. Try to delete the empty file, ignore errors + let filename = self.filename_for_label(&credential.label); + try_syscall!(self.trussed.remove_file(Location::Internal, filename)).ok(); + // 3. Return the original error + write_res? + } + Ok(()) } diff --git a/src/command.rs b/src/command.rs index 79d3ae43b..3ca16f3c1 100644 --- a/src/command.rs +++ b/src/command.rs @@ -30,6 +30,8 @@ pub enum Command<'l> { Validate(Validate<'l>), /// Reverse HOTP validation VerifyCode(VerifyCode<'l>), + /// Send remaining data in the buffer + SendRemaining, } /// TODO: change into enum @@ -491,6 +493,7 @@ impl<'l, const C: usize> TryFrom<&'l iso7816::Command> for Command<'l> { (0x00, oath::Instruction::VerifyCode, 0x00, 0x00) => { Self::VerifyCode(VerifyCode::try_from(data)?) } + (0x00, oath::Instruction::SendRemaining, 0x00, 0x00) => Self::SendRemaining, _ => return Err(Status::InstructionNotSupportedOrInvalid), }) } diff --git a/src/ctaphid.rs b/src/ctaphid.rs index c9587a44f..8fd00f87f 100644 --- a/src/ctaphid.rs +++ b/src/ctaphid.rs @@ -43,15 +43,22 @@ where debug_now!("ISO conversion error: {:?}", _e); app::Error::InvalidLength })?; - self.respond(&ctap_to_iso7816_command, response) - .map_err(|e| { + let res = self.respond(&ctap_to_iso7816_command, response); + + match res { + Ok(_) => return Ok(()), + Err(Status::MoreAvailable(b)) => { + response[0] = 0x61; + response[1] = b; + return Ok(()); + } + Err(e) => { debug_now!("OTP command execution error: {:?}", e); let arr: [u8; 2] = e.into(); response.clear(); response.extend(arr); - app::Error::InvalidCommand - }) - .ok(); + } + } } _ => { return Err(app::Error::InvalidCommand); From b305cc9cdeb668bde81dd70e631f233c0129ed0b Mon Sep 17 00:00:00 2001 From: Szczepan Zalega Date: Mon, 23 Jan 2023 19:00:46 +0100 Subject: [PATCH 8/9] Generalize response length over the provided reply buffer Fill the buffer as long as it is possible, break otherwise. Instead of using a temporary buffer for the unfit data, reiterate over directories again - this saves memory at the cost of the additional computation. --- src/authenticator.rs | 102 +++++++++++++++++++++++++------------------ 1 file changed, 60 insertions(+), 42 deletions(-) diff --git a/src/authenticator.rs b/src/authenticator.rs index 6009ff9aa..78584c165 100644 --- a/src/authenticator.rs +++ b/src/authenticator.rs @@ -327,6 +327,21 @@ where Ok(Default::default()) } + fn try_to_serialize_credential_for_list( + credential: &Credential, + reply: &mut Data, + ) -> core::result::Result<(), u8> { + reply.push(0x72)?; + reply.push((credential.label.len() + 1) as u8)?; + reply.push(oath::combine(credential.kind, credential.algorithm))?; + reply.extend_from_slice(&credential.label).map_err(|_| 0)?; + #[cfg(feature = "devel")] + if reply.len() > 3072 { + return Err(1); + } + Ok(()) + } + /// The YK5 can store a Grande Totale of 32 OATH credentials. pub fn list_credentials(&mut self, reply: &mut Data) -> Result { // TODO check if this one conflicts with send remaining @@ -349,52 +364,55 @@ where 0 }; - let mut maybe_credential = match &self.state.runtime.previously { - Some(s) => { - info_now!("found continuation state: {:?}", s); - let maybe_credential: Option = - match syscall!(self.trussed.read_dir_files_next()).data { - None => None, - Some(c) => self.state.decrypt_content(&mut self.trussed, c).ok(), - }; - maybe_credential - } - None => { - let maybe_credential_enc = syscall!(self.trussed.read_dir_files_first( - Location::Internal, - Self::credential_directory(), - None - )) - .data; - let maybe_credential: Option = match maybe_credential_enc { - None => None, - Some(c) => self.state.decrypt_content(&mut self.trussed, c).ok(), - }; - maybe_credential - } + let mut maybe_credential = { + // To avoid creating additional buffer for the unfit data + // we will rewind the state and restart from there accordingly + let first_file = try_syscall!(self.trussed.read_dir_files_first( + Location::Internal, + Self::credential_directory(), + None + )) + .map_err(|_| iso7816::Status::UnspecifiedNonpersistentExecutionError)? + .data; + + // Rewind if needed, otherwise return first file's content + let file = match &self.state.runtime.previously { + Some(CommandState::ListCredentials(s)) => { + info_now!("found continuation state: {:?}", s); + for _ in 0..s - 1 { + try_syscall!(self.trussed.read_dir_files_next()) + .map_err(|_| iso7816::Status::UnspecifiedNonpersistentExecutionError)?; + } + try_syscall!(self.trussed.read_dir_files_next()) + .map_err(|_| iso7816::Status::UnspecifiedNonpersistentExecutionError)? + .data + } + None => first_file, + _ => Err(iso7816::Status::FunctionNotSupported)?, // this one should never be reached + }; + + let maybe_credential: Option = match file { + None => None, + Some(c) => self.state.decrypt_content(&mut self.trussed, c).ok(), + }; + maybe_credential }; while let Some(credential) = maybe_credential { + // Try to serialize, abort if not succeeded + let current_reply_bytes_count = reply.len(); + let res = Self::try_to_serialize_credential_for_list(&credential, reply); + if res.is_err() { + // Revert reply vector to the last good size, removing debris from the failed + // serialization + reply.truncate(current_reply_bytes_count); + return Err(Status::MoreAvailable(0xFF)); + } + // keep track, in case we need continuation file_index += 1; self.state.runtime.previously = Some(CommandState::ListCredentials(file_index)); - // deserialize - reply.push(0x72).unwrap(); - reply.push((credential.label.len() + 1) as u8).unwrap(); - reply - .push(oath::combine(credential.kind, credential.algorithm)) - .unwrap(); - reply.extend_from_slice(&credential.label).unwrap(); - - // TODO check output buffer load and react accordingly - // instead of the raw counter - establish maximum label length or introduce additional - // buffer to keep data that had not fit into the reply, to be retrieved later - if file_index % 2 == 0 { - // split response - return Err(Status::MoreAvailable(0xFF)); - } - // check if there's more maybe_credential = match syscall!(self.trussed.read_dir_files_next()).data { None => None, @@ -409,10 +427,10 @@ where } fn send_remaining(&mut self, reply: &mut Data<{ R }>) -> Result { - if self.state.runtime.previously.is_none() { - return Err(Status::ConditionsOfUseNotSatisfied); + match self.state.runtime.previously { + None => Err(Status::ConditionsOfUseNotSatisfied), + Some(CommandState::ListCredentials(_)) => self.list_credentials(reply), } - self.list_credentials(reply) } pub fn register(&mut self, register: command::Register<'_>) -> Result { From 98c8b0145127e79cea45302230e16fe355c5b328 Mon Sep 17 00:00:00 2001 From: Szczepan Zalega Date: Tue, 24 Jan 2023 18:41:54 +0100 Subject: [PATCH 9/9] Cleanup devel features, and describe them --- Cargo.toml | 8 +++++++- src/authenticator.rs | 4 +++- src/state.rs | 10 ++++++---- 3 files changed, 16 insertions(+), 6 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 37506caaa..c92730623 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -30,7 +30,13 @@ pretty_env_logger = "0.4.0" [features] default = ["apdu-dispatch"] -devel = ["apdu-dispatch", "log-all", "delog/std-log"] +devel = ["apdu-dispatch", "log-all", "delog/std-log", "devel-counters", "devel-ctaphid-bug"] + +# Count accesses to the read-only and read-write persistence storage +devel-counters = [] + +# Account ctaphid bug about 3072 buffer size. To be removed once fixed. +devel-ctaphid-bug = [] # Allow to use application over CTAPHID interface ctaphid = ["ctaphid-dispatch", "usbd-ctaphid"] diff --git a/src/authenticator.rs b/src/authenticator.rs index 78584c165..c98e858fe 100644 --- a/src/authenticator.rs +++ b/src/authenticator.rs @@ -335,8 +335,10 @@ where reply.push((credential.label.len() + 1) as u8)?; reply.push(oath::combine(credential.kind, credential.algorithm))?; reply.extend_from_slice(&credential.label).map_err(|_| 0)?; - #[cfg(feature = "devel")] + #[cfg(feature = "devel-ctaphid-bug")] if reply.len() > 3072 { + // Finish early due to the usbd-ctaphid bug, which panics on bigger buffers than this + // FIXME Remove once fixed return Err(1); } Ok(()) diff --git a/src/state.rs b/src/state.rs index 50a4435a9..e8eecb809 100644 --- a/src/state.rs +++ b/src/state.rs @@ -21,9 +21,11 @@ pub struct State { // temporary "state", to be removed again // pub hack: Hack, // trussed: RefCell>, - #[cfg(feature = "devel")] + // Count read-write access to the persistence storage. Development only. + #[cfg(feature = "devel-counters")] counter_read_write: u32, - #[cfg(feature = "devel")] + // Count read-only access to the persistence storage. Development only. + #[cfg(feature = "devel-counters")] counter_read_only: u32, } @@ -173,7 +175,7 @@ impl State { { let mut state: Persistent = Self::get_persistent_or_default(trussed); - #[cfg(feature = "devel")] + #[cfg(feature = "devel-counters")] { self.counter_read_write += 1; debug_now!("Getting the state RW {}", self.counter_read_write); @@ -202,7 +204,7 @@ impl State { { let state: Persistent = Self::get_persistent_or_default(trussed); - #[cfg(feature = "devel")] + #[cfg(feature = "devel-counters")] { self.counter_read_only += 1; debug_now!("Getting the state RO {}", self.counter_read_only);