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

Switch PRNG to ChaCha20 #21

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Switch PRNG to ChaCha20 #21

wants to merge 6 commits into from

Conversation

sts10
Copy link
Owner

@sts10 sts10 commented Nov 11, 2023

Phraze currently uses thread_rng from Rust's rand crate as its pseudo random number generator.

I think thread_rng, "under the hood," uses a stream cipher called ChaCha12 -- here's as much proof I could find in the source code. From what I can tell, this is a good choice!

Buttt I couldn't help but notice the same crate offers "ChaCha20", which is 20 rounds of the same algorithm. As the documentation for ChaCha20 notes:

With the ChaCha algorithm it is possible to choose the number of rounds the core algorithm should run. The number of rounds is a tradeoff between performance and security, where 8 rounds is the minimum potentially secure configuration, and 20 rounds is widely used as a conservative choice.

Seeing as Phraze can afford to be a little slower in order to become more secure and more conservative, this PR explicitly uses 20 rounds (ChaCha20).

However! Here's a long discussion among rand crate stake holders arguing that 12 rounds is enough and that 20 is overkill!. But I still thought it'd be interesting to make this PR public and open and see if anyone has any thoughts.

Implementation

I think

ChaCha20Rng::from_entropy();

is the method we want to use, given this description:

Creates a new instance of the RNG seeded via getrandom.

This method is the recommended way to construct non-deterministic PRNGs since it is convenient and secure.

How this change would affect Phraze performance

Criterion benchmark

Using default thread_rng:

time:   [776.03 ns 787.69 ns 799.82 ns]

Using ChaCha20 explicitly:

time:   [1.6018 µs 1.6119 µs 1.6233 µs]
change: [+118.14% +121.94% +125.54%] (p = 0.00 < 0.10)
Performance has regressed.

As expected, doing 20 rounds takes longer -- about 2x longer it turns out.

Hyperfine benchmark

Using Hyperfine to test the performance of the CLI (cargo install --path=. --force && hyperfine -N -w 1000 -m 1000 'phraze'), which we could argue is a more "real world" benchmark, shows a very minor change, in fact within Hyperfine's margin of error.

Using default thread_rng:

  Time (mean ± σ):       1.8 ms ±   0.2 ms    [User: 1.2 ms, System: 0.5 ms]
  Range (min … max):     1.6 ms …   2.9 ms    1596 runs

Using ChaCha20 explicitly:

  Time (mean ± σ):       1.7 ms ±   0.1 ms    [User: 1.1 ms, System: 0.5 ms]
  Range (min … max):     1.6 ms …   2.4 ms    1757 runs

This is almost certainly because the passphrase generation function itself makes up a relatively small amount of the total time it takes to run the command phraze.

Open questions

What do other password generators use here? I can't tell from a glance at the KeePassXC codebase.

Pros of accepting this PR

  • Phraze's code becomes more explicit in which PRNG it uses (rather than rely on an alias)
  • 20 rounds may be overkill, but it's clearly more secure than 12 rounds, and the change seems to affect Phraze's overall performance only minimally (see above).
  • Gives a little future-proof security (though see below for counterargument)

Cons of accepting this PR

  • Makes Phraze more difficult to maintain: By not using the thread_rng alias, we are relying on Phraze maintainers (me?) to keep up with latest best practice for which PRNG and how many rounds are suitable. In contrast, if we stuck with the alias and just kept upgrading the version of the rand crate that Phraze depends on, we'd probably always be using a good PRNG, with a "good enough" number of rounds.

@sts10
Copy link
Owner Author

sts10 commented Nov 11, 2023

Hmm, this comment about the thread_rng alias gives me pause about leaving it:

Security must be considered relative to a threat model and validation requirements. ThreadRng attempts to meet basic security considerations for producing unpredictable random numbers: use a CSPRNG, use a recommended platform-specific seed ([OsRng]), and avoid leaking internal secrets e.g. via [Debug] implementation or serialization. Memory is not zeroized on drop.

While I think from_entropy() uses a platform-specific seed, I don't know if this PR "avoid[s] leaking internal secrets"? I'd assume so, but it scares me a bit that I'm left confused.

@sts10
Copy link
Owner Author

sts10 commented Jul 2, 2024

If I'm reading this correctly, I'm seeing that Linux's /dev/urandom and /dev/random now uses ChaCha20 (20 rounds): https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=818e607b57c94ade9824dad63a96c2ea6b21baf3

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.

1 participant