Skip to content

Commit

Permalink
arithmetic/limbs internals: Clarify limbs < limbs checks.
Browse files Browse the repository at this point in the history
Reject empty or non-matching-length inputs. Refactor the internal
API so that it is clearer that we're nearly always using this
function to reject too-large inputs. This makes it easier to see
that it is OK to return an error when previously we didn't.

This isn't intended to have any effect other than making the code
easier to audit.
  • Loading branch information
briansmith committed Jan 23, 2025
1 parent 1371308 commit b50f143
Show file tree
Hide file tree
Showing 5 changed files with 47 additions and 22 deletions.
4 changes: 1 addition & 3 deletions src/arithmetic/bigint/boxed_limbs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -88,9 +88,7 @@ impl<M> BoxedLimbs<M> {
) -> Result<Self, error::Unspecified> {
let mut r = Self::zero(m.limbs().len());
limb::parse_big_endian_and_pad_consttime(input, &mut r)?;
if !limb::limbs_less_than_limbs_consttime(&r, m.limbs()).leak() {
return Err(error::Unspecified);
}
limb::verify_limbs_less_than_limbs_leak_bit(&r, m.limbs())?;
Ok(r)
}

Expand Down
8 changes: 4 additions & 4 deletions src/arithmetic/bigint/modulusvalue.rs
Original file line number Diff line number Diff line change
Expand Up @@ -59,12 +59,12 @@ impl<M> OwnedModulusValue<M> {
}

pub fn verify_less_than<L>(&self, l: &Modulus<L>) -> Result<(), error::Unspecified> {
if self.len_bits() > l.len_bits()
|| (self.limbs.len() == l.limbs().len()
&& !limb::limbs_less_than_limbs_consttime(&self.limbs, l.limbs()).leak())
{
if self.len_bits() > l.len_bits() {
return Err(error::Unspecified);
}
if self.limbs.len() == l.limbs().len() {
limb::verify_limbs_less_than_limbs_leak_bit(&self.limbs, l.limbs())?;
}
Ok(())
}

Expand Down
4 changes: 1 addition & 3 deletions src/ec/curve25519/scalar.rs
Original file line number Diff line number Diff line change
Expand Up @@ -32,9 +32,7 @@ impl Scalar {
debug_assert!(_empty.is_empty());
let limbs: [limb::Limb; SCALAR_LEN / limb::LIMB_BYTES] =
array::from_fn(|i| limb::Limb::from_le_bytes(limbs_as_bytes[i]));
if !limb::limbs_less_than_limbs_consttime(&limbs, &order).leak() {
return Err(error::Unspecified);
}
limb::verify_limbs_less_than_limbs_leak_bit(&limbs, &order)?;

Ok(Self(bytes))
}
Expand Down
8 changes: 6 additions & 2 deletions src/ec/suite_b/ops.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,11 @@
// CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.

use crate::{
arithmetic::limbs_from_hex, arithmetic::montgomery::*, constant_time::LeakyWord, cpu, error,
arithmetic::limbs_from_hex,
arithmetic::montgomery::*,
constant_time::LeakyWord,
cpu,
error::{self, LenMismatchError},
limb::*,
};
use core::marker::PhantomData;
Expand Down Expand Up @@ -437,8 +441,8 @@ impl Modulus<Q> {

pub fn elem_less_than_vartime(&self, a: &Elem<Unencoded>, b: &PublicElem<Unencoded>) -> bool {
let num_limbs = self.num_limbs.into();
// TODO: let b = Elem::from(b);
limbs_less_than_limbs_vartime(&a.limbs[..num_limbs], &b.limbs[..num_limbs])
.unwrap_or_else(|LenMismatchError { .. }| unreachable!())
}
}

Expand Down
45 changes: 35 additions & 10 deletions src/limb.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,11 @@
//! limbs use the native endianness.
use crate::{
c, constant_time, error,
c, constant_time,
error::{self, LenMismatchError},
polyfill::{slice, usize_from_u32, ArrayFlatMap},
};
use core::num::NonZeroUsize;

#[cfg(any(test, feature = "alloc"))]
use crate::bits;
Expand All @@ -48,17 +50,42 @@ pub fn limbs_equal_limbs_consttime(a: &[Limb], b: &[Limb]) -> LimbMask {
}

#[inline]
pub fn limbs_less_than_limbs_consttime(a: &[Limb], b: &[Limb]) -> LimbMask {
fn limbs_less_than_limbs_constant_time(
a: &[Limb],
b: &[Limb],
) -> Result<LimbMask, LenMismatchError> {
prefixed_extern! {
fn LIMBS_less_than(a: *const Limb, b: *const Limb, num_limbs: c::size_t) -> LimbMask;
fn LIMBS_less_than(a: *const Limb, b: *const Limb, num_limbs: c::NonZero_size_t)
-> LimbMask;
}
// Use `b.len` because usually `b` will be the modulus which is likely to
// have had its length checked already so this can be elided by the
// optimizer.
// XXX: Questionable whether `LenMismatchError` is appropriate.
let len = NonZeroUsize::new(b.len()).ok_or_else(|| LenMismatchError::new(a.len()))?;
if a.len() != len.into() {
return Err(LenMismatchError::new(a.len()));

Check warning on line 67 in src/limb.rs

View check run for this annotation

Codecov / codecov/patch

src/limb.rs#L67

Added line #L67 was not covered by tests
}
Ok(unsafe { LIMBS_less_than(a.as_ptr(), b.as_ptr(), len) })
}

#[inline]
pub(crate) fn verify_limbs_less_than_limbs_leak_bit(
a: &[Limb],
b: &[Limb],
) -> Result<(), error::Unspecified> {
let r = limbs_less_than_limbs_constant_time(a, b).map_err(error::erase::<LenMismatchError>)?;
if r.leak() {
Ok(())
} else {
Err(error::Unspecified)
}
assert_eq!(a.len(), b.len());
unsafe { LIMBS_less_than(a.as_ptr(), b.as_ptr(), b.len()) }
}

#[inline]
pub fn limbs_less_than_limbs_vartime(a: &[Limb], b: &[Limb]) -> bool {
limbs_less_than_limbs_consttime(a, b).leak()
pub fn limbs_less_than_limbs_vartime(a: &[Limb], b: &[Limb]) -> Result<bool, LenMismatchError> {
let r = limbs_less_than_limbs_constant_time(a, b)?;
Ok(r.leak())
}

#[inline]
Expand Down Expand Up @@ -148,9 +175,7 @@ pub fn parse_big_endian_in_range_and_pad_consttime(
result: &mut [Limb],
) -> Result<(), error::Unspecified> {
parse_big_endian_and_pad_consttime(input, result)?;
if !limbs_less_than_limbs_consttime(result, max_exclusive).leak() {
return Err(error::Unspecified);
}
verify_limbs_less_than_limbs_leak_bit(result, max_exclusive)?;
if allow_zero != AllowZero::Yes {
if limbs_are_zero_constant_time(result).leak() {
return Err(error::Unspecified);
Expand Down

0 comments on commit b50f143

Please sign in to comment.