From c014f8e17ed814c78926ed7cfcb163de1a7bf2b9 Mon Sep 17 00:00:00 2001 From: Marcel Jordense Date: Wed, 6 Mar 2024 15:27:11 +0100 Subject: [PATCH] resolve memory leak in security test cases and process review comments Signed-off-by: Marcel Jordense --- .../authentication/src/auth_utils.c | 34 +++++++++++-------- .../tests/common/src/handshake_helper.c | 16 ++++----- .../src/listeners_authentication_utests.c | 8 ++--- src/security/core/tests/common/cert_utils.c | 4 +-- .../include/dds/security/openssl_support.h | 1 + 5 files changed, 35 insertions(+), 28 deletions(-) diff --git a/src/security/builtin_plugins/authentication/src/auth_utils.c b/src/security/builtin_plugins/authentication/src/auth_utils.c index 0085a40ba5..388f58fff6 100644 --- a/src/security/builtin_plugins/authentication/src/auth_utils.c +++ b/src/security/builtin_plugins/authentication/src/auth_utils.c @@ -237,12 +237,12 @@ static DDS_Security_ValidationResult_t check_certificate_type_and_size(X509 *cer DDS_Security_ValidationResult_t check_certificate_expiry(const X509 *cert, DDS_Security_SecurityException *ex) { assert(cert); - if (X509_cmp_current_time(X509_get_notBefore(cert)) == 0) + if (X509_cmp_current_time(X509_get0_notBefore(cert)) == 0) { DDS_Security_Exception_set(ex, DDS_AUTH_PLUGIN_CONTEXT, DDS_SECURITY_ERR_CERT_STARTDATE_IN_FUTURE_CODE, DDS_SECURITY_VALIDATION_FAILED, DDS_SECURITY_ERR_CERT_STARTDATE_IN_FUTURE_MESSAGE); return DDS_SECURITY_VALIDATION_FAILED; } - if (X509_cmp_current_time(X509_get_notAfter(cert)) == 0) + if (X509_cmp_current_time(X509_get0_notAfter(cert)) == 0) { DDS_Security_Exception_set(ex, DDS_AUTH_PLUGIN_CONTEXT, DDS_SECURITY_ERR_CERT_EXPIRED_CODE, DDS_SECURITY_VALIDATION_FAILED, DDS_SECURITY_ERR_CERT_EXPIRED_MESSAGE); return DDS_SECURITY_VALIDATION_FAILED; @@ -908,6 +908,7 @@ static int dh_set_public_key(DH *dhkey, BIGNUM *pubkey) static DDS_Security_ValidationResult_t dh_public_key_to_oct_modp(EVP_PKEY *pkey, unsigned char **buffer, uint32_t *length, DDS_Security_SecurityException *ex) { + DDS_Security_ValidationResult_t result = DDS_SECURITY_VALIDATION_FAILED; DH *dhkey; ASN1_INTEGER *asn1int; *buffer = NULL; @@ -919,31 +920,32 @@ static DDS_Security_ValidationResult_t dh_public_key_to_oct_modp(EVP_PKEY *pkey, if (!(asn1int = BN_to_ASN1_INTEGER (dh_get_public_key(dhkey), NULL))) { DDS_Security_Exception_set_with_openssl_error(ex, DDS_AUTH_PLUGIN_CONTEXT, DDS_SECURITY_ERR_UNDEFINED_CODE, DDS_SECURITY_VALIDATION_FAILED, "Failed to convert DH key to ASN1 integer: "); - DH_free(dhkey); - return DDS_SECURITY_VALIDATION_FAILED; + goto failed_asn1int; } int i2dlen = i2d_ASN1_INTEGER (asn1int, NULL); if (i2dlen <= 0) { DDS_Security_Exception_set_with_openssl_error(ex, DDS_AUTH_PLUGIN_CONTEXT, DDS_SECURITY_ERR_UNDEFINED_CODE, DDS_SECURITY_VALIDATION_FAILED, "Failed to convert DH key to ASN1 integer: "); - DH_free(dhkey); - return DDS_SECURITY_VALIDATION_FAILED; + goto failed_i2d; } *length = (uint32_t) i2dlen; if ((*buffer = ddsrt_malloc (*length)) == NULL) { DDS_Security_Exception_set_with_openssl_error(ex, DDS_AUTH_PLUGIN_CONTEXT, DDS_SECURITY_ERR_UNDEFINED_CODE, DDS_SECURITY_VALIDATION_FAILED, "Failed to convert DH key to ASN1 integer: "); - DH_free(dhkey); - return DDS_SECURITY_VALIDATION_FAILED; + goto failed_i2d; } unsigned char *buffer_arg = *buffer; (void) i2d_ASN1_INTEGER (asn1int, &buffer_arg); + result = DDS_SECURITY_VALIDATION_OK; + +failed_i2d: ASN1_INTEGER_free (asn1int); +failed_asn1int: DH_free (dhkey); - return DDS_SECURITY_VALIDATION_OK; + return result; } static DDS_Security_ValidationResult_t dh_public_key_to_oct_ecdh(EVP_PKEY *pkey, unsigned char **buffer, uint32_t *length, DDS_Security_SecurityException *ex) @@ -1040,6 +1042,7 @@ static DDS_Security_ValidationResult_t dh_oct_to_public_key_ecdh(EVP_PKEY **pkey EC_KEY *eckey; EC_GROUP *group; EC_POINT *point; + if (!(group = EC_GROUP_new_by_curve_name(NID_X9_62_prime256v1))) { DDS_Security_Exception_set_with_openssl_error(ex, DDS_AUTH_PLUGIN_CONTEXT, DDS_SECURITY_ERR_UNDEFINED_CODE, DDS_SECURITY_VALIDATION_FAILED, "Failed to allocate EC group: "); @@ -1210,6 +1213,7 @@ static DDS_Security_ValidationResult_t dh_oct_to_public_key_modp(EVP_PKEY **pkey static DDS_Security_ValidationResult_t dh_oct_to_public_key_ecdh(EVP_PKEY **pkey, const unsigned char *keystr, uint32_t size, DDS_Security_SecurityException *ex) { + DDS_Security_ValidationResult_t result = DDS_SECURITY_VALIDATION_OK; EVP_PKEY_CTX *ctx = NULL; OSSL_PARAM params[3]; params[0] = OSSL_PARAM_construct_utf8_string(OSSL_PKEY_PARAM_GROUP_NAME, SN_X9_62_prime256v1, 0); @@ -1224,17 +1228,19 @@ static DDS_Security_ValidationResult_t dh_oct_to_public_key_ecdh(EVP_PKEY **pkey if (EVP_PKEY_fromdata_init(ctx) != 1) { DDS_Security_Exception_set(ex, DDS_AUTH_PLUGIN_CONTEXT, DDS_SECURITY_ERR_UNDEFINED_CODE, DDS_SECURITY_VALIDATION_FAILED, "EVP_PKEY_fromdata_init(EC) failed: "); - EVP_PKEY_CTX_free(ctx); - return DDS_SECURITY_VALIDATION_FAILED; + result = DDS_SECURITY_VALIDATION_FAILED; + goto failed; } if (EVP_PKEY_fromdata(ctx, pkey, EVP_PKEY_PUBLIC_KEY, params) != 1) { DDS_Security_Exception_set(ex, DDS_AUTH_PLUGIN_CONTEXT, DDS_SECURITY_ERR_UNDEFINED_CODE, DDS_SECURITY_VALIDATION_FAILED, "EVP_PKEY_fromdata(EC) failed: "); - EVP_PKEY_CTX_free(ctx); - return DDS_SECURITY_VALIDATION_FAILED; + result = DDS_SECURITY_VALIDATION_FAILED; + goto failed; } - return DDS_SECURITY_VALIDATION_OK; +failed: + EVP_PKEY_CTX_free(ctx); + return result; } #endif diff --git a/src/security/builtin_plugins/tests/common/src/handshake_helper.c b/src/security/builtin_plugins/tests/common/src/handshake_helper.c index 4400bcc33a..8e4ee733a9 100644 --- a/src/security/builtin_plugins/tests/common/src/handshake_helper.c +++ b/src/security/builtin_plugins/tests/common/src/handshake_helper.c @@ -75,6 +75,7 @@ dh_set_public_key( ASN1_INTEGER * get_pubkey_asn1int(EVP_PKEY *pkey) { + ASN1_INTEGER *result; DH *dhkey = EVP_PKEY_get1_DH(pkey); if (!dhkey) { char *msg = get_openssl_error_message_for_test(); @@ -82,7 +83,9 @@ get_pubkey_asn1int(EVP_PKEY *pkey) ddsrt_free(msg); return NULL; } - return BN_to_ASN1_INTEGER(dh_get_public_key(dhkey), NULL); + result = BN_to_ASN1_INTEGER(dh_get_public_key(dhkey), NULL); + DH_free(dhkey); + return result; } int @@ -411,7 +414,7 @@ get_pubkey_asn1int(EVP_PKEY *pkey) return NULL; } asn1int = BN_to_ASN1_INTEGER(pubkey_bn, NULL); - OPENSSL_free(pubkey_bn); + BN_free(pubkey_bn); return asn1int; } @@ -555,6 +558,7 @@ modp_data_to_pubkey( EVP_PKEY_CTX_free(pctx); fail_ctx: BN_free(pubkey); + ddsrt_free(buffer); fail_pubkey: ASN1_INTEGER_free(asn1int); fail_asni: @@ -919,10 +923,6 @@ get_public_key( return result; } - - - - int check_shared_secret( dds_security_authentication *auth, @@ -1079,9 +1079,9 @@ create_asymmetrical_signature_for_test( ddsrt_free(*signature); } - err_sign: +err_sign: EVP_MD_CTX_destroy(mdctx); - err_create_ctx: +err_create_ctx: return result; } diff --git a/src/security/builtin_plugins/tests/listeners_authentication/src/listeners_authentication_utests.c b/src/security/builtin_plugins/tests/listeners_authentication/src/listeners_authentication_utests.c index a3c09974ab..8d4179fb3e 100644 --- a/src/security/builtin_plugins/tests/listeners_authentication/src/listeners_authentication_utests.c +++ b/src/security/builtin_plugins/tests/listeners_authentication/src/listeners_authentication_utests.c @@ -279,11 +279,11 @@ get_certificate_expiry( /*_In_*/ X509 *cert) { dds_time_t expiry = DDS_TIME_INVALID; - ASN1_TIME *ans1; + const ASN1_TIME *ans1; assert(cert); - ans1 = X509_get_notAfter(cert); + ans1 = X509_get0_notAfter(cert); if (ans1 != NULL) { int days; int seconds; @@ -454,12 +454,12 @@ static DDS_Security_boolean create_certificate_from_csr(const char* csr, long va /* ---------------------------------------------------------- * * Set X509V3 start date (now) and expiration date (+365 days)* * -----------------------------------------------------------*/ - if (!(X509_gmtime_adj(X509_get_notBefore(newcert), -10))) { + if (!(X509_gmtime_adj(X509_getm_notBefore(newcert), -10))) { BIO_printf(outbio, "Error setting start time\n"); return false; } - if (!(X509_gmtime_adj(X509_get_notAfter(newcert), valid_secs))) { + if (!(X509_gmtime_adj(X509_getm_notAfter(newcert), valid_secs))) { BIO_printf(outbio, "Error setting expiration time\n"); return false; } diff --git a/src/security/core/tests/common/cert_utils.c b/src/security/core/tests/common/cert_utils.c index e6a8709565..49a2d5892e 100644 --- a/src/security/core/tests/common/cert_utils.c +++ b/src/security/core/tests/common/cert_utils.c @@ -25,8 +25,8 @@ static X509 * get_x509(int not_valid_before, int not_valid_after, const char * c X509 * cert = X509_new (); CU_ASSERT_FATAL (cert != NULL); ASN1_INTEGER_set (X509_get_serialNumber (cert), 1); - X509_gmtime_adj (X509_get_notBefore (cert), not_valid_before); - X509_gmtime_adj (X509_get_notAfter (cert), not_valid_after); + X509_gmtime_adj (X509_getm_notBefore (cert), not_valid_before); + X509_gmtime_adj (X509_getm_notAfter (cert), not_valid_after); X509_NAME * name = X509_get_subject_name (cert); X509_NAME_add_entry_by_txt (name, "C", MBSTRING_ASC, (unsigned char *) "NL", -1, -1, 0); diff --git a/src/security/openssl/include/dds/security/openssl_support.h b/src/security/openssl/include/dds/security/openssl_support.h index 196d4cc56b..352c16bb9f 100644 --- a/src/security/openssl/include/dds/security/openssl_support.h +++ b/src/security/openssl/include/dds/security/openssl_support.h @@ -38,6 +38,7 @@ #include #endif +/* Setting this macro to 30000 specifies that the code will be compatible with openssl version 3 and lower like version 1.1 */ #define OPENSSL_API_COMPAT 30000 #include