Skip to content

Commit

Permalink
[rust][3p] Fork zeroize to be a no-op.
Browse files Browse the repository at this point in the history
It would be dangerous for Fuchsia engineers to rely on zeroize for
enforcing security invariants, but it is difficult to remove it from
our build graph entirely (see alternatives).

**Background**

zeroize can't handle a variety of cases like a dynamic
buffer which was reallocated partway through writing a secret
into it, or a buffer with a secret small enough for the
containing buffer to implement Copy.

The guarantees it attempts to provide are implemented by
inhibiting compiler optimizations that would eliminate
dead stores, since to LLVM what zeroize does looks like writing to
memory that no one will read. A new LLVM release can always optimize
zeroize out of the resulting binary, meaning that not even a best-
effort attempt to conceal secrets would be made.

Some evidence from upstream that its guarantees can be confusing:

RustCrypto/utils#659
RustCrypto/utils#702

And that they're difficult to implement without invoking UB:

RustCrypto/utils#653

**Alternatives**

*** (1) Make zeroize optional in RustCrypto crates ***

We could fork the crates which depend on zeroize locally and work
with upstream to release versions where the dependency is optional
with the goal of unforking once they were released.

Making the dependency optional requires cargo's weak and namespaced
deps features that were just stabilized in 1.60, while RustCrypto maintains
an MSRV of 1.41, released in February 2020. Bumping MSRV is a
significant action for a widely-used Rust crate, and we should not
expect maintainers to do so lightly or to be able to bump to 1.60 any
time soon.

*** (2) Fork zeroize to remove UB and restrict visibility ***

We could add support to cargo-gnaw for configurable visibility limits
that would allow our transitive RustCrypto crates to use zeroize but not
for it to be added as a dep within the main build graph.

This approach also forks zeroize, but instead of removing all of its
functionality we would fix any UB in the library. From upstream issues
its not clear this can be done in the current semantics of Rust. Even
if it were possible, it will be difficult to be certain we've addressed
all possible UB and this approach is ultimately higher effort than
removing all of the crate's code.

Fixed: 96317
Change-Id: Ia5419d3cf73ef7f971e8a72a56e8ece495078395
Reviewed-on: https://fuchsia-review.googlesource.com/c/fuchsia/+/667952
Reviewed-by: Tyler Mandry <[email protected]>
Reviewed-by: Chris Palmer <[email protected]>
Commit-Queue: Adam Perry <[email protected]>
Fuchsia-Auto-Submit: Adam Perry <[email protected]>
  • Loading branch information
anp authored and Commit Bot committed Apr 11, 2022
1 parent f6be224 commit 8bc610f
Show file tree
Hide file tree
Showing 14 changed files with 202 additions and 638 deletions.
2 changes: 1 addition & 1 deletion third_party/rust_crates/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -13254,7 +13254,7 @@ rust_library("yaml-rust-v0_4_5") {

rust_library("zeroize-v1_3_0") {
crate_name = "zeroize"
crate_root = "//third_party/rust_crates/vendor/zeroize/src/lib.rs"
crate_root = "//third_party/rust_crates/forks/zeroize/src/lib.rs"
output_name = "zeroize-2201a258b0081f1f"
configs -= [
"//build/config/rust:edition_2018",
Expand Down
2 changes: 0 additions & 2 deletions third_party/rust_crates/Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions third_party/rust_crates/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -335,6 +335,7 @@ libm = { path = "forks/libm" }
rustls = { path = "forks/rustls" }
security-framework = { path = "forks/security-framework" }
tracing-core = { path = "forks/tracing-core" }
zeroize = { path = "forks/zeroize" }
zstd-sys = { path = "forks/zstd-sys" }

### Mirrors: forked crates managed as separate git repos
Expand Down
22 changes: 22 additions & 0 deletions third_party/rust_crates/forks/zeroize/README.fuchsia
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
# zeroize fork

## What is this crate used for?

zeroize is a transitive (sometimes optional) dependency of crates in the RustCrypto ecosystem.

* Are there any use restrictions? i.e. only for development hosts

This crate should not be used, this implementation is a no-op that is kept for compatibility with
existing dependents.

* What differs from upstream? Include a changelog if feasible.

Only function signatures and trait definitions are left, no implementation bodies remain.

* Are there any restrictions to how it should be rolled?

Consult with //third_party/rust_crates/OWNERS.

* Is there anything else which makes this dependency "special"?

No.
177 changes: 177 additions & 0 deletions third_party/rust_crates/forks/zeroize/src/lib.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,177 @@
//! This is a no-op implementation of zeroize kept for compatibility. Do not use directly.
#![no_std]

#[cfg(feature = "alloc")]
extern crate alloc;

#[cfg(feature = "zeroize_derive")]
#[cfg_attr(docsrs, doc(cfg(feature = "zeroize_derive")))]
pub use zeroize_derive::Zeroize;

#[cfg(any(target_arch = "x86", target_arch = "x86_64"))]
mod x86;

use core::{ops, ptr, slice::IterMut, sync::atomic};

#[cfg(feature = "alloc")]
use alloc::{boxed::Box, string::String, vec::Vec};

pub trait Zeroize {
fn zeroize(&mut self);
}

pub trait DefaultIsZeroes: Copy + Default + Sized {}

impl<Z> Zeroize for Z
where
Z: DefaultIsZeroes,
{
fn zeroize(&mut self) {}
}

macro_rules! impl_zeroize_with_default {
($($type:ty),+) => {
$(impl DefaultIsZeroes for $type {})+
};
}

impl_zeroize_with_default!(i8, i16, i32, i64, i128, isize);
impl_zeroize_with_default!(u8, u16, u32, u64, u128, usize);
impl_zeroize_with_default!(f32, f64, char, bool);

/// Implement `Zeroize` on arrays of types that impl `Zeroize`
macro_rules! impl_zeroize_for_array {
($($size:expr),+) => {
$(
impl<Z> Zeroize for [Z; $size]
where
Z: Zeroize
{
fn zeroize(&mut self) {
}
}
)+
};
}

impl_zeroize_for_array!(
1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26,
27, 28, 29, 30, 31, 32, 33, 34, 35, 36, 37, 38, 39, 40, 41, 42, 43, 44, 45, 46, 47, 48, 49, 50,
51, 52, 53, 54, 55, 56, 57, 58, 59, 60, 61, 62, 63, 64
);

impl<'a, Z> Zeroize for IterMut<'a, Z>
where
Z: Zeroize,
{
fn zeroize(&mut self) {}
}

impl<Z> Zeroize for Option<Z>
where
Z: Zeroize,
{
fn zeroize(&mut self) {}
}

impl<Z> Zeroize for [Z]
where
Z: DefaultIsZeroes,
{
fn zeroize(&mut self) {}
}

#[cfg(feature = "alloc")]
impl<Z> Zeroize for Vec<Z>
where
Z: Zeroize,
{
fn zeroize(&mut self) {}
}

#[cfg(feature = "alloc")]
#[cfg_attr(docsrs, doc(cfg(feature = "alloc")))]
impl<Z> Zeroize for Box<[Z]>
where
Z: Zeroize,
{
fn zeroize(&mut self) {}
}

#[cfg(feature = "alloc")]
#[cfg_attr(docsrs, doc(cfg(feature = "alloc")))]
impl Zeroize for String {
fn zeroize(&mut self) {}
}

pub trait TryZeroize {
#[must_use]
fn try_zeroize(&mut self) -> bool;
}

#[derive(Clone, Debug, Eq, PartialEq)]
pub struct Zeroizing<Z: Zeroize>(Z);

impl<Z> Zeroizing<Z>
where
Z: Zeroize,
{
/// Move value inside a `Zeroizing` wrapper which ensures it will be
/// zeroized when it's dropped.
pub fn new(value: Z) -> Self {
value.into()
}
}

impl<Z> From<Z> for Zeroizing<Z>
where
Z: Zeroize,
{
fn from(value: Z) -> Zeroizing<Z> {
Zeroizing(value)
}
}

impl<Z> ops::Deref for Zeroizing<Z>
where
Z: Zeroize,
{
type Target = Z;

fn deref(&self) -> &Z {
&self.0
}
}

impl<Z> ops::DerefMut for Zeroizing<Z>
where
Z: Zeroize,
{
fn deref_mut(&mut self) -> &mut Z {
&mut self.0
}
}

impl<Z> Zeroize for Zeroizing<Z>
where
Z: Zeroize,
{
fn zeroize(&mut self) {}
}

impl<Z> Drop for Zeroizing<Z>
where
Z: Zeroize,
{
fn drop(&mut self) {}
}

#[inline]
fn atomic_fence() {}

#[inline]
fn volatile_write<T: Copy + Sized>(dst: &mut T, src: T) {}

#[inline]
unsafe fn volatile_set<T: Copy + Sized>(dst: *mut T, src: T, count: usize) {}
Original file line number Diff line number Diff line change
@@ -1,5 +1,3 @@
//! [`Zeroize`] impls for x86 SIMD registers
use crate::{atomic_fence, volatile_write, Zeroize};

#[cfg(target_arch = "x86")]
Expand All @@ -13,10 +11,7 @@ macro_rules! impl_zeroize_for_simd_register {
#[cfg_attr(docsrs, doc(cfg(target_arch = "x86")))] // also `x86_64`
#[cfg_attr(docsrs, doc(cfg(target_feature = $feature)))]
impl Zeroize for $type {
fn zeroize(&mut self) {
volatile_write(self, unsafe { $zero_value() });
atomic_fence();
}
fn zeroize(&mut self) {}
}
};
}
Expand Down
Loading

0 comments on commit 8bc610f

Please sign in to comment.