Bug #46047 for bind9-public: improve crypto-rand UI and clarify RNG use in general

Tue, 19 Sep 2017 16:06:09 -0400 Evan Hunt <Evan_Hunt@isc.org> - Ticket created

From:	Evan_Hunt@isc.org
To:	bind9-public@isc.org
Subject:	improve crypto-rand UI and clarify RNG use in general
Date:	Tue, 19 Sep 2017 20:06:08 +0000

As discussed in 31459: > 1) when built with --enable-crypto-rand, "crypto" is used instead of > "openssl" or "pkcs11" to indicate use of the crypto library random > number generator > 2) when built with --enable-crypto-rand, random-device defaults to "crypto" > 3) when overridden with -r or the random-device option, crypto-rand is > fully disabled, and a file source is used in its place. > 4) the uses of isc_random_get() that you noted as BAD should be updated > to isc_rng_random(), OR, isc_random_get() should be altered to be a > front-end for isc_rng_random(). > 5) improve comments and write some developer doc that does a better > job explaining how the RNG/PRNG functions interrelate The "BAD" uses of isc_random_get() were: - to initialize FNV hash name (BTW this is BAD) - client cookie nonce (BAD) - nsec3param salt (BAD) - rndc initial serial number of messages (BAD)

Tue, 19 Sep 2017 17:41:56 -0400 Evan Hunt <Evan_Hunt@isc.org> - Correspondence added

Started on this in rt46047: > > 1) when built with --enable-crypto-rand, "crypto" is used instead of > > "openssl" or "pkcs11" to indicate use of the crypto library random > > number generator Instead of "crypto", I decided "random-device none;" or leaving the -r option blank would specify the default behavior. > > 2) when built with --enable-crypto-rand, random-device defaults to > > "crypto" The default in config.c is now "none" when built with crypto-rand > > 3) when overridden with -r or the random-device option, crypto-rand > > is > > fully disabled, and a file source is used in its place. "Fully disabled" is not the case -- openssl still uses its own built-in entropy source. On further thought, this is probably fine, but also note... > > 5) improve comments and write some developer doc that does a better > > job explaining how the RNG/PRNG functions interrelate In the ARM we need to be very clear about *exactly* what behavior changes when specifying the -r or random-device options.

Thu, 21 Sep 2017 02:45:01 -0400 Evan Hunt <Evan_Hunt@isc.org> - Correspondence added

Francis, can you review the changes in rt46047 please? Thanks.

Thu, 21 Sep 2017 02:45:03 -0400 Evan Hunt <Evan_Hunt@isc.org> - Given to Francis Dupont <Francis_Dupont@isc.org>

Thu, 21 Sep 2017 02:45:16 -0400 Evan Hunt <Evan_Hunt@isc.org> - Status changed from 'open' to 'review'

Thu, 21 Sep 2017 03:28:24 -0400 Evan Hunt <Evan_Hunt@isc.org> - Correspondence added

On Thu Sep 21 06:45:01 2017, each wrote: > Francis, can you review the changes in rt46047 please? Thanks. Note, I left this one unfixed: - to initialize FNV hash name (BTW this is BAD) ...because if you're using memory tracing, then this hash has to be set up before the memory context, and that can create a circular dependency if you fix it the way I tried to do. The others, I think, are addressed.

Tue, 26 Sep 2017 13:36:20 -0400 Evan Hunt <Evan_Hunt@isc.org> - Correspondence added

Francis, can you please review this?

Wed, 27 Sep 2017 10:38:39 -0400 Francis Dupont <Francis_Dupont@isc.org> - Correspondence added

On Tue Sep 26 17:36:20 2017, each wrote: > Francis, can you please review this? => yes. Some comments: - in confgen/keygen.c a -r keyboard is handled before the crypto hook, in dnssec/dnssectool.c it is handled after. This is not consistent and IMHO after is better so I changed keygen.c in 940fd79ba238c9c08e80a236cfcfdcec1ebcc732 - in the named "Open the source of entropy" code in server.c cfg_obj_isvoid can be called on NULL. Fixed in 27790304792fee1fc19dd03de3b6de0eef0c7a46 - it is a matter of taste but IMHO in generate_salt i and n should be size_t (or at least unsigned). - you removed the ifdef SC_PLATFORM_CRYPTORANDOM in dst__entropy_getdata. Please put it back as it simplifies the code and makes sure that dst library never uses a not crypto library PNRG. BTW don't confuse isc_entropy_usehook and isc_entropy_sethook, only the first changes the source of entropy. At the exception of the last point the code is good. I am testing it to check if I missed something.

Wed, 27 Sep 2017 10:38:41 -0400 Francis Dupont <Francis_Dupont@isc.org> - Given to Evan Hunt <Evan_Hunt@isc.org>

Wed, 27 Sep 2017 14:23:57 -0400 Evan Hunt <each@isc.org> - Correspondence added

Subject:	Re: [ISC-Bugs #46047] improve crypto-rand UI and clarify RNG use in general
Date:	Wed, 27 Sep 2017 18:23:56 +0000
From:	"Evan Hunt" <each@isc.org>
To:	"Francis Dupont via RT" <bind9-public@isc.org>

On Wed, Sep 27, 2017 at 02:38:40PM +0000, Francis Dupont via RT wrote: > - it is a matter of taste but IMHO in generate_salt > i and n should be size_t (or at least unsigned). I'm fine with this. > - you removed the ifdef SC_PLATFORM_CRYPTORANDOM > in dst__entropy_getdata. Please put it back as it simplifies > the code and makes sure that dst library never uses a > not crypto library PNRG. BTW don't confuse > isc_entropy_usehook and isc_entropy_sethook, only > the first changes the source of entropy. I removed it because, when we're running with cryptorandom, isc_entropy_getdata() is hooked to dst_random_getdata() anyway, and if we're *not* running with cryptorandom, dst_random_getdata() returns NOTIMPLEMENTED, which means it's best if dst_random_getdata() isn't called directly. Doing it the way I did seems safer and cleaner, and it only adds the overhead of a single function call. Come to think of it, I'd prefer it if it were called dst__random_getdata, to discourage people from relying on it or calling it directly. Just use it as an internal hook function only. If you absolutely insist, I'll put it back the way it was, but I really do think this is architecturally better.

Thu, 28 Sep 2017 03:32:22 -0400 Francis Dupont <Francis_Dupont@isc.org> - Correspondence added

On Wed Sep 27 18:23:57 2017, each@isc.org wrote: > > - you removed the ifdef SC_PLATFORM_CRYPTORANDOM > > in dst__entropy_getdata. Please put it back as it simplifies > > the code and makes sure that dst library never uses a > > not crypto library PNRG. BTW don't confuse > > isc_entropy_usehook and isc_entropy_sethook, only > > the first changes the source of entropy. > > I removed it because, when we're running with cryptorandom, > isc_entropy_getdata() is hooked to dst_random_getdata() anyway, => it is not true: isc_entropy_usehook() is called *only* if no random file / random dev is given or configured. BTW without this #ifdef dnssec-keygen -r xxx is no longer a no-op as described in the documentation. > and if > we're *not* running with cryptorandom, dst_random_getdata() returns > NOTIMPLEMENTED, which means it's best if dst_random_getdata() isn't > called directly. => the by-pass is consistent so I don't buy this argument. > Doing it the way I did seems safer and cleaner, and it only adds the > overhead of a single function call. => unfortunately it is not true and it makes dst behavior relying on external to dst state. > Come to think of it, I'd prefer it if it were called dst__random_getdata, > to discourage people from relying on it or calling it directly. Just use > it as an internal hook function only. => dst__random_getdata has the two __ to mark it as internal to dst. > If you absolutely insist, I'll put it back the way it was, but I really > do think this is architecturally better. => I absolutely insist.

Thu, 28 Sep 2017 03:48:38 -0400 Francis Dupont <Francis_Dupont@isc.org> - Correspondence added

On Wed Sep 27 18:23:57 2017, each@isc.org wrote: > On Wed, Sep 27, 2017 at 02:38:40PM +0000, Francis Dupont via RT wrote: > > - it is a matter of taste but IMHO in generate_salt > > i and n should be size_t (or at least unsigned). > > I'm fine with this. => you forgot the cast for n: I removed it in 4e0b2023500a19549f8ea98f88e9e15d45af2497

Thu, 28 Sep 2017 13:04:05 -0400 Evan Hunt <each@isc.org> - Correspondence added

Date:	Thu, 28 Sep 2017 17:04:03 +0000
To:	"Francis Dupont via RT" <bind9-public@isc.org>
Subject:	Re: [ISC-Bugs #46047] improve crypto-rand UI and clarify RNG use in general
CC:
From:	"Evan Hunt" <each@isc.org>

On Thu, Sep 28, 2017 at 07:32:23AM +0000, Francis Dupont via RT wrote: > => dst__random_getdata has the two __ to mark it as internal to dst. No, dst__entropy_getdata() has double underscores, but dst_random_getdata() doesn't. > => I absolutely insist. Okay. With that change, I assume this is now okay to merge.

Thu, 28 Sep 2017 13:11:13 -0400 Evan Hunt <Evan_Hunt@isc.org> - Correspondence added

Merged. The CHANGES note has been revised to read: 4724. [func] By default, BIND now uses the random number functions provided by the crypto library (i.e., OpenSSL or a PKCS#11 provider) as a source of randomness rather than /dev/random. This is suitable for virtual machine environments which have limited entropy pools and lack hardware random number generators. This can be overridden by specifying another entropy source via the "random-device" option in named.conf, or via the -r command line option; however, for functions requiring full cryptographic strength, such as DNSSEC key generation, this cannot be overridden. In particular, the -r command line option no longer has any effect on dnssec-keygen. This can be disabled by building with "configure --disable-crypto-rand". [RT #31459] [RT #46047]

Thu, 28 Sep 2017 13:11:13 -0400 Evan Hunt <Evan_Hunt@isc.org> - Status changed from 'review' to 'resolved'

Thu, 28 Sep 2017 13:11:14 -0400 Evan Hunt <Evan_Hunt@isc.org> - Version Fixed 9.12.0 added

Created:	Tue, 19 Sep 2017 16:06:08 -0400
Updated:	Thu, 28 Sep 2017 13:11:14 -0400
Closed:	Thu, 28 Sep 2017 13:11:13 -0400

Bug #46047 for bind9-public: improve crypto-rand UI and clarify RNG use in general

This bug tracker is no longer active.