Driver-only hashes: X.509 #6171

mprse · 2022-08-04T11:21:32Z

Description

Build X.509 with all hashes provided by drivers and no md.c.

Resolves: #6127
Resolves: #6136
Resolves: #6144

Test coverage:

*** Comparing before-default -> after-default ***
   x509parse: total 723; skipped  26 ->  26
   x509write: total  41; skipped   8 ->   8
         pem: total  13; skipped   0 ->   0
         oid: total  28; skipped   0 ->   0

*** Comparing before-full -> after-full ***
   x509parse: total 723; skipped  25 ->  25
   x509write: total  41; skipped   0 ->   0
         pem: total  13; skipped   0 ->   0
         oid: total  28; skipped   0 ->   0

*** Comparing reference -> drivers ***
   x509parse: total 723; skipped  89 ->  89
   x509write: total  41; skipped   3 ->   3
         pem: total  13; skipped   0 ->   0
         oid: total  28; skipped   0 ->   0

Status

READY

Requires Backporting

NO

Migrations

NO

mprse · 2022-08-05T13:09:51Z

One component is now still failing:

test_psa_crypto_config_accel_hash

pem.c: In function 'pem_des_decrypt':
pem.c:268:17: error: implicit declaration of function 'pem_pbkdf1' [-Werror=implicit-function-declaration]
     if( ( ret = pem_pbkdf1( des_key, 8, des_iv, pwd, pwdlen ) ) != 0 )

In this build we have defined the following symbols:

MBEDTLS_MD_C
MBEDTLS_PSA_CRYPTO_C
PSA_WANT_ALG_MD5

But we don't have:

MBEDTLS_MD5_C
MBEDTLS_USE_PSA_CRYPTO

At the moment to have pem_pbkdf1() we need either MBEDTLS_MD5_C (legacy implementation) or alternatively MBEDTLS_USE_PSA_CRYPTO (psa implementation).

I have stuck with this and I'm wondering if test_psa_crypto_config_accel_hash is still needed? I see that @mpg added test_psa_crypto_config_accel_hash_use_psa component which make sense from my perspective. Here we are using drivers+psa-md5 so we can use psa version.

Alternatively we can set MBEDTLS_MD5_C in test_psa_crypto_config_accel_hash?
I need help here.

EDIT:
Further analysis.
Main question here is to define how this should be fixed. I see the following options:

Remove this component as running driver build without PSA is not needed and next component is drivers with PSA.
Use MBEDTLS_PSA_CRYPTO_C instead MBEDTLS_USE_PSA_CRYPTO in pem.c and x509write_crt.c.
In this case we will be using PSA without MBEDTLS_USE_PSA_CRYPTO, so does this test component still make sense? (if this is explicit no PSA case and PSA is used anyway?)
With these changes I'm able to build this component, but many tests from test_suite_x509parse and test_suite_x509write are failing. Probably similar change is required in other files since we have inconsistency (PSA vs no PSA) and if we move forward then MBEDTLS_USE_PSA_CRYPTO won't have any sense.
Change macros in the legacy_or_psa.h so they relay on MBEDTLS_USE_PSA_CRYPTO instead MBEDTLS_PSA_CRYPTO_C.
In this case this test component passes and outcome-analysis.sh also passes, but this breaks @mpg design, so probably it's not an option here.

cc @gilles-peskine-arm @AndrzejKurek

gilles-peskine-arm · 2022-08-08T19:17:13Z

The library is supposed to work when drivers are enabled and USE_PSA_CRYPTO is disabled. This means that explicit uses of the PSA API will call drivers (for some algorithm), but the X.509 and TLS code uses legacy crypto. There's nothing wrong with that. This component is useful.

At the moment to have pem_pbkdf1() we need either MBEDTLS_MD5_C (legacy implementation) or alternatively MBEDTLS_USE_PSA_CRYPTO (psa implementation).

In this build, MD5 is only available accelerated (PSA_WANT_ALG_MD5 is enabled and MBEDTLS_MD5_C is disabled) but MBEDTLS_USE_PSA_CRYPTO is disabled. So the build should not attempt to enable pem_pbkdf1.

mprse · 2022-08-09T07:26:48Z

This make sens, but if we use MBEDTLS_HAS_MD5_VIA_LOWLEVEL_OR_PSA in places of MBEDTLS_MD5_C and provide an implementation of the internal function pem_pbkdf1() based on PSA, to be used only when MBEDTLS_MD5_C is not available (#6144), then in this configuration pem_pbkdf1() should be available.

mprse · 2022-08-10T06:40:48Z

X.509 can be now build with all hashes provided by drivers and no md.c:

component_test_psa_crypto_config_accel_hash_use_psa : PSA version of pem_pbkdf1()
component_test_psa_crypto_config_accel_hash :pem_pbkdf1() not available
default: MD version of pem_pbkdf1()
full: MD version of pem_pbkdf1().

Test coverage:

*** Comparing before-default -> after-default ***
   x509parse: total 723; skipped  26 ->  26
   x509write: total  41; skipped   8 ->   8
         pem: total  13; skipped   0 ->   0
         oid: total  28; skipped   0 ->   0

*** Comparing before-full -> after-full ***
   x509parse: total 723; skipped  25 ->  25
   x509write: total  41; skipped   0 ->   0
         pem: total  13; skipped   0 ->   0
         oid: total  28; skipped   0 ->   0

*** Comparing reference -> drivers ***
   x509parse: total 723; skipped  89 ->  89
   x509write: total  41; skipped   3 ->   3
         pem: total  13; skipped   0 ->   0
         oid: total  28; skipped   0 ->   0

AndrzejKurek · 2022-08-18T06:57:21Z

tests/suites/test_suite_pkparse.function

@@ -2,6 +2,7 @@
 #include "mbedtls/pk.h"
 #include "mbedtls/pem.h"
 #include "mbedtls/oid.h"
+#include "legacy_or_psa.h"


Suggested change

#include "legacy_or_psa.h"

#if defined(MBEDTLS_USE_PSA_CRYPTO)

#include "psa/crypto.h"

#endif

#include "legacy_or_psa.h"

AndrzejKurek · 2022-08-18T06:57:42Z

tests/suites/test_suite_pem.function

@@ -3,6 +3,8 @@
 #include "mbedtls/pem.h"
 #include "mbedtls/des.h"
 #include "mbedtls/aes.h"
+#include "legacy_or_psa.h"


Suggested change

#include "legacy_or_psa.h"

#if defined(MBEDTLS_USE_PSA_CRYPTO)

#include "psa/crypto.h"

#endif

#include "legacy_or_psa.h"

The test framework automatically includes psa/crypto.h. It should probably include legacy_or_psa.h automatically as well.

Also, legacy_or_psa.h should include psa/crypto.h. Right now, including legacy_or_psa.h doesn't work if psa/crypto.h hasn't already been included.

I think it is good idea to include psa/crypto.h in legacy_or_psa.h.

AndrzejKurek · 2022-08-18T06:57:51Z

tests/suites/test_suite_oid.function

@@ -3,6 +3,7 @@
 #include "mbedtls/asn1.h"
 #include "mbedtls/asn1write.h"
 #include "string.h"
+#include "legacy_or_psa.h"


Suggested change

#include "legacy_or_psa.h"

#if defined(MBEDTLS_USE_PSA_CRYPTO)

#include "psa/crypto.h"

#endif

#include "legacy_or_psa.h"

AndrzejKurek · 2022-08-18T06:59:20Z

library/x509.c

@@ -62,6 +62,8 @@
 #include <time.h>
 #endif

+#include "legacy_or_psa.h"


Suggested change

#include "legacy_or_psa.h"

#if defined(MBEDTLS_USE_PSA_CRYPTO)

#include "psa/crypto.h"

#endif

#include "legacy_or_psa.h"

AndrzejKurek · 2022-08-18T07:08:35Z

library/pem.c

@@ -130,6 +142,120 @@ static int pem_pbkdf1( unsigned char *key, size_t keylen,

    return( ret );
 }
+#else
+#if defined(MBEDTLS_USE_PSA_CRYPTO)


Is this needed? Since PEM_RFC1421 is defined MBEDTLS_HAS_ALG_MD5_VIA_MD_OR_PSA_BASED_ON_USE_PSA has to be defined.
So either we have MD5_C and the case above happens, or we don't, but MBEDTLS_USE_PSA_CRYPTO has to be on. Am I missing something?

Yes, it can be removed.

AndrzejKurek · 2022-08-18T07:09:08Z

library/pem.c

@@ -230,15 +355,13 @@ int mbedtls_pem_read_buffer( mbedtls_pem_context *ctx, const char *header, const
    size_t len;
    unsigned char *buf;
    const unsigned char *s1, *s2, *end;
-#if defined(MBEDTLS_MD5_C) && defined(MBEDTLS_CIPHER_MODE_CBC) &&         \
-    ( defined(MBEDTLS_DES_C) || defined(MBEDTLS_AES_C) )
+#if defined(PEM_RFC1421 )


Suggested change

#if defined(PEM_RFC1421 )

#if defined( PEM_RFC1421 )

Actually, no. We put spaces inside parentheses except for defined and casts. Don't ask me why.

Right, the other way around :D

AndrzejKurek · 2022-08-18T07:09:13Z

library/pem.c

@@ -270,8 +393,7 @@ int mbedtls_pem_read_buffer( mbedtls_pem_context *ctx, const char *header, const

    if( s2 - s1 >= 22 && memcmp( s1, "Proc-Type: 4,ENCRYPTED", 22 ) == 0 )
    {
-#if defined(MBEDTLS_MD5_C) && defined(MBEDTLS_CIPHER_MODE_CBC) &&         \
-    ( defined(MBEDTLS_DES_C) || defined(MBEDTLS_AES_C) )
+#if defined(PEM_RFC1421 )


Suggested change

#if defined(PEM_RFC1421 )

#if defined( PEM_RFC1421 )

AndrzejKurek · 2022-08-18T07:09:20Z

library/pem.c

@@ -357,8 +478,7 @@ int mbedtls_pem_read_buffer( mbedtls_pem_context *ctx, const char *header, const

    if( enc != 0 )
    {
-#if defined(MBEDTLS_MD5_C) && defined(MBEDTLS_CIPHER_MODE_CBC) &&         \
-    ( defined(MBEDTLS_DES_C) || defined(MBEDTLS_AES_C) )
+#if defined(PEM_RFC1421 )


Suggested change

#if defined(PEM_RFC1421 )

#if defined( PEM_RFC1421 )

AndrzejKurek · 2022-08-18T07:09:40Z

library/x509write_crt.c

@@ -254,7 +256,7 @@ int mbedtls_x509write_crt_set_authority_key_identifier( mbedtls_x509write_cert *
                                                     1,
                                                     (MBEDTLS_ASN1_CONTEXT_SPECIFIC | 0) );
 }
-#endif /* MBEDTLS_SHA1_C */
+#endif /* MBEDTLS_HAS_ALG_SHA_1_VIA_MD_OR_PSA */


Suggested change

#endif /* MBEDTLS_HAS_ALG_SHA_1_VIA_MD_OR_PSA */

#endif /* MBEDTLS_HAS_ALG_SHA_1_VIA_MD_OR_PSA_BASED_ON_USE_PSA */

AndrzejKurek · 2022-08-18T07:12:22Z

library/oid.c

-#endif /* MBEDTLS_SHA256_C */
-#if defined(MBEDTLS_SHA384_C)
+#endif /* MBEDTLS_HAS_ALG_SHA_256_VIA_LOWLEVEL_OR_PSA */
+#if defined(MBEDTLS_HAS_ALG_SHA_384_VIA_MD_OR_PSA)


Why are some SHA's guarded by LOWLEVEL_OR_PSA, and others by VIA_MD_OR_PSA? Shouldn't this be uniform?

Hmm. I don't know why it is like this. I agree that it should be LOWLEVEL_OR_PSA everywhere.

AndrzejKurek · 2022-08-18T07:23:12Z

include/mbedtls/mbedtls_config.h

+ *           (MBEDTLS_MD_C or MBEDTLS_USE_PSA_CRYPTO)
+ *
+ * \warning If building without MBEDTLS_MD_C, you must call psa_crypto_init()
+ * before doing any PKCS#1 v2.1 operation.


It seems to me that x509_CREATE_C does not really depend on MD - you probably meant MBEDTLS_X509_CRT_WRITE_C (but please make sure). Also, the description for MBEDTLS_X509_CRT_WRITE_C should end with

Suggested change

* before doing any PKCS#1 v2.1 operation.

* before doing any x509 write operation.

AndrzejKurek · 2022-08-19T09:37:28Z

library/x509.c

@@ -138,31 +140,31 @@ static inline const char* md_type_to_string( mbedtls_md_type_t md_alg )
 {
    switch( md_alg )
    {
-#if defined(MBEDTLS_MD5_C)


Minor: This data would in my opinion fall into the VIA_LOWLEVEL_OR_PSA category, but x509 will be defined only with MBEDTLS_USE_PSA_CRYPTO or MBEDTLS_MD_C, so that's not a problem.

I think MD type refers to MD layer.

I know, what I mean is that the strings below are just metadata, and could be useful without the MBEDTLS_MD_C module, so instead of using, for example MBEDTLS_HAS_ALG_SHA_1_VIA_MD_OR_PSA, MBEDTLS_HAS_ALG_SHA_1_VIA_LOWLEVEL_OR_PSA could be used here, but the x509 module itself requires either MBEDTLS_USE_PSA_CRYPTO or MBEDTLS_MD_C, so it doesn't make a difference - this code will not be used without MBEDTLS_MD_C and without MBEDTLS_PSA_CRYPTO_C.

AndrzejKurek · 2022-08-19T10:04:10Z

tests/suites/test_suite_pem.data

 mbedtls_pem_read_buffer:"^":"$":"^\nProc-Type\: 4,ENCRYPTED\nDEK-Info\: DES-CBC,00$":"pwd":MBEDTLS_ERR_PEM_INVALID_ENC_IV:""

 PEM read (unknown encryption algorithm)
+depends_on:MBEDTLS_HAS_ALG_MD5_VIA_MD_OR_PSA_BASED_ON_USE_PSA:MBEDTLS_CMAC_C


Why MBEDTLS_CMAC_C? It seems that this is a new dependency.

There was function level dependency to MBEDTLS_CIPHER_MODE_CBC and now it is only for CBC cases (below). But without MBEDTLS_CIPHER_MODE_CBC this test was executed and was failing. While debugging I decided to add MBEDTLS_CMAC_C as it was more suitable.

This doesn't make sense: why a dependency on a completely unrelated algorithm? PEM doesn't use CMAC directly or indirectly. Maybe a dependency is missing but CMAC can't be right. Looking at the code, I think there's a missing dependency on MBEDTLS_AES_C (or more precisely MBEDTLS_DEC_C || MBEDTLS_AES_C, but we don't really care about DES-only configurations), because without that the PEM module doesn't even try to parse encryption.

Will check again.

Removed this dependency and checked locally default, full, test_psa_crypto_config_accel_hash_use_psa, test_psa_crypto_config_accel_hash configurations and everything is ok. I also can't find related CI failure in the history as it doesn't go that far.
In this case I will remove this and let's see what CI says.

Yup, CI has failed now. The following component:

^^^^test_when_no_ciphersuites_have_mac: test: !MBEDTLS_SSL_SOME_MODES_USE_MAC: make test -> 2^^^^ PEM read (unknown encryption algorithm) ........................... FAILED ret == res at line 51, suites/test_suite_pem.function From the component: scripts/config.py unset MBEDTLS_CIPHER_NULL_CIPHER scripts/config.py unset MBEDTLS_CIPHER_MODE_CBC scripts/config.py unset MBEDTLS_CMAC_C

The dependency is on MBEDTLS_CIPHER_MODE_CBC (you can confirm by doing a build with default config minus MBEDTLS_CIPHER_MODE_CBC). The test case only fails because mbedtls_pem_read_buffer returns a different error (MBEDTLS_ERR_PEM_FEATURE_UNAVAILABLE).

To be very precise in the tests, we should duplicate the test cases for this function and make them expect MBEDTLS_ERR_PEM_FEATURE_UNAVAILABLE when the encryption mechanism isn't supported. As a quick fix, we can make the test case depend on MBEDTLS_CIPHER_MODE_CBC. (It actually depends on more than that, but there are many missing dependencies on cipher features throughout the test suites and fixing them is out of scope here.)

Thanks for clarification. I have modified the last commit and changed dependency from MBEDTLS_CMAC_C to MBEDTLS_CIPHER_MODE_CBC.

By the way, #1892 tracks the lack of test builds with variations on supported cipher features. No point in fixing preexisting individual cases until we have a way to test.

…ependency MBEDTLS_CMAC_C dependency is ivalid. "PEM read (unknown encryption algorithm)" needs MBEDTLS_CIPHER_MODE_CBC dependency as otherwise this test is failing in test_when_no_ciphersuites_have_mac configuration because mbedtls_pem_read_buffer() returns a different error (MBEDTLS_ERR_PEM_FEATURE_UNAVAILABLE). Signed-off-by: Przemek Stekiel <przemyslaw.stekiel@mobica.com>

mprse · 2022-08-22T06:14:44Z

@superna9999 @AndrzejKurek Sorry guys. One more change was required (modified the last commit).

AndrzejKurek

This test case will still fail with default configuration and the following changes:

./scripts/config.pl unset MBEDTLS_DES_C
./scripts/config.pl unset MBEDTLS_AES_C
./scripts/config.pl unset MBEDTLS_NIST_KW_C
./scripts/config.pl unset MBEDTLS_CMAC_C
./scripts/config.pl unset MBEDTLS_CTR_DRBG_C
./scripts/config.pl unset MBEDTLS_CMAC_C

due to the lack of AES_C and DES_C.
The test case has to reflect the same requirements as PEM_RFC1421.

AndrzejKurek

After speaking personnaly to @mprse and reqding @gilles-peskine-arm comment about not being so precise about the defines - I can approve the simplified dependency here.

mprse · 2022-08-22T14:40:37Z

Can we merge this one? CI is green and PR was already approved by @superna9999 , but it seems he is out of office today.
I only modified one dependency in the test as requested by @gilles-peskine-arm since @superna9999 review.
This one is blocking #6208.

mprse · 2022-08-22T15:07:08Z

@superna9999 Can you review this one, so we can merge it.

mprse added needs-review Every commit must be reviewed by at least two team members, needs-ci Needs to pass CI tests needs-reviewer This PR needs someone to pick it up for review priority-high High priority - will be reviewed soon labels Aug 4, 2022

mprse added this to To Do in Roadmap Board for Mbed TLS via automation Aug 4, 2022

mprse force-pushed the md_x509_test branch from 2228e2e to ac77d5d Compare August 5, 2022 10:22

mprse force-pushed the md_x509_test branch from f5451c4 to b14c807 Compare August 9, 2022 13:21

mprse removed the needs-ci Needs to pass CI tests label Aug 9, 2022

AndrzejKurek mentioned this pull request Aug 10, 2022

[DRAFT] Remove the dependency on MD from TLS tests #6193

Closed

AndrzejKurek self-requested a review August 10, 2022 13:41

mprse force-pushed the md_x509_test branch 4 times, most recently from 46db51e to 2cc6b1b Compare August 12, 2022 07:55

AndrzejKurek mentioned this pull request Aug 17, 2022

Remove the dependency on MD from TLS 1.2 tests #6208

Merged

AndrzejKurek reviewed Aug 18, 2022

View reviewed changes

bensze01 moved this from In Development to Has Approval in Roadmap Board for Mbed TLS Aug 19, 2022

AndrzejKurek reviewed Aug 19, 2022

View reviewed changes

AndrzejKurek previously approved these changes Aug 19, 2022

View reviewed changes

mprse dismissed stale reviews from AndrzejKurek and superna9999 via 00b5c7e August 19, 2022 11:43

Roadmap Board for Mbed TLS automation moved this from Has Approval to In Development Aug 19, 2022

superna9999 previously approved these changes Aug 19, 2022

View reviewed changes

bensze01 moved this from In Development to Has Approval in Roadmap Board for Mbed TLS Aug 19, 2022

AndrzejKurek previously approved these changes Aug 19, 2022

View reviewed changes

mprse dismissed stale reviews from AndrzejKurek and superna9999 via 07c0f12 August 20, 2022 12:28

mprse force-pushed the md_x509_test branch from 00b5c7e to 07c0f12 Compare August 20, 2022 12:28

Roadmap Board for Mbed TLS automation moved this from Has Approval to In Development Aug 20, 2022

mprse requested review from AndrzejKurek and superna9999 August 20, 2022 15:30

AndrzejKurek suggested changes Aug 22, 2022

View reviewed changes

AndrzejKurek approved these changes Aug 22, 2022

View reviewed changes

bensze01 moved this from In Development to Has Approval in Roadmap Board for Mbed TLS Aug 22, 2022

superna9999 approved these changes Aug 22, 2022

View reviewed changes

gilles-peskine-arm added approved Design and code approved - may be waiting for CI or backports and removed needs-review Every commit must be reviewed by at least two team members, needs-reviewer This PR needs someone to pick it up for review labels Aug 22, 2022

gilles-peskine-arm merged commit 03f1c39 into Mbed-TLS:development Aug 22, 2022

Roadmap Board for Mbed TLS automation moved this from Has Approval to Done Aug 22, 2022

daverodgman removed this from Done in Roadmap Board for Mbed TLS Apr 2, 2023

	#endif /* MBEDTLS_HAS_ALG_SHA_1_VIA_MD_OR_PSA */
	#endif /* MBEDTLS_HAS_ALG_SHA_1_VIA_MD_OR_PSA_BASED_ON_USE_PSA */

	* before doing any PKCS#1 v2.1 operation.
	* before doing any x509 write operation.

Driver-only hashes: X.509 #6171

Driver-only hashes: X.509 #6171

Conversation

mprse commented Aug 4, 2022 • edited Loading

Description

Status

Requires Backporting

Migrations

mprse commented Aug 5, 2022 • edited Loading

gilles-peskine-arm commented Aug 8, 2022

mprse commented Aug 9, 2022 • edited Loading

mprse commented Aug 10, 2022

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

mprse Aug 18, 2022 • edited Loading

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

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

mprse commented Aug 22, 2022

AndrzejKurek left a comment

Choose a reason for hiding this comment

AndrzejKurek left a comment

Choose a reason for hiding this comment

mprse commented Aug 22, 2022

mprse commented Aug 22, 2022

mprse commented Aug 4, 2022 •

edited

Loading

mprse commented Aug 5, 2022 •

edited

Loading

mprse commented Aug 9, 2022 •

edited

Loading

mprse Aug 18, 2022 •

edited

Loading