Enforce minimum key size when generating RSA key size

[waleed-elmelegy-arm]

Add configuration to enforce minimum size when
generating a RSA key, it's default value is 1024
bits since this the minimum secure value currently but it can be any value greater than or equal 128
bits. Tests were modified to accommodate for this
change.

Description

Add configuration to enforce minimum size when
generating a RSA key, it's default value is 1024
bits since this the minimum secure value currently but it can be any value greater than or equal 128
bits. Tests were modified to accommodate for this
change.
fixes #7556

PR checklist

Please tick as appropriate and edit the reasons (e.g.: "backport: not needed because this is a new feature")

• changelog provided
• backport not required - adding a new config option seems a bit invasive for a backport
• tests provided, or not required

Notes for the submitter

Please refer to the contributing guidelines, especially the
checklist for PR contributors.

[gilles-peskine-arm]

IIRC numerical values need to go into the “General configuration options” section, due to how config.py works. And for those there's an #if !defined(…) clause in the relevant header (here rsa.h) to set a default if the value is not set in the config file (users can write their own config file where they only enable the options they care about). I'm not sure if that's why test_psa_crypto_config_accel_rsa_signature is failing.

[waleed-elmelegy-arm]

Added it, Thank you this was the reason CI was failing.

[gilles-peskine-arm]

Rather parametrize the test cases on MBEDTLS_RSA_MIN_KEY_SIZE:

Suggested change

	RSA Generate Key - 128bit key
	depends_on:MBEDTLS_RSA_MIN_KEY_SIZE == 128
	mbedtls_rsa_gen_key:128:3:0
	RSA Generate Key - 128bit key (Less than minimum size)
	depends_on:MBEDTLS_RSA_MIN_KEY_SIZE > 128
	mbedtls_rsa_gen_key:128:3:MBEDTLS_ERR_RSA_BAD_INPUT_DATA
	RSA Generate Key - minimum size
	mbedtls_rsa_gen_key:MBEDTLS_RSA_MIN_KEY_SIZE:3:0
	RSA Generate Key - less than minimum size
	depends_on:MBEDTLS_RSA_MIN_KEY_SIZE - 2 >= 128
	mbedtls_rsa_gen_key:MBEDTLS_RSA_MIN_KEY_SIZE - 2:3:MBEDTLS_ERR_RSA_BAD_INPUT_DATA

[waleed-elmelegy-arm]

Done.
(for some reason MBEDTLS_RSA_MIN_KEY_SIZE - 2 was giving an error so I changed it to MBEDTLS_RSA_MIN_KEY_SIZE >= 130 )

[gilles-peskine-arm]

Ah, yes. Dependencies have to be of the form NAME, !NAME or NAME OP INTEGER where OP is a comparison operator. (I don't think there's really a good reason for that.)

[gilles-peskine-arm]

Looks mostly good to me, but a few minor issues, mostly with naming and documentation.

[gilles-peskine-arm]

More precisely, this is the minimum allowed for generation. It's still possible to import a smaller key.

So maybe the name should indicate that? (See also my comment about the PSA constant, which definitely needs a more precise name.) Something like MBEDTLS_RSA_GEN_KEY_MIN_SIZE given that the affected function is called mbedtls_rsa_gen_key.

[daverodgman]

Agree, but maybe MBEDTLS_RSA_GENERATE_KEY_MIN_SIZE rather than MBEDTLS_RSA_GEN_KEY_MIN_SIZE?

[daverodgman]

Also, we should enforce the 128 bit minimum via check_config.h

[gilles-peskine-arm]

Regarding the name: why not stick to something close to the name of the relevant function?

Regarding where to enforce the minimum: this would be better done in rsa.h or rsa.c. check_config.h is more suited to cross-module considerations.

[waleed-elmelegy-arm]

changed it to MBEDTLS_RSA_GEN_KEY_MIN_SIZE and added a check in rsa.h to check for 128 bits minimum, I removed the check from mbedtls_rsa_gen_key

[gilles-peskine-arm]

This is not actually enforced when RSA key generation goes through a driver. I'm not sure we need to enforce it. If we do, we could do it with #if … > 128 #error.

[waleed-elmelegy-arm]

Done.

[tom-cosgrove-arm]

LGTM

[gilles-peskine-arm]

Something went wrong in the latest rebase: a block of changes in test_suite_psa_crypto.data disappeared in "Improve naming of mimimum RSA key size generation configurations"

Other than that, looks good to me.

[gilles-peskine-arm]

Sorry, d7bdbbe is still not right. The test improvements to adjust to a world where RSA key generation has a minimum of 1024, which were added in d9b303c, are still missing.

I think that the pull request is good otherwise, and its history makes sense . "Add a minimum rsa key size config to psa config" now doesn't adjust all the test cases optimally, but that's ok, it's the sort of things that can be spread into multiple commits. So I propose to stop attempting rebases and instead do an adjustment of the tests in a new commit.

The two things that are wrong in d7bdbbe are:

• Some test cases are never executed on the CI, because they try to generate an RSA key that's too small. You can see that in the CI logs, in the step “tests/scripts/analyze_outcomes.py outcomes.csv”. This step is not shown in the default “blue ocean” view due to limitations of this view for which we haven't implemented a workaround yet, you have to go to the pipeline steps in the “classic” view. Search for Warning: Test case not executed: test_suite_psa_crypto;PSA generate key: RSA. This is a warning rather than an error because we have a backlog of test cases that are never executed (or wrongly detected as such), but we shouldn't add new ones.
• Some negative test cases are now failing because they try to generate an RSA key that's too small, rather than due to the other error condition that we wanted to test.

I've left individual comments on each of the test cases that need updating (with one comment covering two test cases).

[gilles-peskine-arm]

LGTM

[gilles-peskine-arm]

Non-blocker: why PSA_VENDOR_RSA_GENERATE_MIN_KEY_BITS <= 2048?

[waleed-elmelegy-arm]

because of this line:
unsigned char ciphertext[256] = "(wabblewebblewibblewobblewubble)"; in psa_exercise_key.c:378
the output size is not enough for the output.

[gilles-peskine-arm]

Ah, I see. The hard-coded sizes are probably back from before there were proper macros for buffer sizes. I've filed #8031 to remember about it. It's not urgent.