Skip to content

Commit

Permalink
error handling: Refactor how "Input too long" errors are handled.
Browse files Browse the repository at this point in the history
Handle "input too long" errors consistently across the `digest` and
`bits` modules. This is a step towards more modules doing the same.
  • Loading branch information
briansmith committed Dec 30, 2024
1 parent 0a00284 commit ba79cc0
Show file tree
Hide file tree
Showing 9 changed files with 121 additions and 58 deletions.
8 changes: 5 additions & 3 deletions src/aead/gcm.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ use self::ffi::{Block, BLOCK_LEN, ZERO_BLOCK};
use super::{aes_gcm, Aad};
use crate::{
bits::{BitLength, FromByteLen as _},
error,
error::{self, InputTooLongError},
polyfill::{sliceutil::overwrite_at_start, NotSend},
};
use cfg_if::cfg_if;
Expand Down Expand Up @@ -57,8 +57,10 @@ impl<'key, K: Gmult> Context<'key, K> {
if in_out_len > aes_gcm::MAX_IN_OUT_LEN {
return Err(error::Unspecified);
}
let in_out_len = BitLength::from_byte_len(in_out_len)?;
let aad_len = BitLength::from_byte_len(aad.as_ref().len())?;
let in_out_len = BitLength::from_byte_len(in_out_len)
.map_err(|_: InputTooLongError| error::Unspecified)?;
let aad_len = BitLength::from_byte_len(aad.as_ref().len())
.map_err(|_: InputTooLongError| error::Unspecified)?;

// NIST SP800-38D Section 5.2.1.1 says that the maximum AAD length is
// 2**64 - 1 bits, i.e. BitLength<u64>::MAX, so we don't need to do an
Expand Down
23 changes: 11 additions & 12 deletions src/bits.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@

//! Bit lengths.
use crate::{error, polyfill};
use crate::{error::InputTooLongError, polyfill};

/// The length of something, in bits.
///
Expand All @@ -27,36 +27,35 @@ pub(crate) trait FromByteLen<T>: Sized {
/// Constructs a `BitLength` from the given length in bytes.
///
/// Fails if `bytes * 8` is too large for a `T`.
fn from_byte_len(bytes: T) -> Result<Self, error::Unspecified>;
fn from_byte_len(bytes: T) -> Result<Self, InputTooLongError<T>>;
}

impl FromByteLen<usize> for BitLength<usize> {
#[inline]
fn from_byte_len(bytes: usize) -> Result<Self, error::Unspecified> {
fn from_byte_len(bytes: usize) -> Result<Self, InputTooLongError> {
match bytes.checked_mul(8) {
Some(bits) => Ok(Self(bits)),
None => Err(error::Unspecified),
None => Err(InputTooLongError::new(bytes)),
}
}
}

impl FromByteLen<u64> for BitLength<u64> {
#[inline]
fn from_byte_len(bytes: u64) -> Result<Self, error::Unspecified> {
fn from_byte_len(bytes: u64) -> Result<Self, InputTooLongError<u64>> {
match bytes.checked_mul(8) {
Some(bits) => Ok(Self(bits)),
None => Err(error::Unspecified),
None => Err(InputTooLongError::new(bytes)),
}
}
}

impl FromByteLen<usize> for BitLength<u64> {
#[inline]
fn from_byte_len(bytes: usize) -> Result<Self, error::Unspecified> {
let bytes = polyfill::u64_from_usize(bytes);
match bytes.checked_mul(8) {
fn from_byte_len(bytes: usize) -> Result<Self, InputTooLongError<usize>> {
match polyfill::u64_from_usize(bytes).checked_mul(8) {
Some(bits) => Ok(Self(bits)),
None => Err(error::Unspecified),
None => Err(InputTooLongError::new(bytes)),
}
}
}
Expand Down Expand Up @@ -102,8 +101,8 @@ impl BitLength<usize> {

#[cfg(feature = "alloc")]
#[inline]
pub(crate) fn try_sub_1(self) -> Result<Self, error::Unspecified> {
let sum = self.0.checked_sub(1).ok_or(error::Unspecified)?;
pub(crate) fn try_sub_1(self) -> Result<Self, crate::error::Unspecified> {
let sum = self.0.checked_sub(1).ok_or(crate::error::Unspecified)?;
Ok(Self(sum))
}
}
Expand Down
20 changes: 12 additions & 8 deletions src/digest.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,8 @@ use self::{
};
use crate::{
bits::{BitLength, FromByteLen as _},
cpu, debug, error,
cpu, debug,
error::{self, InputTooLongError},
polyfill::{self, slice, sliceutil},
};
use core::num::Wrapping;
Expand Down Expand Up @@ -80,12 +81,15 @@ impl BlockContext {
num_pending: usize,
cpu_features: cpu::Features,
) -> Result<Digest, FinishError> {
let completed_bytes = self
let completed_bits = self
.completed_bytes
.checked_add(polyfill::u64_from_usize(num_pending))
.ok_or_else(|| FinishError::too_much_input(self.completed_bytes))?;
let completed_bits = BitLength::from_byte_len(completed_bytes)
.map_err(|_: error::Unspecified| FinishError::too_much_input(self.completed_bytes))?;
.ok_or_else(|| {
// Choosing self.completed_bytes here is lossy & somewhat arbitrary.
InputTooLongError::new(self.completed_bytes)

Check warning on line 89 in src/digest.rs

View check run for this annotation

Codecov / codecov/patch

src/digest.rs#L88-L89

Added lines #L88 - L89 were not covered by tests
})
.and_then(BitLength::from_byte_len)
.map_err(FinishError::input_too_long)?;

let block_len = self.algorithm.block_len();
let block = &mut block[..block_len];
Expand Down Expand Up @@ -143,16 +147,16 @@ impl BlockContext {

pub(crate) enum FinishError {
#[allow(dead_code)]
TooMuchInput(u64),
InputTooLong(InputTooLongError<u64>),
#[allow(dead_code)]
PendingNotAPartialBlock(usize),
}

impl FinishError {
#[cold]
#[inline(never)]
fn too_much_input(completed_bytes: u64) -> Self {
Self::TooMuchInput(completed_bytes)
fn input_too_long(source: InputTooLongError<u64>) -> Self {
Self::InputTooLong(source)
}

// unreachable
Expand Down
39 changes: 39 additions & 0 deletions src/error/input_too_long.rs
Original file line number Diff line number Diff line change
@@ -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 InputTooLongError<T = usize> {
/// 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_input_length: T,
}

impl<T> InputTooLongError<T> {
#[cold]
#[inline(never)]
pub(crate) fn new(imprecise_input_length: T) -> Self {
Self {
imprecise_input_length,
}
}
}
12 changes: 6 additions & 6 deletions src/error/into_unspecified.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,19 +15,19 @@
use crate::error::{KeyRejected, Unspecified};

impl From<untrusted::EndOfInput> for Unspecified {
fn from(_: untrusted::EndOfInput) -> Self {
Self
fn from(source: untrusted::EndOfInput) -> Self {
super::erase(source)
}
}

impl From<core::array::TryFromSliceError> for Unspecified {
fn from(_: core::array::TryFromSliceError) -> Self {
Self
fn from(source: core::array::TryFromSliceError) -> Self {
super::erase(source)
}
}

impl From<KeyRejected> for Unspecified {
fn from(_: KeyRejected) -> Self {
Self
fn from(source: KeyRejected) -> Self {
super::erase(source)
}
}
9 changes: 9 additions & 0 deletions src/error/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,15 @@
pub use self::{key_rejected::KeyRejected, unspecified::Unspecified};

pub(crate) use self::input_too_long::InputTooLongError;

mod input_too_long;
mod into_unspecified;
mod key_rejected;
mod unspecified;

#[cold]
#[inline(never)]
pub(crate) fn erase<T>(_: T) -> Unspecified {
Unspecified
}
5 changes: 3 additions & 2 deletions src/rsa/public_modulus.rs
Original file line number Diff line number Diff line change
Expand Up @@ -45,8 +45,9 @@ impl PublicModulus {
// the public modulus to be exactly 2048 or 3072 bits, but we are more
// flexible to be compatible with other commonly-used crypto libraries.
assert!(min_bits >= MIN_BITS);
let bits_rounded_up =
bits::BitLength::from_byte_len(bits.as_usize_bytes_rounded_up()).unwrap(); // TODO: safe?
let bits_rounded_up = bits::BitLength::from_byte_len(bits.as_usize_bytes_rounded_up())
.map_err(error::erase)
.unwrap(); // TODO: safe?
if bits_rounded_up < min_bits {
return Err(error::KeyRejected::too_small());
}
Expand Down
2 changes: 1 addition & 1 deletion src/rsa/verification.rs
Original file line number Diff line number Diff line change
Expand Up @@ -201,7 +201,7 @@ pub(crate) fn verify_rsa_(
cpu_features: cpu::Features,
) -> Result<(), error::Unspecified> {
let max_bits: bits::BitLength =
bits::BitLength::from_byte_len(PUBLIC_KEY_PUBLIC_MODULUS_MAX_LEN)?;
bits::BitLength::from_byte_len(PUBLIC_KEY_PUBLIC_MODULUS_MAX_LEN).map_err(error::erase)?;

// XXX: FIPS 186-4 seems to indicate that the minimum
// exponent value is 2**16 + 1, but it isn't clear if this is just for
Expand Down
61 changes: 35 additions & 26 deletions src/tests/bits_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,55 +13,64 @@
// CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.

use crate::{
bits::{BitLength, FromByteLen as _},
polyfill::u64_from_usize,
{
bits::{BitLength, FromByteLen as _},
error,
},
};

#[test]
fn test_from_byte_len_overflow() {
const USIZE_MAX_VALID_BYTES: usize = usize::MAX / 8;

// Maximum valid input for BitLength<usize>.
{
let bits = BitLength::<usize>::from_byte_len(USIZE_MAX_VALID_BYTES).unwrap();
assert_eq!(bits.as_usize_bytes_rounded_up(), USIZE_MAX_VALID_BYTES);
assert_eq!(bits.as_bits(), usize::MAX & !0b111);
match BitLength::<usize>::from_byte_len(USIZE_MAX_VALID_BYTES) {
Ok(bits) => {
assert_eq!(bits.as_usize_bytes_rounded_up(), USIZE_MAX_VALID_BYTES);
assert_eq!(bits.as_bits(), usize::MAX & !0b111);
}
Err(_) => {
unreachable!()

Check warning on line 31 in src/tests/bits_tests.rs

View check run for this annotation

Codecov / codecov/patch

src/tests/bits_tests.rs#L31

Added line #L31 was not covered by tests
}
}

// Minimum invalid usize input for BitLength<usize>.
assert_eq!(
assert!(matches!(
BitLength::<usize>::from_byte_len(USIZE_MAX_VALID_BYTES + 1),
Err(error::Unspecified)
);
Err(_)
));

// Minimum invalid usize input for BitLength<u64> on 64-bit targets.
{
let bits = BitLength::<u64>::from_byte_len(USIZE_MAX_VALID_BYTES + 1);
let r = BitLength::<u64>::from_byte_len(USIZE_MAX_VALID_BYTES + 1);
if cfg!(target_pointer_width = "64") {
assert_eq!(bits, Err(error::Unspecified));
assert!(matches!(r, Err(_)));
} else {
let bits = bits.unwrap();
assert_eq!(
bits.as_bits(),
(u64_from_usize(USIZE_MAX_VALID_BYTES) + 1) * 8
);
match r {
Ok(bits) => {
assert_eq!(
bits.as_bits(),
(u64_from_usize(USIZE_MAX_VALID_BYTES) + 1) * 8
);
}
Err(_) => {
unreachable!()

Check warning on line 55 in src/tests/bits_tests.rs

View check run for this annotation

Codecov / codecov/patch

src/tests/bits_tests.rs#L55

Added line #L55 was not covered by tests
}
}
}
}

const U64_MAX_VALID_BYTES: u64 = u64::MAX / 8;

// Maximum valid u64 input for BitLength<u64>.
{
let bits = BitLength::<u64>::from_byte_len(U64_MAX_VALID_BYTES).unwrap();
assert_eq!(bits.as_bits(), u64::MAX & !0b111);
}
match BitLength::<u64>::from_byte_len(U64_MAX_VALID_BYTES) {
Ok(bits) => assert_eq!(bits.as_bits(), u64::MAX & !0b111),
Err(_) => {
unreachable!()

Check warning on line 67 in src/tests/bits_tests.rs

View check run for this annotation

Codecov / codecov/patch

src/tests/bits_tests.rs#L67

Added line #L67 was not covered by tests
}
};

// Minimum invalid usize input for BitLength<u64> on 64-bit targets.
{
let bits = BitLength::<u64>::from_byte_len(U64_MAX_VALID_BYTES + 1);
assert_eq!(bits, Err(error::Unspecified));
}
assert!(matches!(
BitLength::<u64>::from_byte_len(U64_MAX_VALID_BYTES + 1),
Err(_)
));
}

0 comments on commit ba79cc0

Please sign in to comment.