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

Adding a zeroize function for flat types #1045

Merged
merged 13 commits into from
Jan 31, 2024
Merged

Conversation

nstilt1
Copy link
Contributor

@nstilt1 nstilt1 commented Jan 22, 2024

For reducing zeroize code in chacha20, as per @newpavlov's suggestion
I'm unsure of the name, documentation, and whether it should be a method instead of a function.

zeroize/src/lib.rs Outdated Show resolved Hide resolved
Comment on lines 802 to 805
/// # Safety
/// - The type must not contain references to outside data or dynamically sized data
/// - This function can invalidate the type if it is used after this function is called on it. It is
/// advisable to call this method in `impl Drop`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be good to add something here about requiring that the bit pattern of all zeros must be valid for the type

Copy link
Member

@newpavlov newpavlov Jan 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not think this requirement is needed for this function. We just need to mention that the function may leave the memory in a state invalid for type T.

In other words, I think (but not 100% sure) that the following code is safe:

#[repr(transparent)]
struct Foo(NonZeroU32);

impl Drop for Foo {
    fn drop(&mut self) {
        unsafe { zeroize_flat_type(self); }
    }
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not really clear on if that's allowed or not: rust-lang/unsafe-code-guidelines#394

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Notably it seems like it would be UB with drop_in_place or ManuallyDrop::drop which both permit access to the value after running the Drop impl

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both drop_in_place and ManuallyDrop::drop are unsafe, so it should not be a problem. Per ManuallyDrop::drop docs:

However, this “zombie” value should not be exposed to safe code, and this function should not be called more than once. To use a value after it’s been dropped, or drop a value multiple times, can cause Undefined Behavior (depending on what drop does).

For now, we probably should write about validity (with NonZeroU32 as a counter-example), but mention that zeroized state is allowed to break safety invariants of the type.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, that sounds good

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay. Feel free to make any changes

zeroize/src/lib.rs Outdated Show resolved Hide resolved
@tarcieri tarcieri merged commit f5df3cf into RustCrypto:master Jan 31, 2024
21 checks passed
@tarcieri tarcieri mentioned this pull request Apr 24, 2024
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

Successfully merging this pull request may close these issues.

3 participants