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

Use libsodium's CSPRNG interface #2622

Closed

Conversation

paragonie-scott
Copy link

Supersedes #2621.

@paragonie-scott
Copy link
Author

Hmm, apparently it's less trivial than #include "sodium.h". Probably a quick fix though, since libsodium is already used elsewhere.

@daira
Copy link

daira commented Oct 10, 2017

I'm not a Monero developer but as a cryptographer, this looks good to me (modulo the build error of course). I strongly endorse the use of libsodium's default RNG; its choices of what to do on each platform are well-motivated. It's what we use in Zcash (see zcash/zcash#2299 for our notes on the blocking issue, which concluded that libsodium does get that right).

@FiloSottile
Copy link

And to address the points raised on the preceding PR, using the libsodium user space PRNG without a specific reason is a downgrade in quality versus the kernel one.

  • Entropy: quite obviously, its seeded by the kernel PRNG
  • Pool management: the kernel pool is better protected, forward secure and fork safe
  • Actual PRNG output quality: this was a solved problem 10 years ago; even using SHA1 like old Linux or OS X will generate no biases; the Diehard days have passed

@paragonie-scott
Copy link
Author

As I've said elsewhere: I'm not super fluent in C/C++. I know how to read/write it (the easy part), but getting the compiler to cooperate isn't something I've had much luck with.

Anyone have any clues as to what the missing piece is to make sodium.h load? It's already loaded elsewhere in the project.

@moneromooo-monero
Copy link
Collaborator

What is the error ?

@paragonie-scott
Copy link
Author

paragonie-scott commented Oct 10, 2017

Actually, I think I figured it out. The remaining build errors appear to be from existing problems.

Alternative to monero-project#2621, uses libsodium's randombytes_buf() instead.
@paragonie-scott
Copy link
Author

Spoke too soon.

[ 53%] Building C object src/crypto/CMakeFiles/obj_cncrypto.dir/random.c.o
/home/vagrant/slave/monero-android-armv7/build/src/crypto/random.c:34:10: fatal error: 'sodium.h' file not found
#include <sodium.h>
         ^
1 error generated.
make[3]: *** [src/crypto/CMakeFiles/obj_cncrypto.dir/random.c.o] Error 1
make[3]: Leaving directory `/home/vagrant/slave/monero-android-armv7/build/build/release'
make[2]: *** [src/crypto/CMakeFiles/obj_cncrypto.dir/all] Error 2
make[1]: *** [all] Error 2
make: *** [release-static-android] Error 2
make[2]: Leaving directory `/home/vagrant/slave/monero-android-armv7/build/build/release'
make[1]: Leaving directory `/home/vagrant/slave/monero-android-armv7/build/build/release'
program finished with exit code 2
elapsedTime=23.292026

@moneromooo-monero
Copy link
Collaborator

Is it installed (sorry to ask) ?
If you found a sodium.h in crypto_ops_builder, this is not used, it's a tree shen used to generate the crypto code monero uses IIRC.

@paragonie-scott
Copy link
Author

Ah okay, I'll double-check that, then.

@moneromooo-monero
Copy link
Collaborator

moneromooo-monero commented Oct 26, 2017

Alternatively, we could use Bitcoin's RNG: https://github.com/moneromooo-monero/bitmonero/tree/wipe

This doesn't seed with /dev/random at startup though.

(don't mind the HAVE_xxx defines, it's still missing some cmake detection)

@anonimal
Copy link
Contributor

Crypto++'s CSPRNG is more than fine... and will be available to monero via kovri...

@ghost
Copy link

ghost commented Oct 27, 2017

@anonimal Is it possible to PR the Crypto++ CSPRNG prior to Kovri completion?

@anonimal
Copy link
Contributor

@NanoAkron sure, and AFAICT is less brittle than #2731 (also includes RDRAND support, a secure wipe impl, doesn't require openssl, and more). But do you want to pull in the whole library (it's not large) or hack a branch together for just these various parts? It seems easier to just pull in the whole lib.

@ghost
Copy link

ghost commented Oct 28, 2017

@anonimal Afraid I don't personally have the requisite skills. Was more an observation that including all the right hooks just this one time would prevent more work in the future to:

  1. Debate whether to keep the bitcoin PRNG or replace - 2-3 weeks
  2. Strip out the bitcoin PRNG - 2-3 weeks, up to 2 months before merge
  3. Now include the Crypto++ CSPRNG - 2-3 weeks, up to 2 months before merge

So doing it now could save months in the future.

@iamsmooth
Copy link
Contributor

iamsmooth commented Oct 30, 2017

I briefly reviewed the Crypto++ RNG code and maybe it is a documentation issue but I don't really see where it has a complete packaged solution that is comparable to the Bitcoin code or even libsodium. There are various pieces that support OS entropy sources, CPU hardware, a mixing pool, etc. and makes all of these available to user code, but I don't see any single clearly-defined API that handles managing the available sources, proper seeding, etc. into a CSPRNG in a reasonably safe way.

However, as I said it is possible I just missed it given the limited documentation (that I found).

Also, it doesn't appear to have support for getrandom, which is a significant omission when it comes to safe and well-behaved seeding. (libsodium appears to get seeding somewhat wrong without getrandom but does support getrandom so this issue is largely moot.)

@anonimal
Copy link
Contributor

anonimal commented Jul 4, 2018

single clearly-defined API that handles managing the available sources, proper seeding, etc. into a CSPRNG in a reasonably safe way.

Giving someone an API doesn't make something reasonably safe. Certain aspects of crypto should be sandboxed; otherwise, you're homebrewing yourself.

Also, there is a clearly-defined API: to start, look at classes BlockingRng and NonblockingRng. You'll then want to look at AutoSeededRandomPool where you can set blocking/nonblocking (random/urandom) and seed size (if you wish). If you want more fine grained control (more than what we should be touching as an application) then look at RandomPool and define the source of your own entropy. Dig dig dig.

Also, it doesn't appear to have support for getrandom, which is a significant omission when it comes to safe and well-behaved seeding.

??? Here's the implementation, there's not much to it as far as userland is concerned. I've also seen #2731 (comment) but I'm not convinced that getrandom is a requisite. We can definitely PR the addition to crypto++ or open the issue as a topic of discussion (they've merged my PRs in the past, still waiting on this one though weidai11/cryptopp#668).

#2731 treads on the question of to homegrow or not homegrow. I'd personally prefer that we don't and instead shift responsibility to a library.

@noloader
Copy link

noloader commented Jul 17, 2018

@iamsmooth,

There is documentation at RandomNumberGenerator on the Crypto++ wiki. There's also the manual and the RandomNumberGenerator interface (just check what is derived from it). They may not be that useful for those who are new to the Crypto++ library.

If I am parsing things correctly, I think you only need to use AutoSeededRandomPool for Crypto++. The "auto seeded" part means the generator will seed itself using OS supplied entropy. The "random pool" part means it is a PGP-style generator using AES-256 to produce the stream. During instantiation the generator seeds AES using a 32-byte key. Each call to that generates a block mixes the time into the generator.

There are some other details, like how does the generator seed itself using OS. That comes from OS_GenerateRandomBlock, which uses /dev/random, /dev/srandom, /dev/srandom or CryptGenRandom (depending on the platform and how it is called).

In nearly all situations it is hard to shoot yourself in the foot with AutoSeededRandomPool. About the only one I can think of is the VM rollback attack. But all generators suffer them unless a user stirs in unique entropy before extracting random bits.

If you don't like the auto-seeded pool then you can use OS_GenerateRandomBlock directly like Linux's getrandom.

And you have the other generator available, like the Bitcoin generators and the libsodium generators. All of them do the same thing and all of them should be production quality. I would not lament over the details. Find one you like and use it.

@moneromooo-monero
Copy link
Collaborator

We are now linking with libsodium via 0mq, so: #4159

@moneromooo-monero
Copy link
Collaborator

There's a PR with this in that builds, so I'll close this as a dupe.

+duplicate

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants