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

[DO NOT MERGE] Use Bitcoin RNG #2731

Closed

Conversation

moneromooo-monero
Copy link
Collaborator

For visibility, as an alternative to #2622. Plus a /dev/random seed at startup. Other bits (memwipe) will be PR'd separately.

@@ -28,8 +28,14 @@
//
// Parts of this file are originally copyright (c) 2012-2013 The Cryptonote developers

#pragma once
#pragma once
Copy link
Contributor

Choose a reason for hiding this comment

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

Trailing whitespace

@anonimal
Copy link
Contributor

Referencing #2622 (comment).

@iamsmooth
Copy link
Contributor

iamsmooth commented Oct 30, 2017

/dev/random seed at startup is undesirable if and only if getrandom or some equivalent is available. It causes unnecessary blocking at startup (and on headless systems with limited entropy sources this blocking time can be very significant) when any app on the system has recently pulled a lot of entropy, but the non-blocking system source (/dev/urandom) is still perfectly safe to use.

In the cold boot case where the system source is insufficiently seeded, then getrandom itself will block (which is most of the purpose for that facility).

In the case where no getrandom-like facility exists, then I agree with the blocking seed at startup from /dev/random to guard against cool boot situations.

@moneromooo-monero
Copy link
Collaborator Author

getrandom is always called with 0 flags, which mean "/dev/urandom". If not seeding with /dev/urandom when getrandom is available, then maybe seed with getrandom with flags set to GRND_RANDOM ?

@iamsmooth
Copy link
Contributor

iamsmooth commented Oct 30, 2017

Unlike using urandom directly, getrandom with no flags will block on the cold boot case where urandom hasn't been initialized, where 'initialized' means collected CHACHA20_KEY_SIZE*2 (=64) bytes (=512 bits) of entropy. This is the desired behavior.

This is the intended use of getrandom so if getrandom exists it should do this. If getrandom doesn't exist, then a blocking seed at startup (seed using /dev/random) is probably 'best' (or at least safest), although we may get complaints (including 'bug' reports) about it, as entropy collection can be very slow/erratic on some systems, and this blocking can occur any time the wallet or daemon is started, not just after cold boot.

In horribly broken cases where getrandom exists but does not do this correct blocking in the cold boot case, we gain some protection from the bitcoin rng's defensive mixing in of other sources, assuming they exist (would be nice to support some non-x86 platforms).

Code implementing relevant portions of getrandom:

https://github.com/torvalds/linux/blob/master/drivers/char/random.c#L1925
https://github.com/torvalds/linux/blob/master/drivers/char/random.c#L1551
https://github.com/torvalds/linux/blob/master/drivers/char/random.c#L431
https://github.com/torvalds/linux/blob/master/drivers/char/random.c#L809
https://github.com/torvalds/linux/blob/master/drivers/char/random.c#L433

After reviewing this I'm less convinced by @hyc's claim that the libsodium approach (waiting for /dev/random to unblock) is broken, although it is certainly obscure and tricky (and maybe dangerous on non-Linux systems). Reviewed again. I think he's right. On third review, I think it the libsodium approach is okay. Not really relevant here though.

@moneromooo-monero
Copy link
Collaborator Author

One change I made is that Bitcoin had a PRNG for "less strong" randomness. I did not bring this in. This may mean we exhaust entropy a lot quicker (ie, when asking for kilobytes of randomness when making range proofs). For this maybe a PRNG is better ?

@iamsmooth
Copy link
Contributor

iamsmooth commented Nov 1, 2017

In the Linux case at least, It is more friendly to other applications that get this stuff slightly wrong (and this describes a large number of apps, since as we have seen getting it right is not easy nor obvious) to not read from the kernel prng too much (bulk cases, as you mentioned). That avoids causing other apps that read unnecessarily from /dev/random from blocking a lot.

Perhaps in the bulk case it might be better to exclude using the OS source every time, though I think the others (performance counters and such) are still safe to include, as long as doing so doing so doesn't have a meaningful performance cost.

I'll take a look at what the original Bitcoin one was doing and follow up.

It's meant to avoid being optimized out

memory_cleanse lifted from bitcoin
It will be reinitialized later once we know about log file
and other command line configuration
Bitcoin's uses /dev/urandom as well as other sources of
randomness.
@moneromooo-monero
Copy link
Collaborator Author

/dev/random seeding is now only done if we don't have getrandom.

@moneromooo-monero moneromooo-monero mentioned this pull request Nov 24, 2017
@mizuki-hikaru
Copy link
Contributor

@moneromooo-monero Why do you prefer this approach over using libsodium?

@moneromooo-monero
Copy link
Collaborator Author

IIRC It uses both a random source and a PRNG, which protects against the random source being compromised after process start.

@anonimal
Copy link
Contributor

anonimal commented Jul 4, 2018

#2622 (comment)

@moneromooo-monero
Copy link
Collaborator Author

FWIW, the more time passes, the less I like this and favour straight libsodium.

@iamsmooth
Copy link
Contributor

@moneromooo-monero

From a maintainability and clarity point of view there is no question that libsodium is the way to go, and that may indeed carry the day. However, libsodium really does look like a simplistic approach that doesn't address some of the concerns raised here (and in the linked/related discussion) which the the Bitcoin PRNG (or something like it) does address. That means things such as the kernel-level PRNG being broken or compromised.

Still, maintainability and simplicity matter. It is a lot easier to get something wrong in some hidden way (whether malicious or accidental) with a more complex solution, which can also be dangerous.

@moneromooo-monero
Copy link
Collaborator Author

Pointless. Too complicated when we've got two other good alternatives.

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.

5 participants