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

Add implementation of the Fortuna CSPRNG #104

Merged
merged 1 commit into from
Aug 31, 2014
Merged

Conversation

apoelstra
Copy link
Contributor

Closes #103.

@apoelstra
Copy link
Contributor Author

Sorry, the accumulator is not quite correct. I will push another commit which fixes it (and adds a bunch of unit tests verified against the Go and Python implementations of Fortuna). Edit: Fixed.

@apoelstra apoelstra force-pushed the fortuna branch 4 times, most recently from e81a2cd to cef481a Compare August 27, 2014 20:37
@apoelstra
Copy link
Contributor Author

Updated to move counter-increment into its own function, and to remove the Vecs from the pool, using a running Sha256 state instead. This avoids concerns about having entropy stored in RAM and not getting zeroed after use, and also removes all allocations from the Fortuna, always a good thing.

There is some concern about fork-safety, see rust-lang/rust#16799. If your process forks the Fortuna will not see this and you'll get two processes with identical RNG state. Current word from Rust devs is that forking with the stdlib is UB anyway :/.

Thanks ariel for discussion on IRC.

@apoelstra apoelstra force-pushed the fortuna branch 3 times, most recently from 6eb1da6 to 0a3bf27 Compare August 27, 2014 21:49
@@ -0,0 +1,500 @@
// Copyright 2014 The Rust Project Developers. See the COPYRIGHT
// file at the top-level directory of this distribution and at
// http://rust-lang.org/COPYRIGHT.
Copy link
Owner

Choose a reason for hiding this comment

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

When I created rust-crypto, I left this notice on any file that I borrowed from Rust. I think this clause has mostly been omitted on any new files created specifically for rust-crypto, though, since they aren't directly related to the Rust project itself. IANAL, though, so if you feel that this notice is appropriate, just let me know.

@DaGenix
Copy link
Owner

DaGenix commented Aug 28, 2014

PRNGs do make me a bit nervous. My concern is that if there are bugs, that it might be a bit harder to notice with a PRNG than with some other algorithms. I'm not sure if that concern is really founded though - Rust-Crypto is already a toolbox with lots of pointy edges. So I would like to merge this if you feel that its ready.

@apoelstra
Copy link
Contributor Author

I will add a note about the lack of fork resistance. It's unfortunate that we can't do this, because I believe #[no_std] code can fork, and I try to make my library code as allocation-free as possible exactly to support freestanding users. (In this case, with ariel's help there is no allocation at all.)

After that I feel that this is ready for merge.

I share your nervousness about PRNG's. One nice thing though is that a dumb mistake in implementation will cause the output to be catastrophically different, so because I wrote my test code from the output of pycrypto (but did not write my code from theirs!) and got passing tests, that is some assurance. Another nice thing is that this RNG is very simple (the actual generator is just "use AES in CTR mode to get the output, then use it again to reseed" which is a pretty common idiom) and Rust very explicit, so there are no dark corners where I was at all unsure of what was going on...so presumably there is no room for smart mistakes ;)

Edit: Oh, I didn't see your note about the copyright notice. I think you're right that it "rust project devs" doesn't belong there, I've dropped it.

Unit tests verified against pycrypto. Closes DaGenix#103.
@DaGenix
Copy link
Owner

DaGenix commented Aug 31, 2014

Thanks!

DaGenix added a commit that referenced this pull request Aug 31, 2014
Add implementation of the Fortuna CSPRNG
@DaGenix DaGenix merged commit 44317ca into DaGenix:master Aug 31, 2014
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.

Add Fortuna CSPRNG implementation
2 participants