Bug fixes for RAND_add(), RAND_seed() and RAND_load_file() by mspncp · Pull Request #7456 · openssl/openssl

mspncp · 2018-10-21T19:02:02Z

~~(Note: this pull request is based on #7455)~~

It turned out that the heap corruption #7449 was only the tip of an iceberg and solving it revealed several other problems. The long story short is

RAND_add()/RAND_seed() failed for buffer sizes less than 32 bytes (i.e. 256 bits). This bug was introduced by (DRBG: fix reseeding via RAND_add()/RAND_seed() with large input #7382).
The error handling was incorrect, which resulted in the heap corruption (fixed by RAND_add(): fix heap corruption in error path #7455).
RAND_load_file() currently slices its input in chunks in an inconvenient way such that the final chunk can be small (which casued RAND_add() to fail).
The failure of RAND_add() went unnoticed by RAND_load_file() because it did not check RAND_status().

This pull request attempts to

fix RAND_add()/RAND_seed()
make RAND_load_file() smarter in the way it slices the input
add more tests

The third part is an essential conclusion from the analysis of #7449: How can it be that such a cardinal error was not caught by a test? It would have gone unnoticed if it hadn't been for the heap corruption to unveil it. So I tried to improve testing. Currently, the tests are commit separately before fixing the issue they address, such that one can test whether the tests work by resetting the HEAD to the respective commit. In the end, I might squash them into the commits which their failures.

Checklist

tests are added or updated (wip)

mspncp · 2018-10-21T19:56:46Z

One test is missing, because my perl-fu was not sufficient to implement it. It would be great if one of the perl-gurus (@levitte?) could convert this naive shell script into a portable test recipe.

test-rand-load-file

#!/bin/bash

RAND_BUF_SIZE=1024
RAND_DRBG_STRENGTH=256

RAND_LOAD_BUF_SIZE=$((RAND_BUF_SIZE + RAND_DRBG_STRENGTH))


for n in $(seq $((2*${RAND_LOAD_BUF_SIZE}))); do
	truncate -s $n RANDFILE
	../util/shlib_wrap.sh ../apps/openssl rand -rand RANDFILE -hex 10 > /dev/null || echo $n failed
done

The test-rand-load-file script can be tested by letting it fail on 02107968d423.

mspncp · 2018-10-21T20:03:52Z

Note: the test-rand-load-file script was outdated and needed some corrections.

mspncp · 2018-10-21T20:14:18Z

What also needs more testing is the case without os entropy source (OPENSSL_RAND_SEED_NONE).

Originally, Kurt had implemented a test which simulated this situation by replacing the get_entropy() callbacks, but it turned out that ef40152c5e8c can be tested authentically only if one configures --with-rand-seed=none. The new tests test_rand_seed and test_rand_add succeed without os entropy, but there are a lot of other tests which currently fail. Most of them would work if the DRBG would be manually instantiated. Some of the drbgtests need special treatment. I will address this in a separate pull request.

paulidale · 2018-10-21T22:18:16Z

crypto/rand/drbg_lib.c

This makes me nervous. Could we lose the static here, delete the copy in drbgtest.c and statically link the drbgtest executable? The symbol won't be exported from libcrypto, the test will gain access and the duplication is gone.

Of course I tried to to reuse the function from drbg_lib.c in the tests. In fact, I thought I had read in some place that the tests were linked statically. After this proved wrong I did not want to export a private symbol, so I thought adding a warning comment would be the best compromise.

Changing the test framework to link the tests statically is out of my scope, so one of you would have to do that.

@paulidale thanks for your hints regarding build.info, with your explanation the change was really trivial, see #7462.

crypto/rand/drbg_lib.c

paulidale · 2018-10-21T22:23:33Z

crypto/rand/drbg_lib.c

Nit: spaces around the '*'

mspncp · 2018-10-22T16:59:17Z

@paulidale, I rebased on master, without any squashing, i.e. all fixups related to your comments are separate commits. Note also that commit 6f891202ce is a substitute for PR #7462.

d569cf3d41 fixup! DRBG: fix implementations of RAND_add() and RAND_seed()
4a511821a0 fixup! DRBG: fix implementations of RAND_add() and RAND_seed()
d910420c99 DRBG: fix implementations of RAND_add() and RAND_seed()
3f7867476b DRBG: refine tests for RAND_add() and RAND_seed()
8cf8abcd6e RAND_load_file(): avoid adding small chunks to RAND_add()
45588082db RAND_load_file(): return error if reseeding failed
6f891202ce Test: link drbgtest statically against libcrypto [PR #7462]

paulidale

One trivial nit.

paulidale · 2018-10-22T21:48:49Z

crypto/rand/drbg_lib.c

Cast to size_t instead of unsigned int?

mspncp · 2018-10-24T16:22:33Z

@paulidale @levitte it looks like currently the price for avoiding a single duplicated function (rand_drbg_seedlen()) is just too high: we loose the entire drbgtest on windows with shared libraries configured. This is much worse than a little code duplication.

My suggestion now is to revert the static linking of the drbgtest and stick with the code duplication until Richard manages to get the static tests working for shared windows configurations. Maybe it can be done using the ideas from #7462 (comment).

What's your opinion?

paulidale · 2018-10-24T20:55:28Z

Okay with me, apologies for sending you on this quixotic chase. I thought it was easy...

mspncp · 2018-10-25T06:35:50Z

No problem. You were right to ask for it and you couldn't know.

mspncp · 2018-10-25T11:28:20Z

@paulidale I will squash the fixups shortly and put the pull request out of WIP. For better transparency, I push all fixups first before doing the squashing.

commit 64f3eed257a9ae265f5292241f95a09a1bc4b721 (HEAD -> pr-fix-rand-add-and-friends, msp/pr-fix-rand-add-and-friends)
Author: Dr. Matthias St. Pierre [email protected]
Date: Thu Oct 25 13:01:46 2018 +0200

Revert conversion to drbgtest to static linking

Revert "fixup! DRBG: fix implementations of RAND_add() and RAND_seed()"
This reverts commit 4a511821a09e803d6b516d3fff958373bf55fd85.
Revert "Test: link drbgtest statically against libcrypto [PR Test: link drbgtest statically against libcrypto #7462]"
This reverts commit 6f891202ce0131a44034249614ffb75f229cf6b6.

This change is postponed until the technical problems are solved that
prevent tests on Windows to link statically against libcrypto.

mspncp · 2018-10-25T11:44:24Z

Ok, branch has been squashed without moving merge-base. The unsquashed branch was retained at https://github.com/mspncp/openssl/commits/pr-fix-rand-add-and-friends-unsquashed.

# note: you can checkout a local copy using:

git fetch https://github.com/mspncp/openssl.git pr-fix-rand-add-and-friends-unsquashed
git checkout -b pr-fix-rand-add-and-friends-unsquashed FETCH_HEAD

I noticed that the commit messages still need to be completed. I'll do that this evening and then put it out of WIP.

The failure of RAND_load_file was only noticed because of the heap corruption which was reported in openssl#7499 and fixed in commit 5b4cb38. To prevent this in the future, RAND_load_file() now explicitly checks RAND_status() and reports an error if it fails. Related-to openssl#7449

Increase the load buffer size such that it exceeds the chunk size by a comfortable amount. This is done to avoid calling RAND_add() with a small final chunk. Instead, such a small final chunk will be added together with the previous chunk (unless it's the only one). Related-to openssl#7449

Commit 5b4cb38 (openssl#7382) introduced a bug which had the effect that RAND_add()/RAND_seed() failed for buffer sizes less than 32 bytes. The reason was that now the added random data was used exlusively as entropy source for reseeding. When the random input was too short or contained not enough entropy, the DRBG failed without querying the available entropy sources. This commit makes drbg_add() act smarter: it checks the entropy requirements explicitely. If the random input fails this check, it won't be added as entropy input, but only as additional data. More precisely, the behaviour depends on whether an os entropy source was configured (which is the default on most os): - If an os entropy source is avaible then we declare the buffer content as additional data by setting randomness to zero and trigger a regular reseeding. - If no os entropy source is available, a reseeding will fail inevitably. So drbg_add() uses a trick to mix the buffer contents into the DRBG state without forcing a reseeding: it generates a dummy random byte, using the buffer content as additional data. Related-to openssl#7449

mspncp · 2018-10-25T20:04:32Z

@paulidale the commit messages are now complete and the pr is ready for final review.

I decided to squash the tests into commit b75b060, where they belong to thematically. To make reviewing easier for you, I left the unsquashed branch on my repo for you as a reference (noted already above). This is how you can get it and verify that the code trees are identical:

git fetch https://github.com/mspncp/openssl.git pr-fix-rand-add-and-friends-unsquashed
git checkout -b pr-fix-rand-add-and-friends-unsquashed FETCH_HEAD

git diff pr-fix-rand-add-and-friends  pr-fix-rand-add-and-friends-unsquashed

In fact, both commits point to the same tree b53a3911dba0:

~/src/openssl$ git cat-file -p pr-fix-rand-add-and-friends
tree b53a3911dba0d5e421b2e782a6d23a43da699e11
parent 1e57caee700b5b5b54a84a7cfcc529e296856973
author Dr. Matthias St. Pierre <[email protected]> 1540129534 +0200
committer Dr. Matthias St. Pierre <[email protected]> 1540496702 +0200

RAND_add()/RAND_seed(): fix failure on short input or low entropy

...

~/src/openssl$ git cat-file -p pr-fix-rand-add-and-friends-unsquashed
tree b53a3911dba0d5e421b2e782a6d23a43da699e11
parent 64f3eed257a9ae265f5292241f95a09a1bc4b721
author Dr. Matthias St. Pierre <[email protected]> 1540467160 +0200
committer Dr. Matthias St. Pierre <[email protected]> 1540467160 +0200

fixup! DRBG: fix implementations of RAND_add() and RAND_seed()

Please reapprove

mspncp · 2018-10-25T20:11:48Z

Note: the removal of the code duplication in drbgtest depends on #7462, which in turn depends on #7496. That's why I'll fix it as part of #7462.

The failure of RAND_load_file was only noticed because of the heap corruption which was reported in #7499 and fixed in commit 5b4cb38. To prevent this in the future, RAND_load_file() now explicitly checks RAND_status() and reports an error if it fails. Related-to: #7449 Reviewed-by: Paul Dale <[email protected]> (Merged from #7456)

Increase the load buffer size such that it exceeds the chunk size by a comfortable amount. This is done to avoid calling RAND_add() with a small final chunk. Instead, such a small final chunk will be added together with the previous chunk (unless it's the only one). Related-to: #7449 Reviewed-by: Paul Dale <[email protected]> (Merged from #7456)

Commit 5b4cb38 (#7382) introduced a bug which had the effect that RAND_add()/RAND_seed() failed for buffer sizes less than 32 bytes. The reason was that now the added random data was used exlusively as entropy source for reseeding. When the random input was too short or contained not enough entropy, the DRBG failed without querying the available entropy sources. This commit makes drbg_add() act smarter: it checks the entropy requirements explicitely. If the random input fails this check, it won't be added as entropy input, but only as additional data. More precisely, the behaviour depends on whether an os entropy source was configured (which is the default on most os): - If an os entropy source is avaible then we declare the buffer content as additional data by setting randomness to zero and trigger a regular reseeding. - If no os entropy source is available, a reseeding will fail inevitably. So drbg_add() uses a trick to mix the buffer contents into the DRBG state without forcing a reseeding: it generates a dummy random byte, using the buffer content as additional data. Related-to: #7449 Reviewed-by: Paul Dale <[email protected]> (Merged from #7456)

The failure of RAND_load_file was only noticed because of the heap corruption which was reported in openssl#7499 and fixed in commit 5b4cb38. To prevent this in the future, RAND_load_file() now explicitly checks RAND_status() and reports an error if it fails. Related-to: openssl#7449 Reviewed-by: Paul Dale <[email protected]> (Merged from openssl#7456)

mspncp · 2018-10-26T07:48:03Z

@paulidale there was trivial conflict while cherry-picking the third of the three commits (8817215) from master to 1.1.1:

<<<<<<< HEAD
    master->get_entropy = get_pool_entropy;
    master->cleanup_entropy = cleanup_pool_entropy;
    master->reseed_counter++;
    RAND_DRBG_uninstantiate(master);
    memset(rand_add_buf, 0xCD, sizeof(rand_add_buf));
    RAND_add(rand_add_buf, sizeof(rand_add_buf), sizeof(rand_add_buf));
    if (!TEST_true(RAND_DRBG_instantiate(master, NULL, 0)))
        goto error;
=======
    memset(rand_buf, 0xCD, sizeof(rand_buf));
>>>>>>> 8817215d5c... RAND_add()/RAND_seed(): fix failure on short input or low entropy

It was caused by the renaming of reseed_counter to reseed_prop_counter in 8bf3665, which was not cherry-picked to 1.1.1.

The conflict was resolved by picking the (incoming) version from master:

    memset(rand_buf, 0xCD, sizeof(rand_buf));

The cherry-picked commit can be found here: mspncp@026d39b
(the commit also appears in the gh thread above this post)

Please approve the conflict resolution or let me know whether you prefer a separate pull request.

Increase the load buffer size such that it exceeds the chunk size by a comfortable amount. This is done to avoid calling RAND_add() with a small final chunk. Instead, such a small final chunk will be added together with the previous chunk (unless it's the only one). Related-to: #7449 Reviewed-by: Paul Dale <[email protected]> (Merged from #7456)

crypto/rand/randfile.c

In commit 8bf3665 some renamings andd typo fixes were made while adding back the DRBG-HMAC and DRBG-HASH implementation. Since the commit could not be backported, a lot of unnecessary differences between master and 1.1.1 were introduced. These differences result in tiresome merge conflicts when cherry-picking. To minimize these merge-conflicts, this patch ports all 'non-feature' changes of commit 8bf3665 (e.g., renamings of private variables, fixes of typographical errors, comment changes) manually back to 1.1.1. The commits a83dc59 (openssl#7399) and 8817215 (openssl#7456) failed to cherry-pick previously to 1.1.1, with this patch they both cherry-pick without conflicts.

mspncp · 2018-10-26T21:17:12Z

@paulidale I found a better and more general solution, see fix #7505.

With that fix, the remaining commit will cherry-pick cleanly to 1.1.1, so no reapproval is required.

In commit 8bf3665 some renamings andd typo fixes were made while adding back the DRBG-HMAC and DRBG-HASH implementation. Since the commit could not be backported, a lot of unnecessary differences between master and 1.1.1 were introduced. These differences result in tiresome merge conflicts when cherry-picking. To minimize these merge-conflicts, this patch ports all 'non-feature' changes of commit 8bf3665 (e.g., renamings of private variables, fixes of typographical errors, comment changes) manually back to 1.1.1. The commits a83dc59 (#7399) and 8817215 (#7456) failed to cherry-pick previously to 1.1.1, with this patch they both cherry-pick without conflicts. Reviewed-by: Richard Levitte <[email protected]> Reviewed-by: Bernd Edlinger <[email protected]> (Merged from #7505)

Commit 5b4cb38 (#7382) introduced a bug which had the effect that RAND_add()/RAND_seed() failed for buffer sizes less than 32 bytes. The reason was that now the added random data was used exlusively as entropy source for reseeding. When the random input was too short or contained not enough entropy, the DRBG failed without querying the available entropy sources. This commit makes drbg_add() act smarter: it checks the entropy requirements explicitely. If the random input fails this check, it won't be added as entropy input, but only as additional data. More precisely, the behaviour depends on whether an os entropy source was configured (which is the default on most os): - If an os entropy source is avaible then we declare the buffer content as additional data by setting randomness to zero and trigger a regular reseeding. - If no os entropy source is available, a reseeding will fail inevitably. So drbg_add() uses a trick to mix the buffer contents into the DRBG state without forcing a reseeding: it generates a dummy random byte, using the buffer content as additional data. Related-to: #7449 Reviewed-by: Paul Dale <[email protected]> (Merged from #7456) (cherry picked from commit 8817215)

mspncp · 2018-10-27T11:16:31Z

Finally completed, the last commit has been cherry-picked to 1.1.1. Thanks!

mspncp mentioned this pull request Oct 21, 2018

[Crash] RAND_load_file() with specific file-sizes #7449

Closed

paulidale reviewed Oct 21, 2018

View reviewed changes

mspncp mentioned this pull request Oct 22, 2018

Test: link drbgtest statically against libcrypto #7462

Closed

mspncp force-pushed the pr-fix-rand-add-and-friends branch from ef40152 to d569cf3 Compare October 22, 2018 16:56

mspncp added branch: master Applies to master branch branch: 1.1.1 Applies to OpenSSL_1_1_1-stable branch (EOL) labels Oct 22, 2018

paulidale previously approved these changes Oct 22, 2018

View reviewed changes

crypto/rand/drbg_lib.c Outdated

Copy link

Contributor

paulidale Oct 22, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cast to size_t instead of unsigned int?

mattcaswell added this to the 1.1.1a milestone Oct 23, 2018

mspncp mentioned this pull request Oct 23, 2018

Fix data race in RAND_DRBG_generate #7399

Closed

mspncp force-pushed the pr-fix-rand-add-and-friends branch from 5e6248d to 83b746e Compare October 25, 2018 11:38

mspncp added 3 commits October 25, 2018 21:43

mspncp force-pushed the pr-fix-rand-add-and-friends branch from 83b746e to b75b060 Compare October 25, 2018 19:46

mspncp changed the title ~~WIP: some improvements for RAND_add(), RAND_seed() and RAND_load_file()~~ Bug fixes for RAND_add(), RAND_seed() and RAND_load_file() Oct 25, 2018

paulidale approved these changes Oct 25, 2018

View reviewed changes

bernd-edlinger reviewed Oct 26, 2018

View reviewed changes

crypto/rand/randfile.c Show resolved Hide resolved

mspncp mentioned this pull request Oct 26, 2018

Backport some DRBG renamings and typo fixes #7505

Closed

mspncp closed this Oct 27, 2018

mspncp deleted the pr-fix-rand-add-and-friends branch October 28, 2018 19:58

Uh oh!

Comments

Conversation

mspncp commented Oct 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

mspncp commented Oct 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mspncp commented Oct 21, 2018

Uh oh!

mspncp commented Oct 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

paulidale Oct 21, 2018

Choose a reason for hiding this comment

Uh oh!

mspncp Oct 22, 2018

Choose a reason for hiding this comment

Uh oh!

mspncp Oct 22, 2018

Choose a reason for hiding this comment

Uh oh!

Uh oh!

paulidale Oct 21, 2018

Choose a reason for hiding this comment

Uh oh!

mspncp commented Oct 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

paulidale left a comment

Choose a reason for hiding this comment

Uh oh!

paulidale Oct 22, 2018

Choose a reason for hiding this comment

Uh oh!

mspncp commented Oct 24, 2018

Uh oh!

paulidale commented Oct 24, 2018

Uh oh!

mspncp commented Oct 25, 2018

Uh oh!

mspncp commented Oct 25, 2018

Uh oh!

mspncp commented Oct 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mspncp commented Oct 25, 2018

Uh oh!

mspncp commented Oct 25, 2018

Uh oh!

mspncp commented Oct 26, 2018

Uh oh!

Uh oh!

mspncp commented Oct 26, 2018

Uh oh!

mspncp commented Oct 27, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mspncp commented Oct 21, 2018 •

edited

Loading

mspncp commented Oct 21, 2018 •

edited

Loading

mspncp commented Oct 21, 2018 •

edited

Loading

mspncp commented Oct 22, 2018 •

edited

Loading

mspncp commented Oct 25, 2018 •

edited

Loading