Fix reseeding issues introduced by new RAND_OpenSSL() method#4328
Fix reseeding issues introduced by new RAND_OpenSSL() method#4328mspncp wants to merge 6 commits intoopenssl:masterfrom
Conversation
766ac66 to
7a6a26a
Compare
|
Ok, the windows build fails because the polling code in rand_win.c was not updated. I will fix this and have a look at the travis builds, too. |
paulidale
left a comment
There was a problem hiding this comment.
I like where this is going. Great stuff.
crypto/rand/rand_lcl.h
Outdated
| * This input will be sent through the derivation function which 'compresses' | ||
| * the low quality input into a high quality output | ||
| */ | ||
| # define DRBG_MINMAX_FACTOR 32 |
There was a problem hiding this comment.
Could the factor be made larger? Allowing down to 0.1 bits per byte perhaps? There is no harm in making the buffer bigger, a higher quality source will always terminate early I believe.
I have some test data from /proc/net/dev on an embedded ARM processor that gave 0.14 bits per byte when assessed by the NIST SP 800-90B second draft estimates. Small processors like these are notorious for having difficulty finding entropy, and a number of poor sources are often the only recourse.
I actually have far worse data sets, one at 0.033 and one at 0.010 bit per byte but they've problems passing some of the second draft tests. My worst ever collected is an impressive 0.0022 bits per byte (using the first draft SP 800-90B tests), I didn't bother running the second draft tests for this one.
There was a problem hiding this comment.
Of course. We're not talking about a huge amount of memory here. It was just some initial value and I didn't want to be too wasteful.
There was a problem hiding this comment.
I like powers of two, so how about taking 128?
# define DRBG_MINMAX_FACTOR 128This would correspond to an entropy ratio of 0.0625 bits per byte. One caveat however is that the resolution of RAND_POOL_bytes_needed() only goes down to 1 bit per byte:
int RAND_POOL_bytes_needed(RAND_POOL *pool, unsigned int entropy_per_byte)
I would object to replacing the integer by a double, or using something like bits-per-hundert-bytes instead. However, RAND_POOL_bytes_needed() is only a convenience method and you can always do the easy calculations by yourself, using the other accessors.
There was a problem hiding this comment.
This sounds reasonable, I don't mind if it is a power or two or not. I agree, avoid doubles unless we go all in using them (which isn't going to happen).
866967d to
dbfe75f
Compare
dbfe75f to
45fe9bc
Compare
45fe9bc to
6219d3d
Compare
|
Phew! That was a hard job to stop all jobs from complaining. I had a hard time struggling with the subtleties of the OpenSSL build system and with long forgotten differences between C++ and C. Now all CI jobs build, except for the travis job #13671.5, which fails with the message There is nothing much I can do about this error :-| |
crypto/rand/drbg_lib.c
Outdated
There was a problem hiding this comment.
Could you make it more clear that this is about additional data?
There was a problem hiding this comment.
Agreed, thanks for the suggestion!
crypto/rand/drbg_rand.c
Outdated
There was a problem hiding this comment.
I would like to see some check to verify that it's not larger than what the NIST documentation has, which can probably be a compile time error.
There was a problem hiding this comment.
Really? Normally, I am a great friend of static asserts, but in this case I would say that it is overkill: Both factors, DRBG_MINMAX_FACTOR and drbg->min_entropylen are hardcoded and not user input. And we are talking about values like 128 * 256 in the worst case. So I would say, since the 16bit era is history, we can trust the compiler to calculate this value correctly. ;-)
crypto/rand/rand_lib.c
Outdated
There was a problem hiding this comment.
I think we should just add the whole TSC, I see little value in only taking the last 8 bits.
There was a problem hiding this comment.
I guess you can leave it like this for now.
There was a problem hiding this comment.
I prefer adding the whole of it. There is some randomness in the bits outside of the last 8. Not a lot but some. My gathering xors the bytes together to produce a single byte of output but it isn't necessary.
There was a problem hiding this comment.
If you don't mind, I would prefer if you could postpone this question to a followup PR, because it's only indirectly related to the subject of this PR.
Yes - that is a problem unrelated to this PR. Are you able to submit a CLA though to make cla-check happy? |
|
Incidentally - is this submission personal or on behalf of NCP? If the latter then we will need a corporate CLA too (we will need an Individual CLA either way). If the former, it would probably be better to submit from a personal email address if you have one. |
|
A CLA for this company address has been signed long time ago. According to @richsalz, there seems to be a case sensitivity issue that prevents the bot from recognizing it. A for the second question: This PR has been done entirely in my free time, so in that sense it could be considered personal. On the other hand, it is related to my work because we are currently using the FIPS DRBG with OpenSSL 1.0.2 and have an interest to continue using it after the migration to 1.1. I will discuss this with my boss and let you know. |
Right - thanks. I found the CLA. There was a case-sensitivity issue which I have corrected. However I could only find an Individual CLA for yourself, not a corporate CLA for ncp-e.com. We need both unless this is a purely personal contribution. Ideally if its purely personal then you should use a different (personal) email address in your commits so its always clear who owns the code that is being contributed. For now I've suspended the CLA in our database until we figure out who is doing the contribution! |
|
I think this is good work, and am glad to see it getting thorough review feedback. I'm a little busy right now; don't wait for me :) |
Simplify RAND_POOL_add_begin()/RAND_POOL_add_end() calling sequenceCalling # ifdef OPENSSL_RAND_SEED_GETRANDOM
bytes_needed = RAND_POOL_bytes_needed(pool, 8 /*entropy_per_byte*/);
if (bytes_needed > 0) {
buffer = RAND_POOL_add_begin(pool, bytes_needed);
if (buffer != NULL) {
int bytes = getrandom(buffer, bytes_needed, 0);
entropy_available = RAND_POOL_add_end(pool, bytes, bytes);
}
if (entropy_available > 0)
return entropy_available;
}
# endifcan be simplified to: # ifdef OPENSSL_RAND_SEED_GETRANDOM
bytes_needed = RAND_POOL_bytes_needed(pool, 8 /*entropy_per_byte*/);
buffer = RAND_POOL_add_begin(pool, bytes_needed);
if (buffer != NULL) {
int bytes = getrandom(buffer, bytes_needed, 0);
entropy_available = RAND_POOL_add_end(pool, bytes, bytes);
}
if (entropy_available > 0)
return entropy_available;
# endif |
That's it for the moment.Three TODO's are remaining. If no one has any more comments, than I will clean up the code on friday and put it out of WIP. My company signalled that they are willing to sign a corporate cla, but it will take a few more days until I get it. Probably beginning of next week. TODO 1Looks like the implementation of /*
* TODO(DRBG): looking at the source code of ctr_update() it
* becomes obvious that |adin| needs to be passed as |in1| and
* not as |in2| parameter of ctr_update():
*
* ctr_update(drbg, adin, adinlen, NULL, 0, NULL, 0);
*
* Otherwise the line
*
* ctr_XOR(ctr, ctr->KX, drbg->seedlen);
*
* would not get executed. So we have three options
*
* 1) pass |adin| as |entropy| argument to ctr_reseed():
*
* ctr_reseed(drbg, adin, adinlen, NULL, 0);
*
* 2) call ctr_generate() with |adin| to generate dummy output:
*
* ctr_generate(drbg, dummy, sizeof(dummy), adin, adinlen)
*
* 3) make ctr_update() non-static and call it directly
*
* ctr_update(drbg, adin, adinlen, NULL, 0, NULL, 0);
*
* Variants 1) and 2) both invoke 3), but 2) is more heavyweigth
* Variant 3) would be the simplest solution, but the function
* is currently static. So I chose variant 1), even though
* using |adin| for |entropy| looks like a hack.
*
* Any comments? Suggestions?
*/TODO 2I leave this TODO in the code, passing on the torch to others. Anybody with ideas for a good concept may feel free to come up with a PR. /*
* Try the various seeding methods in turn, exit when successful.
*
* TODO(DRBG): If more than one entropy source is available, is it
* preferrable to stop as soon as enough entropy has been collected
* (as favored by @rsalz) or should one rather be defensive and add
* more entropy than requested and/or from different sources?
* If yes, how can this be made configurable?
*/TODO 3This looks like a simple task for a seasoned OpenSSL contributor, so I'll leave it up to somebody else to post a followup PR. /*
* Default security strength (in the sense of [NIST SP 800-90Ar1])
* of the default OpenSSL DRBG, and the corresponding NID.
*
* Currently supported values: 128, 192, 256
*
* TODO(DRBG): would be nice to have the strength configurable
*/
# define RAND_DRBG_STRENGTH 128
# define RAND_DRBG_NID NID_aes_128_ctr |
|
So I have some comments, but that doesn't mean anything needs to change (yet):
|
crypto/rand/drbg_lib.c
Outdated
There was a problem hiding this comment.
Should that just say number of bytes (in pout)? It seems to give the impression that you want to know how much entropy it has as bytes.
There was a problem hiding this comment.
Can you also say that it returns 0 on error?
There was a problem hiding this comment.
The formulation was not very comprehensible. Second attempt:
/*
* Set the RAND_DRBG callbacks for obtaining entropy and nonce.
*
* get_entropy()
*
* This is a request to allocate and fill a buffer of size |min_len| <= size <= |max_len|
* (in bytes) which contains at least |entropy| bits of entropy. The buffers address is
* to be returned in |*pout| and the number of collected randomness bytes as return value.
*
* If the callback succeeds to acquire |entropy| bits of entropy, it returns the number of
* bytes written to the buffer (which may be less than the allocated size of the buffer).
* If the callback fails, it returns a buffer length of 0.
*
crypto/rand/drbg_lib.c
Outdated
There was a problem hiding this comment.
Could you document this function, and say that randomness is ignored because it's used for additional data?
There was a problem hiding this comment.
I would like to add that normally we don't document those functions, because this one is documented by RAND_add()
There was a problem hiding this comment.
Added comments to all DRBG RAND_METHOD callbacks:
/*
* Implements the default OpenSSL RAND_add() method
*
* Since RAND_add() is only used to push additional input,
* the |randomness| parameter is ignored.
*/
static int drbg_add(const void *buf, int num, double randomness)
{
...
}
/*Implements the default OpenSSL RAND_seed() method */
static int drbg_seed(const void *buf, int num)
{
...
}
etc.
crypto/rand/rand_lib.c
Outdated
There was a problem hiding this comment.
Please document this function, at least what the return value means.
There was a problem hiding this comment.
I didn't implement this function ;-) Nevertheless, here is my documentation:
/*
* Acquire entropy using intel specific cpu instructions
*
* Uses the RDSEED instruction if available, otherwise uses
* RDRAND if available.
*
* For a difference between RDSEED and RDRAND, and why RDSEED
* is the preferred choice, see
* https://software.intel.com/en-us/blogs/2012/11/17/the-difference-between-rdrand-and-rdseed
*
* Returns the total entropy count, if it exceeds the requested
* entropy count. Otherwise, returns an entropy count of 0.
*/
int rand_acquire_entropy_from_cpu(RAND_POOL *pool)
{
...
}There was a problem hiding this comment.
But it was rewritten, and the return value changed.
There was a problem hiding this comment.
You're right. So is the documentation of this function ok for you now?
crypto/rand/rand_lib.c
Outdated
There was a problem hiding this comment.
This seems to be duplicate documentation that is also at RAND_DRBG_set_callbacks()?
crypto/rand/rand_lib.c
Outdated
There was a problem hiding this comment.
I see little point in documenting how it used to be done.
There was a problem hiding this comment.
Changed to:
/*
* Implements the get_entropy() callback
*
* If the DRBG has a parent, then the required amount of entropy input
* is fetched using the parent's RAND_DRBG_generate().
*
* Otherwise, the entropy is polled from the system entropy sources
* using RAND_POOL_acquire_entropy().
*/
paulidale
left a comment
There was a problem hiding this comment.
Reconfirmed.
One possible indentation nit.
| /* DRBG helpers */ | ||
| int rand_drbg_restart(RAND_DRBG *drbg, | ||
| const unsigned char *buffer, size_t len, size_t entropy); | ||
|
|
|
As a heads-up/last call, I intend to merge this in about 13 hours, unless objections or other volunteers to merge appear before then. |
|
This travis job fails to boot: https://travis-ci.org/openssl/openssl/jobs/289370926 Certainly not related to my last change ;-) |
Reseeding is handled very differently by the classic RAND_METHOD API and the new RAND_DRBG api. These differences led to some problems when the new RAND_DRBG was made the default OpenSSL RNG. In particular, RAND_add() did not work as expected anymore. These issues are discussed on the thread '[openssl-dev] Plea for a new public OpenSSL RNG API' and in Pull Request #4328. This commit fixes the mentioned issues, introducing the following changes: - Replace the fixed size RAND_BYTES_BUFFER by a new RAND_POOL API which facilitates collecting entropy by the get_entropy() callback. - Don't use RAND_poll()/RAND_add() for collecting entropy from the get_entropy() callback anymore. Instead, replace RAND_poll() by RAND_POOL_acquire_entropy(). - Add a new function rand_drbg_restart() which tries to get the DRBG in an instantiated state by all means, regardless of the current state (uninstantiated, error, ...) the DRBG is in. If the caller provides entropy or additional input, it will be used for reseeding. - Restore the original documented behaviour of RAND_add() and RAND_poll() (namely to reseed the DRBG immediately) by a new implementation based on rand_drbg_restart(). - Add automatic error recovery from temporary failures of the entropy source to RAND_DRBG_generate() using the rand_drbg_restart() function. Reviewed-by: Paul Dale <[email protected]> Reviewed-by: Kurt Roeckx <[email protected]> Reviewed-by: Rich Salz <[email protected]> Reviewed-by: Ben Kaduk <[email protected]> (Merged from #4328)
The DRBG_RESEED state plays an analogue role to the |reseed_required_flag| in Appendix B.3.4 of [NIST SP 800-90A Rev. 1]. The latter is a local variable, the scope of which is limited to the RAND_DRBG_generate() function. Hence there is no need for a DRBG_RESEED state outside of the generate function. This state was removed and replaced by a local variable |reseed_required|. Reviewed-by: Paul Dale <[email protected]> Reviewed-by: Kurt Roeckx <[email protected]> Reviewed-by: Rich Salz <[email protected]> Reviewed-by: Ben Kaduk <[email protected]> (Merged from #4328)
The drbg's lock must be held across calls to RAND_DRBG_generate() to prevent simultaneous modification of internal state. This was observed in practice with simultaneous SSL_new() calls attempting to seed the (separate) per-SSL RAND_DRBG instances from the global rand_drbg instance; this eventually led to simultaneous calls to ctr_BCC_update() attempting to increment drbg->bltmp_pos for their respective partial final block, violating the invariant that bltmp_pos < 16. The AES operations performed in ctr_BCC_blocks() makes the race window quite easy to trigger. A value of bltmp_pos greater than 16 induces catastrophic failure in ctr_BCC_final(), with subtraction overflowing and leading to an attempt to memset() to zero a very large range, which eventually reaches an unmapped page and segfaults. Provide the needed locking in get_entropy_from_parent(), as well as fixing a similar issue in RAND_priv_bytes(). There is also an unlocked call to RAND_DRBG_generate() in ssl_randbytes(), but the requisite serialization is already guaranteed by the requirements on the application's usage of SSL objects, and no further locking is needed for correct behavior. In that case, leave a comment noting the apparent discrepancy and the reason for its safety (at present). Reviewed-by: Paul Dale <[email protected]> Reviewed-by: Kurt Roeckx <[email protected]> Reviewed-by: Rich Salz <[email protected]> (Merged from #4328)
Thanks!Thanks in return to all reviewers for the great effort you put in reviewing this PR. You gave me a hard time and kept me busy for quite a while, but your reviews also give me a good feeling about the final result. The random generator is such a crucial part of the crypto library and requires very much care. And I really don't want to be the guy who is blamed sometimes in the future for having introduced some RANDBLEED bug. ;-) What's nextNow that pull request #4328 has finally been merged, I have regained new drive to follow my long-term goal which is to make the RAND_DRBG public. I added some thoughts about my private roadmap to Issue #4403. You are welcome to participate and comment. |
|
Because I think it's so important to get right, I'm taking my time to review and understand everything. |
|
To all early reviewers of this pr: I would like to recover commit 7a6a26a, which I lost in my local repo a while ago by garbage collection. It is still displayed here on GitHub, together with the entire branch, but github doesn't let me fetch it, because it is not reachable from one of the advertised references. (I know that I can download the patches and recreate the branch, but I'd rather have the original commits) If one of you checked it out for reviewing (or even if you only have a fetch rule for (If you named your github clone differently, you have to replace If you succeed, please send me the link. Thank you in advance! |
Get it from this branch: |
|
Cool! Thank you very much, Matt! You can delete the branch in your repository again, I have pushed it to my repository as msp-github-4328-fix-rand-add-ori, together with the snapshot of the final version msp-github-4328-fix-rand-add. |
In pull request openssl#4328 the seeding of the DRBG via RAND_add()/RAND_seed() was implemented by buffering the data in a random pool where it is picked up later by the rand_drbg_get_entropy() callback. This buffer was limited to the size of 4096 bytes. When a larger input was added via RAND_add() or RAND_seed() to the DRBG, the reseeding failed, but the error returned by the DRBG was ignored by the two calling functions, which both don't return an error code. As a consequence, the data provided by the application was effectively ignored. This commit fixes the problem by a more efficient implementation which does not copy the data in memory and by raising the buffer the size limit to INT32_MAX (2 gigabytes). This is less than the NIST limit of 2^35 bits but it was chosen intentionally to avoid platform dependent problems like integer sizes and/or signed/unsigned conversion. Additionally, the DRBG is now less permissive on errors: In addition to pushing a message to the openssl error stack, it enters the error state, which forces a reinstantiation on next call. Thanks go to Dr. Falko Strenzke for reporting this issue to the openssl-security mailing list. After internal discussion the issue has been categorized as not being security relevant, because the DRBG reseeds automatically and is fully functional even without additional randomness provided by the application. Fixes openssl#7381
In pull request #4328 the seeding of the DRBG via RAND_add()/RAND_seed() was implemented by buffering the data in a random pool where it is picked up later by the rand_drbg_get_entropy() callback. This buffer was limited to the size of 4096 bytes. When a larger input was added via RAND_add() or RAND_seed() to the DRBG, the reseeding failed, but the error returned by the DRBG was ignored by the two calling functions, which both don't return an error code. As a consequence, the data provided by the application was effectively ignored. This commit fixes the problem by a more efficient implementation which does not copy the data in memory and by raising the buffer the size limit to INT32_MAX (2 gigabytes). This is less than the NIST limit of 2^35 bits but it was chosen intentionally to avoid platform dependent problems like integer sizes and/or signed/unsigned conversion. Additionally, the DRBG is now less permissive on errors: In addition to pushing a message to the openssl error stack, it enters the error state, which forces a reinstantiation on next call. Thanks go to Dr. Falko Strenzke for reporting this issue to the openssl-security mailing list. After internal discussion the issue has been categorized as not being security relevant, because the DRBG reseeds automatically and is fully functional even without additional randomness provided by the application. Fixes #7381 Reviewed-by: Paul Dale <[email protected]> (Merged from #7382)
In pull request #4328 the seeding of the DRBG via RAND_add()/RAND_seed() was implemented by buffering the data in a random pool where it is picked up later by the rand_drbg_get_entropy() callback. This buffer was limited to the size of 4096 bytes. When a larger input was added via RAND_add() or RAND_seed() to the DRBG, the reseeding failed, but the error returned by the DRBG was ignored by the two calling functions, which both don't return an error code. As a consequence, the data provided by the application was effectively ignored. This commit fixes the problem by a more efficient implementation which does not copy the data in memory and by raising the buffer the size limit to INT32_MAX (2 gigabytes). This is less than the NIST limit of 2^35 bits but it was chosen intentionally to avoid platform dependent problems like integer sizes and/or signed/unsigned conversion. Additionally, the DRBG is now less permissive on errors: In addition to pushing a message to the openssl error stack, it enters the error state, which forces a reinstantiation on next call. Thanks go to Dr. Falko Strenzke for reporting this issue to the openssl-security mailing list. After internal discussion the issue has been categorized as not being security relevant, because the DRBG reseeds automatically and is fully functional even without additional randomness provided by the application. Fixes #7381 Reviewed-by: Paul Dale <[email protected]> (Merged from #7382) (cherry picked from commit 3064b55)
Checklist
Introduction
The broken 'RAND_add()/RAND_bytes()' pattern
In the thread [openssl-dev] Plea for a new public OpenSSL RNG API I explained in detail that the different reseeding concepts of the classic OpenSSL RNG and the new RAND_DRBG API (pushing entropy via RAND_add() vs. pulling entropy via get_entropy() callback) make it so difficult to get both APIs work together in harmony, at least as far as reseeding is concerned. The following is a brief excerpt from that thread:
In this pull request I propose a solution to this problem for general discussion.
Some background information on seeding the DRBG
NIST SP 800-90Ar1 has the concept of
security_strength. Currently there are three different security strengths for the CTR_DRBG, specified by the NIDsNID_aes_128_ctr,NID_aes_192_ctr,NID_aes_256_ctr. Their parameter values are hardcoded (in ctr_init()) and taken from 'Table 3: Definitions for the CTR_DRBG' on page 49 of NIST SP 800-90Ar1. In particular, there is no need to have an extraRANDOMNESS_NEEDEDconstant.The role of the derivation function for seeding
The internal state of the
CTR_DRBGconsists of two vectorsso the total number of seed bytes (seedlen in Table 3) equals
seedlen = keylen + blocklen. (As mentioned earlier: in the NIST document, everything is in bits, whereas in OpenSSL, entropy is in bits, buffer lengths are in bytes).Seeding without derivation function
If no derivation function is used, the unmodified input is used to seed (K,V). This means, one has to provide exactly seedlen=keylen+blocken bytes, which in the case of a high quality entropy source contains much more entropy than needed: 8 * seedlen > security_strength.
Seeding with derivation function
If a derivation function is used (which we always do), then the input can be of variable length between
drbg->min_entropylenanddrbg->max_entropylen. The DF takes a variable length input and produces a pseudorandom output from which (K,V) is seeded. This is done using AES-CTR. In some sense, the DF acts as a meat grinder, compressing the entropy of the input.Using the derivation has two advantages: 1) min_entropylen = keylen < seedlen , so you need less input if your input has full entropy and 2) The input can be of variable size min_entropylen < inputsize < max_entropylen , which is good if your input has low entropy (say, only 2 bits per byte).
All values can be found in Table 3 and are hardcoded in ctr_init():
The easiest way would be if get_entropy() would allocate a buffer of length max_entropylen and fill it with bytes until the entropy threshold has been reached. However,
max_entropylen = DRBG_MAX_LENGTHis an astronomically high value. But this value is only a theoretical upper limit from the NIST document. I changed this members value to a more realistic value:This corresponds to the very pessimistic estimate that the entropy of the input lies between 1/4 bits-per-byte and 8 bits-per-byte. That should be enough.
Now in the callback, the buffer is filled until the required entropy threshold is reached. If the buffer overflows (which should never happen), get_entropy() fails. To facilitate this, I added a new "class" RAND_POOL which acts as container for this variable sized random input.
This is my vision of how
get_entropy()should be used. Even ifget_entropy()pulls from/dev/urandomor other full-entropy sources, the implementers should have the freedom to return more than the requested entropy from alternative entropy sources and/or be allowed to return cumulate low entropy input.Summary of the pull request
The solution is split in two parts:
drbg_add()drbg_get_entropy(), and restores the original semantics ofRAND_poll()This splitting is only for the moment to keep the changes overseeable and facilitate reviewing. Also, to keep things simple as long this is still WIP, I will avoid force pushing and address comments by adding new commits. I will squash the commits with a proper commit message when this goes out of WIP.
Commit 1: Make drbg_add() reseed the RAND_DRBG immediately
The obvious solution to use
RAND_DRBG_reseed()creates a circular dependency, because nowRAND_add()is involved both in pushing and pulling randomness:Using
RAND_DRBG_generate()with additional input (generating and discarding some dummy output) instead ofRAND_DRBG_reseed()does not solve the problem, becauseRAND_DRBG_generatE()can autoreseed occcasionally.Instead, we add a new function
rand_drbg_add()which is identical toRAND_DRBG_reseed(), except that it does not pull fresh entropy (and consequently does not reset the reseed_counter). This function is used only by the RAND_METHODdrbg_add().Now the left hand side becomes:
The first commit breaks RAND_poll_ex(), but this function is rewritten and replaced by
RAND_pool_fill()the second commit.Commit 2: Cutting the Gordian knot: Decouple the reseeding code from the two RNG APIs
We do this by adding a new API for collecting entropy: The static fixed size
RAND_BYTES_BUFFERis replaced by a more elaborateRAND_POOL"class" which acts as a container for collecting variable length randomness input for the derivation function.The functions
RAND_poll()anddrbg_get_entropy()manage their own instances of such aRAND_POOLobject, which they allocate and pass down the stack to the functionRAND_POOL_fill()(formerly:RAND_poll_ex(). Since the lifetime and scope of theseRAND_POOLobjects is limited to the stack frame ofRAND_poll()anddrbg_get_entropy(), respectively, there is no need for locking.Note also, that the
RAND_POOLis completely decoupled from the RNG, theRAND_POOL_fill()callback has no notion which kind of RNG is reseeded from the data fed into the pool. Instead, theRAND_POOLclass provides a rich API (@richsalz: no pun intended ;-) ) which appears a little oversized if one adheres to the principle that the operating system's RNG is the optimal entropy source with "full entropy" (8 bits per byte) and that it is not necessary to consider additional entropy sources or sources with lower entropy rates.However, if one seeks for a flexible way of mixing in various entropy resources, as requested by multiple contributers, then such an API may come in handy. Currently, its just a blueprint and proof-of-concept. It provides a set of accessors providing information to the callbacks on which they could base there decisions on how much entropy should be added from the availabe resources.
I don't have a concept yet, how such a variable reseeding scheme could be managed and configured. But that's not part of this PR anyway. Input welcome, here or on openssl-dev.
Remarks
Randomness vs. Entropy
Many occurrances of the term "entropy" have been replaced by the more vague term "randomness" only recently. Although being mathematician, I have no problems using "entropy" synonymously to "randomness" in colloquial language. So originally I considered that discussion vain.
However, now this recent change change turns out to be really handy, because it enables me to consistently distinguish between a
double randomnessargument and aint entropyargument. The former is a floating point value measured in bytes, whereas the other is an integer value measured in bits and has a strict meaning in the sense of NIST SP800-90Ar1.TODO(DRBG)
Some places where I seek particular feedback or closer review have been marked with
TODO(DRBG). These TODO's need to be resolved and removed before the PR can go out of WIP.In particular, please have a closer look a the
TODO(DRBG)comment precedingrand_drbg_add().Reviewing and Testing
Since the RNG is a critical part of OpenSSL, any change to the code is like an open heart surgery. I'm not a seasoned committer and don't have the resources to test it on all platforms and under all circumstances. So I am grateful for any independent testing, in particular the DRBG chaining and forking stuff, since I a am only familiar with
libcryptoand not such withlibssl.What I tested: I successfully ran the standard tests and did a little debugging with gdb to check the main code paths: