From a466b53b4ad76d1974ee70918c104940b201de0d Mon Sep 17 00:00:00 2001 From: Joshua Liebow-Feeser Date: Thu, 28 Sep 2023 19:50:34 +0000 Subject: [PATCH] Document or remove some uses of `unsafe` --- Cargo.toml | 1 + src/aead/gcm.rs | 6 +++++ src/digest.rs | 50 +++++++++++++++++++++++++++++--------- src/endian.rs | 24 +++++++++++++++--- src/rsa/public_exponent.rs | 15 +++++++++--- 5 files changed, 78 insertions(+), 18 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 9e1791d81..b1c892e44 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -166,6 +166,7 @@ name = "ring" [dependencies] getrandom = { version = "0.2.8" } untrusted = { version = "0.9" } +zerocopy = { version = "0.7.5", features = ["derive"] } [target.'cfg(any(target_arch = "x86",target_arch = "x86_64", all(any(target_arch = "aarch64", target_arch = "arm"), any(target_os = "android", target_os = "fuchsia", target_os = "linux", target_os = "windows"))))'.dependencies] spin = { version = "0.9.2", default-features = false, features = ["once"] } diff --git a/src/aead/gcm.rs b/src/aead/gcm.rs index 45737401c..3b04c7077 100644 --- a/src/aead/gcm.rs +++ b/src/aead/gcm.rs @@ -126,6 +126,12 @@ impl Context { debug_assert!(input_bytes > 0); let input = input.as_ptr() as *const [u8; BLOCK_LEN]; + // SAFETY: + // - `[[u8; BLOCK_LEN]]` has the same bit validity as `[u8]`. + // - `[[u8; BLOCK_LEN]]` has the same alignment requirement as `[u8]`. + // - `input_bytes / BLOCK_LEN` ensures that the total length in bytes of + // the new `[[u8; BLOCK_LEN]]` will not be longer than the original + // `[u8]`. let input = unsafe { core::slice::from_raw_parts(input, input_bytes / BLOCK_LEN) }; // Although these functions take `Xi` and `h_table` as separate diff --git a/src/digest.rs b/src/digest.rs index 0d6f67af8..98dd6f46e 100644 --- a/src/digest.rs +++ b/src/digest.rs @@ -24,12 +24,9 @@ // The goal for this implementation is to drive the overhead as close to zero // as possible. -use crate::{ - c, cpu, debug, - endian::{ArrayEncoding, BigEndian}, - polyfill, -}; +use crate::{c, cpu, debug, endian::BigEndian, polyfill}; use core::num::Wrapping; +use zerocopy::{AsBytes, FromBytes}; mod sha1; mod sha2; @@ -114,7 +111,9 @@ impl BlockContext { Digest { algorithm: self.algorithm, - value: (self.algorithm.format_output)(self.state), + // SAFETY: Invariant on this field promises that this is the correct + // format function for this algorithm's block size. + value: unsafe { (self.algorithm.format_output)(self.state) }, } } } @@ -248,8 +247,11 @@ impl Digest { impl AsRef<[u8]> for Digest { #[inline(always)] fn as_ref(&self) -> &[u8] { - let as64 = unsafe { &self.value.as64 }; - &as64.as_byte_array()[..self.algorithm.output_len] + let data = (&self.value as *const Output).cast::(); + // SAFETY: So long as `self.algorithm` is the correct algorithm, all + // code initializes all bytes of `self.value` in the range `[0, + // self.algorithm.output_len)`. + unsafe { core::slice::from_raw_parts(data, self.algorithm.output_len) } } } @@ -270,7 +272,9 @@ pub struct Algorithm { len_len: usize, block_data_order: unsafe extern "C" fn(state: &mut State, data: *const u8, num: c::size_t), - format_output: fn(input: State) -> Output, + // INVARIANT: This is always set to the correct output for the algorithm's + // block size. + format_output: unsafe fn(input: State) -> Output, initial_state: State, @@ -474,14 +478,38 @@ pub const MAX_OUTPUT_LEN: usize = 512 / 8; /// algorithms in this module. pub const MAX_CHAINING_LEN: usize = MAX_OUTPUT_LEN; -fn sha256_format_output(input: State) -> Output { +fn sha256_format_output(input: State) -> Output +where + [BigEndian; 256 / 8 / core::mem::size_of::>()]: FromBytes, + [BigEndian; 512 / 8 / core::mem::size_of::>()]: AsBytes, +{ + // SAFETY: There are two cases: + // - The union is initialized as `as32`, in which case this is trivially + // sound. + // - The union is initialized as `as64`. In this case, the `as64` variant is + // longer than the `as32` variant, so all bytes of `as32` are initialized + // as they are in the prefix of `as64`. Since `as64`'s type is `AsBytes` + // (see the where bound on this function), all of its bytes are + // initialized (ie, none are padding). Since `as32`'s type is `FromBytes`, + // any initialized sequence of bytes constitutes a valid instance of the + // type, so this is sound. let input = unsafe { &input.as32 }; Output { as32: input.map(BigEndian::from), } } -fn sha512_format_output(input: State) -> Output { +/// # Safety +/// +/// The caller must ensure that all bytes of `State` have been initialized. +unsafe fn sha512_format_output(input: State) -> Output +where + [BigEndian; 512 / 8 / core::mem::size_of::>()]: FromBytes, +{ + // SAFETY: Caller has promised that all bytes are initialized. Since + // `input.as64` is `FromBytes`, we know that this is sufficient to guarantee + // that the input has been initialized to a valid instance of the type of + // `input.as64`. let input = unsafe { &input.as64 }; Output { as64: input.map(BigEndian::from), diff --git a/src/endian.rs b/src/endian.rs index 88d20e3e7..9ccfc859a 100644 --- a/src/endian.rs +++ b/src/endian.rs @@ -1,10 +1,12 @@ -use core::num::Wrapping; +use core::{mem, num::Wrapping}; + +use zerocopy::{AsBytes, FromBytes, FromZeroes}; /// An `Encoding` of a type `T` can be converted to/from its byte /// representation without any byte swapping or other computation. /// /// The `Self: Copy` constraint addresses `clippy::declare_interior_mutable_const`. -pub trait Encoding: From + Into +pub trait Encoding: From + Into + FromBytes + AsBytes where Self: Copy, { @@ -25,7 +27,7 @@ pub trait FromByteArray { macro_rules! define_endian { ($endian:ident) => { - #[derive(Clone, Copy)] + #[derive(Clone, Copy, FromZeroes, FromBytes, AsBytes)] #[repr(transparent)] pub struct $endian(T); @@ -48,7 +50,7 @@ macro_rules! impl_from_byte_array { { #[inline] fn from_byte_array(a: &[u8; $elems * core::mem::size_of::<$base>()]) -> Self { - unsafe { core::mem::transmute_copy(a) } + zerocopy::transmute!(*a) } } }; @@ -58,11 +60,25 @@ macro_rules! impl_array_encoding { ($endian:ident, $base:ident, $elems:expr) => { impl ArrayEncoding<[u8; $elems * core::mem::size_of::<$base>()]> for [$endian<$base>; $elems] + where + Self: AsBytes, { #[inline] fn as_byte_array(&self) -> &[u8; $elems * core::mem::size_of::<$base>()] { + // This is a sanity check; it should never fail. + assert_eq!( + mem::size_of::(), + $elems * core::mem::size_of::<$base>() + ); let as_bytes_ptr = self.as_ptr() as *const [u8; $elems * core::mem::size_of::<$base>()]; + // SAFETY: + // - `Self: AsBytes`, so it's sound to observe the bytes of + // `self` and to have a reference to those bytes alive at the + // same time as `&self`. + // - As confirmed by the preceding assertion, the sizes of + // `Self` and the return type are equal. + // - `[u8; N]` has no alignment requirement. unsafe { &*as_bytes_ptr } } } diff --git a/src/rsa/public_exponent.rs b/src/rsa/public_exponent.rs index 26f865452..0c1d633c5 100644 --- a/src/rsa/public_exponent.rs +++ b/src/rsa/public_exponent.rs @@ -12,8 +12,14 @@ impl PublicExponent { // TODO: Use `NonZeroU64::new(...).unwrap()` when `feature(const_panic)` is // stable. - pub(super) const _3: Self = Self(unsafe { NonZeroU64::new_unchecked(3) }); - pub(super) const _65537: Self = Self(unsafe { NonZeroU64::new_unchecked(65537) }); + pub(super) const _3: Self = Self(match NonZeroU64::new(3) { + Some(nz) => nz, + None => unreachable!(), + }); + pub(super) const _65537: Self = Self(match NonZeroU64::new(65537) { + Some(nz) => nz, + None => unreachable!(), + }); // This limit was chosen to bound the performance of the simple // exponentiation-by-squaring implementation in `elem_exp_vartime`. In @@ -29,7 +35,10 @@ impl PublicExponent { // // TODO: Use `NonZeroU64::new(...).unwrap()` when `feature(const_panic)` is // stable. - const MAX: Self = Self(unsafe { NonZeroU64::new_unchecked((1u64 << 33) - 1) }); + const MAX: Self = Self(match NonZeroU64::new((1u64 << 33) - 1) { + Some(nz) => nz, + None => unreachable!(), + }); pub(super) fn from_be_bytes( input: untrusted::Input,