From fa0c22b494b5e7bb2d614bcec1c479a86c7d62c5 Mon Sep 17 00:00:00 2001 From: Brian Smith Date: Wed, 8 Jan 2025 18:01:32 -0800 Subject: [PATCH] digest/hmac: Clarify error handling. Computing an HMAC can only fail if the input is too long. The other `FinishError` condition, where we failed to meet the precondition for `BlockContext::try_finish()`, is obviously, using information local to the function, unreachable. Similarly, when there is only a single slice of bytes as input, then the `InputTooLongError` should be the `usize` type, not `u64`. The case where the u64 -> usize condition fails is not interesting, so we use a saturating conversion instead of giving ourselves the future work of proving that it is unreachable. Finally, remove the `From for error::Unspecified` implementation, in favor of `error::erase`, following the new convention. --- src/digest.rs | 33 +++++++++++++++++++-------------- src/hmac.rs | 49 +++++++++++++++++++++++++++++++++++++++---------- 2 files changed, 58 insertions(+), 24 deletions(-) diff --git a/src/digest.rs b/src/digest.rs index 1435da425f..9fa8ade625 100644 --- a/src/digest.rs +++ b/src/digest.rs @@ -24,8 +24,7 @@ use self::{ }; use crate::{ bits::{BitLength, FromByteLen as _}, - cpu, debug, - error::{self, InputTooLongError}, + cpu, debug, error, polyfill::{self, slice, sliceutil}, }; use core::num::Wrapping; @@ -145,9 +144,11 @@ impl BlockContext { } } +pub(crate) type InputTooLongError = error::InputTooLongError; + pub(crate) enum FinishError { #[allow(dead_code)] - InputTooLong(InputTooLongError), + InputTooLong(InputTooLongError), #[allow(dead_code)] PendingNotAPartialBlock(usize), } @@ -155,7 +156,7 @@ pub(crate) enum FinishError { impl FinishError { #[cold] #[inline(never)] - fn input_too_long(source: InputTooLongError) -> Self { + fn input_too_long(source: InputTooLongError) -> Self { Self::InputTooLong(source) } @@ -170,12 +171,6 @@ impl FinishError { } } -impl From for error::Unspecified { - fn from(_: FinishError) -> Self { - Self - } -} - /// A context for multi-step (Init-Update-Finish) digest calculations. /// /// # Examples @@ -270,13 +265,23 @@ impl Context { pub fn finish(self) -> Digest { let cpu = cpu::features(); self.try_finish(cpu) - .map_err(error::Unspecified::from) + .map_err(error::erase::) .unwrap() } - pub(crate) fn try_finish(mut self, cpu_features: cpu::Features) -> Result { + pub(crate) fn try_finish( + mut self, + cpu_features: cpu::Features, + ) -> Result { self.block .try_finish(&mut self.pending, self.num_pending, cpu_features) + .map_err(|err| match err { + FinishError::InputTooLong(i) => i, + FinishError::PendingNotAPartialBlock(_) => { + // Due to invariant. + unreachable!() + } + }) } /// The algorithm that this context is using. @@ -304,7 +309,7 @@ impl Context { pub fn digest(algorithm: &'static Algorithm, data: &[u8]) -> Digest { let cpu = cpu::features(); Digest::compute_from(algorithm, data, cpu) - .map_err(error::Unspecified::from) + .map_err(error::erase::) .unwrap() } @@ -322,7 +327,7 @@ impl Digest { algorithm: &'static Algorithm, data: &[u8], cpu: cpu::Features, - ) -> Result { + ) -> Result { let mut ctx = Context::new(algorithm); ctx.update(data); ctx.try_finish(cpu) diff --git a/src/hmac.rs b/src/hmac.rs index 9923cd85a5..0378577702 100644 --- a/src/hmac.rs +++ b/src/hmac.rs @@ -111,10 +111,12 @@ use crate::{ constant_time, cpu, - digest::{self, Digest}, + digest::{self, Digest, FinishError}, error, hkdf, rand, }; +pub(crate) use crate::digest::InputTooLongError; + /// An HMAC algorithm. #[derive(Clone, Copy, Debug, PartialEq, Eq)] pub struct Algorithm(&'static digest::Algorithm); @@ -193,7 +195,7 @@ impl Key { let mut key_bytes = [0; digest::MAX_OUTPUT_LEN]; let key_bytes = &mut key_bytes[..algorithm.0.output_len()]; fill(key_bytes)?; - Self::try_new(algorithm, key_bytes, cpu).map_err(error::Unspecified::from) + Self::try_new(algorithm, key_bytes, cpu).map_err(error::erase::) } /// Construct an HMAC signing key using the given digest algorithm and key @@ -217,7 +219,7 @@ impl Key { /// removed in a future version of *ring*. pub fn new(algorithm: Algorithm, key_value: &[u8]) -> Self { Self::try_new(algorithm, key_value, cpu::features()) - .map_err(error::Unspecified::from) + .map_err(error::erase::) .unwrap() } @@ -225,7 +227,7 @@ impl Key { algorithm: Algorithm, key_value: &[u8], cpu_features: cpu::Features, - ) -> Result { + ) -> Result { let digest_alg = algorithm.0; let mut key = Self { inner: digest::BlockContext::new(digest_alg), @@ -272,14 +274,16 @@ impl Key { Algorithm(self.inner.algorithm) } - fn sign(&self, data: &[u8], cpu: cpu::Features) -> Result { + fn sign(&self, data: &[u8], cpu: cpu::Features) -> Result { let mut ctx = Context::with_key(self); ctx.update(data); ctx.try_sign(cpu) } fn verify(&self, data: &[u8], tag: &[u8], cpu: cpu::Features) -> Result<(), VerifyError> { - let computed = self.sign(data, cpu).map_err(VerifyError::DigestError)?; + let computed = self + .sign(data, cpu) + .map_err(VerifyError::InputTooLongError)?; constant_time::verify_slices_are_equal(computed.as_ref(), tag) .map_err(|_: error::Unspecified| VerifyError::Mismatch) } @@ -339,11 +343,15 @@ impl Context { /// instead. pub fn sign(self) -> Tag { self.try_sign(cpu::features()) - .map_err(error::Unspecified::from) + .map_err(error::erase::) .unwrap() } - fn try_sign(self, cpu_features: cpu::Features) -> Result { + fn try_sign(self, cpu_features: cpu::Features) -> Result { + // Consequently, `num_pending` is valid. + debug_assert_eq!(self.inner.algorithm(), self.outer.algorithm); + debug_assert!(self.inner.algorithm().output_len() < self.outer.algorithm.block_len()); + let inner = self.inner.try_finish(cpu_features)?; let inner = inner.as_ref(); let num_pending = inner.len(); @@ -351,9 +359,24 @@ impl Context { const _BUFFER_IS_LARGE_ENOUGH_TO_HOLD_INNER: () = assert!(digest::MAX_OUTPUT_LEN < digest::MAX_BLOCK_LEN); buffer[..num_pending].copy_from_slice(inner); + self.outer .try_finish(buffer, num_pending, cpu_features) .map(Tag) + .map_err(|err| match err { + FinishError::InputTooLong(i) => { + // Unreachable, as we gave the inner context exactly the + // same input we gave the outer context, and + // `inner.try_finish` already succeeded. However, it is + // quite difficult to prove this, and we already return + // `InputTooLongError`, so just forward it along. + i + } + FinishError::PendingNotAPartialBlock(_) => { + // Follows from the assertions above. + unreachable!() + } + }) } } @@ -365,7 +388,7 @@ impl Context { /// return value of `sign` to a tag. Use `verify` for verification instead. pub fn sign(key: &Key, data: &[u8]) -> Tag { key.sign(data, cpu::features()) - .map_err(error::Unspecified::from) + .map_err(error::erase::) .unwrap() } @@ -382,8 +405,14 @@ pub fn verify(key: &Key, data: &[u8], tag: &[u8]) -> Result<(), error::Unspecifi } enum VerifyError { + // Theoretically somebody could have calculated a valid tag with a gigantic + // input that we do not support. If we were to support every theoretically + // valid input length, for *every* digest algorithm, then we could argue + // that hitting the input length limit implies a mismatch since nobody + // could have calculated such a tag with the given input. #[allow(dead_code)] - DigestError(digest::FinishError), + InputTooLongError(InputTooLongError), + Mismatch, }