rust-random / rand

A Rust library for random number generation.
https://crates.io/crates/rand
Other
1.68k stars 430 forks source link

Testing functions requiring CryptoRng #1319

Closed ignassew closed 1 year ago

ignassew commented 1 year ago

I want to test encrypting some value with rsa public key using the RSA crate., but it only accepts CryptoRng for the rng: RSA/keys.rs#L141

StepRng doesn't implement CryptoRng. I could go around this by creating my own mocking rng, but it doesn't fix the core problem.

I think StepRng should support CryptoRng. It's obviously not a cryptographically secure rng, but it being in the mock module should be enough of a warning to not use it in actual code.

If this issue gets accepted, I can make a PR for it.

What are your thoughts?

newpavlov commented 1 year ago

You should use CSPRNG like rand_chacha::ChaCha8Rng seeded with a fixed seed:

use rand_chacha::{ChaCha8Rng, rand_core::SeedableRng};

let mut rng = ChaCha8Rng::seed_from_u64(42);
ignassew commented 1 year ago

My test involves comparing encrypted output from other language (python) which doesn't implement ChaCha in their standard library.

I want to compare the output of these 2 functions:

import base64

from Crypto.Cipher import PKCS1_v1_5
from Crypto.PublicKey import RSA

rsa_public_key = "MIGfMA0GC...vwIDAQAB"
rsa_public_key = RSA.import_key(base64.b64decode(rsa_public_key))

def grb(n: int) -> bytes:
    return b"\x01"

cipher = PKCS1_v1_5.new(rsa_public_key, randfunc=grb)
x = cipher.encrypt("x".encode())

print(base64.b64encode(x))

This will return: b'cRU5r4Z...XKIUwA4wE='

Also the rust implementation:

use rand::{prelude::*, rngs::mock::StepRng};
use rsa::{
    pkcs8::DecodePublicKey, PaddingScheme, PublicKey, RsaPublicKey,
};

struct CryptoStepRng {
    inner: StepRng,
}

impl CryptoStepRng {
    pub fn new(initial: u64, increment: u64) -> Self {
        CryptoStepRng {
            inner: StepRng::new(initial, increment),
        }
    }
}

impl RngCore for CryptoStepRng {
    fn next_u32(&mut self) -> u32 {
        self.inner.next_u32()
    }

    fn next_u64(&mut self) -> u64 {
        self.inner.next_u64()
    }

    fn fill_bytes(&mut self, dest: &mut [u8]) {
        self.inner.fill_bytes(dest)
    }

    fn try_fill_bytes(
        &mut self,
        dest: &mut [u8],
    ) -> Result<(), rand::Error> {
        self.inner.try_fill_bytes(dest)
    }
}

impl CryptoRng for CryptoStepRng {}

fn main() {
    let mut mrng = CryptoStepRng::new(1, 0);

    let rsa_public_key_string = "MIGfMA0GC...vwIDAQAB";
    let rsa_public_key_bytes =
        base64::decode(rsa_public_key_string).unwrap();
    let rsa_public_key = RsaPublicKey::from_public_key_der(
        &rsa_public_key_bytes.as_slice(),
    )
    .unwrap();

    let aes_key_encrypted = rsa_public_key
        .encrypt(&mut mrng, PaddingScheme::new_pkcs1v15_encrypt(), b"x")
        .unwrap();
    let aes_key_encrypted = base64::encode(aes_key_encrypted);
    println!("{}", &aes_key_encrypted);
}

As expected, also returns: b'cRU5r4Z...XKIUwA4wE='

This is one of the example where it is useful for StepRng to have CryptoRng implemented. Trying to figure out how to generate ChaCha rng in python would be a lot more complicated.

vks commented 1 year ago

If you really want to, you can always implement you own RNG type by wrapping StepRng and implement CryptoRng for it.

ignassew commented 1 year ago

That's exactly what I'm doing in the provided example, but I think StepRng should support that outside of the box, and I don't see any reason why it doesn't. It's a mock ring, and I think it's usefulness could be greatly improved by adding CryptoRng support.

newpavlov commented 1 year ago

StepRng is a very bad PRNG. In some cases it may even make test code unusable, e.g. imagine key generation which needs the most significant bit set to 1. Using it to test cryptographic code is certainly a bad practice, so I don't think we should make it easier. In your case it would be better to use something like ReadRng, which would use a fixed random string instead of an input file.

I am inclined to close this issue as "won't fix".

Fethbita commented 1 month ago

Hi! I know this is a closed issue but I would like to bring attention to this just for a short while. I propose adding some examples in the documentation or the book to test functions that use rand crate, in my particular example OsRng.fill_bytes. I would like to mock this in my unit tests so that it returns hardcoded values because I want to test that the function works as expected with the test vectors provided by standards bodies such as NIST or ICAO.

I believe this is quite a common case where functions that use random generators need to be unit tested and documenting a process would be very helpful.

newpavlov commented 1 month ago

@Fethbita Please open a separate issue or (even better) PR for this.

I think that mocking OsRng is a bad approach. Any kind of mocking will devolve into a mess because of its globality (how can you mock it in the presence of several tests which require their own values while they are executed in parallel?). Instead you should implement your tested algorithm generically over RngCore/CryptoRng with OsRng being the default. This way you will be able to use pseudo-RNG with hardcoded stream in tests, while users will work with properly generated entropy.

Fethbita commented 1 month ago

Thanks for the tip @newpavlov. As recommended, I went with a PR: https://github.com/rust-random/book/pull/64.