Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

zeroize: possible UB in Zeroize implementation for Option<T> #653

Open
tarcieri opened this issue Nov 5, 2021 · 3 comments
Open

zeroize: possible UB in Zeroize implementation for Option<T> #653

tarcieri opened this issue Nov 5, 2021 · 3 comments

Comments

@tarcieri
Copy link
Member

tarcieri commented Nov 5, 2021

Originally filed by @jessa0 as iqlusioninc/crates#782:

It seems the Zeroize implementation for Option<T> where T: Zeroize has language-level UB here:

volatile_set(self as *mut _ as *mut u8, 0, mem::size_of::<Self>());

I believe, as a repr(Rust) enum, the memory layout and set of valid bit-patterns for Option is not defined, and that setting an enum's storage to an invalid bit-pattern while a reference to it exists, even if the value is never read, is instant language-level UB. The documentation for Option does mention guarantees for several special cases, but the None case still isn't defined for many of those cases, and the Zeroize implementation is more generic than that. Here's an example of a miri error in such a situation, that scottmcm came up with on URLO here.

vsrinivas pushed a commit to vsrinivas/fuchsia that referenced this issue Apr 11, 2022
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]>
@mkj
Copy link

mkj commented Dec 2, 2022

The write_bytes docs seem fairly clear that writing invalid bit-patterns without reading should not be UB? (added from rust-lang/unsafe-code-guidelines#330 (comment)).

Interestingly that miri playground example now works - I bisected it to nightly-2021-09-15, but can't see what changed then. It might be OK for other reasons though.

As an aside, I wonder if the Z: Zeroize bound could be removed from Option, allowing Option<Z> to zeroize any uncooperative types...? (no, that won't work for types with heap allocations)

@tarcieri
Copy link
Member Author

tarcieri commented Dec 8, 2022

Reads are still possible after Zeroize::zeroize since the API doesn't require dropping the value, although the question is whether we need to worry about reads after None has been written to the value

@Kixunil
Copy link

Kixunil commented Jul 24, 2024

Found this exact same thing. I believe just holding onto the pointer should help since Rust makes no assumptions about memory behind pointers and the previous reference is inactive according to the stacked borrows when the pointer is used:

        let this = self as *mut Self;
        unsafe {
            volatile_set(this.cast::<u8>(), 0, mem::size_of::<Self>());
        }

        unsafe { ptr::write_volatile(this, None) }

Also to put it differently, if this were UB then memcpy would be also UB because it may briefly break invariants during copying.

Alternatively and perhaps more efficiently you could construct the value to overwrite first and then do the overwriting, assuming Rust won't try to optimize to not copy the padding. But perhaps you can somehow avoid it by making the type opaque.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants