From 3c28ff49eea8ae86f21e1afcbb28e66c2e12432c Mon Sep 17 00:00:00 2001 From: kaczmarczyck <43844792+kaczmarczyck@users.noreply.github.com> Date: Mon, 3 Oct 2022 16:33:34 +0200 Subject: [PATCH] Simplifies Env upgrade API (#551) * removes read_partition and partition_length from upgrade API * renames partition to bundle, also data type change from slice to Vec * removes hash from Env API * fixes comment --- src/api/upgrade_storage/mod.rs | 30 +++++++------------------ src/ctap/command.rs | 18 ++++++++++++--- src/ctap/mod.rs | 39 ++++++++++++++------------------- src/env/test/upgrade_storage.rs | 28 ++++++++++------------- src/env/tock/storage.rs | 36 +++++++++--------------------- 5 files changed, 62 insertions(+), 89 deletions(-) diff --git a/src/api/upgrade_storage/mod.rs b/src/api/upgrade_storage/mod.rs index 9426165..733a68a 100644 --- a/src/api/upgrade_storage/mod.rs +++ b/src/api/upgrade_storage/mod.rs @@ -12,41 +12,27 @@ // See the License for the specific language governing permissions and // limitations under the License. +use alloc::vec::Vec; use persistent_store::StorageResult; pub(crate) mod helper; /// Accessors to storage locations used for upgrading from a CTAP command. pub trait UpgradeStorage { - /// Reads a slice of the partition, if within bounds. + /// Processes the given data as part of an upgrade. /// - /// The offset is relative to the start of the partition, excluding holes. The partition is - /// presented as one connected component. Therefore, the offset does not easily translate - /// to physical memory address address of the slice. + /// The offset indicates the data location inside the bundle. /// /// # Errors /// - /// Returns [`StorageError::OutOfBounds`] if the requested slice is not inside the partition. - fn read_partition(&self, offset: usize, length: usize) -> StorageResult<&[u8]>; + /// - Returns [`StorageError::OutOfBounds`] if the data does not fit. + /// - Returns [`StorageError::CustomError`] if any Metadata or other check fails. + fn write_bundle(&mut self, offset: usize, data: Vec) -> StorageResult<()>; - /// Writes the given data to the given offset address, if within bounds of the partition. - /// - /// The offset is relative to the start of the partition, excluding holes. - /// See `read_partition`. - /// - /// # Errors - /// - /// - Returns [`StorageError::OutOfBounds`] if the data does not fit the partition. - /// - Returns [`StorageError::CustomError`] if any Metadata check fails. - fn write_partition(&mut self, offset: usize, data: &[u8]) -> StorageResult<()>; - - /// Returns an identifier for the partition. + /// Returns an identifier for the requested bundle. /// /// Use this to determine whether you are writing to A or B. - fn partition_identifier(&self) -> u32; - - /// Returns the length of the partition. - fn partition_length(&self) -> usize; + fn bundle_identifier(&self) -> u32; /// Returns the currently running firmware version. fn running_firmware_version(&self) -> u64; diff --git a/src/ctap/command.rs b/src/ctap/command.rs index cd98989..23a5ee1 100644 --- a/src/ctap/command.rs +++ b/src/ctap/command.rs @@ -594,7 +594,7 @@ impl TryFrom for AuthenticatorVendorConfigureParameters { pub struct AuthenticatorVendorUpgradeParameters { pub offset: usize, pub data: Vec, - pub hash: Vec, + pub hash: [u8; 32], } impl TryFrom for AuthenticatorVendorUpgradeParameters { @@ -610,7 +610,8 @@ impl TryFrom for AuthenticatorVendorUpgradeParameters { } let offset = extract_unsigned(ok_or_missing(offset)?)? as usize; let data = extract_byte_string(ok_or_missing(data)?)?; - let hash = extract_byte_string(ok_or_missing(hash)?)?; + let hash = <[u8; 32]>::try_from(extract_byte_string(ok_or_missing(hash)?)?) + .map_err(|_| Ctap2StatusCode::CTAP1_ERR_INVALID_PARAMETER)?; Ok(AuthenticatorVendorUpgradeParameters { offset, data, hash }) } } @@ -1080,6 +1081,17 @@ mod test { Err(Ctap2StatusCode::CTAP2_ERR_MISSING_PARAMETER) ); + // Invalid hash size + let cbor_value = cbor_map! { + 0x01 => 0x1000, + 0x02 => [0xFF; 0x100], + 0x03 => [0x44; 33], + }; + assert_eq!( + AuthenticatorVendorUpgradeParameters::try_from(cbor_value), + Err(Ctap2StatusCode::CTAP1_ERR_INVALID_PARAMETER) + ); + // Missing hash let cbor_value = cbor_map! { 0x01 => 0x1000, @@ -1101,7 +1113,7 @@ mod test { Ok(AuthenticatorVendorUpgradeParameters { offset: 0x1000, data: vec![0xFF; 0x100], - hash: vec![0x44; 32], + hash: [0x44; 32], }) ); } diff --git a/src/ctap/mod.rs b/src/ctap/mod.rs index 6ca77f5..1f3bdeb 100644 --- a/src/ctap/mod.rs +++ b/src/ctap/mod.rs @@ -1399,19 +1399,14 @@ impl CtapState { params: AuthenticatorVendorUpgradeParameters, ) -> Result { let AuthenticatorVendorUpgradeParameters { offset, data, hash } = params; - let upgrade_locations = env - .upgrade_storage() - .ok_or(Ctap2StatusCode::CTAP1_ERR_INVALID_COMMAND)?; - upgrade_locations - .write_partition(offset, &data) - .map_err(|_| Ctap2StatusCode::CTAP1_ERR_INVALID_PARAMETER)?; - let written_slice = upgrade_locations - .read_partition(offset, data.len()) - .map_err(|_| Ctap2StatusCode::CTAP1_ERR_INVALID_PARAMETER)?; - let written_hash = Sha256::hash(written_slice); - if hash != written_hash { + let calculated_hash = Sha256::hash(&data); + if hash != calculated_hash { return Err(Ctap2StatusCode::CTAP2_ERR_INTEGRITY_FAILURE); } + env.upgrade_storage() + .ok_or(Ctap2StatusCode::CTAP1_ERR_INVALID_COMMAND)? + .write_bundle(offset, data) + .map_err(|_| Ctap2StatusCode::CTAP1_ERR_INVALID_PARAMETER)?; Ok(ResponseData::AuthenticatorVendorUpgrade) } @@ -1424,7 +1419,7 @@ impl CtapState { .ok_or(Ctap2StatusCode::CTAP1_ERR_INVALID_COMMAND)?; Ok(ResponseData::AuthenticatorVendorUpgradeInfo( AuthenticatorVendorUpgradeInfoResponse { - info: upgrade_locations.partition_identifier(), + info: upgrade_locations.bundle_identifier(), }, )) } @@ -3393,9 +3388,9 @@ mod test { const METADATA_LEN: usize = 0x1000; let metadata = vec![0xFF; METADATA_LEN]; - let metadata_hash = Sha256::hash(&metadata).to_vec(); + let metadata_hash = Sha256::hash(&metadata); let data = vec![0xFF; 0x1000]; - let hash = Sha256::hash(&data).to_vec(); + let hash = Sha256::hash(&data); // Write to partition. let response = ctap_state.process_vendor_upgrade( @@ -3403,7 +3398,7 @@ mod test { AuthenticatorVendorUpgradeParameters { offset: 0x20000, data: data.clone(), - hash: hash.clone(), + hash, }, ); assert_eq!(response, Ok(ResponseData::AuthenticatorVendorUpgrade)); @@ -3414,7 +3409,7 @@ mod test { AuthenticatorVendorUpgradeParameters { offset: 0, data: metadata.clone(), - hash: metadata_hash.clone(), + hash: metadata_hash, }, ); assert_eq!(response, Ok(ResponseData::AuthenticatorVendorUpgrade)); @@ -3425,7 +3420,7 @@ mod test { AuthenticatorVendorUpgradeParameters { offset: METADATA_LEN, data: data.clone(), - hash: hash.clone(), + hash, }, ); assert_eq!(response, Ok(ResponseData::AuthenticatorVendorUpgrade)); @@ -3439,7 +3434,7 @@ mod test { hash: metadata_hash, }, ); - assert_eq!(response, Err(Ctap2StatusCode::CTAP1_ERR_INVALID_PARAMETER)); + assert_eq!(response, Err(Ctap2StatusCode::CTAP2_ERR_INTEGRITY_FAILURE)); // Write outside of the partition. let response = ctap_state.process_vendor_upgrade( @@ -3458,7 +3453,7 @@ mod test { AuthenticatorVendorUpgradeParameters { offset: 0x20000, data, - hash: [0xEE; 32].to_vec(), + hash: [0xEE; 32], }, ); assert_eq!(response, Err(Ctap2StatusCode::CTAP2_ERR_INTEGRITY_FAILURE)); @@ -3471,7 +3466,7 @@ mod test { let mut ctap_state = CtapState::new(&mut env, CtapInstant::new(0)); let data = vec![0xFF; 0x1000]; - let hash = Sha256::hash(&data).to_vec(); + let hash = Sha256::hash(&data); let response = ctap_state.process_vendor_upgrade( &mut env, AuthenticatorVendorUpgradeParameters { @@ -3487,14 +3482,14 @@ mod test { fn test_vendor_upgrade_info() { let mut env = TestEnv::new(); let ctap_state = CtapState::new(&mut env, CtapInstant::new(0)); - let partition_identifier = env.upgrade_storage().unwrap().partition_identifier(); + let bundle_identifier = env.upgrade_storage().unwrap().bundle_identifier(); let upgrade_info_reponse = ctap_state.process_vendor_upgrade_info(&mut env); assert_eq!( upgrade_info_reponse, Ok(ResponseData::AuthenticatorVendorUpgradeInfo( AuthenticatorVendorUpgradeInfoResponse { - info: partition_identifier, + info: bundle_identifier, } )) ); diff --git a/src/env/test/upgrade_storage.rs b/src/env/test/upgrade_storage.rs index 5021834..3a28e77 100644 --- a/src/env/test/upgrade_storage.rs +++ b/src/env/test/upgrade_storage.rs @@ -31,9 +31,8 @@ impl BufferUpgradeStorage { partition: vec![0xff; PARTITION_LENGTH].into_boxed_slice(), }) } -} -impl UpgradeStorage for BufferUpgradeStorage { + #[cfg(test)] fn read_partition(&self, offset: usize, length: usize) -> StorageResult<&[u8]> { if length == 0 { return Err(StorageError::OutOfBounds); @@ -45,8 +44,10 @@ impl UpgradeStorage for BufferUpgradeStorage { Err(StorageError::OutOfBounds) } } +} - fn write_partition(&mut self, offset: usize, data: &[u8]) -> StorageResult<()> { +impl UpgradeStorage for BufferUpgradeStorage { + fn write_bundle(&mut self, offset: usize, data: Vec) -> StorageResult<()> { if offset == 0 && data.len() != METADATA_LENGTH { return Err(StorageError::OutOfBounds); } @@ -55,21 +56,17 @@ impl UpgradeStorage for BufferUpgradeStorage { } let partition_range = ModRange::new(0, self.partition.len()); if partition_range.contains_range(&ModRange::new(offset, data.len())) { - self.partition[offset..][..data.len()].copy_from_slice(data); + self.partition[offset..][..data.len()].copy_from_slice(&data); Ok(()) } else { Err(StorageError::OutOfBounds) } } - fn partition_identifier(&self) -> u32 { + fn bundle_identifier(&self) -> u32 { 0x60000 } - fn partition_length(&self) -> usize { - PARTITION_LENGTH - } - fn running_firmware_version(&self) -> u64 { 0 } @@ -80,13 +77,13 @@ mod tests { use super::*; #[test] - fn read_write_partition() { + fn read_write_bundle() { let mut storage = BufferUpgradeStorage::new().unwrap(); assert_eq!(storage.read_partition(0, 2).unwrap(), &[0xFF, 0xFF]); - assert!(storage.write_partition(1, &[0x88, 0x88]).is_ok()); + assert!(storage.write_bundle(1, vec![0x88, 0x88]).is_ok()); assert_eq!(storage.read_partition(0, 2).unwrap(), &[0xFF, 0x88]); assert_eq!( - storage.write_partition(PARTITION_LENGTH - 1, &[0x88, 0x88]), + storage.write_bundle(PARTITION_LENGTH - 1, vec![0x88, 0x88],), Err(StorageError::OutOfBounds) ); assert_eq!( @@ -98,11 +95,11 @@ mod tests { Err(StorageError::OutOfBounds) ); assert_eq!( - storage.write_partition(4, &[]), + storage.write_bundle(4, vec![]), Err(StorageError::OutOfBounds) ); assert_eq!( - storage.write_partition(PARTITION_LENGTH + 4, &[]), + storage.write_bundle(PARTITION_LENGTH + 4, vec![]), Err(StorageError::OutOfBounds) ); assert_eq!(storage.read_partition(4, 0), Err(StorageError::OutOfBounds)); @@ -115,7 +112,6 @@ mod tests { #[test] fn partition_slice() { let storage = BufferUpgradeStorage::new().unwrap(); - assert_eq!(storage.partition_identifier(), 0x60000); - assert_eq!(storage.partition_length(), PARTITION_LENGTH); + assert_eq!(storage.bundle_identifier(), 0x60000); } } diff --git a/src/env/tock/storage.rs b/src/env/tock/storage.rs index 3d78cc8..b2d7e29 100644 --- a/src/env/tock/storage.rs +++ b/src/env/tock/storage.rs @@ -352,18 +352,7 @@ impl TockUpgradeStorage { } impl UpgradeStorage for TockUpgradeStorage { - fn read_partition(&self, offset: usize, length: usize) -> StorageResult<&[u8]> { - if length == 0 { - return Err(StorageError::OutOfBounds); - } - if let Some(address) = self.partition.find_address(offset, length) { - Ok(unsafe { read_slice(address, length) }) - } else { - Err(StorageError::OutOfBounds) - } - } - - fn write_partition(&mut self, offset: usize, data: &[u8]) -> StorageResult<()> { + fn write_bundle(&mut self, offset: usize, data: Vec) -> StorageResult<()> { if data.is_empty() { return Err(StorageError::OutOfBounds); } @@ -382,23 +371,23 @@ impl UpgradeStorage for TockUpgradeStorage { for address in write_range.aligned_iter(self.page_size) { erase_page(address, self.page_size)?; } - write_slice(address, data)?; + write_slice(address, &data)?; + let written_slice = unsafe { read_slice(address, data.len()) }; + if written_slice != data { + return Err(StorageError::CustomError); + } // Case: Last slice is written. - if data.len() == self.partition_length() - offset { + if data.len() == self.partition.length() - offset { let metadata = unsafe { read_slice(self.metadata.start(), self.metadata.length()) }; self.check_partition_hash(&metadata)?; } Ok(()) } - fn partition_identifier(&self) -> u32 { + fn bundle_identifier(&self) -> u32 { self.identifier } - fn partition_length(&self) -> usize { - self.partition.length() - } - fn running_firmware_version(&self) -> u64 { let running_metadata = unsafe { read_slice( @@ -438,7 +427,7 @@ fn check_metadata( } let metadata_address = LittleEndian::read_u32(&metadata[METADATA_SIGN_OFFSET + 8..][..4]); - if metadata_address != upgrade_locations.partition_identifier() { + if metadata_address != upgrade_locations.bundle_identifier() { return Err(StorageError::CustomError); } @@ -495,13 +484,8 @@ mod test { let mut metadata = vec![0xFF; METADATA_LEN]; LittleEndian::write_u32(&mut metadata[METADATA_SIGN_OFFSET + 8..][..4], 0x60000); - let partition_length = upgrade_locations.partition_length(); let mut signed_over_data = metadata[METADATA_SIGN_OFFSET..].to_vec(); - signed_over_data.extend( - upgrade_locations - .read_partition(0, partition_length) - .unwrap(), - ); + signed_over_data.extend(&[0xFF; 0x20000]); let signed_hash = Sha256::hash(&signed_over_data); metadata[..32].copy_from_slice(&signed_hash);