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

chacha20: returning rand_core feature #333

Merged
merged 96 commits into from
Apr 29, 2024

Conversation

nstilt1
Copy link
Contributor

@nstilt1 nstilt1 commented Oct 25, 2023

I borrowed some code from version 0.8.1 of chacha20, as well as rand_chacha and was able to get it to compile and pass tests. The main issues in my code that I identified were:

  • usage of [u8; 12] instead of u128 for set/get_stream(). It used to have a u64 when it had a 64 bit counter
    • This has been fixed, set_stream() takes the lower 96 bits of a u128 input. It now has support for using a [u8; 12] with set/get_stream_bytes()
  • usage of ParBlock<LesserBlock> as a temporary output buffer prior to copying it into results in fn generate(). It will probably leave a copy of itself in memory after use. It might be possible to change the BlockRngResults type to ParBlock<LesserBlock>
    • This has been fixed, although it uses a little bit of "unsafe" code to treat the 2D array as a 1D array in
      • AsRef<[u32]> for BlockRngResults and
      • AsMut<[u32]> for BlockRngResults
  • when calling set_word_pos() with an input that wasn't divisible by 16, get_word_pos() would round the input down to the nearest multiple of 16.
    • This has been fixed. It looks like I missed something from rand_chacha.

I've also added ZeroizeOnDrop for the BlockRngResults.

rust-random/rand#934

@nstilt1 nstilt1 marked this pull request as ready for review October 25, 2023 23:59
@nstilt1
Copy link
Contributor Author

nstilt1 commented Oct 26, 2023

Alright, I think I've added just about all I can think of. The #[cfg_attr... in lib.rs and some impls in rng.rs are there because rand/src/std.rs has a

#[derive(Clone, Debug, PartialEq, Eq)]
pub struct StdRng(ChaCha12Rng);

There are also some functions in rng.rs that don't use #[inline]. I'm not sure if they need it or not. Do you have any questions, suggestions, or feedback? Thanks for your time.

@nstilt1 nstilt1 marked this pull request as draft October 27, 2023 11:22
@tarcieri
Copy link
Member

I'd suggest less unsafe code and more of a KISS implementation in an initial PR, and then circling back on some of it in a followup so it can be considered separately.

@tarcieri
Copy link
Member

tarcieri commented Jan 7, 2024

FYI, #338 bumps the crate to v0.10.0-pre, but also includes some other breaking API changes.

Rebasing on it shouldn't be too hard, I hope!

@nstilt1
Copy link
Contributor Author

nstilt1 commented Jan 18, 2024

I've made set_block_pos and get_block_pos as per some feedback in a recent rand issue #1369. I feel like those methods should be on the RNG Core rather than the RNG, but if it were to live on the RNG Core, then the RNG's index won't be changed or reset when the block_pos is set. That shouldn't be an issue as long as it is documented, but if the index should be reset, then the method might need to belong to the RNG.

Some questions I have:

  1. Should the $ChaChaXRngCore be a wrapper around ChaChaCore to prevent cipher methods from being available to the end user?
  2. Should the public set_block_pos and get_block_pos methods be on the RNG and reset the index, or on the core and not reset the index?

@nstilt1
Copy link
Contributor Author

nstilt1 commented Jan 24, 2024

I've now got a working version where the backends seem to be safely zeroizing when the ChaChaCore is dropped, but it depends on the PR for zeroize.

@dhardy dhardy mentioned this pull request Feb 8, 2024
23 tasks
@tarcieri
Copy link
Member

tarcieri commented Mar 5, 2024

@nstilt1 can you rebase? This is looking mostly good and I'd like to get it merged.

@nstilt1
Copy link
Contributor Author

nstilt1 commented Mar 12, 2024

Sure thing. I apologize for the delay. I uh... I was mis-using ChaCha in several ways. I should have it rebased by tomorrow.

…os to Rng instead of RngCore; updated docs to reflect changes
@nstilt1
Copy link
Contributor Author

nstilt1 commented Apr 6, 2024

*block_pos. I moved set/get_block_pos to the RNG rather than its core, where it wasn't able to change the RNG's index after trying to change the block pos

Comment on lines +1 to +38
// Tested for N=32; could be bugs in the loop bounds for other N
// returns bytes written, like fwrite: N means no error, 0 means error in all fwrites
size_t LongNumPrint( uint8_t *num, size_t N)
{
// caller can print a name if it wants

const int revbufsize = 8192; // 8kiB on the stack should be fine
alignas(32) char revbuf[revbufsize];

if (N<32) {
// TODO: maybe use a smaller revbuf for this case to avoid touching new stack pages
ASCIIrev32B(revbuf, num); // the data we want is at the *end* of a 32-byte reverse
return fwrite(revbuf+32-N, 1, N, stdout);
}

size_t bytes_written = 0;
const uint8_t *inp = num+N; // start with last 32 bytes of num[]
do {
size_t chunksize = (inp - num >= revbufsize) ? revbufsize : inp - num;

const uint8_t *inp_stop = inp - chunksize + 32; // leave one full vector for the end
uint8_t *outp = revbuf;
while (inp > inp_stop) { // may run 0 times
inp -= 32;
ASCIIrev32B(outp, inp);
outp += 32;
}
// reverse first (lowest address) 32 bytes of this chunk of num
// into last 32 bytes of this chunk of revbuf
// if chunksize%32 != 0 this will overlap, which is fine.
ASCIIrev32B(revbuf + chunksize - 32, inp_stop - 32);
bytes_written += fwrite(revbuf, 1, chunksize, stdout);
inp = inp_stop - 32;
} while ( inp > num );

return bytes_written;
// caller can putchar('\n') if it wants
}
Copy link
Member

Choose a reason for hiding this comment

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

Huh?

@tarcieri
Copy link
Member

tarcieri commented Apr 28, 2024

This looks very close now, though I'm not sure what's up with rust-toolchain.toml.save or the empty and seemingly unrelated and empty salsa20/benches/mod.rs

@tarcieri
Copy link
Member

In the interest of moving this along since it's important, been taking awhile, and rather invasive, I will go ahead and merge and we can address the remaining issues as followups

@tarcieri tarcieri merged commit adfead3 into RustCrypto:master Apr 29, 2024
57 checks passed
@tarcieri tarcieri changed the title Returning rand_core feature for chacha20 for use in rand_chacha chacha20: returning rand_core feature May 7, 2024
@nstilt1
Copy link
Contributor Author

nstilt1 commented May 30, 2024

I have no idea how that code ended up there, but it is from this post: https://stackoverflow.com/questions/61165307/

I think I was trying to explore palindromic cubes, looking for the fastest way to check if a large number is a palindrome... but I don't know how that code ended up there

tarcieri added a commit that referenced this pull request May 31, 2024
tarcieri added a commit that referenced this pull request May 31, 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.

None yet

2 participants