Switch to drbg #4019

richsalz · 2017-07-25T20:13:05Z

Doesn't work yet :( I don't have the init done at the right time I think.

richsalz · 2017-07-25T20:32:07Z

I think I'm confused about the strength, min_entropy, and max_entropy fields in the DRBG object.

kroeckx · 2017-07-25T21:10:17Z

fuzz/client.c

Are you sure this is going to do the right thing? Is setting rand_predictable enough?

You might want to remove the ENTROPY_NEEDED define.

Yes it is predictable enough and I'll remove the define thanks.

Please note that the intention of calling RAND_add() and RAND_status() here is to make sure that that the RNG is initialized and that all global variables are set to a specific state that doesn't change anymore when something asks for random bytes or the status.

Yes, I know. And it's no longer necessary, the RAND facility auto-initializes just like the rest of OpenSSL.

"Like the rest of OpenSSL" clearly is already a problem, otherwise that would be an empty function.

I want the global state before and after FuzzerTestOneInput() to be exactly the same. That means if someone internally calls RAND_bytes() or RAND_status(), it should not alter any global variable.

If you say that it's "like the rest of OpenSSL", it probably means it will be initialized on first use, which is fine. The code there is so that this first time has happened, and that this initialization doesn't happen during the first (or 100th) call to FuzzerTestOneInput(), and then next call this whole thing doesn't get executed anymore. The point of calling those functions is that the global state doesn't change anymore. Maybe calling RAND_add() isn't the best way to accomplish this (anymore), maybe it should now just have a call to RAND_bytes(). But in any case it's doubtful that just removing those lines will have the desired effect.

IIRC BoringSSL's RAND_add() is just a wrapper that calls RAND_bytes(&c,1) into a char on the stack.

Boring switched to DRBG and this is their current RAND_add()

void RAND_add(const void *buf, int num, double entropy) {}

See #4025 which does this the "right" way.

(And any summary for the memcmp/Crypto_memcmp switch?)

kroeckx · 2017-07-25T21:15:51Z

crypto/rand/rand_unix.c

Is there a reason to remove that comment? Afaik, this is about a specific hardware / OS combination where the OS doesn't provide an interface to get random data and we generate it ourself.

It's a legacy O/S that we still support. If you want me to restore the comment I can, but I found it more misleading than not.

BTW, I restored the comment slightly edited.

kroeckx · 2017-07-25T21:23:11Z

I'm not sure we want this as the default RNG, but it clearly should be an option.

richsalz · 2017-07-25T22:43:35Z

I am boggled by that statement. :) Why would we not want it?

paulidale · 2017-07-26T03:59:58Z

crypto/rand/rand_lib.c

A thought:

for (i=0; rand_bytes.curr < entropy && i < 10; i++)

Adding robustness when another process steals the bits added by RAND_poll before we grab the lock anew.

For 10 in an arbitrary integer base.

Anyway, just a thought. The caller is going to have to deal with a zero return regardless.

neat trick. added.

kroeckx · 2017-07-26T07:02:34Z

My understanding is that this is slower than it should be.

richsalz · 2017-07-26T12:20:05Z

On modern processors with AESNI instructions it should be faster. When finished it will have much less thread contention (because there will be per-thread DRBG's). But that doesn't matter :) The impact should be miniscule and it's more secure. The old hacky system has never been analyzed.

kaduk · 2017-07-26T12:23:38Z

Someone might not want it if their risk-avoidance strategy indicates minimizing the number of things changing at a time. Though the old hacky scheme has never been analyzed, it's a risk that has been accepted forever by "everyone" using openssl. Though the new thing looks better, it's a different risk profile.
Which does not directly address the question of default-ness, but does attempt to answer why someone would not want it, at least not right away.

richsalz · 2017-07-26T12:27:57Z

And they're going to want TLS 1.3? :) I don't buy that argument.

kroeckx · 2017-07-26T16:17:33Z

I have seen at least one paper about our current scheme, and it's not that good. I don't want to keep the current scheme, I'm just not sure that the NIST DRBG are the best.

richsalz · 2017-07-26T16:19:20Z

This PR, once I fix it, is clearly better than our current scheme :)
If you have something you think is better, I would honestly love to see it.

kroeckx · 2017-07-26T16:21:03Z

If I find some time I will try to write it.

richsalz · 2017-07-27T02:10:15Z

All tests pass now, but I am not happy about the RAND_add call in rsa_crpt.c But other then that, this is ready for review.

kaduk · 2017-07-27T20:53:08Z

Is this actually ready for review or are there more commits incoming?

richsalz · 2017-07-27T23:42:25Z

Added a commit to fix the build error, but yes can be reviewed now.

paulidale · 2017-07-28T01:02:41Z

crypto/rand/rand_unix.c

Just wondering if the ok++ statements could be replaced by jump to cleanse and return success. This won't make any difference if only one source is defined but it will if several are. It seems reasonable to define both OPENSSL_RAND_SEED_RDCPU and OPENSSL_RAND_SEED_DEVRANDOM when building for x86 platforms. The rdcpu path checks for the capability on the CPU and would fail if the HRNG wasn't supported.

If one path is executed successfully, it means enough random bytes have been collected to satisfy the current request, so an early exit makes sense. Using multiple sources, however, is safer in the sense that one being compromised (e.g. hardware failure) doesn't cause an overall failure. Multiple sources will require more resources however -- in the example above, rdcpu will be very fast whereas opening and reading files will be much slower (& will block sometimes).

The resource versus security trade off could be delegated to the user via a configuration option although I fear the code would get messy by doing so. Given that the user is specifying the randomness sources, the user is implicitly trusting them and the short circuit would be a reasonable option.

If this is done OPENSSL_RAND_SEED_GETRANDOM should be moved before OPENSSL_RAND_SEED_DEVRANDOM because getrandom(2) is preferable to reading device files and both are asking the kernel for the bytes. Maybe do this anyway so the more desirable of the two is first.

I'm about to push a commit that re-orders and returns after the first success, thanks!

richsalz · 2017-07-28T02:01:06Z

That seems reasonable. Right now three tests fail under one compiler. I think I'm overwriting something.

kaduk

I only did a cursory review (i.e., single pass over diff, no cross-referencing of global locking strategy analysis).
It might be useful to add some more comments detailing what the locks cover, what the lock ordering requirements are (if any), and whether any routines must only be called with certain locks held.

kaduk · 2017-07-28T13:45:09Z

crypto/rand/drbg_lib.c

If drbg is the global rand_drbg, then don't we need to hold the rand_drbg.lock in order to be touching its contents?
Furthermore, won't the cleanse mess up the lock's initialization and cause potentially nasty libc/pthread errors when someone tries to reinitialize the global rand_drbg?

well it's an impossible condition since app code can never get to rand_drbg :) But I made it be a no-op and pointed to rand_cleanup_int() in a comment. Thanks.

kaduk · 2017-07-28T13:54:33Z

crypto/rand/drbg_lib.c

Unlocked access?

kaduk · 2017-07-28T13:57:41Z

crypto/rand/ossl_rand.c

I thought we were going to leave the old RNG around as an option.

that was never my intent.

kaduk · 2017-07-28T13:59:48Z

crypto/rand/rand_lcl.h

side note: having it larger than any "reasonable value" means that consumers are unlikely to exercise the code paths that it is relevant for, during testing.

it comes directly from the FIPS code that I used.

kroeckx · 2017-07-28T16:49:36Z

I planned to add a RAND_poll_ex() function too, but then for a different use case. Not all RNGs need the same amount of entropy, and I want to be able to tell how much entropy I minimum want.

kroeckx · 2017-07-28T16:52:37Z

Something that might be related, I didn't have time to look at the changes yet, is that I want to be able to cascade RNGs, and so I might want to tell from which source I want to get data. It doesn't always have to come from the OS.

richsalz · 2017-07-28T16:52:45Z

That's an interesting use-case; luckily the current RAND_poll_ex is very short :)

richsalz · 2017-07-28T16:55:32Z

The DRBG code has the concept of cascading CSPRNG's, yes. The main/global/"core" DRBG gets its randomness from the OS (or however configured via the with-rand-seed parameter), and child DRBG's, probably one in each SSL object, get their randomness from the global one.

richsalz · 2017-07-28T19:45:50Z

done, ready for review.

richsalz · 2017-07-28T20:26:15Z

BTW, single-threaded performance is twice as fast:

; time openssl rand 102410243 > /dev/null
2.78user 0.02system 0:02.80elapsed 99%CPU (0avgtext+0avgdata 1972maxresident)k
0inputs+8outputs (0major+563minor)pagefaults 0swaps
; time ./apps/openssl rand 102410243 > /dev/null
1.23user 0.02system 0:01.25elapsed 100%CPU (0avgtext+0avgdata 3484maxresident)k
0inputs+0outputs (0major+941minor)pagefaults 0swaps

kroeckx · 2017-07-28T23:18:53Z

I seem to have a chacha20 based one working that's 20 times faster than the current.

richsalz · 2017-07-28T23:20:40Z

congrats. look forward to seeing the PR. :)

If RAND_add wraps around, XOR with existing. Add test to drbgtest that does the wrap-around.

richsalz · 2017-08-02T20:16:54Z

ping.

snhenson · 2017-08-02T22:48:39Z

I'm a bit late to this party, sorry if this has been asked before... but how hard would it be to extend RAND_METHOD so it has all the necessary functionality and have DRBG as the first example of the use of the extended method?

richsalz · 2017-08-03T00:24:14Z

RAND_METHOD is a public structure, so we can't change it for now :(

snhenson · 2017-08-03T01:02:51Z

Ugh, there isn't even a "flags" field which we used as a workaround once before. Dang that's a pity.

paulidale

There is one change in crypto/rand/rand_unix.c that I think should be included now rather than later. The one in crypto/rand/rand_win.c is likewise.

Consider this approved both with and without either of those changes. If without, a new PR would be nice.

The rest can be deferred to a different PR or done here.

paulidale · 2017-08-03T01:11:30Z

crypto/rand/rand_lib.c

    unsigned char c;
    int i;

    for (i = 0; i < 10; i++) {


Ten bytes is probably excessive here, especially when credited as forty bits.
Maybe four bytes instead.

paulidale · 2017-08-03T01:14:04Z

crypto/rand/rand_lib.c

+    ret &= rand_bytes.lock != NULL;
+    rand_bytes.curr = 0;
+    rand_bytes.size = MAX_RANDOMNESS_HELD;
+    /* TODO: Should this be secure malloc? */


Ideally yes, it should be a secure malloc.

both changes to use secure heap are done in the new PR.

paulidale · 2017-08-03T01:16:11Z

crypto/rand/rand_lib.c

+    rand_drbg.lock = CRYPTO_THREAD_lock_new();
+    ret &= rand_drbg.lock != NULL;
+    rand_drbg.size = RANDOMNESS_NEEDED;
+    rand_drbg.randomness = OPENSSL_malloc(rand_drbg.size);


Ideally, this ought to be secure too.

The per thread ones perhaps but not will be okay in the absence of knowledge of the number of threads.

The per SSL ones not.

paulidale · 2017-08-03T01:19:42Z

crypto/rand/rand_lib.c

    CRYPTO_THREAD_lock_free(rand_meth_lock);
-    rand_drbg_cleanup();
+    CRYPTO_THREAD_lock_free(rand_bytes.lock);
+    OPENSSL_clear_free(rand_drbg.randomness, rand_drbg.size);


Should rand_bytes.buff be cleared too?
Should RAND_DRBG_uninstantiate clear rand_drbg.randomness?

Thd DRBG clean-entropy callback is called as soon as the entropy is consumed, so I think it's not needed. But for parallel construction I did it anyway in the new PR.

paulidale · 2017-08-03T01:22:00Z

crypto/rand/rand_unix.c

 #   endif

-#   ifdef OPENSSL_RAND_SEED_GETRANDOM
+#   ifdef OPENSSL_RAND_SEED_RDTSC


Suggest moving RDTSC and RDCPU up above GETRANDOM. RDTSC isn't producing much but is very fast and might as well be included. RDCPU is very fast and would be preferable to getrandom(2) if both are enabled.

RDTSC is still disabled.

paulidale · 2017-08-03T01:23:45Z

crypto/rand/rand_win.c

 # ifdef OPENSSL_RAND_SEED_RDCPU
-    if (rand_rdcpu())
+    if (rand_read_cpu(cb, arg))
        ok++;


The same short circuiting as used in rand_unix would be beneficial here too.
The source ordering here is good.

snhenson · 2017-08-03T10:37:43Z

Thinking about it while we couldn't change existing RAND_METHOD fields we could extend it by placing additional fields at the end. That has been done in the past with e.g. RSA_METHOD.

richsalz · 2017-08-03T13:04:03Z

But that would break ABI compatibility, wouldn't it?

If RAND_add wraps around, XOR with existing. Add test to drbgtest that does the wrap-around. Re-order seeding and stop after first success. Add RAND_poll_ex() Use the DF and therefore lower RANDOMNESS_NEEDED. Also, for child DRBG's, mix in the address as the personalization bits. Centralize the entropy callbacks, from drbg_lib to rand_lib. (Conceptually, entropy is part of the enclosing application.) Thanks to Dr. Matthias St Pierre for the suggestion. Various code cleanups: -Make state an enum; inline RANDerr calls. -Add RAND_POLL_RETRIES (thanks Pauli for the idea) -Remove most RAND_seed calls from rest of library -Rename DRBG_CTX to RAND_DRBG, etc. -Move some code from drbg_lib to drbg_rand; drbg_lib is now only the implementation of NIST DRBG. -Remove blocklength Reviewed-by: Paul Dale <paul.dale@oracle.com> (Merged from #4019)

richsalz · 2017-08-03T13:46:33Z

Squashed and merged. I will create a new PR to address the last issues you just raised, @paulidale

snhenson · 2017-08-03T16:15:48Z

It would break ABI compliance in the strict sense that compatibility testers would complain about increase in structure sizes. In a looser sense applications would still be compatible and nothing would crash.

davidben · 2017-08-03T16:46:33Z

I imagine adding fields to the end was accompanied by a flag or version field? If you just add fields to the end, when the caller fills in a RAND_METHOD for you, they will fill in a smaller struct and OpenSSL reading the extra fields will go off the end of the object.

You could do a new RAND_METHOD_EX with a new API. Old one translates the old struct into the new one.

snhenson · 2017-08-03T17:44:26Z

Yes in the case of RSA_METHOD there was a flag which indicated that some of the new fields were valid. With RAND_METHOD there is no flags field but one (messy) way would be to place a unique value in one of the existing fields and only access anything past the end if that was present. That mess could all go away once we could make RAND_METHOD opaque.

richsalz · 2017-08-03T18:53:55Z

There's no real need to hack this up now. In a future release we'll make it opaque and do all sorts of wonderful things :)

The generic part of the FIPS DRBG was implemented in fips_drbg_lib.c and the algorithm specific parts in fips_drbg_<alg>.c for <alg> in {ctr, hash, hmac}. Additionally, there was the module fips_drbg_rand.c which contained 'gluing' code between the RAND_METHOD api and the FIPS DRBG. When the FIPS code was ported to master in openssl#4019, for some reason the ctr-drbg implementation from fips_drbg_ctr.c ended up in drbg_rand.c instead of drbg_ctr.c. This commit renames the module drbg_rand.c back to drbg_ctr.c, thereby restoring a simple relationship between the original fips modules and the drbg modules in master: fips_drbg_lib.c => drbg_lib.c /* generic part of implementation */ fips_drbg_<alg>.c => drbg_<alg>.c /* algorithm specific implementations */

The DRGB concept described in NIST SP 800-90A provides for having different algorithms to generate random output. In fact, the FIPS object module used to implement three of them, CTR DRBG, HASH DRBG and HMAC DRBG. When the FIPS code was ported to master in openssl#4019, two of the three algorithms were dropped, and together with those the entire code that made RAND_DRBG generic was removed, since only one concrete implementation was left. This commit restores the original generic implementation of the DRBG, making it possible again to add additional implementations using different algorithms (like RAND_DRBG_CHACHA20) in the future.

The generic part of the FIPS DRBG was implemented in fips_drbg_lib.c and the algorithm specific parts in fips_drbg_<alg>.c for <alg> in {ctr, hash, hmac}. Additionally, there was the module fips_drbg_rand.c which contained 'gluing' code between the RAND_METHOD api and the FIPS DRBG. When the FIPS code was ported to master in #4019, for some reason the ctr-drbg implementation from fips_drbg_ctr.c ended up in drbg_rand.c instead of drbg_ctr.c. This commit renames the module drbg_rand.c back to drbg_ctr.c, thereby restoring a simple relationship between the original fips modules and the drbg modules in master: fips_drbg_lib.c => drbg_lib.c /* generic part of implementation */ fips_drbg_<alg>.c => drbg_<alg>.c /* algorithm specific implementations */ Reviewed-by: Paul Dale <paul.dale@oracle.com> Reviewed-by: Tim Hudson <tjh@openssl.org> (Merged from #4998)

The DRGB concept described in NIST SP 800-90A provides for having different algorithms to generate random output. In fact, the FIPS object module used to implement three of them, CTR DRBG, HASH DRBG and HMAC DRBG. When the FIPS code was ported to master in #4019, two of the three algorithms were dropped, and together with those the entire code that made RAND_DRBG generic was removed, since only one concrete implementation was left. This commit restores the original generic implementation of the DRBG, making it possible again to add additional implementations using different algorithms (like RAND_DRBG_CHACHA20) in the future. Reviewed-by: Paul Dale <paul.dale@oracle.com> Reviewed-by: Tim Hudson <tjh@openssl.org> (Merged from #4998)

richsalz self-assigned this Jul 25, 2017

richsalz added the branch: master Merge to master branch label Jul 25, 2017

kroeckx reviewed Jul 25, 2017

View reviewed changes

paulidale reviewed Jul 26, 2017

View reviewed changes

paulidale reviewed Jul 28, 2017

View reviewed changes

kaduk suggested changes Jul 28, 2017

View reviewed changes

richsalz changed the title ~~WIP Switch to drbg~~ Switch to drbg Jul 28, 2017

Rich Salz added 4 commits August 1, 2017 21:57

Udpate drbg_add

382815b

If RAND_add wraps around, XOR with existing. Add test to drbgtest that does the wrap-around.

Fix build

2041511

Call OPENSSL_clear_free

0f1a115

Fix windows build

81eb1cf

richsalz mentioned this pull request Aug 2, 2017

Add priv rand #4076

Closed

paulidale approved these changes Aug 3, 2017

View reviewed changes

richsalz closed this Aug 3, 2017

richsalz deleted the switch-to-drbg branch August 3, 2017 13:53

mspncp mentioned this pull request Dec 29, 2017

Restore the generic DRBG implementation #4998

Closed

Uh oh!

Switch to drbg #4019

Switch to drbg #4019

Uh oh!

Conversation

richsalz commented Jul 25, 2017

Uh oh!

richsalz commented Jul 25, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kroeckx Jul 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kroeckx commented Jul 25, 2017

Uh oh!

richsalz commented Jul 25, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kroeckx commented Jul 26, 2017

Uh oh!

richsalz commented Jul 26, 2017

Uh oh!

kaduk commented Jul 26, 2017

Uh oh!

richsalz commented Jul 26, 2017

Uh oh!

kroeckx commented Jul 26, 2017 via email

Uh oh!

richsalz commented Jul 26, 2017

Uh oh!

kroeckx commented Jul 26, 2017

Uh oh!

richsalz commented Jul 27, 2017

Uh oh!

kaduk commented Jul 27, 2017

Uh oh!

richsalz commented Jul 27, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

richsalz commented Jul 28, 2017

Uh oh!

kaduk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kroeckx Jul 26, 2017 •

edited

Loading