From 8509f5ecc1357292a86dca1c051ccb530f982e40 Mon Sep 17 00:00:00 2001 From: Jack Date: Tue, 29 Sep 2026 15:21:47 +0200 Subject: [PATCH] SFT-7497: input validation in the firmware header path Tighten input handling so malformed local input produces an error rather than an unwind. MAX_PUBLIC_KEYS is the number of Foundation keys, so derive the index bound from the array as a strict upper bound and say so in its doc. Make the lookups return Option through a single checked helper. verify_signature() now verifies the header and returns VerifySignatureError::InvalidHeader rather than asserting on it, and maps a None lookup onto the matching index error. The CLI checks the file is at least HEADER_LEN bytes before slicing it. Nothing that verified successfully before changes. Note for downstream: VerifySignatureError gains a variant, and public_key1()/public_key2() return Option. --- firmware/src/bin/foundation-firmware.rs | 7 +++ firmware/src/lib.rs | 75 +++++++++++++++--------- firmware/tests/test-vectors.rs | 78 ++++++++++++++++++++++++- 3 files changed, 132 insertions(+), 28 deletions(-) diff --git a/firmware/src/bin/foundation-firmware.rs b/firmware/src/bin/foundation-firmware.rs index e6397ec..ac8b592 100644 --- a/firmware/src/bin/foundation-firmware.rs +++ b/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"), diff --git a/firmware/src/lib.rs b/firmware/src/lib.rs index c5f4138..dc837b9 100644 --- a/firmware/src/lib.rs +++ b/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 { + 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 { + Self::lookup(self.public_key2) + } + + fn lookup(index: u32) -> Option { + let index = usize::try_from(index).ok()?; + foundation_public_keys().get(index).copied() } } @@ -410,7 +410,11 @@ pub fn verify_signature( 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( 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, } } diff --git a/firmware/tests/test-vectors.rs b/firmware/tests/test-vectors.rs index eccd0bd..d3ce751 100644 --- a/firmware/tests/test-vectors.rs +++ b/firmware/tests/test-vectors.rs @@ -1,7 +1,7 @@ // SPDX-FileCopyrightText: © 2024 Foundation Devices, Inc. // 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(); +}