Conditionally open random devices on initialisation.#7429
Conditionally open random devices on initialisation.#7429paulidale wants to merge 1 commit intoopenssl:masterfrom
Conversation
|
Looks good to me at first sight, but I'd like to check the code example in detail in the debugger... |
|
Pauli, I did a lot of debugging and testing today and the result is an alternative proposal for fixing issue #7419. Instead of opening the random devices unconditionally, my approach is to open them only on first use. This has the advantage that in the case where the This change does not reintroduce a regression for chroot applications compiled with 1.1.0, since the SSLEAY RNG also reseeds on first use. For full details, see pull request #7437 |
|
Matthias St. Pierre wrote:
Pauli, I did a lot of debugging and testing today and the result is an alternative proposal for fixing issue #7419. Instead of opening the random devices unconditionally, my approach is to open them only on first use. This has the advantage that in the case where the `getrandom()` system call is available, no file handles will be opened unnecessarily. This works without requiring that the application calls `RAND_keep_random_devices_open(0);`
This change does not reintroduce a regression for chroot applications compiled with 1.1.0, since the SSLEAY RNG also reseeds on first use. For full details, see pull request #7437
Open random on "first use"
After this program fork with close all file descriptors starting from
stderr + N.
I'm not sure that proposed solution is acceptable. It requires a number
of test with external applications.
Roumen
|
|
Roumen, I'm not sure in which direction your comment goes, could you explain it a bit more, please? The problem you are pointing to has been discussed and addressed in #6432: openssl/crypto/rand/rand_unix.c Lines 298 to 315 in c7504ae |
|
Matthias St. Pierre wrote:
Roumen, I'm not sure in which direction your comment goes, could you explain it a bit more, please?
Looks like I' lost between all those comments : ...open only on first
use..., ...keep open..., ...open unconditionally...
[SNIP]
Ok. In mean exactly the same as in comment for function
check_random_device().
So please ignore my comment to this thread.
Regards,
Roumen Petrov
|
mspncp
added
branch: master
| for (i = 0; i < OSSL_NELEM(random_devices); i++) | ||
| random_devices[i].fd = -1; | ||
| open_random_devices(); | ||
| if (keep_random_devices_open) |
There was a problem hiding this comment.
I have an issue with this change.
This assumes that rand_pool_keep_random_devices_open is called before
rand_pool_init but that cannot work, because rand_pool_keep_random_devices_open
calls either open_random_devices or rand_pool_cleanup, and those operate
on uninitialized data random_devices. I believe it is a severe bug that
RAND_keep_random_devices_open does not call RUN_ONCE(&rand_init, do_rand_init));
There was a problem hiding this comment.
Good catch. This is a separate problem and I'll create a new PR to fix it -- we've not reached a decision as to including this PR or using #7437 instead.
|
Closing in favour of #7437 |
4 participants
The
rand_pool_initfunction was always opening the random devices regardless of the keep them open setting. Now, it conditionally opens them.Fixes #7419