pbkdf2: Clarify error handling and panic conditions.

src/error/mod.rs

@@ -16,11 +16,14 @@
 
 pub use self::{key_rejected::KeyRejected, unspecified::Unspecified};
 
-pub(crate) use self::input_too_long::InputTooLongError;
+pub(crate) use self::{
+    input_too_long::InputTooLongError, too_much_output_requested::TooMuchOutputRequestedError,
+};
 
 mod input_too_long;
 mod into_unspecified;
 mod key_rejected;
+mod too_much_output_requested;
 mod unspecified;
 
 #[cold]

src/error/too_much_output_requested.rs

@@ -0,0 +1,39 @@
+// Copyright 2024 Brian Smith.
+//
+// Permission to use, copy, modify, and/or distribute this software for any
+// purpose with or without fee is hereby granted, provided that the above
+// copyright notice and this permission notice appear in all copies.
+//
+// THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHORS DISCLAIM ALL WARRANTIES
+// WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
+// MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHORS BE LIABLE FOR ANY
+// SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
+// WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION
+// OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN
+// CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
+
+pub struct TooMuchOutputRequestedError {
+    /// Note that this might not actually be the (exact) length of the input,
+    /// and its units might be lost. For example, it could be any of the
+    /// following:
+    ///
+    ///    * The length in bytes of the entire input.
+    ///    * The length in bytes of some *part* of the input.
+    ///    * A bit length.
+    ///    * A length in terms of "blocks" or other grouping of input values.
+    ///    * Some intermediate quantity that was used when checking the input
+    ///      length.
+    ///    * Some arbitrary value.
+    #[allow(dead_code)]
+    imprecise_output_length: usize,
+}
+
+impl TooMuchOutputRequestedError {
+    #[cold]
+    #[inline(never)]
+    pub(crate) fn new(imprecise_output_length: usize) -> Self {
+        Self {
+            imprecise_output_length,
+        }
+    }
+}

src/hmac.rs

@@ -223,7 +223,7 @@ impl Key {
             .unwrap()
     }
 
-    fn try_new(
+    pub(crate) fn try_new(
         algorithm: Algorithm,
         key_value: &[u8],
         cpu_features: cpu::Features,
@@ -274,7 +274,11 @@ impl Key {
         Algorithm(self.inner.algorithm)
     }
 
-    fn sign(&self, data: &[&[u8]], cpu: cpu::Features) -> Result<Tag, InputTooLongError> {
+    pub(crate) fn sign(
+        &self,
+        data: &[&[u8]],
+        cpu: cpu::Features,
+    ) -> Result<Tag, InputTooLongError> {
         let mut ctx = Context::with_key(self);
         data.iter().for_each(|data| ctx.update(data));
         ctx.try_sign(cpu)

src/pbkdf2.rs

@@ -112,7 +112,12 @@
 //!     assert!(db.verify_password("alice", "@74d7]404j|W}6u").is_ok());
 //! }
 
-use crate::{constant_time, digest, error, hmac};
+use self::{derive_error::DeriveError, verify_error::VerifyError};
+use crate::{
+    constant_time, cpu, digest,
+    error::{self, TooMuchOutputRequestedError},
+    hmac::{self, InputTooLongError},
+};
 use core::num::NonZeroU32;
 
 /// A PBKDF2 algorithm.
@@ -150,15 +155,39 @@ pub static PBKDF2_HMAC_SHA512: Algorithm = Algorithm(hmac::HMAC_SHA512);
 ///
 /// # Panics
 ///
-/// `derive` panics if `out.len()` is larger than (2**32 - 1) * the digest
-/// algorithm's output length, per the PBKDF2 specification.
+/// Panics if `out.len()` is larger than (2**32 - 1) * the digest algorithm's
+/// output length, per the PBKDF2 specification.
+///
+/// Panics if `salt` is so astronomically gigantic that it isn't a valid input
+/// to the underlying digest function.
+///
+/// Panics if `secret` is so astronomically gigantic that it isn't a valid
+/// input to the underlying digest function.
 pub fn derive(
     algorithm: Algorithm,
     iterations: NonZeroU32,
     salt: &[u8],
     secret: &[u8],
     out: &mut [u8],
 ) {
+    let cpu = cpu::features();
+    try_derive(algorithm, iterations, salt, secret, out, cpu)
+        .map_err(|err| match err {
+            DeriveError::SecretTooLong(_)
+            | DeriveError::SaltTooLong(_)
+            | DeriveError::TooMuchOutputRequested(_) => error::Unspecified,
+        })
+        .unwrap()
+}
+
+fn try_derive(
+    algorithm: Algorithm,
+    iterations: NonZeroU32,
+    salt: &[u8],
+    secret: &[u8],
+    out: &mut [u8],
+    cpu: cpu::Features,
+) -> Result<(), DeriveError> {
     let digest_alg = algorithm.0.digest_algorithm();
     let output_len = digest_alg.output_len();
 
@@ -167,27 +196,38 @@ pub fn derive(
     // hasn't been optimized to the same extent as fastpbkdf2. In particular,
     // this implementation is probably doing a lot of unnecessary copying.
 
-    let secret = hmac::Key::new(algorithm.0, secret);
+    let secret =
+        hmac::Key::try_new(algorithm.0, secret, cpu).map_err(DeriveError::secret_too_long)?;
 
     // Clear |out|.
     out.fill(0);
 
     let mut idx: u32 = 0;
 
+    let out_len = out.len();
     for chunk in out.chunks_mut(output_len) {
-        idx = idx.checked_add(1).expect("derived key too long");
-        derive_block(&secret, iterations, salt, idx, chunk);
+        idx = idx.checked_add(1).ok_or_else(|| {
+            // XXX: It would be better to check this ahead of time and avoid
+            // writing any output in the error case.
+            DeriveError::too_much_output_requested(TooMuchOutputRequestedError::new(out_len))
+        })?;
+        derive_block(&secret, iterations, salt, idx, chunk, cpu)
+            .map_err(DeriveError::salt_too_long)?;
     }
+    Ok(())
 }
 
-fn derive_block(secret: &hmac::Key, iterations: NonZeroU32, salt: &[u8], idx: u32, out: &mut [u8]) {
-    let mut ctx = hmac::Context::with_key(secret);
-    ctx.update(salt);
-    ctx.update(&u32::to_be_bytes(idx));
-
-    let mut u = ctx.sign();
+fn derive_block(
+    secret: &hmac::Key,
+    iterations: NonZeroU32,
+    salt: &[u8],
+    idx: u32,
+    out: &mut [u8],
+    cpu: cpu::Features,
+) -> Result<(), InputTooLongError> {
+    let mut u = secret.sign(&[salt, &u32::to_be_bytes(idx)], cpu)?;
 
-    let mut remaining: u32 = iterations.into();
+    let mut remaining: u32 = iterations.get();
     loop {
         constant_time::xor_assign_at_start(&mut out[..], u.as_ref());
 
@@ -196,7 +236,32 @@ fn derive_block(secret: &hmac::Key, iterations: NonZeroU32, salt: &[u8], idx: u3
         }
         remaining -= 1;
 
-        u = hmac::sign(secret, u.as_ref());
+        u = secret
+            .sign(&[u.as_ref()], cpu)
+            .map_err(|err: InputTooLongError| {
+                // Unreachable because the output of HMAC is never too long to
+                // be an input for the same algorithm, but we can't prove with only
+                // locally-available information.
+                err
+            })?;
+    }
+    Ok(())
+}
+
+cold_exhaustive_error! {
+    enum derive_error::DeriveError {
+        secret_too_long => SecretTooLong(InputTooLongError),
+        salt_too_long => SaltTooLong(InputTooLongError),
+        too_much_output_requested => TooMuchOutputRequested(TooMuchOutputRequestedError),
+    }
+}
+
+cold_exhaustive_error! {
+    enum verify_error::VerifyError {
+        mismatch => Mismatch(()),
+        secret_too_long => SecretTooLong(InputTooLongError),
+        salt_too_long => SaltTooLong(InputTooLongError),
+        previously_derived_empty => PreviouslyDerivedEmpty(usize),
     }
 }
 
@@ -215,39 +280,53 @@ fn derive_block(secret: &hmac::Key, iterations: NonZeroU32, salt: &[u8], idx: u3
 /// | `secret`                   | P (password)
 /// | `previously_derived`       | dk (derived key)
 /// | `previously_derived.len()` | dkLen (derived key length)
-///
-/// # Panics
-///
-/// `verify` panics if `out.len()` is larger than (2**32 - 1) * the digest
-/// algorithm's output length, per the PBKDF2 specification.
 pub fn verify(
     algorithm: Algorithm,
     iterations: NonZeroU32,
     salt: &[u8],
     secret: &[u8],
     previously_derived: &[u8],
 ) -> Result<(), error::Unspecified> {
+    let cpu = cpu::features();
+    try_verify(algorithm, iterations, salt, secret, previously_derived, cpu)
+        .map_err(error::erase::<VerifyError>)
+}
+
+fn try_verify(
+    algorithm: Algorithm,
+    iterations: NonZeroU32,
+    salt: &[u8],
+    secret: &[u8],
+    previously_derived: &[u8],
+    cpu: cpu::Features,
+) -> Result<(), VerifyError> {
     let digest_alg = algorithm.0.digest_algorithm();
 
     if previously_derived.is_empty() {
-        return Err(error::Unspecified);
+        return Err(VerifyError::previously_derived_empty(0));
     }
 
     let mut derived_buf = [0u8; digest::MAX_OUTPUT_LEN];
 
     let output_len = digest_alg.output_len();
-    let secret = hmac::Key::new(algorithm.0, secret);
+    let secret =
+        hmac::Key::try_new(algorithm.0, secret, cpu).map_err(VerifyError::secret_too_long)?;
     let mut idx: u32 = 0;
 
     let mut matches = 1;
 
     for previously_derived_chunk in previously_derived.chunks(output_len) {
-        idx = idx.checked_add(1).expect("derived key too long");
+        idx = idx.checked_add(1).ok_or_else(|| {
+            // `previously_derived` is so gigantic that PBKDF2 couldn't
+            // have been used to compute it.
+            VerifyError::mismatch(())
+        })?;
 
         let derived_chunk = &mut derived_buf[..previously_derived_chunk.len()];
         derived_chunk.fill(0);
 
-        derive_block(&secret, iterations, salt, idx, derived_chunk);
+        derive_block(&secret, iterations, salt, idx, derived_chunk, cpu)
+            .map_err(VerifyError::salt_too_long)?;
 
         // XXX: This isn't fully constant-time-safe. TODO: Fix that.
         #[allow(clippy::bool_to_int_with_if)]
@@ -264,7 +343,7 @@ pub fn verify(
     }
 
     if matches == 0 {
-        return Err(error::Unspecified);
+        return Err(VerifyError::mismatch(()));
     }
 
     Ok(())