SFT-7497: input validation in the firmware header path

firmware/src/bin/foundation-firmware.rs

@@ -38,6 +38,13 @@ fn main() -> Result<()> {
     let file_buf = fs::read(file_name).context("failed to read firmware")?;
 
     let header_len = usize::try_from(HEADER_LEN).unwrap();
+    if file_buf.len() < header_len {
+        bail!(
+            "firmware file is too small: {} bytes, the header alone is {header_len}",
+            file_buf.len()
+        );
+    }
+
     let header = match header(&file_buf[..header_len]).finish() {
         Ok((_, hdr)) => hdr,
         Err(_) => bail!("failed to parse firmware header"),

firmware/src/lib.rs

@@ -66,8 +66,9 @@ const FOUNDATION_PUBLIC_KEYS: [[u8; 65]; 4] = [
     ],
 ];
 
-/// Maximum index in the [`Signature::public_key1`] and
-/// [`Signature::public_key2`] fields if it isn't an user key ([`USER_KEY`]).
+/// Number of Foundation public keys, so a valid [`Signature::public_key1`] or
+/// [`Signature::public_key2`] index is strictly less than this, unless it is the
+/// user key ([`USER_KEY`]).
 pub const MAX_PUBLIC_KEYS: u32 = FOUNDATION_PUBLIC_KEYS.len() as u32;
 
 /// The header of the firmware.
@@ -100,13 +101,13 @@ impl Header {
         }
 
         if !self.is_signed_by_user() {
-            if self.signature.public_key1 > MAX_PUBLIC_KEYS {
+            if self.signature.public_key1 >= MAX_PUBLIC_KEYS {
                 return Err(VerifyHeaderError::InvalidPublicKey1Index(
                     self.signature.public_key1,
                 ));
             }
 
-            if self.signature.public_key2 > MAX_PUBLIC_KEYS {
+            if self.signature.public_key2 >= MAX_PUBLIC_KEYS {
                 return Err(VerifyHeaderError::InvalidPublicKey2Index(
                     self.signature.public_key2,
                 ));
@@ -204,26 +205,25 @@ pub struct Signature {
 }
 
 impl Signature {
-    /// Return the first public key.
+    /// Return the first public key, or `None` if `public_key1` is out of range.
     ///
-    /// # Panics
-    ///
-    /// This function can panic if `public_key1` is out of range.  The header
-    /// should have been verified before with [`Header::verify`].
-    pub fn public_key1(&self) -> PublicKey {
-        let public_keys = foundation_public_keys();
-        public_keys[usize::try_from(self.public_key1).unwrap()]
+    /// [`Header::verify`] rejects an out of range index, so a verified header
+    /// always yields `Some`.
+    pub fn public_key1(&self) -> Option<PublicKey> {
+        Self::lookup(self.public_key1)
     }
 
-    /// Return the second public key.
-    ///
-    /// # Panics
+    /// Return the second public key, or `None` if `public_key2` is out of range.
     ///
-    /// This function can panic if `public_key2` is out of range.  The header
-    /// should have been verified before with [`Header::verify`].
-    pub fn public_key2(&self) -> PublicKey {
-        let public_keys = foundation_public_keys();
-        public_keys[usize::try_from(self.public_key2).unwrap()]
+    /// [`Header::verify`] rejects an out of range index, so a verified header
+    /// always yields `Some`.
+    pub fn public_key2(&self) -> Option<PublicKey> {
+        Self::lookup(self.public_key2)
+    }
+
+    fn lookup(index: u32) -> Option<PublicKey> {
+        let index = usize::try_from(index).ok()?;
+        foundation_public_keys().get(index).copied()
     }
 }
 
@@ -410,7 +410,11 @@ pub fn verify_signature<C: Verification>(
     firmware_hash: &sha256d::Hash,
     user_public_key: Option<&PublicKey>,
 ) -> Result<(), VerifySignatureError> {
-    assert!(header.verify().is_ok());
+    // Verifying here rather than asserting on it, so this path fails closed for a
+    // caller that did not verify first.
+    header
+        .verify()
+        .map_err(VerifySignatureError::InvalidHeader)?;
 
     let message = Message::from_digest(firmware_hash.to_byte_array());
 
@@ -443,18 +447,28 @@ pub fn verify_signature<C: Verification>(
             signature1.normalize_s();
             signature2.normalize_s();
 
-            header
-                .signature
-                .public_key1()
+            // verify() above rejects an out of range index, so these are Some. The
+            // checked lookup keeps that a local fact rather than an assumption.
+            let public_key1 = header.signature.public_key1().ok_or({
+                VerifySignatureError::InvalidHeader(VerifyHeaderError::InvalidPublicKey1Index(
+                    header.signature.public_key1,
+                ))
+            })?;
+
+            let public_key2 = header.signature.public_key2().ok_or({
+                VerifySignatureError::InvalidHeader(VerifyHeaderError::InvalidPublicKey2Index(
+                    header.signature.public_key2,
+                ))
+            })?;
+
+            public_key1
                 .verify(secp, &message, &signature1)
                 .map_err(|error| VerifySignatureError::FailedSignature1 {
                     index: header.signature.public_key1,
                     error,
                 })?;
 
-            header
-                .signature
-                .public_key2()
+            public_key2
                 .verify(secp, &message, &signature2)
                 .map_err(|error| VerifySignatureError::FailedSignature2 {
                     index: header.signature.public_key2,
@@ -492,6 +506,9 @@ pub enum VerifySignatureError {
     },
     /// The firmware was signed by the user but no user public key was found.
     MissingUserPublicKey,
+    /// The header did not verify, so there was nothing to check a signature
+    /// against.
+    InvalidHeader(VerifyHeaderError),
 }
 
 impl core::fmt::Display for VerifySignatureError {
@@ -505,6 +522,9 @@ impl core::fmt::Display for VerifySignatureError {
             VerifySignatureError::MissingUserPublicKey => {
                 write!(f, "firmware is user signed but user public key is missing")
             }
+            VerifySignatureError::InvalidHeader(error) => {
+                write!(f, "header verification failed: {error}")
+            }
         }
     }
 }
@@ -516,6 +536,7 @@ impl std::error::Error for VerifySignatureError {
             VerifySignatureError::InvalidUserSignature { error, .. } => Some(error),
             VerifySignatureError::FailedSignature1 { error, .. } => Some(error),
             VerifySignatureError::FailedSignature2 { error, .. } => Some(error),
+            VerifySignatureError::InvalidHeader(error) => Some(error),
             _ => None,
         }
     }

firmware/tests/test-vectors.rs

@@ -1,7 +1,7 @@
 // SPDX-FileCopyrightText: © 2024 Foundation Devices, Inc. <hello@foundationdevices.com>
 // SPDX-License-Identifier: GPL-3.0-or-later
 
-use foundation_firmware::{header, VerifyHeaderError};
+use foundation_firmware::{header, VerifyHeaderError, MAX_PUBLIC_KEYS, USER_KEY};
 use foundation_test_vectors::firmware::{
     INVALID_MAGIC, INVALID_MAX_LENGTH, INVALID_MIN_LENGTH, INVALID_PUBLIC_KEY1,
     INVALID_PUBLIC_KEY2, INVALID_TIMESTAMP, VALID_HEADER,
@@ -61,3 +61,79 @@ pub fn invalid_timestamp() {
     let (_, header) = header(INVALID_TIMESTAMP).finish().unwrap();
     assert_eq!(header.verify(), Err(VerifyHeaderError::InvalidTimestamp));
 }
+
+/// The number of bytes `header()` consumes: an `Information` plus two indexed
+/// compact signatures.
+const PARSED_HEADER_LEN: usize = 34 + (4 + 64) * 2;
+
+#[test]
+pub fn public_key_index_at_the_key_count() {
+    // There are MAX_PUBLIC_KEYS keys, so that value is one past the last index.
+    for index in [MAX_PUBLIC_KEYS, MAX_PUBLIC_KEYS + 1, u32::MAX] {
+        let (_, mut header) = header(VALID_HEADER).finish().unwrap();
+        header.signature.public_key1 = index;
+
+        assert_eq!(
+            header.verify(),
+            Err(VerifyHeaderError::InvalidPublicKey1Index(index)),
+            "index {index} accepted for public_key1"
+        );
+        assert_eq!(header.signature.public_key1(), None);
+    }
+}
+
+#[test]
+pub fn second_public_key_index_at_the_key_count() {
+    for index in [MAX_PUBLIC_KEYS, MAX_PUBLIC_KEYS + 1, u32::MAX] {
+        let (_, mut header) = header(VALID_HEADER).finish().unwrap();
+        header.signature.public_key1 = 0;
+        header.signature.public_key2 = index;
+
+        assert_eq!(
+            header.verify(),
+            Err(VerifyHeaderError::InvalidPublicKey2Index(index)),
+            "index {index} accepted for public_key2"
+        );
+        assert_eq!(header.signature.public_key2(), None);
+    }
+}
+
+#[test]
+pub fn every_in_range_index_resolves() {
+    let (_, mut header) = header(VALID_HEADER).finish().unwrap();
+
+    for index in 0..MAX_PUBLIC_KEYS {
+        header.signature.public_key1 = index;
+        header.signature.public_key2 = index;
+
+        assert!(header.signature.public_key1().is_some(), "index {index}");
+        assert!(header.signature.public_key2().is_some(), "index {index}");
+    }
+}
+
+#[test]
+pub fn a_user_signed_header_does_not_index_the_foundation_keys() {
+    let (_, mut header) = header(VALID_HEADER).finish().unwrap();
+    header.signature.public_key1 = USER_KEY;
+
+    // The index checks are skipped for a user signed image, so this stays valid,
+    // and the out of range index is never looked up.
+    header.verify().unwrap();
+    assert!(header.is_signed_by_user());
+    assert_eq!(header.signature.public_key1(), None);
+}
+
+#[test]
+pub fn empty_and_truncated_input_do_not_parse() {
+    assert!(header(&[]).finish().is_err());
+
+    for len in 1..PARSED_HEADER_LEN {
+        assert!(
+            header(&VALID_HEADER[..len]).finish().is_err(),
+            "{len} bytes parsed as a header"
+        );
+    }
+
+    // ... and a whole one still does.
+    header(&VALID_HEADER[..PARSED_HEADER_LEN]).finish().unwrap();
+}