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 zeroize for BlockRng and BlockRng64 #1374

Closed
wants to merge 3 commits into from

Conversation

nstilt1
Copy link

@nstilt1 nstilt1 commented Jan 21, 2024

Regarding #1348 and #934
This appears to work with my working branch of chacha20 without any breaking changes for libraries that use a [Item; N]. Also, I just discovered that using -C target-cpu=native can optimize the code in such a way that the difference between the performance of using pointers and a buffer is about 0.5%... so I think I'm going to close out #1367

@nstilt1
Copy link
Author

nstilt1 commented Jan 21, 2024

What about... adding ZeroizeOnDrop as a trait bound for the core of BlockRng and BlockRng64? Something like

pub struct BlockRng<R: BlockRngCore + ?Sized> {
    results: R::Results,
    index: usize,
    /// The *core* part of the RNG, implementing the `generate` function.
    #[cfg(not(feature = "zeroize"))]
    pub core: R,
    /// The *core* part of the RNG, implementing the `generate` function.
    #[cfg(feature = "zeroize")]
    pub core: R + ZeroizeOnDrop
}

Just to ensure that the core implements it.

Copy link
Member

@dhardy dhardy left a comment

Choose a reason for hiding this comment

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

By itself this code doesn't do anything at all, but having a feature-flag-dependant bound does complicate the API. We should consider simply making zeroize a hard dependency.

@newpavlov thoughts please since you are also a maintainer of zeroize?

Comment on lines 71 to +76
/// Results element type, e.g. `u32`.
#[cfg(not(feature = "zeroize"))]
type Item;
/// Results element type, e.g. `u32`.
#[cfg(feature = "zeroize")]
type Item: DefaultIsZeroes;
Copy link
Member

Choose a reason for hiding this comment

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

This has potential to cause unexpected broken builds.

Copy link
Member

@newpavlov newpavlov left a comment

Choose a reason for hiding this comment

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

In my opinion, this is a wrong use of zeroize. I don't think rand_core needs any particular zeroize support. It's not worth to zeroize the index field and zeroization of core and results fields should be handled by RNG implementation crates.

@dhardy
Copy link
Member

dhardy commented Jan 22, 2024

We don't care about the index, no, but what about BlockRng::results (currently has type R::Results)?

@newpavlov
Copy link
Member

newpavlov commented Jan 22, 2024

Right now it's an associated type selected by an implementation crate, so it could use zeroize::Zeroizing([u32; 16]) for it, if zeroize feature for this crate is enabled.

@dhardy
Copy link
Member

dhardy commented Jan 22, 2024

In the wrapper? This requires making BlockRng::results pub, which should be acceptable.

@newpavlov
Copy link
Member

No, it's not needed. The Zeroizing wrapper has a zeroizing Drop impl, so zeroization of the result field will be done automatically.

@dhardy
Copy link
Member

dhardy commented Jan 22, 2024

👍

@dhardy dhardy closed this Jan 22, 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