From d2195f8b35f0105b9692ec2e25090890f8e53bf3 Mon Sep 17 00:00:00 2001 From: mrohera Date: Fri, 5 Oct 2018 20:02:24 -0700 Subject: [PATCH] Ensure that cert verify returns verification failure and not a failure (#381) --- .../src/edge_hsm_client_store.c | 2 +- .../azure-iot-hsm-c/src/edge_pki_openssl.c | 73 +++++++++++-------- .../edge_openssl_pki_ut/edge_openssl_pki_ut.c | 64 +++++++++++++--- 3 files changed, 97 insertions(+), 42 deletions(-) diff --git a/edgelet/hsm-sys/azure-iot-hsm-c/src/edge_hsm_client_store.c b/edgelet/hsm-sys/azure-iot-hsm-c/src/edge_hsm_client_store.c index 05f0aff8e2f..37c27f1f93b 100644 --- a/edgelet/hsm-sys/azure-iot-hsm-c/src/edge_hsm_client_store.c +++ b/edgelet/hsm-sys/azure-iot-hsm-c/src/edge_hsm_client_store.c @@ -1641,7 +1641,7 @@ static int generate_edge_hsm_certificates_if_needed(void) else if ((load_status == LOAD_ERR_VERIFICATION_FAILED) || (load_status == LOAD_ERR_NOT_FOUND)) { - LOG_DEBUG("Load status %d. Generating owner and device CA certs and keys", load_status); + LOG_INFO("Load status %d. Regenerating owner and device CA certs and keys", load_status); if (create_owner_ca_cert() != 0) { result = __FAILURE__; diff --git a/edgelet/hsm-sys/azure-iot-hsm-c/src/edge_pki_openssl.c b/edgelet/hsm-sys/azure-iot-hsm-c/src/edge_pki_openssl.c index b93aca18153..4bd29d928e0 100644 --- a/edgelet/hsm-sys/azure-iot-hsm-c/src/edge_pki_openssl.c +++ b/edgelet/hsm-sys/azure-iot-hsm-c/src/edge_pki_openssl.c @@ -632,12 +632,13 @@ static int cert_set_core_properties return result; } -static int validate_certificate_expiration(X509* x509_cert, double *exp_seconds_left) +static int validate_certificate_expiration(X509* x509_cert, double *exp_seconds_left, bool *is_expired) { int result; time_t exp_time; double seconds_left = 0; + *is_expired = true; time_t now = time(NULL); ASN1_TIME *exp_asn1 = X509_get_notAfter(x509_cert); if ((exp_asn1->type != ASN1_TIME_STRING_UTC_FORMAT) && @@ -651,13 +652,16 @@ static int validate_certificate_expiration(X509* x509_cert, double *exp_seconds_ LOG_ERROR("Could not parse expiration date from certificate"); result = __FAILURE__; } - else if ((seconds_left = difftime(exp_time, now)) <= 0) - { - LOG_ERROR("Certificate has expired"); - result = __FAILURE__; - } else { + if ((seconds_left = difftime(exp_time, now)) <= 0) + { + LOG_ERROR("Certificate has expired"); + } + else + { + *is_expired = false; + } result = 0; } @@ -688,9 +692,11 @@ static int cert_set_expiration { // determine max validity in seconds of issuer double exp_seconds_left_from_now = 0; - if (validate_certificate_expiration(issuer_cert, &exp_seconds_left_from_now) != 0) + bool is_expired = true; + int status = validate_certificate_expiration(issuer_cert, &exp_seconds_left_from_now, &is_expired); + if ((status != 0) || (is_expired)) { - LOG_ERROR("Issuer certificate expiration failure"); + LOG_ERROR("Issuer certificate expiration failure. Status %d, verify status: %d", status, is_expired); result = __FAILURE__; } else @@ -1355,19 +1361,12 @@ static int check_certificates int result; X509_STORE_CTX *store_ctxt = NULL; X509* x509_cert = NULL; - double exp_seconds = 0; if ((x509_cert = load_certificate_file(cert_file)) == NULL) { LOG_ERROR("Could not create X509 to verify certificate %s", cert_file); result = __FAILURE__; } - else if (validate_certificate_expiration(x509_cert, &exp_seconds) != 0) - { - LOG_ERROR("Certificate file has expired %s", cert_file); - X509_free(x509_cert); - result = __FAILURE__; - } else if ((store_ctxt = X509_STORE_CTX_new()) == NULL) { LOG_ERROR("Could not create X509 store context"); @@ -1386,26 +1385,42 @@ static int check_certificates } else { + double exp_seconds = 0; int status; - if ((status = X509_verify_cert(store_ctxt)) <= 0) + bool is_expired = true; + + status = validate_certificate_expiration(x509_cert, &exp_seconds, &is_expired); + if (status != 0) { - const char *msg; - int err_code = X509_STORE_CTX_get_error(store_ctxt); - msg = X509_verify_cert_error_string(err_code); - if (msg == NULL) - { - msg = ""; - } - LOG_ERROR("Could not verify certificate %s using issuer certificate %s.", - cert_file, issuer_cert_file); - LOG_ERROR("Verification status: %d, Error: %d, Msg: '%s'", status, err_code, msg); + LOG_ERROR("Verifying certificate expiration failed for %s", cert_file); + result = __FAILURE__; } else { - LOG_DEBUG("Certificate validated %s", cert_file); - *verify_status = true; + if (is_expired) + { + LOG_INFO("Certificate file has expired %s", cert_file); + } + else if ((status = X509_verify_cert(store_ctxt)) <= 0) + { + const char *msg; + int err_code = X509_STORE_CTX_get_error(store_ctxt); + msg = X509_verify_cert_error_string(err_code); + if (msg == NULL) + { + msg = ""; + } + LOG_ERROR("Could not verify certificate %s using issuer certificate %s.", + cert_file, issuer_cert_file); + LOG_ERROR("Verification status: %d, Error: %d, Msg: '%s'", status, err_code, msg); + } + else + { + LOG_DEBUG("Certificate validated %s", cert_file); + *verify_status = true; + } + result = 0; } - result = 0; } X509_STORE_CTX_free(store_ctxt); X509_free(x509_cert); diff --git a/edgelet/hsm-sys/azure-iot-hsm-c/tests/edge_openssl_pki_ut/edge_openssl_pki_ut.c b/edgelet/hsm-sys/azure-iot-hsm-c/tests/edge_openssl_pki_ut/edge_openssl_pki_ut.c index 9d99d3988aa..49a8a646785 100644 --- a/edgelet/hsm-sys/azure-iot-hsm-c/tests/edge_openssl_pki_ut/edge_openssl_pki_ut.c +++ b/edgelet/hsm-sys/azure-iot-hsm-c/tests/edge_openssl_pki_ut/edge_openssl_pki_ut.c @@ -294,6 +294,14 @@ static ASN1_TIME TEST_ASN1_TIME_AFTER = { .flags = 0 }; +unsigned char ASN1_DATA_EXPIRED[] = "EXP012345678"; +static ASN1_TIME TEST_ASN1_TIME_AFTER_EXPIRED = { + .length = VALID_ASN1_TIME_STRING_UTC_LEN, + .type = VALID_ASN1_TIME_STRING_UTC_FORMAT, + .data = ASN1_DATA_EXPIRED, + .flags = 0 +}; + static ASN1_TIME TEST_UTC_NOW_TIME_FROM_ASN1 = { VALID_ASN1_TIME_STRING_UTC_LEN, VALID_ASN1_TIME_STRING_UTC_FORMAT, @@ -638,7 +646,14 @@ static time_t test_hook_get_utc_time_from_asn_string (void)length; time_t now = time(NULL); - return now + TEST_UTC_TIME_FROM_ASN1; + int offset = TEST_UTC_TIME_FROM_ASN1; + + if (memcmp(time_value, ASN1_DATA_EXPIRED, sizeof(ASN1_DATA_EXPIRED)) == 0) + { + // this ensures that certificate will always be evaluated as expired + offset = -5; + } + return now + offset; } static ASN1_TIME* test_hook_X509_gmtime_adj(ASN1_TIME *s, long adj) @@ -1621,6 +1636,7 @@ static void test_helper_verify_certificate const char *key_file, const char *issuer_cert_file, bool force_set_verify_return_value, + ASN1_TIME *force_set_asn1_time, char *failed_function_list, size_t failed_function_size ) @@ -1681,14 +1697,6 @@ static void test_helper_verify_certificate test_helper_load_cert_file(TEST_CERT_FILE, TEST_X509, &i, failed_function_list, failed_function_size); - STRICT_EXPECTED_CALL(mocked_X509_get_notAfter(TEST_X509)); - ASSERT_IS_TRUE_WITH_MSG((i < failed_function_size), "Line:" TOSTRING(__LINE__)); - i++; - - STRICT_EXPECTED_CALL(get_utc_time_from_asn_string(TEST_ASN1_TIME_AFTER.data, VALID_ASN1_TIME_STRING_UTC_LEN)); - ASSERT_IS_TRUE_WITH_MSG((i < failed_function_size), "Line:" TOSTRING(__LINE__)); - failed_function_list[i++] = 1; - STRICT_EXPECTED_CALL(X509_STORE_CTX_new()); ASSERT_IS_TRUE_WITH_MSG((i < failed_function_size), "Line:" TOSTRING(__LINE__)); failed_function_list[i++] = 1; @@ -1705,6 +1713,15 @@ static void test_helper_verify_certificate ASSERT_IS_TRUE_WITH_MSG((i < failed_function_size), "Line:" TOSTRING(__LINE__)); failed_function_list[i++] = 1; + ASN1_TIME *asn1_time = (force_set_asn1_time != NULL) ? force_set_asn1_time : &TEST_ASN1_TIME_AFTER; + STRICT_EXPECTED_CALL(mocked_X509_get_notAfter(TEST_X509)).SetReturn(asn1_time); + ASSERT_IS_TRUE_WITH_MSG((i < failed_function_size), "Line:" TOSTRING(__LINE__)); + i++; + + STRICT_EXPECTED_CALL(get_utc_time_from_asn_string(asn1_time->data, VALID_ASN1_TIME_STRING_UTC_LEN)); + ASSERT_IS_TRUE_WITH_MSG((i < failed_function_size), "Line:" TOSTRING(__LINE__)); + failed_function_list[i++] = 1; + int return_value = (force_set_verify_return_value)?1:0; STRICT_EXPECTED_CALL(X509_verify_cert(TEST_STORE_CTXT)).SetReturn(return_value); ASSERT_IS_TRUE_WITH_MSG((i < failed_function_size), "Line:" TOSTRING(__LINE__)); @@ -2970,7 +2987,7 @@ BEGIN_TEST_SUITE(edge_openssl_pki_unittests) size_t failed_function_size = MAX_FAILED_FUNCTION_LIST_SIZE; char failed_function_list[MAX_FAILED_FUNCTION_LIST_SIZE]; memset(failed_function_list, 0, failed_function_size); - test_helper_verify_certificate(TEST_CERT_FILE, TEST_KEY_FILE, TEST_ISSUER_CERT_FILE, true, failed_function_list, failed_function_size); + test_helper_verify_certificate(TEST_CERT_FILE, TEST_KEY_FILE, TEST_ISSUER_CERT_FILE, true, NULL, failed_function_list, failed_function_size); bool verify_status = true; // act @@ -3020,7 +3037,7 @@ BEGIN_TEST_SUITE(edge_openssl_pki_unittests) size_t failed_function_size = MAX_FAILED_FUNCTION_LIST_SIZE; char failed_function_list[MAX_FAILED_FUNCTION_LIST_SIZE]; memset(failed_function_list, 0, failed_function_size); - test_helper_verify_certificate(TEST_CERT_FILE, TEST_KEY_FILE, TEST_ISSUER_CERT_FILE, false, failed_function_list, failed_function_size); + test_helper_verify_certificate(TEST_CERT_FILE, TEST_KEY_FILE, TEST_ISSUER_CERT_FILE, false, NULL, failed_function_list, failed_function_size); bool verify_status = false; // act @@ -3034,6 +3051,29 @@ BEGIN_TEST_SUITE(edge_openssl_pki_unittests) // cleanup } + /** + * Test function for API + * verify_certificate + */ + TEST_FUNCTION(verify_certificate_expired_certificate_verifies_false_and_returns_success) + { + // arrange + size_t failed_function_size = MAX_FAILED_FUNCTION_LIST_SIZE; + char failed_function_list[MAX_FAILED_FUNCTION_LIST_SIZE]; + memset(failed_function_list, 0, failed_function_size); + test_helper_verify_certificate(TEST_CERT_FILE, TEST_KEY_FILE, TEST_ISSUER_CERT_FILE, false, &TEST_ASN1_TIME_AFTER_EXPIRED, failed_function_list, failed_function_size); + bool verify_status = true; + + // act + int status = verify_certificate(TEST_CERT_FILE, TEST_KEY_FILE, TEST_ISSUER_CERT_FILE, &verify_status); + + // assert + ASSERT_ARE_EQUAL_WITH_MSG(int, 0, status, "Line:" TOSTRING(__LINE__)); + ASSERT_IS_FALSE_WITH_MSG(verify_status, "Line:" TOSTRING(__LINE__)); + + // cleanup + } + /** * Test function for API * verify_certificate @@ -3047,7 +3087,7 @@ BEGIN_TEST_SUITE(edge_openssl_pki_unittests) size_t failed_function_size = MAX_FAILED_FUNCTION_LIST_SIZE; char failed_function_list[MAX_FAILED_FUNCTION_LIST_SIZE]; memset(failed_function_list, 0, failed_function_size); - test_helper_verify_certificate(TEST_CERT_FILE, TEST_KEY_FILE, TEST_ISSUER_CERT_FILE, true, failed_function_list, failed_function_size); + test_helper_verify_certificate(TEST_CERT_FILE, TEST_KEY_FILE, TEST_ISSUER_CERT_FILE, true, NULL, failed_function_list, failed_function_size); umock_c_negative_tests_snapshot(); for (size_t i = 0; i < umock_c_negative_tests_call_count(); i++)