Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Unsets global libcrypto rand #4424

Merged
merged 7 commits into from
Feb 23, 2024
Merged

fix: Unsets global libcrypto rand #4424

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
dfb2373
Unsets global rand
maddeleine Feb 17, 2024
dcff9e0
Removes .c include
maddeleine Feb 17, 2024
2a01e21
Gating test on custom rand
maddeleine Feb 17, 2024
40aa1ec
Fixing #if
maddeleine Feb 19, 2024
74ae433
PR feedback
maddeleine Feb 20, 2024
4cf7204
PR feedback
maddeleine Feb 22, 2024
9b1b6d0
Merge branch 'main' into ossl_rand
maddeleine Feb 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 1 addition & 6 deletions tests/s2n_test.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,17 +61,12 @@ int test_count;
* not initialise at the start of the test. Useful for tests that e.g spawn a
* number of independent childs at the start of a unit test and where you want
* each child to have its own independently initialised s2n.
*
* BEGIN_TEST() prints unit test information to stdout. But this often gets
* buffered by the kernel and will then be flushed in each child spawned. The
* result is a number of repeated messages being send to stdout and, in turn,
* appear in the logs. At the moment, we think this is better than risking not
* having any printing at all.
*/
#define BEGIN_TEST_NO_INIT() \
do { \
test_count = 0; \
fprintf(stdout, "Running %-50s ... ", __FILE__); \
fflush(stdout); \
EXPECT_SUCCESS_WITHOUT_COUNT(s2n_in_unit_test_set(true)); \
S2N_TEST_OPTIONALLY_ENABLE_FIPS_MODE(); \
} while(0)
Expand Down
18 changes: 18 additions & 0 deletions tests/unit/s2n_random_test.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -792,6 +792,23 @@ static int s2n_random_rand_bytes_after_cleanup_cb(struct random_test_case *test_
return S2N_SUCCESS;
}

static int s2n_random_rand_bytes_before_init(struct random_test_case *test_case)
{
#if S2N_LIBCRYPTO_SUPPORTS_CUSTOM_RAND
/* Calling RAND_bytes will set a global random method */
unsigned char rndbytes[16] = { 0 };
maddeleine marked this conversation as resolved.
Show resolved Hide resolved
EXPECT_EQUAL(RAND_bytes(rndbytes, sizeof(rndbytes)), 1);

EXPECT_SUCCESS(s2n_init());

/* The global random method is overridden after calling s2n_init() */
const RAND_METHOD *custom_rand_method = RAND_get_rand_method();
EXPECT_NOT_NULL(custom_rand_method);
EXPECT_EQUAL(custom_rand_method->bytes, s2n_openssl_compat_rand);
#endif
return S2N_SUCCESS;
maddeleine marked this conversation as resolved.
Show resolved Hide resolved
}

static int s2n_random_invalid_urandom_fd_cb(struct random_test_case *test_case)
{
EXPECT_SUCCESS(s2n_disable_atexit());
Expand Down Expand Up @@ -862,6 +879,7 @@ struct random_test_case random_test_cases[] = {
{ "Test failure.", s2n_random_test_case_failure_cb, CLONE_TEST_DETERMINE_AT_RUNTIME, 1 },
{ "Test libcrypto's RAND engine is reset correctly after manual s2n_cleanup()", s2n_random_rand_bytes_after_cleanup_cb, CLONE_TEST_DETERMINE_AT_RUNTIME, EXIT_SUCCESS },
{ "Test getting entropy with an invalid file descriptor", s2n_random_invalid_urandom_fd_cb, CLONE_TEST_DETERMINE_AT_RUNTIME, EXIT_SUCCESS },
{ "Test libcrypto's global RAND is unset after calling s2n_init()", s2n_random_rand_bytes_before_init, CLONE_TEST_DETERMINE_AT_RUNTIME, EXIT_SUCCESS },
};

int main(int argc, char **argv)
Expand Down
4 changes: 4 additions & 0 deletions utils/s2n_random.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -553,6 +553,10 @@ S2N_RESULT s2n_rand_init(void)
return S2N_RESULT_OK;
}

/* Unset any existing random engine */
RAND_set_rand_engine(NULL);
RAND_set_rand_method(NULL);
maddeleine marked this conversation as resolved.
Show resolved Hide resolved

Copy link
Contributor

Choose a reason for hiding this comment

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

Why do these methods work, but how we set the engine / method below don't work? Should we be using these methods to set our randomness instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's due to how Openssl gets a random method to use when it needs random bytes. Basically there is a global variable called default_rand_meth which is used over any random engine. Normally, we are able to set our random engine as the default. However, if you call RAND_bytes before setting up our engine, that default_rand_method gets set to the Openssl randomness function, and therefore our randomness method will never be chosen over Openssl's.
To get around this we have to call a function that alters the default_rand_method variable after it's already been set. So we basically have two options, either clean up the default random method before setting up the engine code(what I did), or calling RAND_set_rand_engine() with our custom rand method. I don't think it really matters which one we choose.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Another piece of the puzzle, apparently we used to use the RAND_set_rand_method() before replacing it with ENGINE code. But it's very weird that the ENGINE code doesn't fully replace the RAND code.

Copy link
Contributor

Choose a reason for hiding this comment

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

The behavior of the default_rand_meth global is very confusing. If I'm reading the OpenSSL documentation correctly, this shouldn't be happening:
https://www.openssl.org/docs/man1.0.2/man3/RAND_set_rand_method.html#notes

If a default ENGINE is specified for RAND functionality using an ENGINE API function, that will override any RAND defaults set using the RAND API (ie. RAND_set_rand_method()). For this reason, the ENGINE API is the recommended way to control default implementations for use in RAND and other cryptographic algorithms.

But other than calling RAND_set_rand_method()/RAND_set_rand_engine(), there doesn't seem to be a way to override the default_rand_meth variable after it's set, except for OPENSSL_cleanup().

Given that the RAND methods are deprecated, it seems like it might be best to use Madeleine's current implementation and keep the ENGINE APIs to actually set our random implementation.

Copy link
Contributor Author

@maddeleine maddeleine Feb 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Both ENGINE APIs and RAND APIs are deprecated. Rather than spending time getting our ENGINE code right, I think we should apply this fix and work towards this solution in the long term: #4348. If this is a bug, I doubt Openssl would care about it since technically the new thing for this functionality is "providers".
Also a note: I don't think there is a way to do what we want with the ENGINE APIs after staring at the codepaths for a bit. Nothing touches that default_rand_meth except RAND APIs.

/* Create an engine */
ENGINE *e = ENGINE_new();

Expand Down
1 change: 1 addition & 0 deletions utils/s2n_random.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,3 +40,4 @@ S2N_RESULT s2n_get_public_random_bytes_used(uint64_t *bytes_used);
S2N_RESULT s2n_get_private_random_data(struct s2n_blob *blob);
S2N_RESULT s2n_get_private_random_bytes_used(uint64_t *bytes_used);
S2N_RESULT s2n_public_random(int64_t max, uint64_t *output);
int s2n_openssl_compat_rand(unsigned char *buf, int num);
Loading