From 8136e824b33632108df6f56079c58e43a0215131 Mon Sep 17 00:00:00 2001 From: Andres AG Date: Fri, 9 Dec 2016 17:26:23 +0000 Subject: [PATCH 1/2] Fix verify out flags from x509_crt_verify_top() This change fixes a regression introduced by an earlier commit that modified x509_crt_verify_top() to ensure that valid certificates that are after past or future valid in the chain are processed. However the change introduced a change in behaviour that caused the verification flags MBEDTLS_X509_BADCERT_EXPIRED and MBEDTLS_BADCERT_FUTURE to always be set whenever there is a failure in the verification regardless of the cause. The fix maintains both behaviours: * Ensure that valid certificates after future and past are verified * Ensure that the correct verification flags are set. To do so, a temporary pointer to the first future or past valid certificate is maintained while traversing the chain. If a truly valid certificate is found then that one is used, otherwise if no valid certificate is found and the end of the chain is reached, the program reverts back to using the future or past valid certificate. --- ChangeLog | 9 +++++++++ library/x509_crt.c | 33 +++++++++++++++++++++------------ 2 files changed, 30 insertions(+), 12 deletions(-) diff --git a/ChangeLog b/ChangeLog index 92d7977e0..4008783d6 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,5 +1,14 @@ mbed TLS ChangeLog (Sorted per branch, date) += mbed TLS x.x.x branch released xxxx-xx-xx + +Bugfix + * Fix output certificate verification flags set by x509_crt_verify_top() when + traversing a chain of trusted CA. The issue would cause both flags, + MBEDTLS_X509_BADCERT_NOT_TRUSTED and MBEDTLS_X509_BADCERT_EXPIRED, to be + set when the verification conditions are not met regardless of the cause. + Found by Harm Verhagen and inestlerode. #665 #561 + = mbed TLS 2.1.6 branch released 2016-10-17 Security diff --git a/library/x509_crt.c b/library/x509_crt.c index 81402ba55..84a2d4f16 100644 --- a/library/x509_crt.c +++ b/library/x509_crt.c @@ -1893,6 +1893,7 @@ static int x509_crt_verify_top( int check_path_cnt; unsigned char hash[MBEDTLS_MD_MAX_SIZE]; const mbedtls_md_info_t *md_info; + mbedtls_x509_crt *future_past_ca = NULL; if( mbedtls_x509_time_is_past( &child->valid_to ) ) *flags |= MBEDTLS_X509_BADCERT_EXPIRED; @@ -1947,16 +1948,6 @@ static int x509_crt_verify_top( continue; } - if( mbedtls_x509_time_is_past( &trust_ca->valid_to ) ) - { - continue; - } - - if( mbedtls_x509_time_is_future( &trust_ca->valid_from ) ) - { - continue; - } - if( mbedtls_pk_verify_ext( child->sig_pk, child->sig_opts, &trust_ca->pk, child->sig_md, hash, mbedtls_md_get_size( md_info ), child->sig.p, child->sig.len ) != 0 ) @@ -1964,6 +1955,20 @@ static int x509_crt_verify_top( continue; } + if( mbedtls_x509_time_is_past( &trust_ca->valid_to ) || + mbedtls_x509_time_is_future( &trust_ca->valid_from ) ) + { + if ( future_past_ca == NULL ) + future_past_ca = trust_ca; + + continue; + } + + break; + } + + if( trust_ca != NULL || ( trust_ca = future_past_ca ) != NULL ) + { /* * Top of chain is signed by a trusted CA */ @@ -1971,8 +1976,6 @@ static int x509_crt_verify_top( if( x509_profile_check_key( profile, child->sig_pk, &trust_ca->pk ) != 0 ) *flags |= MBEDTLS_X509_BADCERT_BAD_KEY; - - break; } /* @@ -1992,6 +1995,12 @@ static int x509_crt_verify_top( ((void) ca_crl); #endif + if( mbedtls_x509_time_is_past( &trust_ca->valid_to ) ) + ca_flags |= MBEDTLS_X509_BADCERT_EXPIRED; + + if( mbedtls_x509_time_is_future( &trust_ca->valid_from ) ) + ca_flags |= MBEDTLS_X509_BADCERT_FUTURE; + if( NULL != f_vrfy ) { if( ( ret = f_vrfy( p_vrfy, trust_ca, path_cnt + 1, From 3da3b6eccbb21a6d052218d48b4e6de11e728078 Mon Sep 17 00:00:00 2001 From: Andres AG Date: Tue, 13 Dec 2016 09:59:07 +0000 Subject: [PATCH 2/2] Add tests for out flags from x509_crt_verify_top() The tests load certificate chains from files. The CA chains contain a past or future certificate and an invalid certificate. The test then checks that the flags set are MBEDTLS_X509_BADCERT_EXPIRED or MBEDTLS_X509_BADCERT_FUTURE. --- .../test-ca2_cat-future-invalid.crt | 27 +++++++++++++++++++ .../data_files/test-ca2_cat-past-invalid.crt | 27 +++++++++++++++++++ tests/suites/test_suite_x509parse.data | 8 ++++++ 3 files changed, 62 insertions(+) create mode 100644 tests/data_files/test-ca2_cat-future-invalid.crt create mode 100644 tests/data_files/test-ca2_cat-past-invalid.crt diff --git a/tests/data_files/test-ca2_cat-future-invalid.crt b/tests/data_files/test-ca2_cat-future-invalid.crt new file mode 100644 index 000000000..b1cfbf054 --- /dev/null +++ b/tests/data_files/test-ca2_cat-future-invalid.crt @@ -0,0 +1,27 @@ +-----BEGIN CERTIFICATE----- +MIICIDCCAaWgAwIBAgIBCjAKBggqhkjOPQQDAjA+MQswCQYDVQQGEwJOTDERMA8G +A1UEChMIUG9sYXJTU0wxHDAaBgNVBAMTE1BvbGFyc3NsIFRlc3QgRUMgQ0EwHhcN +MTMwOTI0MTU1MjA0WhcNMjMwOTIyMTU1MjA0WjA0MQswCQYDVQQGEwJOTDERMA8G +A1UEChMIUG9sYXJTU0wxEjAQBgNVBAMTCWxvY2FsaG9zdDBZMBMGByqGSM49AgEG +CCqGSM49AwEHA0IABIFZMXZJJPoVraugMW4O7TMR+pElVcGwwZwDcj6Yui2kcjeJ +H0M3jR+OOtjwV+gvT8kApPfbcw+yxgSU0UA7OOOjgZ0wgZowCQYDVR0TBAIwADAd +BgNVHQ4EFgQUfmWPPjMDFOXhvmCy4IV/jOdgK3swbgYDVR0jBGcwZYAUnW0gJEkB +PyvLeLUZvH4kydv7NnyhQqRAMD4xCzAJBgNVBAYTAk5MMREwDwYDVQQKEwhQb2xh +clNTTDEcMBoGA1UEAxMTUG9sYXJzc2wgVGVzdCBFQyBDQYIJAMFD4n5iQ8zoMAoG +CCqGSM49BAMCA2kAMGYCMQCsYTyleBFuI4nizuxo/ie5dxJnD0ynwCnRJ+84PZP4 +AQA3HdUz0qNYs4CZ2am9Gz0CMQDr2TNLFA3C3S3pmgXMT0eKzR1Ca1/Nulf0llQZ +Xj09kLboxuemP40IIqhQnpYptMg= +-----END CERTIFICATE----- +-----BEGIN CERTIFICATE----- +MIIB+zCCAYCgAwIBAgIBATAMBggqhkjOPQQDAgUAMD4xCzAJBgNVBAYTAk5MMREw +DwYDVQQKEwhQb2xhclNTTDEcMBoGA1UEAxMTUG9sYXJzc2wgVGVzdCBFQyBDQTAe +Fw0yMzA5MjIxNTQ5NDlaFw0zMDEyMzEyMzU5NTlaMD4xCzAJBgNVBAYTAk5MMREw +DwYDVQQKEwhQb2xhclNTTDEcMBoGA1UEAxMTUG9sYXJzc2wgVGVzdCBFQyBDQTB2 +MBAGByqGSM49AgEGBSuBBAAiA2IABMPaKzRBN1gvh1b+/Im6KUNLTuBuww5XUzM5 +WNRStJGVOQsj318XJGJI/BqVKc4sLYfCiFKAr9ZqqyHduNMcbli4yuiyaY7zQa0p +w7RfdadHb9UZKVVpmlM7ILRmFmAzHqNQME4wDAYDVR0TBAUwAwEB/zAdBgNVHQ4E +FgQUnW0gJEkBPyvLeLUZvH4kydv7NnwwHwYDVR0jBBgwFoAUnW0gJEkBPyvLeLUZ +vH4kydv7NnwwDAYIKoZIzj0EAwIFAANnADBkAjB1ZNdOM7KRJiPo45hP17A1sJSH +qHFPEJbml6KdNevoVZ1HqvP8AoFGcPJRpQVtzC0CMDa7JEqn0dOss8EmW9pVF/N2 ++XvzNczj89mWMgPhJJlT+MONQx3LFQO+TMSI9hLdkw== +-----END CERTIFICATE----- diff --git a/tests/data_files/test-ca2_cat-past-invalid.crt b/tests/data_files/test-ca2_cat-past-invalid.crt new file mode 100644 index 000000000..febad7408 --- /dev/null +++ b/tests/data_files/test-ca2_cat-past-invalid.crt @@ -0,0 +1,27 @@ +-----BEGIN CERTIFICATE----- +MIIB/TCCAYCgAwIBAgIBATAMBggqhkjOPQQDAgUAMD4xCzAJBgNVBAYTAk5MMREw +DwYDVQQKEwhQb2xhclNTTDEcMBoGA1UEAxMTUG9sYXJzc2wgVGVzdCBFQyBDQTAe +Fw0wMzA5MjQxNTQ5NDhaFw0xMzA5MjQxNTQ5NDhaMD4xCzAJBgNVBAYTAk5MMREw +DwYDVQQKEwhQb2xhclNTTDEcMBoGA1UEAxMTUG9sYXJzc2wgVGVzdCBFQyBDQTB2 +MBAGByqGSM49AgEGBSuBBAAiA2IABMPaKzRBN1gvh1b+/Im6KUNLTuBuww5XUzM5 +WNRStJGVOQsj318XJGJI/BqVKc4sLYfCiFKAr9ZqqyHduNMcbli4yuiyaY7zQa0p +w7RfdadHb9UZKVVpmlM7ILRmFmAzHqNQME4wDAYDVR0TBAUwAwEB/zAdBgNVHQ4E +FgQUnW0gJEkBPyvLeLUZvH4kydv7NnwwHwYDVR0jBBgwFoAUnW0gJEkBPyvLeLUZ +vH4kydv7NnwwDAYIKoZIzj0EAwIFAANpADBmAjEAvQ/49lXXrLYdOIGtTaYWjpZP +tRBXQiGPMzUvmKBk7gM7bF4iFPsdJikyXHmuwv3RAjEA8vtUX8fAAB3fbh5dEXRm +l7tz0Sw/RW6AHFtaIauGkhHqeKIaKIi6WSgHu6x97uyg +-----END CERTIFICATE----- +-----BEGIN CERTIFICATE----- +MIICIDCCAaWgAwIBAgIBCjAKBggqhkjOPQQDAjA+MQswCQYDVQQGEwJOTDERMA8G +A1UEChMIUG9sYXJTU0wxHDAaBgNVBAMTE1BvbGFyc3NsIFRlc3QgRUMgQ0EwHhcN +MTMwOTI0MTU1MjA0WhcNMjMwOTIyMTU1MjA0WjA0MQswCQYDVQQGEwJOTDERMA8G +A1UEChMIUG9sYXJTU0wxEjAQBgNVBAMTCWxvY2FsaG9zdDBZMBMGByqGSM49AgEG +CCqGSM49AwEHA0IABIFZMXZJJPoVraugMW4O7TMR+pElVcGwwZwDcj6Yui2kcjeJ +H0M3jR+OOtjwV+gvT8kApPfbcw+yxgSU0UA7OOOjgZ0wgZowCQYDVR0TBAIwADAd +BgNVHQ4EFgQUfmWPPjMDFOXhvmCy4IV/jOdgK3swbgYDVR0jBGcwZYAUnW0gJEkB +PyvLeLUZvH4kydv7NnyhQqRAMD4xCzAJBgNVBAYTAk5MMREwDwYDVQQKEwhQb2xh +clNTTDEcMBoGA1UEAxMTUG9sYXJzc2wgVGVzdCBFQyBDQYIJAMFD4n5iQ8zoMAoG +CCqGSM49BAMCA2kAMGYCMQCsYTyleBFuI4nizuxo/ie5dxJnD0ynwCnRJ+84PZP4 +AQA3HdUz0qNYs4CZ2am9Gz0CMQDr2TNLFA3C3S3pmgXMT0eKzR1Ca1/Nulf0llQZ +Xj09kLboxuemP40IIqhQnpYptMg= +-----END CERTIFICATE----- diff --git a/tests/suites/test_suite_x509parse.data b/tests/suites/test_suite_x509parse.data index 6511cef9f..9a44dbe72 100644 --- a/tests/suites/test_suite_x509parse.data +++ b/tests/suites/test_suite_x509parse.data @@ -715,6 +715,14 @@ X509 Certificate verification #85 (Not yet valid CA and valid CA) depends_on:MBEDTLS_PEM_PARSE_C:MBEDTLS_ECP_C:MBEDTLS_ECP_DP_SECP256R1_ENABLED:MBEDTLS_ECP_DP_SECP384R1_ENABLED x509_verify:"data_files/server5.crt":"data_files/test-ca2_cat-past-present.crt":"data_files/crl-ec-sha1.pem":"NULL":0:0:"NULL" +X509 Certificate verification #86 (Not yet valid CA and invalid CA) +depends_on:MBEDTLS_PEM_PARSE_C:MBEDTLS_ECP_C:MBEDTLS_ECP_DP_SECP256R1_ENABLED:MBEDTLS_ECP_DP_SECP384R1_ENABLED:MBEDTLS_SHA1_C:MBEDTLS_SHA256_C +x509_verify:"data_files/server5.crt":"data_files/test-ca2_cat-future-invalid.crt":"data_files/crl-ec-sha1.pem":"NULL":MBEDTLS_ERR_X509_CERT_VERIFY_FAILED:MBEDTLS_X509_BADCERT_FUTURE:"NULL" + +X509 Certificate verification #87 (Expired CA and invalid CA) +depends_on:MBEDTLS_PEM_PARSE_C:MBEDTLS_ECP_C:MBEDTLS_ECP_DP_SECP256R1_ENABLED:MBEDTLS_ECP_DP_SECP384R1_ENABLED:MBEDTLS_SHA1_C:MBEDTLS_SHA256_C +x509_verify:"data_files/server5.crt":"data_files/test-ca2_cat-past-invalid.crt":"data_files/crl-ec-sha1.pem":"NULL":MBEDTLS_ERR_X509_CERT_VERIFY_FAILED:MBEDTLS_X509_BADCERT_EXPIRED:"NULL" + X509 Certificate verification callback: trusted EE cert depends_on:MBEDTLS_PEM_PARSE_C:MBEDTLS_ECDSA_C:MBEDTLS_SHA256_C:MBEDTLS_ECP_DP_SECP256R1_ENABLED x509_verify_callback:"data_files/server5-selfsigned.crt":"data_files/server5-selfsigned.crt":0:"depth 0 - serial 53\:A2\:CB\:4B\:12\:4E\:AD\:83\:7D\:A8\:94\:B2 - subject CN=selfsigned, OU=testing, O=PolarSSL, C=NL\n"