Merge remote-tracking branch 'agent/benma-agent/backup-buffer-zeroization'

### messages/backup.proto
@@ -14,8 +14,7 @@ message BackupMetaData {
 }
 
 /**
- * BackupData is encoded in the data field of the BackupContent
- * and depends on the BackupMode.
+ * BackupData is the plaintext data message of BackupContent.
  * Defining it as a protobuf message allows language/architecture independent
  * encoding/decoding.
  */
@@ -40,12 +39,11 @@ message BackupContent {
     // be loaded and the checksum verified. Other than that, it serves no purpose, as it is not
     // needed to deserialize or interpret the data.
     uint32 length = 3;
-    bytes data = 4;
+    BackupData data = 4;
 }
 
-/* NOTE! Once the firmware is released to the general public and there are actual backups it is
- * strictly forbidden to modify BackupV1 and any types contained within BackupV1 because the
- * checksum covers all fields. */
+/* The wire format and checksum semantics of BackupV1 and its nested types must
+ * remain unchanged so existing backups can be restored and verified. */
 
 message BackupV1 {
     BackupContent content = 1;

### src/rust/bitbox-proto/src/generated/shiftcrypto.bitbox02.backups.rs
@@ -10,8 +10,7 @@ pub struct BackupMetaData {
     pub mode: i32,
 }
 /// *
-/// BackupData is encoded in the data field of the BackupContent
-/// and depends on the BackupMode.
+/// BackupData is the plaintext data message of BackupContent.
 /// Defining it as a protobuf message allows language/architecture independent
 /// encoding/decoding.
 #[allow(clippy::derive_partial_eq_without_eq)]
@@ -45,8 +44,8 @@ pub struct BackupContent {
     /// needed to deserialize or interpret the data.
     #[prost(uint32, tag = "3")]
     pub length: u32,
-    #[prost(bytes = "vec", tag = "4")]
-    pub data: ::prost::alloc::vec::Vec<u8>,
+    #[prost(message, optional, tag = "4")]
+    pub data: ::core::option::Option<BackupData>,
 }
 #[allow(clippy::derive_partial_eq_without_eq)]
 #[derive(Clone, PartialEq, ::prost::Message)]

### src/rust/bitbox-proto/src/lib.rs
@@ -10,10 +10,47 @@ pub mod pb_backup {
     include!("./generated/shiftcrypto.bitbox02.backups.rs");
 }
 
+impl zeroize::Zeroize for pb_backup::BackupData {
+    fn zeroize(&mut self) {
+        self.seed_length.zeroize();
+        self.seed.zeroize();
+        self.birthdate.zeroize();
+        self.generator.zeroize();
+    }
+}
+
+// Keep seed cleanup on the owning message, including when it is nested in a backup.
+impl Drop for pb_backup::BackupData {
+    fn drop(&mut self) {
+        zeroize::Zeroize::zeroize(self);
+    }
+}
+
 // Also clear passphrases discarded during decoding, invalid-state handling or cancellation.
 impl Drop for pb::UnlockHostInfoRequest {
     fn drop(&mut self) {
         use zeroize::Zeroize;
         self.passphrase.zeroize();
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use zeroize::Zeroize;
+
+    #[test]
+    fn test_backup_data_zeroize() {
+        let mut data = pb_backup::BackupData {
+            seed_length: 32,
+            seed: prost::alloc::vec![0x5a; 32],
+            birthdate: 1234,
+            generator: "test".into(),
+        };
+        data.zeroize();
+        assert_eq!(data.seed_length, 0);
+        assert!(data.seed.is_empty());
+        assert_eq!(data.birthdate, 0);
+        assert!(data.generator.is_empty());
+    }
+}

### src/rust/bitbox02-rust/src/backup.rs
@@ -12,7 +12,7 @@ use alloc::vec::Vec;
 
 use hmac::{Hmac, Mac};
 use sha2::{Digest, Sha256};
-use zeroize::{Zeroize, Zeroizing};
+use zeroize::Zeroizing;
 
 #[derive(Debug)]
 pub enum Error {
@@ -46,15 +46,6 @@ impl BackupData {
     }
 }
 
-impl Zeroize for BackupData {
-    fn zeroize(&mut self) {
-        self.0.seed_length.zeroize();
-        self.0.seed.zeroize();
-        self.0.birthdate.zeroize();
-        self.0.generator.zeroize();
-    }
-}
-
 /// Pad a seed with zeroes to the right up to 32 bytes.  That the seed field in backups is always 32
 /// bytes is a historical "accident" as it was easier to fix the size when using C (and nanopb
 /// protobuf), with the seed_length indicating the actual length.
@@ -122,14 +113,13 @@ fn compute_checksum(
     Ok(hasher.finalize().to_vec())
 }
 
-fn load_from_buffer(buf: &[u8]) -> Result<(Zeroizing<BackupData>, pb_backup::BackupMetaData), ()> {
+fn load_from_buffer(buf: &[u8]) -> Result<(BackupData, pb_backup::BackupMetaData), ()> {
     let backup = pb_backup::Backup::decode(buf).or(Err(()))?;
     match backup.backup_version {
         Some(pb_backup::backup::BackupVersion::BackupV1(pb_backup::BackupV1 {
             content: Some(content),
         })) => {
-            let mut backup_data: Zeroizing<BackupData> = Default::default();
-            backup_data.0.merge(content.data.as_slice()).or(Err(()))?;
+            let backup_data = BackupData(Box::new(content.data.ok_or(())?));
             if !matches!(backup_data.0.seed_length, 16 | 24 | 32) {
                 return Err(());
             }
@@ -148,7 +138,7 @@ fn load_from_buffer(buf: &[u8]) -> Result<(Zeroizing<BackupData>, pb_backup::Bac
 fn load_from_buffer_for_dir(
     buf: &[u8],
     dir: &str,
-) -> Result<(Zeroizing<BackupData>, pb_backup::BackupMetaData), ()> {
+) -> Result<(BackupData, pb_backup::BackupMetaData), ()> {
     let (backup_data, metadata) = load_from_buffer(buf)?;
     if id(backup_data.get_seed()) != dir {
         return Err(());
@@ -183,7 +173,7 @@ fn bitwise_recovery(buf1: &[u8], buf2: &[u8], buf3: &[u8]) -> Result<Zeroizing<V
 pub async fn load(
     hal: &mut impl crate::hal::Hal,
     dir: &str,
-) -> Result<(Zeroizing<BackupData>, pb_backup::BackupMetaData), ()> {
+) -> Result<(BackupData, pb_backup::BackupMetaData), ()> {
     let files = hal.sd().list_subdir(Some(dir)).await?;
     if files.len() != 3 {
         return Err(());
@@ -214,12 +204,12 @@ pub async fn create(
     backup_create_timestamp: u32,
     seed_birthdate_timestamp: u32,
 ) -> Result<(), Error> {
-    let backup_data = zeroize::Zeroizing::new(BackupData(Box::new(pb_backup::BackupData {
+    let backup_data = pb_backup::BackupData {
         seed_length: seed.len() as _,
         seed: padded_seed(seed).to_vec(),
         birthdate: seed_birthdate_timestamp,
         generator: crate::version::FIRMWARE_VERSION_SHORT.into(),
-    })));
+    };
     let length: u32 = {
         // See the documentation in the backup.proto file - the length field is obsolete, but for
         // backwards compatbility still set, as it is part of the checksum.
@@ -234,24 +224,26 @@ pub async fn create(
     // We cap at 19/63 because the previous implementation in C/nanopb used null terminated strings
     // with a buffer of 20/64 bytes when decoding the protobuf message. This allows the backup to be
     // restored on older firmware.
-    if backup_data.0.generator.len() > 19 || metadata.name.len() > 63 {
+    if backup_data.generator.len() > 19 || metadata.name.len() > 63 {
         return Err(Error::Generic);
     }
 
-    let checksum = compute_checksum(&metadata, &backup_data.0, length).or(Err(Error::Generic))?;
+    let checksum = compute_checksum(&metadata, &backup_data, length).or(Err(Error::Generic))?;
     let backup = pb_backup::Backup {
         backup_version: Some(pb_backup::backup::BackupVersion::BackupV1(
             pb_backup::BackupV1 {
                 content: Some(pb_backup::BackupContent {
                     checksum,
                     metadata: Some(metadata),
                     length,
-                    data: backup_data.0.encode_to_vec(),
+                    data: Some(backup_data),
                 }),
             },
         )),
     };
-    let backup_encoded = backup.encode_to_vec();
+    let backup_encoded = Zeroizing::new(backup.encode_to_vec());
+    // Wipe the seed-bearing message before any SD operation can suspend or return early.
+    drop(backup);
     let dir = id(seed);
     let files = hal
         .sd()
@@ -313,6 +305,94 @@ mod tests {
     use crate::hal::testing::TestingHal;
     use core::convert::TryInto;
 
+    // The former backup representation, with an independently serialized message in field 4.
+    #[derive(Clone, PartialEq, Message)]
+    struct LegacyBackupContent {
+        #[prost(bytes = "vec", tag = "1")]
+        checksum: Vec<u8>,
+        #[prost(message, optional, tag = "2")]
+        metadata: Option<pb_backup::BackupMetaData>,
+        #[prost(uint32, tag = "3")]
+        length: u32,
+        #[prost(bytes = "vec", tag = "4")]
+        data: Vec<u8>,
+    }
+
+    #[derive(Clone, PartialEq, Message)]
+    struct LegacyBackupV1 {
+        #[prost(message, optional, tag = "1")]
+        content: Option<LegacyBackupContent>,
+    }
+
+    #[derive(Clone, PartialEq, Message)]
+    struct LegacyBackup {
+        #[prost(message, optional, tag = "1")]
+        backup_v1: Option<LegacyBackupV1>,
+    }
+
+    #[async_test::test]
+    async fn test_create_load_wire_compatibility() {
+        let seed_bytes =
+            hex_lit::hex!("5220a4e9ceeac6805df23609f6b478bb28ca69b51695ed7c03bf743aa5dee37e");
+        for seed_length in [16, 24, 32] {
+            let seed = &seed_bytes[..seed_length];
+            for (name, timestamp, birthdate) in [
+                (String::new(), 0, 0),
+                (String::from("test backup"), 1601281809, 1601249409),
+                ("a".repeat(63), u32::MAX, u32::MAX),
+            ] {
+                let data = pb_backup::BackupData {
+                    seed_length: seed_length as _,
+                    seed: padded_seed(seed).to_vec(),
+                    birthdate,
+                    generator: crate::version::FIRMWARE_VERSION_SHORT.into(),
+                };
+                let metadata = pb_backup::BackupMetaData {
+                    timestamp,
+                    name: name.clone(),
+                    mode: pb_backup::BackupMode::Plaintext as _,
+                };
+                let mut legacy = LegacyBackup {
+                    backup_v1: Some(LegacyBackupV1 {
+                        content: Some(LegacyBackupContent {
+                            checksum: compute_checksum(&metadata, &data, 0).unwrap(),
+                            metadata: Some(metadata.clone()),
+                            length: 0,
+                            data: data.encode_to_vec(),
+                        }),
+                    }),
+                };
+                let legacy_bytes = legacy.encode_to_vec();
+                let dir = id(seed);
+                let (loaded, loaded_metadata) =
+                    load_from_buffer_for_dir(&legacy_bytes, &dir).unwrap();
+                assert_eq!(loaded.0.as_ref(), &data);
+                assert_eq!(loaded_metadata, metadata);
+
+                let mut hal = TestingHal::new();
+                create(&mut hal, seed, &name, timestamp, birthdate)
+                    .await
+                    .unwrap();
+                for file in hal.sd.list_subdir(Some(&dir)).await.unwrap() {
+                    let bytes = hal.sd.load_bin(&file, &dir).await.unwrap();
+                    assert_eq!(bytes.as_slice(), legacy_bytes.as_slice());
+                    assert_eq!(LegacyBackup::decode(bytes.as_slice()).unwrap(), legacy);
+                }
+
+                // Legacy nonzero length values remain checksum input, not a parsing boundary.
+                let content = legacy.backup_v1.as_mut().unwrap().content.as_mut().unwrap();
+                content.length = 71;
+                assert!(load_from_buffer(&legacy.encode_to_vec()).is_err());
+                let content = legacy.backup_v1.as_mut().unwrap().content.as_mut().unwrap();
+                content.checksum = compute_checksum(&metadata, &data, content.length).unwrap();
+                let (loaded, loaded_metadata) =
+                    load_from_buffer_for_dir(&legacy.encode_to_vec(), &dir).unwrap();
+                assert_eq!(loaded.0.as_ref(), &data);
+                assert_eq!(loaded_metadata, metadata);
+            }
+        }
+    }
+
     #[test]
     fn test_id() {
         // Seeds of different lengths (16, 24, 32 bytes)
@@ -357,7 +437,7 @@ mod tests {
                             checksum,
                             metadata: Some(metadata),
                             length,
-                            data: data.encode_to_vec(),
+                            data: Some(data),
                         }),
                     },
                 )),
@@ -382,7 +462,7 @@ mod tests {
                         checksum: vec![],
                         metadata: None,
                         length: 0,
-                        data: data.encode_to_vec(),
+                        data: Some(data),
                     }),
                 },
             )),