From 053b966134dec5bdde30da92b581be0f523c4c6b Mon Sep 17 00:00:00 2001 From: Leigh Date: Fri, 6 Jan 2017 14:58:54 +0000 Subject: [PATCH 1/4] Fix memleaks from #1755 and some pre-existing ones --- ext/openssl/openssl.c | 36 +++++++++++++++++++++++++----------- 1 file changed, 25 insertions(+), 11 deletions(-) diff --git a/ext/openssl/openssl.c b/ext/openssl/openssl.c index 6d3f9ef4bbc..5de4869908a 100644 --- a/ext/openssl/openssl.c +++ b/ext/openssl/openssl.c @@ -703,6 +703,8 @@ static void add_assoc_name_entry(zval * val, char * key, X509_NAME * name, int s add_assoc_stringl(&subitem, sname, (char *)to_add, to_add_len); } } + + OPENSSL_free(to_add); } if (key != NULL) { zend_hash_str_update(Z_ARRVAL_P(val), key, strlen(key), &subitem); @@ -2004,7 +2006,10 @@ PHP_FUNCTION(openssl_x509_parse) char *extname; BIO *bio_out; BUF_MEM *bio_buf; - char * hexserial; + ASN1_INTEGER *asn1_serial; + BIGNUM *bn_serial; + char *str_serial; + char *hex_serial; char buf[256]; if (zend_parse_parameters(ZEND_NUM_ARGS(), "z|b", &zcert, &useshortnames) == FAILURE) { @@ -2032,19 +2037,28 @@ PHP_FUNCTION(openssl_x509_parse) add_assoc_name_entry(return_value, "issuer", X509_get_issuer_name(cert), useshortnames); add_assoc_long(return_value, "version", X509_get_version(cert)); - add_assoc_string(return_value, "serialNumber", i2s_ASN1_INTEGER(NULL, X509_get_serialNumber(cert))); + asn1_serial = X509_get_serialNumber(cert); - /* Return the hex representation of the serial number, as defined by OpenSSL */ - hexserial = BN_bn2hex(ASN1_INTEGER_to_BN(X509_get_serialNumber(cert), NULL)); - - /* If we received null back from BN_bn2hex, there was a critical error in openssl, - * and we should not continue. - */ - if (!hexserial) { + bn_serial = ASN1_INTEGER_to_BN(asn1_serial, NULL); + /* Can return NULL on error or memory allocation failure */ + if (!bn_serial) { RETURN_FALSE; } - add_assoc_string(return_value, "serialNumberHex", hexserial); - OPENSSL_free(hexserial); + + hex_serial = BN_bn2hex(bn_serial); + BN_free(bn_serial); + /* Can return NULL on error or memory allocation failure */ + if (!hex_serial) { + RETURN_FALSE; + } + + str_serial = i2s_ASN1_INTEGER(NULL, asn1_serial); + add_assoc_string(return_value, "serialNumber", str_serial); + OPENSSL_free(str_serial); + + /* Return the hex representation of the serial number, as defined by OpenSSL */ + add_assoc_string(return_value, "serialNumberHex", hex_serial); + OPENSSL_free(hex_serial); add_assoc_asn1_string(return_value, "validFrom", X509_get_notBefore(cert)); add_assoc_asn1_string(return_value, "validTo", X509_get_notAfter(cert)); From 9a0dac124e42cf5919ecbdff6ee814688a789292 Mon Sep 17 00:00:00 2001 From: Leigh Date: Fri, 6 Jan 2017 15:24:37 +0000 Subject: [PATCH 2/4] Conditionally free depending on how assignment happened --- ext/openssl/openssl.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/ext/openssl/openssl.c b/ext/openssl/openssl.c index 5de4869908a..323bdcf4582 100644 --- a/ext/openssl/openssl.c +++ b/ext/openssl/openssl.c @@ -667,9 +667,9 @@ static void add_assoc_name_entry(zval * val, char * key, X509_NAME * name, int s } for (i = 0; i < X509_NAME_entry_count(name); i++) { - unsigned char *to_add; + unsigned char *to_add = NULL; int to_add_len = 0; - + int needs_free = 0; ne = X509_NAME_get_entry(name, i); obj = X509_NAME_ENTRY_get_object(ne); @@ -684,6 +684,7 @@ static void add_assoc_name_entry(zval * val, char * key, X509_NAME * name, int s str = X509_NAME_ENTRY_get_data(ne); if (ASN1_STRING_type(str) != V_ASN1_UTF8STRING) { to_add_len = ASN1_STRING_to_UTF8(&to_add, str); + needs_free = 1; } else { to_add = ASN1_STRING_data(str); to_add_len = ASN1_STRING_length(str); @@ -704,8 +705,13 @@ static void add_assoc_name_entry(zval * val, char * key, X509_NAME * name, int s } } - OPENSSL_free(to_add); + if (needs_free) { + OPENSSL_free(to_add); + to_add = NULL; + needs_free = 0; + } } + if (key != NULL) { zend_hash_str_update(Z_ARRVAL_P(val), key, strlen(key), &subitem); } From 62e9e1fecd1f2fd9981244889389dfd3f92eb4d2 Mon Sep 17 00:00:00 2001 From: Leigh Date: Fri, 6 Jan 2017 15:49:15 +0000 Subject: [PATCH 3/4] Some commentary, change free method --- ext/openssl/openssl.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/ext/openssl/openssl.c b/ext/openssl/openssl.c index 323bdcf4582..2e90c7c1d2c 100644 --- a/ext/openssl/openssl.c +++ b/ext/openssl/openssl.c @@ -683,9 +683,11 @@ static void add_assoc_name_entry(zval * val, char * key, X509_NAME * name, int s str = X509_NAME_ENTRY_get_data(ne); if (ASN1_STRING_type(str) != V_ASN1_UTF8STRING) { + /* ASN1_STRING_to_UTF8(3): The converted data is copied into a newly allocated buffer */ to_add_len = ASN1_STRING_to_UTF8(&to_add, str); needs_free = 1; } else { + /* ASN1_STRING_data(3): Since this is an internal pointer it should not be freed or modified in any way */ to_add = ASN1_STRING_data(str); to_add_len = ASN1_STRING_length(str); } @@ -706,8 +708,8 @@ static void add_assoc_name_entry(zval * val, char * key, X509_NAME * name, int s } if (needs_free) { - OPENSSL_free(to_add); - to_add = NULL; + /* ASN1_STRING_to_UTF8(3): The buffer out should be freed using free(3) */ + free(to_add); needs_free = 0; } } From 21f287915cfda72db3097965d1ee6e987c21cd50 Mon Sep 17 00:00:00 2001 From: Leigh Date: Sat, 7 Jan 2017 09:43:05 +0000 Subject: [PATCH 4/4] Remove superfluous variable reinit --- ext/openssl/openssl.c | 1 - 1 file changed, 1 deletion(-) diff --git a/ext/openssl/openssl.c b/ext/openssl/openssl.c index 2e90c7c1d2c..d7091b8e42c 100644 --- a/ext/openssl/openssl.c +++ b/ext/openssl/openssl.c @@ -710,7 +710,6 @@ static void add_assoc_name_entry(zval * val, char * key, X509_NAME * name, int s if (needs_free) { /* ASN1_STRING_to_UTF8(3): The buffer out should be freed using free(3) */ free(to_add); - needs_free = 0; } }