Fix clear_entropy infinite loop

[Aurum-Vale]

This PR fixes a bug when instantiating repeatedly a DRBG, below is an example on how to trigger the bug with the current master branch, using the test entropy source flag at compile time.

#define ASKED_STRENGTH 256
#define DST_LEN 32

int main()
{
		drbg_ctx drbg;

		for (int i = 0; i < 100; i++) {
			// printf("Starting iteration %i\n", i);
			const unsigned char pers_string[] = "DRBG_PERS";
			uint32_t asked_strength = ASKED_STRENGTH;

			drbg_error ret =
					drbg_instantiate(&drbg, pers_string, sizeof(pers_string) - 1, &asked_strength, true, DRBG_CTR, NULL);

			if (ret != DRBG_OK) {
				printf("Failed instantiation %i with error %i\n", i, ret);
				continue;
			}

			// Necessary call to trigger the bug
			uint8_t dst[DST_LEN];
			drbg_generate(&drbg, NULL, 0, dst, DST_LEN, true);

			drbg_uninstantiate(&drbg);
		}

	return 0;
}

The bug is a combination of a logic bug in the DRBG interface, and of an improper uninitialization of the entropy pool in the test entropy source.

The test entropy source is improperly uninitialized with clear_entropy(), with the current position of the cursor pool not being reset. This eventually leads to the clear_entropy() function failing its sanity check and returning an error code.

The error code triggers a goto instruction in _drbg_instantiate(), as is the common pattern throughout the codebase, but in this case, the goto instruction happens after the label. This causes an infinite loop. Removing the goto instruction avoids the infinite loop, and does not prevent further entropy clearing.

Additionally, I made it so the drbg_uninstantiate() function returns an error code if needed, while still enforcing proper uninitialization (prior to that, it always returned DRBG_OK).

The PR may need to be formated to your own code style. I am not used to PRs from a fork, so I am eager to receive any feedback.

[rben-dev]

Hi @Aurum-Vale

Thanks for the PR, well spotted for the dangling goto err. Following the same logic, can you please also patch this one:

libdrbg/drbg.c

Line 402 in ea2d18b

if(entropy_pool != NULL){

and add it to this PR?

The rest of the PR seems all right to me :-) @ae-anssi : if you are OK with this, I think we can merge after the new commit!

Regards,

[Aurum-Vale]

Hi, I've added a commit as you asked.

[rben-dev]

Thanks @Aurum-Vale. Waiting for @ae-anssi review for the merge.

[ae-anssi]

Hi @Aurum-Vale. Just merged the PR. Thanks for spotting those and submitted fixes!

@rben-dev : somewhat unrelated but I guess our distrib all have gcc as default. While testing with "CC=clang make", I get errors due to the -Wunsafe-buffer-usage. Not sure this can be easily handled (see llvm/llvm-project#64646).

[rben-dev]

Thanks @ae-anssi for the merge. Regarding the clang error well spotted, this is indeed a false positive: I will push a fix to avoid it