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

Only read 32 bytes from /dev/urandom for a seed. #15

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

rdebath
Copy link

@rdebath rdebath commented Apr 26, 2017

The problem is you're using the standard I/O functions to read this Unix device; that is not a good idea.

The stdio read functions are buffered so the fread() call will always read a full block from the input. For files and terminals this isn't a problem; the former will just read the data the latter will just return whatever's available.

The problem with /dev/urandom is that a full block of data will be available so a whole block will be read, however, this over a hundred times as much randomness as we actually need. This takes quite a while to generate as the kernel tries very hard to give good results. As the extra data is never used this doesn't actually deplete the theoretical entropy that the kernel has, but it doesn't know that. So this can cause problems too, especially if other applications use the blocking version of the device or you have a daemon that hunts for more randomness.

The Unix functions open(), read() and close() will treat the device nicely.

@rdebath
Copy link
Author

rdebath commented Apr 27, 2017

Added another commit to the repo, this one is an extension of the test program so that all the pieces of the library can be exercised (or even actually USED) from the command line or a shell script.

The Sha512 output is the same strings as the standard sha512sum program, the Ed25519 hex inputs and outputs are in standard hex forms as searchable on the web.

@radii
Copy link
Contributor

radii commented May 1, 2017

Thanks for identifying the problem and writing a solution @rdebath ! I agree that defaulting to buffered IO on /dev/urandom is a pretty bad outcome.

Rather than switching from <stdio.h> to <unistd.h> IO, I think we can ask libc to avoid buffering by doing setbuf(f, NULL). Does that seem right to you?

Use setbuf() to remove the buffering.
@rdebath
Copy link
Author

rdebath commented May 1, 2017

Just double checked with strace and that works just as well, good call!

@orlp
Copy link
Owner

orlp commented May 1, 2017

I'm willing to use unbuffered IO, however I have no interest in the command line interface test program or the makefile that comes with it. That's a bunch more code for me to maintain that's outside the scope of this project. Feel free to host it as your own project though.

GlitchedPolygons added a commit to GlitchedPolygons/GlitchEd25519 that referenced this pull request Oct 9, 2020
Check out the orlp/ed25519 repo's PR orlp#15 for more details: orlp#15
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.

3 participants