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

Please Implement Clone for PaddingScheme #130

Closed
icf3ver opened this issue Dec 30, 2021 · 2 comments
Closed

Please Implement Clone for PaddingScheme #130

icf3ver opened this issue Dec 30, 2021 · 2 comments

Comments

@icf3ver
Copy link

icf3ver commented Dec 30, 2021

Please implement the recommended traits for the enum PaddingScheme.

Clone is missing from PaddingScheme. Please fix this.

While copy may not be possible due to problems when copying RngCore--namely that an accidental copy may generate the same numbers--, clone is an important implementation which is advised by the creators of rand. With the added implementation of box-clone on Box<dyn DynDigest> and on Box<dyn RngCore>, written out in full bellow, it should be possible to derive Clone on PaddingScheme.

for DynDigest:

pub trait DynDigest {
    ...
}

pub trait DynDigestBoxClone {
    fn clone_dyn_digest(&self) -> Box<dyn DynDigest>;
}

impl<T> DynDigestBoxClone for T where T: 'static + DynDigest + Clone {
    fn clone_dyn_digest(&self) -> Box<dyn DynDigest> {
        Box::new(self.clone())
    }
}

impl Clone for Box<dyn DynDigest> {
    fn clone(&self) -> Self {
        self.clone_dyn_digest()
    }
}

and the same for Box<dyn RngCore>:

use rand::RngCore;

pub trait RngCoreClone: RngCore {
    fn clone_rng_core(&self) -> Box<dyn RngCoreClone>;
}

impl<T: 'static + RngCore + Clone> RngCoreClone for T {
    fn clone_rng_core(&self) -> Box<dyn RngCoreClone> {
        Box::new(self.clone())
    }
}

impl Clone for Box<dyn RngCoreClone> {
    fn clone(&self) -> Box<dyn RngCoreClone> {
        self.clone_rng_core()
    }
}

and switch to using Box<dyn RngCoreClone> rather than Box<dyn RngCore>.

Thanks.

Best regards,
littleTitan

@roblabla
Copy link
Contributor

namely that an accidental copy may generate the same numbers--, clone is an important implementation which is advised by the creators of rand

Cloning the RNG feels like a huge footgun here. I'm pretty sure generating the same salt twice in PSS will cripple its security, which this proposal would make way too easy to do accidentally.

FWIW, this is an enum, so you can easily write your own "clone" method from user code, e.g.

fn clone_with_thread_rng(scheme: &PaddingScheme) {
    match scheme {
        PaddingScheme::PKCS1v15Encrypt => PaddingScheme::PKCS1v15Encrypt,
        PaddingScheme::PKCS1v15Sign { hash } => PaddingScheme::PKCS1v15Sign { hash },
        PaddingScheme::OAEP { digest, mgf_digest, label } => PaddingScheme::OAEP { digest: digest.box_clone(), mgf_digest: mgf_digest.box_clone(), label: label.clone() },
        PaddingScheme::PSS { _salt_rng, digest, salt_len } => PaddingScheme::PSS { salt_rng: Box::new(rand::thread_rng()), digest: digest.box_clone(), salt_len },
    }
}

This will create a new instance of PaddingScheme with only salt_rng changed to a new instance of thread_rng(), which may be enough for your use-cases.

@tarcieri
Copy link
Member

After #244, PaddingScheme is now a trait, so this is no longer relevant

@tarcieri tarcieri closed this as not planned Won't fix, can't repro, duplicate, stale Apr 18, 2023
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