Reorganise reseeding #1941

torben-hansen · 2024-10-22T23:29:11Z

Call-outs:

To avoid synchronisation issues for codepoints that reads/mutates the ctr-drbg state in #1919, we must reorganise the reseed logic. This decouples the code that reads/mutates the ctr-drbg state and the code that gathers entropy. The latter is not an issue. The former will later be wrapped as critical code needing synchronisation as part of the global zeroisation.

Testing:

Added new tests that verifies we can predict the time of reseeding based on the configured reseed bound.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and the ISC license.

codecov-commenter · 2024-10-22T23:50:59Z

Codecov Report

Attention: Patch coverage is 90.32258% with 3 lines in your changes missing coverage. Please review.

Project coverage is 78.57%. Comparing base (ccb97ef) to head (59a871f).

Files with missing lines	Patch %	Lines
crypto/fipsmodule/rand/new_rand.c	86.95%	3 Missing ⚠️

Additional details and impacted files

@@                    Coverage Diff                    @@
##           randomness_generation    #1941      +/-   ##
=========================================================
+ Coverage                  78.46%   78.57%   +0.10%     
=========================================================
  Files                        585      585              
  Lines                      97061    98008     +947     
  Branches                   13920    13922       +2     
=========================================================
+ Hits                       76160    77010     +850     
- Misses                     20282    20380      +98     
+ Partials                     619      618       -1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

andrewhop · 2024-10-25T19:41:18Z

crypto/fipsmodule/rand/new_rand.c

+  uint8_t personalization_string[CTR_DRBG_ENTROPY_LEN],
+  size_t personalization_string_len) {


Why take the string length if you're requiring a specific size? You don't take a size for the seed. Also do we need a compiler flag to enforce this? I tried passing in a huge array for seed/personalization string and nothing complained.

because you might not use the personalization string in which case personalization_string_len would be 0.

For consistency CTR_DRBG_reseed doesn't define the argument in this way. Should we just make them both consistent?

const uint8_t *additional_data, size_t additional_data_len

I just use the name personalization_string to mean both additional data and personalization string. the latter is the name used in SP800-90A for the extra input for reseeding, while the latter is the name used for the extra input for initial seeding. I renamed stuff. now use "extra entropy" as a general name and flip to either "personalization string" or "additional data" depending on context.

andrewhop · 2024-10-25T19:44:16Z

crypto/fipsmodule/rand/new_rand.c

+  // must_reseed_before_generate is 1 if we must reseed before invoking the
+  // CTR-DRBG generate function CTR_DRBG_generate().
+  int must_reseed_before_generate = 0;
+
+  // Ensure the CTR-DRBG state is safe to use.
  if (rand_ensure_ctr_drbg_uniqueness(state) == 1) {
-    rand_ctr_drbg_reseed(state);
+    must_reseed_before_generate = 1;
  }


Why is all of this checked up here and not down around 267?

Because it doesn't have to be checked iteratively. So, preventing redundant work.

andrewhop · 2024-10-25T19:46:03Z

crypto/fipsmodule/rand/new_rand.c

+  // CTR-DRBG generate function CTR_DRBG_generate().
+  int must_reseed_before_generate = 0;
+
+  // Ensure the CTR-DRBG state is safe to use.
  if (rand_ensure_ctr_drbg_uniqueness(state) == 1) {


NP: It's not immediately obvious what this function does when reading the name, I had to look up what this function does when reading this. What about is_uniqueness_broken or need_to_reseed_for_uniqueness

Renamed to rand_check_ctr_drbg_uniqueness and also flipped the return value: 0 is now "bad"and1` is "good"

andrewhop · 2024-10-25T19:51:51Z

crypto/fipsmodule/rand/new_rand.c

+
+      must_reseed_before_generate = 0;
+
+      // TODO: unlock here


Why are you planning to lock up on 253 to unlock here and then lock again on 279? Is the only important part to lock in rand_ctr_drbg_reseed? Could that handle the locking inside that function?

Need to synchronise access to the CTR-DRBG with the global memory manager that can also mutate the state.

line 253 and 304 wraps the critical code under the while-loop. The powerpc then necessitates the unlock-lock cycle if needing to reseed due to reseed interval - but only if that condition ever becomes true. I don't want to handle that in a child function because then a post-condition to that function will be that the lock as already been acquired otherwise unlocking will fail and abort the process.

andrewhop · 2024-10-25T19:55:00Z

crypto/fipsmodule/rand/new_rand_test.cc

+  // Should be able to perform kCtrDrbgReseedInterval-2 generate calls before a
+  // reseed is emitted. Requesting
+  // CTR_DRBG_MAX_GENERATE_LENGTH * (kCtrDrbgReseedInterval-2) + 1 would require
+  // quite a large buffer. Instead iterate until we need
+  // 5 iterations and request 5 * CTR_DRBG_MAX_GENERATE_LENGTH+1, which is a
+  // much smaller buffer.


What are you trying to test here that isn't covered in the first section?

yeah, it's sorta the same thing. Just trying to make sure to exercise multiple iterations of the while-loop that calls the CTR_DRBG_generate() function.

andrewhop · 2024-10-25T19:59:26Z

crypto/fipsmodule/rand/new_rand.c

-  uint8_t seed[CTR_DRBG_ENTROPY_LEN];
-  uint8_t personalization_string[CTR_DRBG_ENTROPY_LEN];
-  size_t personalization_string_len = 0;
-  rand_get_ctr_drbg_seed_entropy(state->entropy_source, seed,
-    personalization_string, &personalization_string_len);


I don't get why you moved this out of this function. Also it's only used in one spot, does it make more sense to just put this all in RAND_bytes_core?

I need to decouple the two operations 1) gathering entropy 2) using that entropy to reseed. rand_get_ctr_drbg_seed_entropy() is used twice. rand_ctr_drbg_reseed() is now only used once, but I still like to factor out a wrapper for reseeding. I think it makes reading the core function easier.

skmcgrail · 2024-10-28T17:39:53Z

crypto/fipsmodule/rand/new_rand.c

+  uint8_t personalization_string[CTR_DRBG_ENTROPY_LEN],
+  size_t personalization_string_len) {


For consistency CTR_DRBG_reseed doesn't define the argument in this way. Should we just make them both consistent?

const uint8_t *additional_data, size_t additional_data_len

crypto/fipsmodule/rand/new_rand.c

skmcgrail · 2024-10-28T17:43:13Z

crypto/fipsmodule/rand/new_rand.c

+static void rand_ctr_drbg_reseed(struct rand_thread_local_state *state,
+  uint8_t seed[CTR_DRBG_ENTROPY_LEN],
+  uint8_t personalization_string[CTR_DRBG_ENTROPY_LEN],
+  size_t personalization_string_len) {

  GUARD_PTR_ABORT(state);


Should seed and personalization_string be guarded for NULL or do we enforce that earlier on higher in the function call stack? Since these are just passed as pointers technically, not sure if the compiler would catch these otherwise?

I only use array types when calling these functions. so in practice no problem.

dkostic · 2024-11-01T18:26:20Z

crypto/fipsmodule/rand/new_rand.c

+    if (must_reseed_before_generate == 1 ||
+       (state->generate_calls_since_seed + 1) > kCtrDrbgReseedInterval) {
+
+      must_reseed_before_generate = 0;


nit: maybe move this to after reseeding is done.

To avoid synchronisation issues for codepoints that reads/mutates the ctr-drbg state in aws#1919, we must reorganise the reseed logic. This decouples the code that reads/mutates the ctr-drbg state and the code that gathers entropy. The latter is not an issue. The former will later be wrapped as critical code needing synchronisation as part of the global zeroisation.

Restrict RAND_bytes request length and reorganise reseeding

9e502fd

torben-hansen added 2 commits October 23, 2024 12:25

clarifications

9c78d2e

move reseed to under while-loop instead

88160a0

torben-hansen changed the title ~~Restrict RAND_bytes request length and reorganise reseeding~~ Reorganise reseeding Oct 23, 2024

Roll back some changes

8a0ce84

torben-hansen requested review from skmcgrail and andrewhop October 23, 2024 22:21

torben-hansen marked this pull request as ready for review October 23, 2024 22:21

torben-hansen requested a review from a team as a code owner October 23, 2024 22:21

andrewhop reviewed Oct 25, 2024

View reviewed changes

Rename function and change return value semantics

e631db2

skmcgrail reviewed Oct 28, 2024

View reviewed changes

torben-hansen requested review from skmcgrail and andrewhop October 28, 2024 20:51

Rename things

59a871f

skmcgrail approved these changes Oct 29, 2024

View reviewed changes

dkostic approved these changes Nov 1, 2024

View reviewed changes

torben-hansen merged commit 72f0a9f into aws:randomness_generation Nov 1, 2024
100 of 106 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reorganise reseeding #1941

Reorganise reseeding #1941

torben-hansen commented Oct 22, 2024 •

edited

Loading

codecov-commenter commented Oct 22, 2024 •

edited

Loading

andrewhop Oct 25, 2024

torben-hansen Oct 28, 2024

skmcgrail Oct 28, 2024

torben-hansen Oct 28, 2024

andrewhop Oct 25, 2024

torben-hansen Oct 28, 2024

andrewhop Oct 25, 2024

torben-hansen Oct 28, 2024

andrewhop Oct 25, 2024

torben-hansen Oct 28, 2024

andrewhop Oct 25, 2024

torben-hansen Oct 28, 2024

andrewhop Oct 25, 2024

torben-hansen Oct 28, 2024

skmcgrail Oct 28, 2024

skmcgrail Oct 28, 2024

torben-hansen Oct 28, 2024

dkostic Nov 1, 2024

		uint8_t personalization_string[CTR_DRBG_ENTROPY_LEN],
		size_t personalization_string_len) {

Reorganise reseeding #1941

Reorganise reseeding #1941

Conversation

torben-hansen commented Oct 22, 2024 • edited Loading

Call-outs:

Testing:

codecov-commenter commented Oct 22, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

torben-hansen commented Oct 22, 2024 •

edited

Loading

codecov-commenter commented Oct 22, 2024 •

edited

Loading