diff mbox series

[08/18] crypto: testmgr - run only subset of DH vectors based on config

Message ID 20211201004858.19831-9-nstange@suse.de (mailing list archive)
State Superseded
Delegated to: Herbert Xu
Headers show
Series crypto: dh - infrastructure for NVM in-band auth and FIPS conformance | expand

Commit Message

Nicolai Stange Dec. 1, 2021, 12:48 a.m. UTC
With the previous patches, the testmgr now has up to four test vectors for
DH which all test more or less the same thing:
- the two vectors from before this series,
- the vector for the ffdhe2048 group, enabled if
  CONFIG_CRYPTO_DH_GROUPS_RFC7919 is set and
- the vector for the modp2048 group, similarly enabled if
  CONFIG_CRYPTO_DH_GROUPS_RFC3526 is set.

In order to avoid too much redundancy during DH testing, enable only a
subset of these depending on the kernel config:
- if CONFIG_CRYPTO_DH_GROUPS_RFC7919 is set, enable only the ffdhe2048
  vector,
- otherwise, if CONFIG_CRYPTO_DH_GROUPS_RFC3526 is set, enable only
  the modp2048 vector and
- only enable the original two vectors if neither of these options
  has been selected.

Note that an upcoming patch will make the DH implementation to reject any
domain parameters not corresponding to some safe-prime group approved by
SP800-56Arev3 in FIPS mode. Thus, having CONFIG_FIPS enabled, but
both of CONFIG_CRYPTO_DH_GROUPS_RFC7919 and
CONFIG_CRYPTO_DH_GROUPS_RFC3526 unset wouldn't make much sense as it would
render the DH implementation unusable in FIPS mode. Conversely, any
reasonable configuration would ensure that the original, non-conforming
test vectors would not get to run in FIPS mode.

Signed-off-by: Nicolai Stange <nstange@suse.de>
---
 crypto/testmgr.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Comments

Hannes Reinecke Dec. 1, 2021, 7:28 a.m. UTC | #1
On 12/1/21 1:48 AM, Nicolai Stange wrote:
> With the previous patches, the testmgr now has up to four test vectors for
> DH which all test more or less the same thing:
> - the two vectors from before this series,
> - the vector for the ffdhe2048 group, enabled if
>    CONFIG_CRYPTO_DH_GROUPS_RFC7919 is set and
> - the vector for the modp2048 group, similarly enabled if
>    CONFIG_CRYPTO_DH_GROUPS_RFC3526 is set.
> 
> In order to avoid too much redundancy during DH testing, enable only a
> subset of these depending on the kernel config:
> - if CONFIG_CRYPTO_DH_GROUPS_RFC7919 is set, enable only the ffdhe2048
>    vector,
> - otherwise, if CONFIG_CRYPTO_DH_GROUPS_RFC3526 is set, enable only
>    the modp2048 vector and
> - only enable the original two vectors if neither of these options
>    has been selected.
> 
> Note that an upcoming patch will make the DH implementation to reject any
> domain parameters not corresponding to some safe-prime group approved by
> SP800-56Arev3 in FIPS mode. Thus, having CONFIG_FIPS enabled, but
> both of CONFIG_CRYPTO_DH_GROUPS_RFC7919 and
> CONFIG_CRYPTO_DH_GROUPS_RFC3526 unset wouldn't make much sense as it would
> render the DH implementation unusable in FIPS mode. Conversely, any
> reasonable configuration would ensure that the original, non-conforming
> test vectors would not get to run in FIPS mode.
> 

For some weird reason the NVMe spec mandates for its TLS profile the 
ffdhe3072 group, so I would prefer if you would be using that as the 
default group for testing.

> Signed-off-by: Nicolai Stange <nstange@suse.de>
> ---
>   crypto/testmgr.h | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/crypto/testmgr.h b/crypto/testmgr.h
> index d18844c7499e..b295512c8f22 100644
> --- a/crypto/testmgr.h
> +++ b/crypto/testmgr.h
> @@ -1331,8 +1331,7 @@ static const struct kpp_testvec dh_tv_template[] = {
>   	.expected_a_public_size = 256,
>   	.expected_ss_size = 256,
>   	},
> -#endif /* IS_ENABLED(CONFIG_CRYPTO_DH_GROUPS_RFC7919) */
> -#if IS_ENABLED(CONFIG_CRYPTO_DH_GROUPS_RFC3526)
> +#elif IS_ENABLED(CONFIG_CRYPTO_DH_GROUPS_RFC3526)
>   	{
>   	.secret =
>   #ifdef __LITTLE_ENDIAN
> @@ -1423,7 +1422,7 @@ static const struct kpp_testvec dh_tv_template[] = {
>   	.expected_a_public_size = 256,
>   	.expected_ss_size = 256,
>   	},
> -#endif /* IS_ENABLED(CONFIG_CRYPTO_DH_GROUPS_RFC3526) */
> +#else
>   	{
>   	.secret =
>   #ifdef __LITTLE_ENDIAN
> @@ -1642,6 +1641,7 @@ static const struct kpp_testvec dh_tv_template[] = {
>   	.expected_a_public_size = 256,
>   	.expected_ss_size = 256,
>   	},
> +#endif
>   };
>   
>   static const struct kpp_testvec curve25519_tv_template[] = {
> 
... and maybe add a config option to run a full test.

Cheers,

Hannes
Nicolai Stange Dec. 9, 2021, 9:18 a.m. UTC | #2
Hannes Reinecke <hare@suse.de> writes:

> On 12/1/21 1:48 AM, Nicolai Stange wrote:
>> With the previous patches, the testmgr now has up to four test vectors for
>> DH which all test more or less the same thing:
>> - the two vectors from before this series,
>> - the vector for the ffdhe2048 group, enabled if
>>    CONFIG_CRYPTO_DH_GROUPS_RFC7919 is set and
>> - the vector for the modp2048 group, similarly enabled if
>>    CONFIG_CRYPTO_DH_GROUPS_RFC3526 is set.
>>
>> In order to avoid too much redundancy during DH testing, enable only a
>> subset of these depending on the kernel config:
>> - if CONFIG_CRYPTO_DH_GROUPS_RFC7919 is set, enable only the ffdhe2048
>>    vector,
>> - otherwise, if CONFIG_CRYPTO_DH_GROUPS_RFC3526 is set, enable only
>>    the modp2048 vector and
>> - only enable the original two vectors if neither of these options
>>    has been selected.
>>
>> Note that an upcoming patch will make the DH implementation to reject any
>> domain parameters not corresponding to some safe-prime group approved by
>> SP800-56Arev3 in FIPS mode. Thus, having CONFIG_FIPS enabled, but
>> both of CONFIG_CRYPTO_DH_GROUPS_RFC7919 and
>> CONFIG_CRYPTO_DH_GROUPS_RFC3526 unset wouldn't make much sense as it would
>> render the DH implementation unusable in FIPS mode. Conversely, any
>> reasonable configuration would ensure that the original, non-conforming
>> test vectors would not get to run in FIPS mode.
>>
>
> For some weird reason the NVMe spec mandates for its TLS profile the
> ffdhe3072 group, so I would prefer if you would be using that as the
> default group for testing.

Done for v2.

>
>> Signed-off-by: Nicolai Stange <nstange@suse.de>
>> ---
>>   crypto/testmgr.h | 6 +++---
>>   1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/crypto/testmgr.h b/crypto/testmgr.h
>> index d18844c7499e..b295512c8f22 100644
>> --- a/crypto/testmgr.h
>> +++ b/crypto/testmgr.h
>> @@ -1331,8 +1331,7 @@ static const struct kpp_testvec dh_tv_template[] = {
>>   	.expected_a_public_size = 256,
>>   	.expected_ss_size = 256,
>>   	},
>> -#endif /* IS_ENABLED(CONFIG_CRYPTO_DH_GROUPS_RFC7919) */
>> -#if IS_ENABLED(CONFIG_CRYPTO_DH_GROUPS_RFC3526)
>> +#elif IS_ENABLED(CONFIG_CRYPTO_DH_GROUPS_RFC3526)
>>   	{
>>   	.secret =
>>   #ifdef __LITTLE_ENDIAN
>> @@ -1423,7 +1422,7 @@ static const struct kpp_testvec dh_tv_template[] = {
>>   	.expected_a_public_size = 256,
>>   	.expected_ss_size = 256,
>>   	},
>> -#endif /* IS_ENABLED(CONFIG_CRYPTO_DH_GROUPS_RFC3526) */
>> +#else
>>   	{
>>   	.secret =
>>   #ifdef __LITTLE_ENDIAN
>> @@ -1642,6 +1641,7 @@ static const struct kpp_testvec dh_tv_template[] = {
>>   	.expected_a_public_size = 256,
>>   	.expected_ss_size = 256,
>>   	},
>> +#endif
>>   };
>>     static const struct kpp_testvec curve25519_tv_template[] = {
>>
> ... and maybe add a config option to run a full test.

I didn't do this at this point, because I don't see much value in
running tests on more than one randomly selected DH group, i.e. on
ffdhe3072 and modp2048: both test vectors test the same code paths.

It might perhaps make sense to run tests for all the DH safe-prime
groups each for verifying that the resp. ->p primes are all correct. But
that would be a large TV dump and I'm not sure it would be desirable...

Thanks,

Nicolai
diff mbox series

Patch

diff --git a/crypto/testmgr.h b/crypto/testmgr.h
index d18844c7499e..b295512c8f22 100644
--- a/crypto/testmgr.h
+++ b/crypto/testmgr.h
@@ -1331,8 +1331,7 @@  static const struct kpp_testvec dh_tv_template[] = {
 	.expected_a_public_size = 256,
 	.expected_ss_size = 256,
 	},
-#endif /* IS_ENABLED(CONFIG_CRYPTO_DH_GROUPS_RFC7919) */
-#if IS_ENABLED(CONFIG_CRYPTO_DH_GROUPS_RFC3526)
+#elif IS_ENABLED(CONFIG_CRYPTO_DH_GROUPS_RFC3526)
 	{
 	.secret =
 #ifdef __LITTLE_ENDIAN
@@ -1423,7 +1422,7 @@  static const struct kpp_testvec dh_tv_template[] = {
 	.expected_a_public_size = 256,
 	.expected_ss_size = 256,
 	},
-#endif /* IS_ENABLED(CONFIG_CRYPTO_DH_GROUPS_RFC3526) */
+#else
 	{
 	.secret =
 #ifdef __LITTLE_ENDIAN
@@ -1642,6 +1641,7 @@  static const struct kpp_testvec dh_tv_template[] = {
 	.expected_a_public_size = 256,
 	.expected_ss_size = 256,
 	},
+#endif
 };
 
 static const struct kpp_testvec curve25519_tv_template[] = {