test/drbgtest.c: Fix error check test #11195
Conversation
| @@ -358,16 +358,18 @@ static int error_check(DRBG_SELFTEST_DATA *td) | |||
| goto err; | |||
|
|
|||
| /* Test insufficient entropy */ | |||
| if (!init(drbg, td, &t)) | |||
| goto err; | |||
| t.entropylen = drbg->min_entropylen - 1; | |||
ciz
Feb 27, 2020
Author
Contributor
t is reset to the defaults in init(), so t.entropylen needs to be set after the init(), otherwise the following generation will succeed.
t is reset to the defaults in init(), so t.entropylen needs to be set after the init(), otherwise the following generation will succeed.
| @@ -414,7 +418,7 @@ static int error_check(DRBG_SELFTEST_DATA *td) | |||
| * failure. | |||
| */ | |||
| t.entropylen = 0; | |||
| if (TEST_false(RAND_DRBG_generate(drbg, buff, td->exlen, 1, | |||
| if (!TEST_false(RAND_DRBG_generate(drbg, buff, td->exlen, 1, | |||
ciz
Feb 27, 2020
Author
Contributor
This is an expected fail. I must say these !TEST_something are really confusing and hard to parse.
This is an expected fail. I must say these !TEST_something are really confusing and hard to parse.
mspncp
Feb 27, 2020
Contributor
This is an expected fail.
Correct. This error seems to have slipped in already when this module was added.
I must say these !TEST_something are really confusing and hard to parse.
I fully agree. I had a hard time too reading these conditions at the beginning. My strategy is to just ignore the surrounding if (! .... ), because it simply means "break on failure of the test condition", so stripping it leaves only the test condition: Instead of
if (!TEST_false(RAND_DRBG_generate(...)))
I just read
TEST_false(RAND_DRBG_generate(...))
This is an expected fail.
Correct. This error seems to have slipped in already when this module was added.
I must say these !TEST_something are really confusing and hard to parse.
I fully agree. I had a hard time too reading these conditions at the beginning. My strategy is to just ignore the surrounding if (! .... ), because it simply means "break on failure of the test condition", so stripping it leaves only the test condition: Instead of
if (!TEST_false(RAND_DRBG_generate(...)))
I just read
TEST_false(RAND_DRBG_generate(...))
| @@ -423,7 +427,7 @@ static int error_check(DRBG_SELFTEST_DATA *td) | |||
| if (!instantiate(drbg, td, &t)) | |||
| goto err; | |||
| reseed_counter_tmp = drbg->reseed_gen_counter; | |||
| drbg->reseed_gen_counter = drbg->reseed_interval; | |||
| drbg->reseed_gen_counter = drbg->reseed_interval + 1; | |||
ciz
Feb 27, 2020
Author
Contributor
This no longer works. Since 8bf3665#diff-9181ac017a6177a5f2619f65c9b7a346R682, the counter must be larger than the interval to trigger the reseeding.
This no longer works. Since 8bf3665#diff-9181ac017a6177a5f2619f65c9b7a346R682, the counter must be larger than the interval to trigger the reseeding.
mspncp
Feb 27, 2020
Contributor
I'll need to check that tomorrow...
I'll need to check that tomorrow...
mspncp
Feb 28, 2020
Contributor
IIRC this was a deliberate change by @slontis because in the NIST document the counter counts from 1 and not from 0. (see 8bf3665#diff-9181ac017a6177a5f2619f65c9b7a346R516.) Is that correct @slontis?
IIRC this was a deliberate change by @slontis because in the NIST document the counter counts from 1 and not from 0. (see 8bf3665#diff-9181ac017a6177a5f2619f65c9b7a346R516.) Is that correct @slontis?
|
Looks good at first sight. I'll have to do some more checks tomorrow, it's too late now. Here are some first comments. (Note: the patch applies cleanly to the 1.1.1 stable branch as well). |
| /* | ||
| * Nice idea, however RAND_DRBG_uninstantiate() cleanses the data via | ||
| * drbg_ctr_uninstantiate(), but right after that it resets drbg->data.ctr | ||
| * using RAND_DRBG_set(), so the following memcmp will fail. | ||
| */ | ||
| #if 0 | ||
| if (!TEST_mem_eq(zero, sizeof(drbg->data), &drbg->data, sizeof(drbg->data))) | ||
| goto err; | ||
| #endif |
mspncp
Feb 27, 2020
Contributor
Interesting. Note that there is a similar test in providers/fips/self_test_kats.c, which was added only recently by me in #11111. To make it work, I delayed the reinitialization of the drbg->data until the next instantiation. I didn't know that this was tested here, too.
Meanwhile this test should succeed, since your branch already contains my fixes:
- e704521 Check that the DRBG's internal state has been zeroized after uninstantiation
- 75ff4f7 DRBG: delay initialization of DRBG method until instantiation
So please revert this change.
Interesting. Note that there is a similar test in providers/fips/self_test_kats.c, which was added only recently by me in #11111. To make it work, I delayed the reinitialization of the drbg->data until the next instantiation. I didn't know that this was tested here, too.
Meanwhile this test should succeed, since your branch already contains my fixes:
- e704521 Check that the DRBG's internal state has been zeroized after uninstantiation
- 75ff4f7 DRBG: delay initialization of DRBG method until instantiation
So please revert this change.
mspncp
Feb 27, 2020
Contributor
Some part of your comment could remain, to explain the check.
Some part of your comment could remain, to explain the check.
ciz
Feb 28, 2020
Author
Contributor
You're right. It works on git master. My code was originally written for 1.1.1d, where the test failed. I'll re-enable the check, which should also fix the Travis build broken due to the unused variable zero.
You're right. It works on git master. My code was originally written for 1.1.1d, where the test failed. I'll re-enable the check, which should also fix the Travis build broken due to the unused variable zero.
mspncp
Feb 28, 2020
Contributor
You are right, I was already thinking about that. But I did not have the time to write a reply yet.
We could indeed cherry-pick 75ff4f7 to 1.1.1 as a bug fix, since it breaks this test. @paulidale @slontis what's your opinion?
If this is not possible, we would have to disable or remove the test in 1.1.1. It might be necessary to craft a separate pull request for 1.1.1. But for the moment, let's concentrate on master.
You are right, I was already thinking about that. But I did not have the time to write a reply yet.
We could indeed cherry-pick 75ff4f7 to 1.1.1 as a bug fix, since it breaks this test. @paulidale @slontis what's your opinion?
If this is not possible, we would have to disable or remove the test in 1.1.1. It might be necessary to craft a separate pull request for 1.1.1. But for the moment, let's concentrate on master.
paulidale
Feb 28, 2020
Contributor
I'm inclined to not cherry-pick the delayed reinitialisation. 1.1.1 has no need to formally verify that the internal state has been zeroed and there isn't a security problem since we know it will have been.
I'm inclined to not cherry-pick the delayed reinitialisation. 1.1.1 has no need to formally verify that the internal state has been zeroed and there isn't a security problem since we know it will have been.
mspncp
Feb 28, 2020
Contributor
Ok, then we'll just drop the test when cherry-picking to 1.1.1.
Ok, then we'll just drop the test when cherry-picking to 1.1.1.
| @@ -414,7 +418,7 @@ static int error_check(DRBG_SELFTEST_DATA *td) | |||
| * failure. | |||
| */ | |||
| t.entropylen = 0; | |||
| if (TEST_false(RAND_DRBG_generate(drbg, buff, td->exlen, 1, | |||
| if (!TEST_false(RAND_DRBG_generate(drbg, buff, td->exlen, 1, | |||
mspncp
Feb 27, 2020
Contributor
This is an expected fail.
Correct. This error seems to have slipped in already when this module was added.
I must say these !TEST_something are really confusing and hard to parse.
I fully agree. I had a hard time too reading these conditions at the beginning. My strategy is to just ignore the surrounding if (! .... ), because it simply means "break on failure of the test condition", so stripping it leaves only the test condition: Instead of
if (!TEST_false(RAND_DRBG_generate(...)))
I just read
TEST_false(RAND_DRBG_generate(...))
This is an expected fail.
Correct. This error seems to have slipped in already when this module was added.
I must say these !TEST_something are really confusing and hard to parse.
I fully agree. I had a hard time too reading these conditions at the beginning. My strategy is to just ignore the surrounding if (! .... ), because it simply means "break on failure of the test condition", so stripping it leaves only the test condition: Instead of
if (!TEST_false(RAND_DRBG_generate(...)))
I just read
TEST_false(RAND_DRBG_generate(...))
| @@ -423,7 +427,7 @@ static int error_check(DRBG_SELFTEST_DATA *td) | |||
| if (!instantiate(drbg, td, &t)) | |||
| goto err; | |||
| reseed_counter_tmp = drbg->reseed_gen_counter; | |||
| drbg->reseed_gen_counter = drbg->reseed_interval; | |||
| drbg->reseed_gen_counter = drbg->reseed_interval + 1; | |||
mspncp
Feb 27, 2020
Contributor
I'll need to check that tomorrow...
I'll need to check that tomorrow...
When applying this to 1.1.1, either the removal of check Also, the reseed counter adjustment below isn't needed, as the breaking change was added to master only (commit 8bf3665)
I can create a separate pull request once this one is accepted. |
|
@slontis, I'm beginning to doubt whether it was correct to change this reset value from zero to one: openssl/crypto/rand/drbg_lib.c Line 751 in e0f2639 If I understand it correctly, the Lines 430 to 439 in e0f2639 the counter ends up at 2, not 1, although there is only one generate call after the reseed. The reason is that this counter is incremented at the end of every RAND_DRBG_generate() call (second watchpoint hit). This is probably the reason why the counter used to be reset to zero. Here's a transcript of my debug session. The interesting parts are the two watchpoint hits. Start of test
First watchpoint hitopenssl/crypto/rand/drbg_lib.c Line 751 in e0f2639
Second watchpoint hitopenssl/crypto/rand/drbg_lib.c Line 944 in e0f2639
End of test
|
|
Hmm it was back in July last year that I did this change so forgive me if its not real clear what I did in there.. The commit message does however discuss my reasoning So as long as the Hash DRBG doesnt get broken I dont mind what happens. This one is the only type that used the value as part of its calculation. |
Ok, thanks for the explanation. I'll take a look - maybe do a comparison with the old FOM implementation - whether it is worth reverting the change or not. Maybe it's better to leave this question for a separate pr anyway. |
|
Is the pull request fine now or are there any changes required from me? |
Sorry for letting this pr stall. I had too much work lately due to corona lockout in my company. I'll try to take a look in the next days. |
No problem at all. I was just wondering whether I hadn't miss something. |
|
@ciz here is finally my reply to your comment:
After digging a little into history I came to the conclusion that it was me in commit a93ba40 and not @slontis in commit 8bf3665 who introduced the problem: Original Code (OpenSSL-fips-2_0-stable)In the original FIPS code, the counter was initialized to 1 openssl/fips/rand/fips_drbg_lib.c Line 270 in d11e6a4 openssl/fips/rand/fips_drbg_lib.c Line 346 in d11e6a4 and the stop condition was openssl/fips/rand/fips_drbg_lib.c Line 422 in d11e6a4 openssl/fips/rand/fips_drbg_lib.c Line 447 in d11e6a4 In the next section you can see that I lowered the initial counter value from 1 to 0 but kept the stop condition. IIRC, I believed at that time that the stop condition was off-by-one such that the number of generate calls between consecutive reseeds was one less than it should have been. Revisiting the code it seems that I was wrong. Maybe I was a little bit confused by the strange logic in openssl/fips/rand/fips_drbg_lib.c Lines 420 to 451 in d11e6a4 The condition After #4402 (a93ba40)Here you can see the situation after my change. (The openssl/crypto/rand/drbg_lib.c Line 224 in a93ba40 openssl/crypto/rand/drbg_lib.c Line 306 in a93ba40 openssl/crypto/rand/drbg_lib.c Line 475 in a93ba40 openssl/crypto/rand/drbg_lib.c Line 499 in a93ba40 After #6779 (8bf3665)When @slontis noticed that the starting count was wrong, he incremented it back to 1 and adjusted the stop condition accordingly. openssl/crypto/rand/drbg_lib.c Line 402 in 8bf3665 openssl/crypto/rand/drbg_lib.c Line 516 in 8bf3665 openssl/crypto/rand/drbg_lib.c Line 682 in 8bf3665 openssl/crypto/rand/drbg_lib.c Line 711 in 8bf3665 ConclusionSo I think the proper fix is to leave the initial value at 1 and just revert @slontis' change of the stop condition and your change in the test. Would you mind doing this for me in your pull request? (Note that @slontis' change is only on master, not on 1.1.1, so I'd recommend to revert it in a separate commit which can be dropped for 1.1.1.) |
|
(@slontis could you please double-check my last post?) |
|
Your logic makes sense to me... |
|
Sorry for taking so long, I've completely missed your comments. I fixed the condition in drbg_lib.c and the manual setting of reseed_gen_counter at both places in drbtest.c. |
|
@ciz I was on vacation for two weeks. I'll take a look next week. |
|
@ciz unfortunately I let your pull request stall for too long, now your changes collided with @paulidale's heavy EVP_RAND replumbing (#11682). Instead of letting you suffer for something which was my fault, I did the rebase myself, which was quite nasty. In fact so nasty, that I decided to squash your fixup commits first before rebasing your branch ( After some efforts, the outcome looks very reasonable, in fact it would have probably been easier to redo your changes on the tip of master instead of going through the pain of rebasing. @ciz I would ask you to help me verify that I did not introduce any errors during my rebase. To make it easier for you, I published the three intermediate stages as pull requests in my personal fork:
You can verify my rebase in two steps:
If you think gh-11195-squashed-and-rebased is ok, you can force-push it to your |
|
Unfortunately, the
Running the test in the debugger, I get a SIGSEGV
|
|
Ok, I found the reason for the memset crash: The following
(all tests were done on commit 7befa2a of my gh-11195-squashed-and-rebased branch) |
|
I wanted to see whether it was an off-by-one error, so I tried
It looks like the random generator is truncating outlen from 131072 to 65536 instead of failing. If that is true, that would be an unacceptable behaviour. But I still don't understand all details. To be continued...
|
|
On master (bb2d726) the test succeeds, so it must have something to do with some change on |
|
Ok, the explanation why the test doesn't crash on master is simple: due to the errors in the test logic, the test |
|
I found the root cause. It has nothing to do with your pull request. Need some time to think about the fix, though. |
|
The current master breaks down long requests into smaller pieces. |
|
Yes thanks, I know. I looked in the wrong place for a while, because I thought that the test worked on master and failed in this branch only. But when I noticed that the test wasn't executed on master because of the broken test logic (which this pr fixes), started looking in your code and it was easy to find. |
|
Congratulations @ciz, it looks like your fix of the test logic uncovered another bug which slipped into master because the test suite didn't detect it properly before. I took a short look at it yesterday night and I think I know what's going on there. It has nothing to do with your your changes, they are fine. Your pull request only needs to hang around a little more, until I have the fix for the error. I'll take care of it, all you need to do is to wait (unless you want to hunt the bug for yourself). |
|
Next bugfix, still not the last one. I'm already hunting the next one. |
|
@paulidale with the next bug I need your assistance: The upcoming drbgtest failure (Travis is still running) is caused by the fact that the callbacks of the Actually, the callbacks get cleared by
The RAND_DRBG callbacks are
and the associated PROV_DRBG callbacks are
Here is the situation after
but the PROV_DRBG callbacks have been cleared:
Now my question: is there a way to uninstantiate the RAND_DRBG without having to call RAND_DRBG_set() (e.g., just call |
|
@paulidale as I already anticipated in #11195 (comment),
I am paying a bitter penance for letting @ciz' pull request stall too long. If it had gone into master before #11682, then you would have had to fix your bugs yourself ;-). |
The condition in test_error_checks() was inverted, so it succeeded as long as error_check() failed. Incidently, error_check() contained several bugs that assured it always failed, thus giving overall drbg test success.
The behaviour of RAND_DRBG_generate() has changed. Previously, it would fail for requests larger than max_request, now it automatically splits large input into chunks (which was previously done only by RAND_DRBG_bytes() before calling RAND_DRBG_generate()). So this test has not only become obsolete, the fact that it succeeded unexpectedly also caused a buffer overflow that terminated the test.
Fixes a compiler warning which was treated as an error: test/drbgtest.c:179:13: error: unused function 'max_request' [-Werror,-Wunused-function] DRBG_SIZE_T(max_request)
It's the generate counter (drbg->reseed_gen_counter), not the reseed counter which needs to be raised above the reseed_interval. This mix-up was partially caused by some recent renamings of DRBG members variables, but that will be dealt with in a separate commit.
The RAND_DRBG callbacks are wrappers around the EVP_RAND callbacks. During uninstantiation, the EVP_RAND callbacks got lost while the RAND_DRBG callbacks remained, because RAND_DRBG_uninstantiate() calls RAND_DRBG_set(), which recreates the EVP_RAND object. This was causing drbgtest failures. This commit fixes the problem by adding code to RAND_DRBG_set() for saving and restoring the EVP_RAND callbacks.
|
The second force-push was a rebase on master without further changes in order to pickup @paulidale's change ce3080e. |
|
Looks good. |
|
This pull request is ready to merge |
The condition in test_error_checks() was inverted, so it succeeded as long as error_check() failed. Incidently, error_check() contained several bugs that assured it always failed, thus giving overall drbg test success. Reviewed-by: Paul Dale <paul.dale@oracle.com> Reviewed-by: Matthias St. Pierre <Matthias.St.Pierre@ncp-e.com> (Merged from #11195)
The reseed counter condition was broken since a93ba40, where the initial value was wrongly changed from one to zero. Commit 8bf3665 fixed the initialization, but also adjusted the check, so the problem remained. This change restores original (OpenSSL-fips-2_0-stable) behavior. Reviewed-by: Paul Dale <paul.dale@oracle.com> Reviewed-by: Matthias St. Pierre <Matthias.St.Pierre@ncp-e.com> (Merged from #11195)
The behaviour of RAND_DRBG_generate() has changed. Previously, it would fail for requests larger than max_request, now it automatically splits large input into chunks (which was previously done only by RAND_DRBG_bytes() before calling RAND_DRBG_generate()). So this test has not only become obsolete, the fact that it succeeded unexpectedly also caused a buffer overflow that terminated the test. Reviewed-by: Paul Dale <paul.dale@oracle.com> (Merged from #11195)
It's the generate counter (drbg->reseed_gen_counter), not the reseed counter which needs to be raised above the reseed_interval. This mix-up was partially caused by some recent renamings of DRBG members variables, but that will be dealt with in a separate commit. Reviewed-by: Paul Dale <paul.dale@oracle.com> (Merged from #11195)
The RAND_DRBG callbacks are wrappers around the EVP_RAND callbacks. During uninstantiation, the EVP_RAND callbacks got lost while the RAND_DRBG callbacks remained, because RAND_DRBG_uninstantiate() calls RAND_DRBG_set(), which recreates the EVP_RAND object. This was causing drbgtest failures. This commit fixes the problem by adding code to RAND_DRBG_set() for saving and restoring the EVP_RAND callbacks. Reviewed-by: Paul Dale <paul.dale@oracle.com> (Merged from #11195)
|
Merged to the master branch. Thank you for your patience, @ciz! a27cb95 Fix: uninstantiation breaks the RAND_DRBG callback mechanism |
The condition in test_error_checks() was inverted, so the test succeeded as long as error_check() failed. Incidently, error_check() contained several bugs that assured it always failed, thus giving overall drbg test success. Remove the broken explicit zero check. RAND_DRBG_uninstantiate() cleanses the data via drbg_ctr_uninstantiate(), but right after that it resets drbg->data.ctr using RAND_DRBG_set(), so TEST_mem_eq(zero, sizeof(drbg->data)) always failed. (backport from #11195) Signed-off-by: Vitezslav Cizek <vcizek@suse.com> Reviewed-by: Paul Dale <paul.dale@oracle.com> Reviewed-by: Matthias St. Pierre <Matthias.St.Pierre@ncp-e.com> (Merged from #12517)
The condition in test_error_checks() was inverted, so the test succeeded
as long as error_check() failed. Incidently, error_check() contained
several bugs that assured it always failed, thus giving overall drbg
test success.