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

Hardening PRNG #19

Closed
nopjmp opened this issue Apr 5, 2015 · 6 comments
Closed

Hardening PRNG #19

nopjmp opened this issue Apr 5, 2015 · 6 comments

Comments

@nopjmp
Copy link

nopjmp commented Apr 5, 2015

I noticed that tgl_prng_seed is adding Time, PID and 32bytes of random data. It would be best to seed all of the PRNG from random bytes (from each operating systems PRNG system) instead. We could then remove the 2 unused arguments since they are defines to "password" files (currently set to NULL through defines).

If there are no objections, I can work on a patch in my own tree and submit a pull request once it is complete.

@BenWiederhake
Copy link
Contributor

What's the difference between using 32 bytes of random data (thus seeding it from the OS's PRNG) and your proposed method? The only difference I can see is removal of the seed-through-file feature, and the bare minimum guarantee (time, PID, and hw counter which you didn't mention).

@nopjmp
Copy link
Author

nopjmp commented Nov 16, 2015

Sorry for my lack of detail as I'm not a cryptographer, but from what I've read, you should never use the time, PID or a hw counter to seed PRNG. The systems PRNG system is able to take in more random sources such as Ethernet interrupt timing and keyboard / mouse input. This isn't very good on embedded hardware, but its better than using the time and/or pid.

@majn
Copy link
Contributor

majn commented Nov 16, 2015

From the OpenSSL documentation:

OpenSSL makes sure that the PRNG state is unique for each thread. On systems that provide / /dev/urandom, the randomness device is used to seed the PRNG transparently. However, on all other systems, the application is responsible for seeding the PRNG by calling RAND_add(), RAND_egd or RAND_load_file.

So basically, there is no point in seeding with /dev/urandom, OpenSSL already uses that device to ensure that the randomness source is properly seeded.

@nopjmp
Copy link
Author

nopjmp commented Nov 16, 2015

Is there any reason we should be using RRAND_add() with PID and time data then? I'm not aware of any modern systems without /dev/urandom.

My point is tgl has a function tgl_prng_seed that calls RAND_add with data from PID, time and /dev/random. Almost all systems with a /dev/random will have a /dev/urandom.

@majn
Copy link
Contributor

majn commented Nov 16, 2015

I'm not aware of any modern systems without /dev/urandom

Windows, but OpenSSL handles that case too ;)

My point is tgl has a function tgl_prng_seed that calls RAND_add with data from PID, time and /dev/random. Almost all systems with a /dev/random will have a /dev/urandom.

I guess that those are still fine for additional strengthening, since add_random only adds to the entropy and doesn't "replace" the old random values. Even when they can be guessed it won't do any harm. If we propose any change, we should just remove the pointless calls to /dev/random and /dev/urandom.

@BenWiederhake
Copy link
Contributor

but from what I've read, you should never use the time, PID or a hw counter to seed PRNG

Nearly. There is one extremely important detail, as in "this-is-the-thing-that-might-break-your-neck":

You should never use only the time, PID or a hw counter to seed a PRNG.

Since we use "secure" sources (explicit entropy from /dev/{,u}random and implicit from crypto-library boot), it doesn't really matter whether we also throw in the PID and time in addition to these other sources.

For completeness, let's analyze each part:

  • The not-guessable-but-low-entropy stuff like time, PID, hw counter gives a lower bound, even though it would be too low if we were to use it stand-alone. I think it's nice-to-have.
  • The thing that I called "implicit entropy" (done during boot of the crypto lib, either OpenSSL or gcrypt) is everything we need, and there is no point in going beyond that if you trust that this code is called properly. However, I don't trust that it is called at the right time. OpenSSL is weird, gcrypt is never initialized explicitly from tgl nor telegram-purple, and it would need a lot of documentation to even prove that this initialization should happen with out way of using it.
  • The thing that I called "explicit entropy" (copying from /dev/{,u}random) is again everything we need, and there is no point in going beyond that if you trust the OS PRNG. I do so, because otherwise you can't trust anything.

The only thing I would modify is the reading of /dev/random: It's pointless. There is exactly one difference between urandom and random, and that is: One device always returns something of high-quality (/dev/urandom!), the other one only if some arbitrary counter feels like it (/dev/random).
And that's only a few lines and not really worth the trouble.

So if you really want to change anything from here, you'd have to convince @vysheng to fully trust the crypto library; both OpenSSL and gcrypt! Only then we can talk about completely removing /dev/{,u}random and pid/time/hw counter.

@nopjmp nopjmp closed this as completed Aug 16, 2016
BenWiederhake added a commit to BenWiederhake/tgl that referenced this issue Apr 15, 2018
The biggest irony is that the string apparently is discarded
adter copying it.  So nobody cares about it anyway.

Fixes vysheng#19
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

No branches or pull requests

3 participants