Merge pull request #3611 from gilles-peskine-arm/psa-coverity-cleanups-202008
Minor fixes in PSA code and tests
diff --git a/ChangeLog.d/crl-revocationDate.txt b/ChangeLog.d/crl-revocationDate.txt
new file mode 100644
index 0000000..a8ad532
--- /dev/null
+++ b/ChangeLog.d/crl-revocationDate.txt
@@ -0,0 +1,11 @@
+Security
+ * When checking X.509 CRLs, a certificate was only considered as revoked if
+ its revocationDate was in the past according to the local clock if
+ available. In particular, on builds without MBEDTLS_HAVE_TIME_DATE,
+ certificates were never considered as revoked. On builds with
+ MBEDTLS_HAVE_TIME_DATE, an attacker able to control the local clock (for
+ example, an untrusted OS attacking a secure enclave) could prevent
+ revocation of certificates via CRLs. Fixed by no longer checking the
+ revocationDate field, in accordance with RFC 5280. Reported by
+ yuemonangong in #3340. Reported independently and fixed by
+ Raoul Strackx and Jethro Beekman in #3433.
diff --git a/library/x509_crt.c b/library/x509_crt.c
index fcc2ed2..71e9cec 100644
--- a/library/x509_crt.c
+++ b/library/x509_crt.c
@@ -2322,8 +2322,7 @@
if( crt->serial.len == cur->serial.len &&
memcmp( crt->serial.p, cur->serial.p, crt->serial.len ) == 0 )
{
- if( mbedtls_x509_time_is_past( &cur->revocation_date ) )
- return( 1 );
+ return( 1 );
}
cur = cur->next;
diff --git a/tests/data_files/Makefile b/tests/data_files/Makefile
index 1145fae..88f265c 100644
--- a/tests/data_files/Makefile
+++ b/tests/data_files/Makefile
@@ -1070,7 +1070,10 @@
crl.pem: $(test_ca_crt) $(test_ca_key_file_rsa) $(test_ca_config_file)
$(OPENSSL) ca -gencrl -batch -cert $(test_ca_crt) -keyfile $(test_ca_key_file_rsa) -key $(test_ca_pwd_rsa) -config $(test_ca_server1_config_file) -md sha1 -crldays 3653 -out $@
-server1_all: crl.pem server1.crt server1.noauthid.crt server1.crt.openssl server1.v1.crt server1.v1.crt.openssl server1.key_usage.crt server1.key_usage_noauthid.crt server1.key_usage.crt.openssl server1.cert_type.crt server1.cert_type_noauthid.crt server1.cert_type.crt.openssl server1.der server1.der.openssl server1.v1.der server1.v1.der.openssl server1.key_usage.der server1.key_usage.der.openssl server1.cert_type.der server1.cert_type.der.openssl
+crl-futureRevocationDate.pem: $(test_ca_crt) $(test_ca_key_file_rsa) $(test_ca_config_file) test-ca.server1.future-crl.db test-ca.server1.future-crl.opensslconf
+ $(FAKETIME) '2028-12-31' $(OPENSSL) ca -gencrl -config test-ca.server1.future-crl.opensslconf -crldays 365 -passin "pass:$(test_ca_pwd_rsa)" -out $@
+
+server1_all: crl.pem crl-futureRevocationDate.pem server1.crt server1.noauthid.crt server1.crt.openssl server1.v1.crt server1.v1.crt.openssl server1.key_usage.crt server1.key_usage_noauthid.crt server1.key_usage.crt.openssl server1.cert_type.crt server1.cert_type_noauthid.crt server1.cert_type.crt.openssl server1.der server1.der.openssl server1.v1.der server1.v1.der.openssl server1.key_usage.der server1.key_usage.der.openssl server1.cert_type.der server1.cert_type.der.openssl
# server2*
diff --git a/tests/data_files/Readme-x509.txt b/tests/data_files/Readme-x509.txt
index 6f54ed0..d07241a 100644
--- a/tests/data_files/Readme-x509.txt
+++ b/tests/data_files/Readme-x509.txt
@@ -111,7 +111,7 @@
- crl-ec-sha*.pem: (2) server6.crt
- crl-future.pem: (2) server6.crt + unknown
- crl-rsa-pss-*.pem: (1) server9{,badsign,with-ca}.crt + cert_sha384.crt + unknown
-- crl.pem, crl_expired.pem: (1) server1{,.cert_type,.key_usage,.v1}.crt + unknown
+- crl.pem, crl-futureRevocationDate.pem, crl_expired.pem: (1) server1{,.cert_type,.key_usage,.v1}.crt + unknown
- crl_md*.pem: crl_sha*.pem: (1) same as crl.pem
- crt_cat_*.pem: (1+2) concatenations in various orders:
ec = crl-ec-sha256.pem, ecfut = crl-future.pem
diff --git a/tests/data_files/crl-futureRevocationDate.pem b/tests/data_files/crl-futureRevocationDate.pem
new file mode 100644
index 0000000..f147a8f
--- /dev/null
+++ b/tests/data_files/crl-futureRevocationDate.pem
@@ -0,0 +1,11 @@
+-----BEGIN X509 CRL-----
+MIIBqzCBlDANBgkqhkiG9w0BAQUFADA7MQswCQYDVQQGEwJOTDERMA8GA1UECgwI
+UG9sYXJTU0wxGTAXBgNVBAMMEFBvbGFyU1NMIFRlc3QgQ0EXDTI4MTIzMDIzMDAw
+MFoXDTI5MTIzMDIzMDAwMFowKDASAgEBFw0yOTAxMDExMjQ0MDdaMBICAQMXDTI5
+MDEwMTEyNDQwN1owDQYJKoZIhvcNAQEFBQADggEBAKbL1mDpzCbLJmRZKM2KHPvK
+ijS4UMnanzzYpLAwom1NI69v2fE1/EfiXv0empE6mFqnLwOG4ZP8fECfxjMXO2Ee
+VhxYiRjly6q9hfIUk1e+N9ct8unNnLEBvf6Syfy9+FSO3Q/ahljpYlXsXxg62WXl
+9xp5b5Ok+/0sCv0eL5uFQKXQa8hS9dZo6py7jvFDQC+wVau1mXjQW85iXMLm7vik
+4lR+kfZloeq1jIbsx8cdMi32YVt7uccaqoFcjtrdrWfGmi0wvlDc8K5J0l4tIxZY
+9P+T4fzSgQLdqGZ3xADheEaGTRVL/5oe5L4zRH32BZONMFCijv+j1SpWLxHE8cM=
+-----END X509 CRL-----
diff --git a/tests/data_files/test-ca.server1.future-crl.db b/tests/data_files/test-ca.server1.future-crl.db
new file mode 100644
index 0000000..763aa12
--- /dev/null
+++ b/tests/data_files/test-ca.server1.future-crl.db
@@ -0,0 +1,2 @@
+R 210212144406Z 290101124407Z 01 unknown /C=NL/O=PolarSSL/CN=PolarSSL Server 1
+R 210212144400Z 290101124407Z 03 unknown /C=NL/O=PolarSSL/CN=PolarSSL Test CA
diff --git a/tests/data_files/test-ca.server1.future-crl.opensslconf b/tests/data_files/test-ca.server1.future-crl.opensslconf
new file mode 100644
index 0000000..e9ce754
--- /dev/null
+++ b/tests/data_files/test-ca.server1.future-crl.opensslconf
@@ -0,0 +1,18 @@
+ [ ca ]
+ default_ca = test-ca
+
+ [ test-ca ]
+ certificate = test-ca.crt
+ private_key = test-ca.key
+ serial = test-ca.server1.serial
+ default_md = sha1
+ default_startdate = 110212144406Z
+ default_enddate = 210212144406Z
+ new_certs_dir = ./
+ database = ./test-ca.server1.future-crl.db
+ policy = policy_match
+
+ [policy_match]
+ countryName = supplied
+ organizationName = supplied
+ commonName = supplied
diff --git a/tests/scripts/all.sh b/tests/scripts/all.sh
index 636eb31..533fa2c 100755
--- a/tests/scripts/all.sh
+++ b/tests/scripts/all.sh
@@ -1498,6 +1498,16 @@
make test
}
+component_test_no_date_time () {
+ msg "build: default config without MBEDTLS_HAVE_TIME_DATE"
+ scripts/config.py unset MBEDTLS_HAVE_TIME_DATE
+ CC=gcc cmake
+ make
+
+ msg "test: !MBEDTLS_HAVE_TIME_DATE - main suites"
+ make test
+}
+
component_test_platform_calloc_macro () {
msg "build: MBEDTLS_PLATFORM_{CALLOC/FREE}_MACRO enabled (ASan build)"
scripts/config.py set MBEDTLS_PLATFORM_MEMORY
diff --git a/tests/suites/test_suite_psa_crypto.function b/tests/suites/test_suite_psa_crypto.function
index e48cb90..665580b 100644
--- a/tests/suites/test_suite_psa_crypto.function
+++ b/tests/suites/test_suite_psa_crypto.function
@@ -3028,17 +3028,21 @@
psa_algorithm_t alg = alg_arg;
psa_mac_operation_t operation = PSA_MAC_OPERATION_INIT;
psa_key_attributes_t attributes = PSA_KEY_ATTRIBUTES_INIT;
- /* Leave a little extra room in the output buffer. At the end of the
- * test, we'll check that the implementation didn't overwrite onto
- * this extra room. */
- uint8_t actual_mac[PSA_MAC_MAX_SIZE + 10];
+ uint8_t *actual_mac = NULL;
size_t mac_buffer_size =
PSA_MAC_FINAL_SIZE( key_type, PSA_BYTES_TO_BITS( key->len ), alg );
size_t mac_length = 0;
+ const size_t output_sizes_to_test[] = {
+ 0,
+ 1,
+ expected_mac->len - 1,
+ expected_mac->len,
+ expected_mac->len + 1,
+ };
- memset( actual_mac, '+', sizeof( actual_mac ) );
TEST_ASSERT( mac_buffer_size <= PSA_MAC_MAX_SIZE );
- TEST_ASSERT( expected_mac->len <= mac_buffer_size );
+ /* We expect PSA_MAC_FINAL_SIZE to be exact. */
+ TEST_ASSERT( expected_mac->len == mac_buffer_size );
PSA_ASSERT( psa_crypto_init( ) );
@@ -3048,27 +3052,41 @@
PSA_ASSERT( psa_import_key( &attributes, key->x, key->len, &handle ) );
- /* Calculate the MAC. */
- PSA_ASSERT( psa_mac_sign_setup( &operation,
- handle, alg ) );
- PSA_ASSERT( psa_mac_update( &operation,
- input->x, input->len ) );
- PSA_ASSERT( psa_mac_sign_finish( &operation,
- actual_mac, mac_buffer_size,
- &mac_length ) );
+ for( size_t i = 0; i < ARRAY_LENGTH( output_sizes_to_test ); i++ )
+ {
+ const size_t output_size = output_sizes_to_test[i];
+ psa_status_t expected_status =
+ ( output_size >= expected_mac->len ? PSA_SUCCESS :
+ PSA_ERROR_BUFFER_TOO_SMALL );
- /* Compare with the expected value. */
- ASSERT_COMPARE( expected_mac->x, expected_mac->len,
- actual_mac, mac_length );
+ test_set_step( output_size );
+ ASSERT_ALLOC( actual_mac, output_size );
- /* Verify that the end of the buffer is untouched. */
- TEST_ASSERT( mem_is_char( actual_mac + mac_length, '+',
- sizeof( actual_mac ) - mac_length ) );
+ /* Calculate the MAC. */
+ PSA_ASSERT( psa_mac_sign_setup( &operation,
+ handle, alg ) );
+ PSA_ASSERT( psa_mac_update( &operation,
+ input->x, input->len ) );
+ TEST_EQUAL( psa_mac_sign_finish( &operation,
+ actual_mac, output_size,
+ &mac_length ),
+ expected_status );
+ PSA_ASSERT( psa_mac_abort( &operation ) );
+
+ if( expected_status == PSA_SUCCESS )
+ {
+ ASSERT_COMPARE( expected_mac->x, expected_mac->len,
+ actual_mac, mac_length );
+ }
+ mbedtls_free( actual_mac );
+ actual_mac = NULL;
+ }
exit:
psa_mac_abort( &operation );
psa_destroy_key( handle );
PSA_DONE( );
+ mbedtls_free( actual_mac );
}
/* END_CASE */
@@ -3084,6 +3102,7 @@
psa_algorithm_t alg = alg_arg;
psa_mac_operation_t operation = PSA_MAC_OPERATION_INIT;
psa_key_attributes_t attributes = PSA_KEY_ATTRIBUTES_INIT;
+ uint8_t *perturbed_mac = NULL;
TEST_ASSERT( expected_mac->len <= PSA_MAC_MAX_SIZE );
@@ -3095,19 +3114,58 @@
PSA_ASSERT( psa_import_key( &attributes, key->x, key->len, &handle ) );
+ /* Test the correct MAC. */
PSA_ASSERT( psa_mac_verify_setup( &operation,
handle, alg ) );
- PSA_ASSERT( psa_destroy_key( handle ) );
PSA_ASSERT( psa_mac_update( &operation,
input->x, input->len ) );
PSA_ASSERT( psa_mac_verify_finish( &operation,
expected_mac->x,
expected_mac->len ) );
+ /* Test a MAC that's too short. */
+ PSA_ASSERT( psa_mac_verify_setup( &operation,
+ handle, alg ) );
+ PSA_ASSERT( psa_mac_update( &operation,
+ input->x, input->len ) );
+ TEST_EQUAL( psa_mac_verify_finish( &operation,
+ expected_mac->x,
+ expected_mac->len - 1 ),
+ PSA_ERROR_INVALID_SIGNATURE );
+
+ /* Test a MAC that's too long. */
+ ASSERT_ALLOC( perturbed_mac, expected_mac->len + 1 );
+ memcpy( perturbed_mac, expected_mac->x, expected_mac->len );
+ PSA_ASSERT( psa_mac_verify_setup( &operation,
+ handle, alg ) );
+ PSA_ASSERT( psa_mac_update( &operation,
+ input->x, input->len ) );
+ TEST_EQUAL( psa_mac_verify_finish( &operation,
+ perturbed_mac,
+ expected_mac->len + 1 ),
+ PSA_ERROR_INVALID_SIGNATURE );
+
+ /* Test changing one byte. */
+ for( size_t i = 0; i < expected_mac->len; i++ )
+ {
+ test_set_step( i );
+ perturbed_mac[i] ^= 1;
+ PSA_ASSERT( psa_mac_verify_setup( &operation,
+ handle, alg ) );
+ PSA_ASSERT( psa_mac_update( &operation,
+ input->x, input->len ) );
+ TEST_EQUAL( psa_mac_verify_finish( &operation,
+ perturbed_mac,
+ expected_mac->len ),
+ PSA_ERROR_INVALID_SIGNATURE );
+ perturbed_mac[i] ^= 1;
+ }
+
exit:
psa_mac_abort( &operation );
psa_destroy_key( handle );
PSA_DONE( );
+ mbedtls_free( perturbed_mac );
}
/* END_CASE */
diff --git a/tests/suites/test_suite_x509parse.data b/tests/suites/test_suite_x509parse.data
index f8e3891..275afb7 100644
--- a/tests/suites/test_suite_x509parse.data
+++ b/tests/suites/test_suite_x509parse.data
@@ -911,6 +911,14 @@
depends_on:MBEDTLS_SHA256_C:MBEDTLS_PEM_PARSE_C:MBEDTLS_RSA_C:MBEDTLS_PKCS1_V15:MBEDTLS_ECDSA_C:MBEDTLS_SHA1_C
x509_verify:"data_files/cert_sha256.crt":"data_files/test-ca.crt":"data_files/crl-ec-sha256.pem":"NULL":0:0:"next":"NULL"
+X509 CRT verification #98 (Revoked Cert, revocation date in the future, _with_ MBEDTLS_HAVE_TIME_DATE)
+depends_on:MBEDTLS_PEM_PARSE_C:MBEDTLS_SHA1_C:MBEDTLS_RSA_C:MBEDTLS_PKCS1_V15:MBEDTLS_HAVE_TIME_DATE
+x509_verify:"data_files/server1.crt":"data_files/test-ca.crt":"data_files/crl-futureRevocationDate.pem":"NULL":MBEDTLS_ERR_X509_CERT_VERIFY_FAILED:MBEDTLS_X509_BADCERT_REVOKED|MBEDTLS_X509_BADCRL_FUTURE:"compat":"NULL"
+
+X509 CRT verification #99 (Revoked Cert, revocation date in the future, _without_ MBEDTLS_HAVE_TIME_DATE)
+depends_on:MBEDTLS_PEM_PARSE_C:MBEDTLS_SHA1_C:MBEDTLS_RSA_C:MBEDTLS_PKCS1_V15:!MBEDTLS_HAVE_TIME_DATE
+x509_verify:"data_files/server1.crt":"data_files/test-ca.crt":"data_files/crl-futureRevocationDate.pem":"NULL":MBEDTLS_ERR_X509_CERT_VERIFY_FAILED:MBEDTLS_X509_BADCERT_REVOKED:"compat":"NULL"
+
X509 CRT verification: domain identical to IPv4 in SubjectAltName
depends_on:MBEDTLS_PEM_PARSE_C:MBEDTLS_ECDSA_C:MBEDTLS_SHA256_C:MBEDTLS_ECP_DP_SECP256R1_ENABLED:MBEDTLS_RSA_C
x509_verify:"data_files/server5-tricky-ip-san.crt":"data_files/server5-tricky-ip-san.crt":"data_files/crl_sha256.pem":"abcd":MBEDTLS_ERR_X509_CERT_VERIFY_FAILED:MBEDTLS_X509_BADCERT_CN_MISMATCH:"":"NULL"