drbg_get_entropy: force a reseed before calling ssleay_rand_bytes() [1.0.2 only]#7259
Conversation
|
@paulidale @kaduk please let me know which variant you prefer. |
crypto/rand/rand_lib.c
Outdated
There was a problem hiding this comment.
Oh, I forgot to remove the counter... hang on.
Fixes openssl#7240 In FIPS mode, the default FIPS DRBG uses the drbg_get_entropy() callback to reseed itself, which is provided by the wrapping libcrypto library. This callback in turn uses ssleay_rand_bytes() to generate random bytes. Now ssleay_rand_bytes() calls RAND_poll() once on first call to seed itself, but RAND_poll() is never called again (unless the application calls RAND_poll() explicitely). This implies that whenever the DRBG reseeds itself (which happens every 2^14 generate requests) this happens without obtaining fresh random data from the operating system's entropy sources. This patch forces a reseed from system entropy sources on every call to drbg_get_entropy(). In contrary to the automatic reseeding of the DRBG in master, this reseeding does not break applications running in a chroot() environment (see c7504ae), because the SSLEAY PRNG does not maintain an error state. (It does not even check the return value of RAND_poll() on its instantiation.) In the worst case, if no random device is available for reseeding, no fresh entropy will be added to the SSLEAY PRNG but it will happily continue to generate random bytes as 'entropy' input for the DRBG's reseeding, which is just as good (or bad) as before this patch.
mspncp
force-pushed
the
pr-drbg-get-entropy-force-reseed-variant
branch
from
c332ae4 to
dd6c77e
Compare
|
The appveyor build was broken. I squashed the fixup and force-pushed to kick the bots anew. |
paulidale
left a comment
paulidale
left a comment
There was a problem hiding this comment.
I think this is the cleaner of the two but both look okay.
|
Thanks Paul. I'm slightly favouring this variant, too. I'll wait a few more hours to give @kaduk the opportunity to respond before merging. |
|
(The "few more hours" came in overnight for me. This does look like a better approach in principle, but give me some time to eat breakfast, etc., and double-check the other uses of the 'initialized' flag (e.g., that we are not introducing any leaks.) |
…ytes() [The following lines will be added as penultimate paragraph to the commit message] To prevent ssleay_rand_bytes_from_system() (and hence RAND_poll()) from being called twice during instantiation, a separate drbg_get_nonce() callback has been introduced, which is identical with the previous implementation of drbg_get_entropy().
|
No need to hurry. I did some testing and some more is coming up... (BTW: I normally take into account the time zone of my collegues ;-) ) |
|
I noticed that Here is the test setup I used: Patch for md_rand.c diff --git a/crypto/rand/md_rand.c b/crypto/rand/md_rand.c
index abca70f23a..1f08fdb916 100644
--- a/crypto/rand/md_rand.c
+++ b/crypto/rand/md_rand.c
@@ -563,6 +563,7 @@ int ssleay_rand_bytes(unsigned char *buf, int num, int pseudo, int lock)
int ssleay_rand_bytes_from_system(unsigned char *buf, int num)
{
+ fprintf(stderr, "ssleay_rand_bytes_from_system\n");
initialized = 0;
return ssleay_rand_bytes(buf, num, 0, 0);
}Makefile test.c gdb log test output |
|
Fix pushed (to be squashed). Please note that a6358f5 amends the commit message. |
kaduk
left a comment
kaduk
left a comment
There was a problem hiding this comment.
Approved provided that my assumption is correct...
| FIPS_drbg_set_callbacks(dctx, | ||
| drbg_get_entropy, drbg_free_entropy, 20, | ||
| drbg_get_entropy, drbg_free_entropy); | ||
| drbg_get_nonce, drbg_free_entropy); |
There was a problem hiding this comment.
Someone is going to double-take that drbg_free_entropy is paired with drbg_get_nonce, but the behavior should be fine.
There was a problem hiding this comment.
If you think it is confusing the users, I could fix it in one of the following ways:
- copy&paste
drbg_free_entropytodrbg_free_nonce - rename
drbg_free_entropytodrbg_free_entropy_or_nonce
Please let me know what you prefer.
There was a problem hiding this comment.
I don't think any change is needed.
| return 0; | ||
| } | ||
| return min_len; | ||
| } |
There was a problem hiding this comment.
A little unfortunate to have the duplicate code, but I think it's unavoidable here.
There was a problem hiding this comment.
Initially, I thought about basing the two callbacks on a common implementation with an extra parameter, but that didn't really make it look better. If you prefer, I can still do it.
There was a problem hiding this comment.
I don't think it would actually look better; no change, please.
| int ssleay_rand_bytes_from_system(unsigned char *buf, int num) | ||
| { | ||
| initialized = 0; | ||
| return ssleay_rand_bytes(buf, num, 0, 0); |
There was a problem hiding this comment.
I assume that the locking that allows us to pass lock=0 to ssleay_rand_bytes() also serves to serialize the accesses to initialized (that is, someone calling this function already holds the CRYPTO_RAND_LOCK).
There was a problem hiding this comment.
That's my assumption, too, but I will double-check it in the debugger using my test program.
mspncp
changed the title
Concurrent access to ssleay_rand_bytes_from_systemTL;DRThe situation is not as simple as I thought, but it is nevertheless harmless. The long storyFirst of all: concurrently clearing the For the sake of completeness, here the result of my experiments: Instantiation of the FIPS_DRBGThe first call to In the case of my test program it happens in Probably the Reseeding of the FIPS_DRBGThe FIPS object module only has a single FIPS_DRBG instance, which is wired to the RAND api when FIPS mode is enabled. So reseeding only occurs via Exotic border casesThere are hypothetical cases where |
|
Conclusion: after rechecking the facts and having obtained two approvals, I'm assured this is ready to be merged. I'll do it this evening (CEST). |
mspncp
added
the
approval: done
Fixes #7240 In FIPS mode, the default FIPS DRBG uses the drbg_get_entropy() callback to reseed itself, which is provided by the wrapping libcrypto library. This callback in turn uses ssleay_rand_bytes() to generate random bytes. Now ssleay_rand_bytes() calls RAND_poll() once on first call to seed itself, but RAND_poll() is never called again (unless the application calls RAND_poll() explicitely). This implies that whenever the DRBG reseeds itself (which happens every 2^14 generate requests) this happens without obtaining fresh random data from the operating system's entropy sources. This patch forces a reseed from system entropy sources on every call to drbg_get_entropy(). In contrary to the automatic reseeding of the DRBG in master, this reseeding does not break applications running in a chroot() environment (see c7504ae), because the SSLEAY PRNG does not maintain an error state. (It does not even check the return value of RAND_poll() on its instantiation.) In the worst case, if no random device is available for reseeding, no fresh entropy will be added to the SSLEAY PRNG but it will happily continue to generate random bytes as 'entropy' input for the DRBG's reseeding, which is just as good (or bad) as before this patch. To prevent ssleay_rand_bytes_from_system() (and hence RAND_poll()) from being called twice during instantiation, a separate drbg_get_nonce() callback has been introduced, which is identical with the previous implementation of drbg_get_entropy(). Reviewed-by: Paul Dale <[email protected]> Reviewed-by: Ben Kaduk <[email protected]> (Merged from #7259)
|
Merged. Thanks for reviewing! |
3 participants
Fixes #7240
Alternative to #7247
In FIPS mode, the default FIPS DRBG uses the drbg_get_entropy()
callback to reseed itself, which is provided by the wrapping
libcrypto library. This callback in turn uses ssleay_rand_bytes()
to generate random bytes.
Now ssleay_rand_bytes() calls RAND_poll() once on first call to
seed itself, but RAND_poll() is never called again (unless the
application calls RAND_poll() explicitely). This implies that
whenever the DRBG reseeds itself (which happens every 2^14
generate requests) this happens without obtaining fresh random
data from the operating system's entropy sources.
This patch forces a reseed from system entropy sources on every
call to drbg_get_entropy(). In contrary to the automatic reseeding
of the DRBG in master, this reseeding does not break applications
running in a chroot() environment (see c7504ae), because the
SSLEAY PRNG does not maintain an error state. (It does not even
check the return value of RAND_poll() on its instantiation.)
In the worst case, if no random device is available for reseeding,
no fresh entropy will be added to the SSLEAY PRNG but it will happily
continue to generate random bytes as 'entropy' input for the DRBG's
reseeding, which is just as good (or bad) as before this patch.
Checklist