From ea4cf7dbe978f1fa16daf6295d7385b311edba7d Mon Sep 17 00:00:00 2001 From: Hilko Bengen Date: Sat, 2 Dec 2017 19:59:37 +0100 Subject: [PATCH 1/3] Adapt most of the X509 support to OpenSSL 1.1 --- src/File.cc | 4 +- src/file_analysis/analyzer/x509/X509.cc | 46 +++++++------ src/file_analysis/analyzer/x509/X509.h | 41 +++++++++++ src/file_analysis/analyzer/x509/X509Common.cc | 9 ++- src/file_analysis/analyzer/x509/functions.bif | 68 +++++++++++-------- src/main.cc | 2 - 6 files changed, 116 insertions(+), 54 deletions(-) diff --git a/src/File.cc b/src/File.cc index e0e0d63332..609ea4f0ac 100644 --- a/src/File.cc +++ b/src/File.cc @@ -692,7 +692,7 @@ void BroFile::InitEncrypt(const char* keyfile) // Depending on the OpenSSL version, EVP_*_cbc() // returns a const or a non-const. EVP_CIPHER* cipher_type = (EVP_CIPHER*) EVP_bf_cbc(); - cipher_ctx = new EVP_CIPHER_CTX; + cipher_ctx = EVP_CIPHER_CTX_new(); unsigned char secret[EVP_PKEY_size(pub_key)]; unsigned char* psecret = secret; @@ -747,7 +747,7 @@ void BroFile::FinishEncrypt() return; } - delete cipher_ctx; + EVP_CIPHER_CTX_free(cipher_ctx); cipher_ctx = 0; } } diff --git a/src/file_analysis/analyzer/x509/X509.cc b/src/file_analysis/analyzer/x509/X509.cc index 2274f8fe3d..c9e78bb0b9 100644 --- a/src/file_analysis/analyzer/x509/X509.cc +++ b/src/file_analysis/analyzer/x509/X509.cc @@ -139,7 +139,9 @@ RecordVal* file_analysis::X509::ParseCertificate(X509Val* cert_val, const char* // we only read 255 bytes because byte 256 is always 0. // if the string is longer than 255, that will be our null-termination, // otherwhise i2t does null-terminate. - if ( ! i2t_ASN1_OBJECT(buf, 255, ssl_cert->cert_info->key->algor->algorithm) ) + ASN1_OBJECT *algorithm; + X509_PUBKEY_get0_param(&algorithm, NULL, NULL, NULL, X509_get_X509_PUBKEY(ssl_cert)); + if ( ! i2t_ASN1_OBJECT(buf, 255, algorithm) ) buf[0] = 0; pX509Cert->Assign(7, new StringVal(buf)); @@ -150,14 +152,12 @@ RecordVal* file_analysis::X509::ParseCertificate(X509Val* cert_val, const char* // actually should be (namely - rsaEncryption), so that OpenSSL will parse out the // key later. Otherwise it will just fail to parse the certificate key. - ASN1_OBJECT* old_algorithm = 0; - if ( OBJ_obj2nid(ssl_cert->cert_info->key->algor->algorithm) == NID_md5WithRSAEncryption ) - { - old_algorithm = ssl_cert->cert_info->key->algor->algorithm; - ssl_cert->cert_info->key->algor->algorithm = OBJ_nid2obj(NID_rsaEncryption); - } + if ( X509_get_signature_nid(ssl_cert) == NID_md5WithRSAEncryption ) + X509_PUBKEY_set0_param(X509_get_X509_PUBKEY(ssl_cert), OBJ_nid2obj(NID_rsaEncryption), 0, NULL, NULL, 0); + else + algorithm = 0; - if ( ! i2t_ASN1_OBJECT(buf, 255, ssl_cert->sig_alg->algorithm) ) + if ( ! i2t_ASN1_OBJECT(buf, 255, OBJ_nid2obj(X509_get_signature_nid(ssl_cert))) ) buf[0] = 0; pX509Cert->Assign(8, new StringVal(buf)); @@ -166,14 +166,16 @@ RecordVal* file_analysis::X509::ParseCertificate(X509Val* cert_val, const char* EVP_PKEY *pkey = X509_extract_key(ssl_cert); if ( pkey != NULL ) { - if ( pkey->type == EVP_PKEY_DSA ) + if ( EVP_PKEY_base_id(pkey) == EVP_PKEY_DSA ) pX509Cert->Assign(9, new StringVal("dsa")); - else if ( pkey->type == EVP_PKEY_RSA ) + else if ( EVP_PKEY_base_id(pkey) == EVP_PKEY_RSA ) { pX509Cert->Assign(9, new StringVal("rsa")); - char *exponent = BN_bn2dec(pkey->pkey.rsa->e); + const BIGNUM *e; + RSA_get0_key(EVP_PKEY_get0_RSA(pkey), NULL, &e, NULL); + char *exponent = BN_bn2dec(e); if ( exponent != NULL ) { pX509Cert->Assign(11, new StringVal(exponent)); @@ -182,7 +184,7 @@ RecordVal* file_analysis::X509::ParseCertificate(X509Val* cert_val, const char* } } #ifndef OPENSSL_NO_EC - else if ( pkey->type == EVP_PKEY_EC ) + else if ( EVP_PKEY_base_id(pkey) == EVP_PKEY_EC ) { pX509Cert->Assign(9, new StringVal("ecdsa")); pX509Cert->Assign(12, KeyCurve(pkey)); @@ -191,8 +193,8 @@ RecordVal* file_analysis::X509::ParseCertificate(X509Val* cert_val, const char* // set key algorithm back. We do not have to free the value that we created because (I think) it // comes out of a static array from OpenSSL memory. - if ( old_algorithm ) - ssl_cert->cert_info->key->algor->algorithm = old_algorithm; + if ( algorithm ) + X509_PUBKEY_set0_param(X509_get_X509_PUBKEY(ssl_cert), algorithm, 0, NULL, NULL, 0); unsigned int length = KeyLength(pkey); if ( length > 0 ) @@ -370,7 +372,7 @@ StringVal* file_analysis::X509::KeyCurve(EVP_PKEY *key) // well, we do not have EC-Support... return NULL; #else - if ( key->type != EVP_PKEY_EC ) + if ( EVP_PKEY_base_id(key) != EVP_PKEY_EC ) { // no EC-key - no curve name return NULL; @@ -378,7 +380,7 @@ StringVal* file_analysis::X509::KeyCurve(EVP_PKEY *key) const EC_GROUP *group; int nid; - if ( (group = EC_KEY_get0_group(key->pkey.ec)) == NULL) + if ( (group = EC_KEY_get0_group(EVP_PKEY_get0_EC_KEY(key))) == NULL) // I guess we could not parse this return NULL; @@ -399,12 +401,16 @@ unsigned int file_analysis::X509::KeyLength(EVP_PKEY *key) { assert(key != NULL); - switch(key->type) { + switch(EVP_PKEY_base_id(key)) { case EVP_PKEY_RSA: - return BN_num_bits(key->pkey.rsa->n); + const BIGNUM *n; + RSA_get0_key(EVP_PKEY_get0_RSA(key), &n, NULL, NULL); + return BN_num_bits(n); case EVP_PKEY_DSA: - return BN_num_bits(key->pkey.dsa->p); + const BIGNUM *p; + DSA_get0_pqg(EVP_PKEY_get0_DSA(key), &p, NULL, NULL); + return BN_num_bits(p); #ifndef OPENSSL_NO_EC case EVP_PKEY_EC: @@ -414,7 +420,7 @@ unsigned int file_analysis::X509::KeyLength(EVP_PKEY *key) // could not malloc bignum? return 0; - const EC_GROUP *group = EC_KEY_get0_group(key->pkey.ec); + const EC_GROUP *group = EC_KEY_get0_group(EVP_PKEY_get0_EC_KEY(key)); if ( ! group ) { diff --git a/src/file_analysis/analyzer/x509/X509.h b/src/file_analysis/analyzer/x509/X509.h index 325e8becac..60f67a9fd1 100644 --- a/src/file_analysis/analyzer/x509/X509.h +++ b/src/file_analysis/analyzer/x509/X509.h @@ -8,6 +8,47 @@ #include "Val.h" #include "X509Common.h" +#if (OPENSSL_VERSION_NUMBER < 0x10002000L) + +#define X509_get_signature_nid(x) OBJ_obj2nid((x)->sig_alg->algorithm) + +#define X509_OBJECT_new() (X509_OBJECT*)malloc(sizeof(X509_OBJECT)) +#define X509_OBJECT_free(a) free(a) + +static X509 *X509_OBJECT_get0_X509(const X509_OBJECT *a) +{ + if (a == NULL || a->type != X509_LU_X509) + return NULL; + return a->data.x509; +} + +#define OCSP_SINGLERESP_get0_id(s) (s)->certId +#define OCSP_resp_get0_certs(x) (x)->certs + +#endif + +#if (OPENSSL_VERSION_NUMBER < 0x1010000fL) + +#define EVP_PKEY_get0_DSA(p) ((p)->pkey.dsa) +#define EVP_PKEY_get0_EC_KEY(p) ((p)->pkey.ec) +#define EVP_PKEY_get0_RSA(p) ((p)->pkey.rsa) + +static void DSA_get0_pqg(const DSA *d, + const BIGNUM **p, const BIGNUM **q, const BIGNUM **g) +{ + if (p != NULL) *p = d->p; + if (q != NULL) *q = d->q; + if (g != NULL) *g = d->g; +} + +static void RSA_get0_key(const RSA *r, + const BIGNUM **n, const BIGNUM **e, const BIGNUM **d) +{ + if (n != NULL) *n = r->n; + if (e != NULL) *e = r->e; + if (d != NULL) *d = r->d; +} +#endif namespace file_analysis { diff --git a/src/file_analysis/analyzer/x509/X509Common.cc b/src/file_analysis/analyzer/x509/X509Common.cc index dba593a3eb..2e0a03ac8f 100644 --- a/src/file_analysis/analyzer/x509/X509Common.cc +++ b/src/file_analysis/analyzer/x509/X509Common.cc @@ -213,7 +213,7 @@ void file_analysis::X509Common::ParseSignedCertificateTimestamps(X509_EXTENSION* reporter->Error("X509::ParseSignedCertificateTimestamps could not parse SCT"); } - M_ASN1_OCTET_STRING_free(inner); + ASN1_OCTET_STRING_free(inner); OPENSSL_free(ext_val_second_pointer); interp->FlowEOF(); @@ -239,7 +239,12 @@ void file_analysis::X509Common::ParseExtension(X509_EXTENSION* ex, EventHandlerP BIO *bio = BIO_new(BIO_s_mem()); if( ! X509V3_EXT_print(bio, ex, 0, 0)) - M_ASN1_OCTET_STRING_print(bio,ex->value); + { + unsigned char **buf = NULL; + int len = i2d_ASN1_OCTET_STRING(X509_EXTENSION_get_data(ex), buf); + if (len >=0 ) + BIO_write(bio, *buf, len); + } StringVal* ext_val = GetExtensionFromBIO(bio); diff --git a/src/file_analysis/analyzer/x509/functions.bif b/src/file_analysis/analyzer/x509/functions.bif index 1c1ec51818..217a845aa3 100644 --- a/src/file_analysis/analyzer/x509/functions.bif +++ b/src/file_analysis/analyzer/x509/functions.bif @@ -239,6 +239,8 @@ function x509_get_certificate_string%(cert: opaque of x509, pem: bool &default=F ## x509_get_certificate_string x509_verify function x509_ocsp_verify%(certs: x509_opaque_vector, ocsp_reply: string, root_certs: table_string_of_string, verify_time: time &default=network_time()%): X509::Result %{ + stack_st_X509* ocsp_certs; + RecordVal* rval = 0; X509_STORE* ctx = x509_get_root_store(root_certs->AsTableVal()); if ( ! ctx ) @@ -317,10 +319,11 @@ function x509_ocsp_verify%(certs: x509_opaque_vector, ocsp_reply: string, root_c // the lookup. // Yay. - if ( ! basic->certs ) + ocsp_certs = sk_X509_dup(OCSP_resp_get0_certs(basic)); + if ( !ocsp_certs ) { - basic->certs = sk_X509_new_null(); - if ( ! basic->certs ) + ocsp_certs = sk_X509_new_null(); + if ( !ocsp_certs ) { rval = x509_result_record(-1, "Could not allocate basic x509 stack"); goto x509_ocsp_cleanup; @@ -330,7 +333,7 @@ function x509_ocsp_verify%(certs: x509_opaque_vector, ocsp_reply: string, root_c issuer_certificate = 0; for ( int i = 0; i < sk_X509_num(untrusted_certs); i++) { - sk_X509_push(basic->certs, X509_dup(sk_X509_value(untrusted_certs, i))); + sk_X509_push(ocsp_certs, X509_dup(sk_X509_value(untrusted_certs, i))); if ( X509_NAME_cmp(X509_get_issuer_name(cert), X509_get_subject_name(sk_X509_value(untrusted_certs, i))) == 0 ) issuer_certificate = sk_X509_value(untrusted_certs, i); @@ -346,7 +349,7 @@ function x509_ocsp_verify%(certs: x509_opaque_vector, ocsp_reply: string, root_c if ( !s igner ) // if we did not find it in the certificates that were sent, search in the root store - signer = x509_get_ocsp_signer(basic->certs, basic->tbsResponseData->responderId); + signer = x509_get_ocsp_signer(ocsp_certs, basic->tbsResponseData->responderId); */ if ( ! signer ) @@ -356,15 +359,15 @@ function x509_ocsp_verify%(certs: x509_opaque_vector, ocsp_reply: string, root_c } csc = X509_STORE_CTX_new(); - X509_STORE_CTX_init(csc, ctx, signer, basic->certs); + X509_STORE_CTX_init(csc, ctx, signer, ocsp_certs); X509_STORE_CTX_set_time(csc, 0, (time_t) verify_time); X509_STORE_CTX_set_purpose(csc, X509_PURPOSE_OCSP_HELPER); result = X509_verify_cert(csc); if ( result != 1 ) { - const char *reason = X509_verify_cert_error_string((*csc).error); - rval = x509_result_record(result, X509_verify_cert_error_string((*csc).error)); + const char *reason = X509_verify_cert_error_string(X509_STORE_CTX_get_error(csc)); + rval = x509_result_record(result, X509_verify_cert_error_string(X509_STORE_CTX_get_error(csc))); goto x509_ocsp_cleanup; } @@ -392,15 +395,17 @@ function x509_ocsp_verify%(certs: x509_opaque_vector, ocsp_reply: string, root_c else { // issuer not in list sent by server, check store - X509_OBJECT obj; - int lookup = X509_STORE_get_by_subject(csc, X509_LU_X509, X509_get_subject_name(cert), &obj); + X509_OBJECT *obj = X509_OBJECT_new(); + int lookup = X509_STORE_get_by_subject(csc, X509_LU_X509, X509_get_subject_name(cert), obj); if ( lookup <= 0) { rval = x509_result_record(lookup, "Could not find issuer of host certificate"); + X509_OBJECT_free(obj); goto x509_ocsp_cleanup; } - certid = OCSP_cert_to_id(NULL, cert, obj.data.x509); + certid = OCSP_cert_to_id(NULL, cert,X509_OBJECT_get0_X509( obj)); + X509_OBJECT_free(obj); } @@ -411,18 +416,22 @@ function x509_ocsp_verify%(certs: x509_opaque_vector, ocsp_reply: string, root_c } // for now, assume we have one reply... - single = sk_OCSP_SINGLERESP_value(basic->tbsResponseData->responses, 0); + single = OCSP_resp_get0(basic, 0); if ( ! single ) { rval = x509_result_record(-1, "Could not lookup OCSP response information"); goto x509_ocsp_cleanup; } - if ( OCSP_id_cmp(certid, single->certId) != 0 ) + if ( OCSP_id_cmp(certid, (OCSP_CERTID*)OCSP_SINGLERESP_get0_id(single)) != 0 ) return x509_result_record(-1, "OCSP reply is not for host certificate"); // next - check freshness of proof... - if ( ! ASN1_GENERALIZEDTIME_check(single->thisUpdate) || ! ASN1_GENERALIZEDTIME_check(single->nextUpdate) ) + ASN1_GENERALIZEDTIME *thisUpdate; + ASN1_GENERALIZEDTIME *nextUpdate; + int type; + type = OCSP_single_get0_status(single, NULL, NULL, &thisUpdate, &nextUpdate); + if ( ! ASN1_GENERALIZEDTIME_check(thisUpdate) || ! ASN1_GENERALIZEDTIME_check(nextUpdate) ) { rval = x509_result_record(-1, "OCSP reply contains invalid dates"); goto x509_ocsp_cleanup; @@ -435,16 +444,16 @@ function x509_ocsp_verify%(certs: x509_opaque_vector, ocsp_reply: string, root_c // Well, we will do it manually. - if ( X509_cmp_time(single->thisUpdate, &vtime) > 0 ) + if ( X509_cmp_time(thisUpdate, &vtime) > 0 ) rval = x509_result_record(-1, "OCSP reply specifies time in future"); - else if ( X509_cmp_time(single->nextUpdate, &vtime) < 0 ) + else if ( X509_cmp_time(nextUpdate, &vtime) < 0 ) rval = x509_result_record(-1, "OCSP reply expired"); - else if ( single->certStatus->type != V_OCSP_CERTSTATUS_GOOD ) - rval = x509_result_record(-1, OCSP_cert_status_str(single->certStatus->type)); + else if ( type != V_OCSP_CERTSTATUS_GOOD ) + rval = x509_result_record(-1, OCSP_cert_status_str(type)); // if we have no error so far, we are done. if ( !rval ) - rval = x509_result_record(1, OCSP_cert_status_str(single->certStatus->type)); + rval = x509_result_record(1, OCSP_cert_status_str(type)); x509_ocsp_cleanup: @@ -521,18 +530,18 @@ function x509_verify%(certs: x509_opaque_vector, root_certs: table_string_of_str if ( ! untrusted_certs ) return x509_result_record(-1, "Problem initializing list of untrusted certificates"); - X509_STORE_CTX csc; - X509_STORE_CTX_init(&csc, ctx, cert, untrusted_certs); - X509_STORE_CTX_set_time(&csc, 0, (time_t) verify_time); - X509_STORE_CTX_set_flags(&csc, X509_V_FLAG_USE_CHECK_TIME); + X509_STORE_CTX *csc = X509_STORE_CTX_new(); + X509_STORE_CTX_init(csc, ctx, cert, untrusted_certs); + X509_STORE_CTX_set_time(csc, 0, (time_t) verify_time); + X509_STORE_CTX_set_flags(csc, X509_V_FLAG_USE_CHECK_TIME); - int result = X509_verify_cert(&csc); + int result = X509_verify_cert(csc); VectorVal* chainVector = 0; if ( result == 1 ) // we have a valid chain. try to get it... { - STACK_OF(X509)* chain = X509_STORE_CTX_get1_chain(&csc); // get1 = deep copy + STACK_OF(X509)* chain = X509_STORE_CTX_get1_chain(csc); // get1 = deep copy if ( ! chain ) { @@ -564,11 +573,11 @@ function x509_verify%(certs: x509_opaque_vector, root_certs: table_string_of_str x509_verify_chainerror: - X509_STORE_CTX_cleanup(&csc); + X509_STORE_CTX_cleanup(csc); sk_X509_free(untrusted_certs); - RecordVal* rrecord = x509_result_record(csc.error, X509_verify_cert_error_string(csc.error), chainVector); + RecordVal* rrecord = x509_result_record(X509_STORE_CTX_get_error(csc), X509_verify_cert_error_string(X509_STORE_CTX_get_error(csc)), chainVector); return rrecord; %} @@ -660,9 +669,12 @@ function sct_verify%(cert: opaque of x509, logid: string, log_key: string, signa uint32 cert_length; if ( precert ) { - // we also could use i2d_re_X509_tbs, for OpenSSL >= 1.0.2 +#if (OPENSSL_VERSION_NUMBER < 0x10002000L) x->cert_info->enc.modified = 1; cert_length = i2d_X509_CINF(x->cert_info, &cert_out); +#else + i2d_re_X509_tbs(x, &cert_out); +#endif data.append(reinterpret_cast(issuer_key_hash->Bytes()), issuer_key_hash->Len()); } else diff --git a/src/main.cc b/src/main.cc index 0ca39e9e2d..5851cd586b 100644 --- a/src/main.cc +++ b/src/main.cc @@ -21,8 +21,6 @@ extern "C" { #include #include -extern "C" void OPENSSL_add_all_algorithms_conf(void); - #include "bsd-getopt-long.h" #include "input.h" #include "DNS_Mgr.h" From 6a93abea3250f47ed46bb99913cba85c50e3b26a Mon Sep 17 00:00:00 2001 From: Johanna Amann Date: Thu, 7 Dec 2017 14:01:47 -0800 Subject: [PATCH 2/3] Adjust coding style & fix test failures. I am still not 100% convinced that there is no memory leak hidden somwehere... This also makes everything compile with OpenSSL 1.0.2 for me. --- src/file_analysis/analyzer/x509/X509.cc | 4 +- src/file_analysis/analyzer/x509/X509.h | 45 +++++++++++-------- src/file_analysis/analyzer/x509/X509Common.cc | 11 +++-- src/file_analysis/analyzer/x509/functions.bif | 37 ++++++++------- 4 files changed, 53 insertions(+), 44 deletions(-) diff --git a/src/file_analysis/analyzer/x509/X509.cc b/src/file_analysis/analyzer/x509/X509.cc index c9e78bb0b9..6f3cce23f4 100644 --- a/src/file_analysis/analyzer/x509/X509.cc +++ b/src/file_analysis/analyzer/x509/X509.cc @@ -152,7 +152,7 @@ RecordVal* file_analysis::X509::ParseCertificate(X509Val* cert_val, const char* // actually should be (namely - rsaEncryption), so that OpenSSL will parse out the // key later. Otherwise it will just fail to parse the certificate key. - if ( X509_get_signature_nid(ssl_cert) == NID_md5WithRSAEncryption ) + if ( OBJ_obj2nid(algorithm) == NID_md5WithRSAEncryption ) X509_PUBKEY_set0_param(X509_get_X509_PUBKEY(ssl_cert), OBJ_nid2obj(NID_rsaEncryption), 0, NULL, NULL, 0); else algorithm = 0; @@ -380,7 +380,7 @@ StringVal* file_analysis::X509::KeyCurve(EVP_PKEY *key) const EC_GROUP *group; int nid; - if ( (group = EC_KEY_get0_group(EVP_PKEY_get0_EC_KEY(key))) == NULL) + if ( (group = EC_KEY_get0_group(EVP_PKEY_get0_EC_KEY(key))) == NULL ) // I guess we could not parse this return NULL; diff --git a/src/file_analysis/analyzer/x509/X509.h b/src/file_analysis/analyzer/x509/X509.h index 60f67a9fd1..1e7cd05a62 100644 --- a/src/file_analysis/analyzer/x509/X509.h +++ b/src/file_analysis/analyzer/x509/X509.h @@ -12,42 +12,49 @@ #define X509_get_signature_nid(x) OBJ_obj2nid((x)->sig_alg->algorithm) -#define X509_OBJECT_new() (X509_OBJECT*)malloc(sizeof(X509_OBJECT)) -#define X509_OBJECT_free(a) free(a) - -static X509 *X509_OBJECT_get0_X509(const X509_OBJECT *a) -{ - if (a == NULL || a->type != X509_LU_X509) - return NULL; - return a->data.x509; -} - -#define OCSP_SINGLERESP_get0_id(s) (s)->certId -#define OCSP_resp_get0_certs(x) (x)->certs - #endif #if (OPENSSL_VERSION_NUMBER < 0x1010000fL) +#define X509_OBJECT_new() (X509_OBJECT*)malloc(sizeof(X509_OBJECT)) +#define X509_OBJECT_free(a) free(a) + +#define OCSP_SINGLERESP_get0_id(s) (s)->certId +#define OCSP_resp_get0_certs(x) (x)->certs + #define EVP_PKEY_get0_DSA(p) ((p)->pkey.dsa) #define EVP_PKEY_get0_EC_KEY(p) ((p)->pkey.ec) #define EVP_PKEY_get0_RSA(p) ((p)->pkey.rsa) +static X509 *X509_OBJECT_get0_X509(const X509_OBJECT *a) +{ + if ( a == nullptr || a->type != X509_LU_X509 ) + return nullptr; + return a->data.x509; +} + static void DSA_get0_pqg(const DSA *d, const BIGNUM **p, const BIGNUM **q, const BIGNUM **g) { - if (p != NULL) *p = d->p; - if (q != NULL) *q = d->q; - if (g != NULL) *g = d->g; + if ( p != nullptr ) + *p = d->p; + if ( q != nullptr ) + *q = d->q; + if ( g != nullptr ) + *g = d->g; } static void RSA_get0_key(const RSA *r, const BIGNUM **n, const BIGNUM **e, const BIGNUM **d) { - if (n != NULL) *n = r->n; - if (e != NULL) *e = r->e; - if (d != NULL) *d = r->d; + if ( n != nullptr ) + *n = r->n; + if ( e != nullptr ) + *e = r->e; + if ( d != nullptr ) + *d = r->d; } + #endif namespace file_analysis { diff --git a/src/file_analysis/analyzer/x509/X509Common.cc b/src/file_analysis/analyzer/x509/X509Common.cc index 2e0a03ac8f..b101f502ff 100644 --- a/src/file_analysis/analyzer/x509/X509Common.cc +++ b/src/file_analysis/analyzer/x509/X509Common.cc @@ -240,10 +240,13 @@ void file_analysis::X509Common::ParseExtension(X509_EXTENSION* ex, EventHandlerP BIO *bio = BIO_new(BIO_s_mem()); if( ! X509V3_EXT_print(bio, ex, 0, 0)) { - unsigned char **buf = NULL; - int len = i2d_ASN1_OCTET_STRING(X509_EXTENSION_get_data(ex), buf); - if (len >=0 ) - BIO_write(bio, *buf, len); + unsigned char *buf = nullptr; + int len = i2d_ASN1_OCTET_STRING(X509_EXTENSION_get_data(ex), &buf); + if ( len >=0 ) + { + BIO_write(bio, &buf, len); + OPENSSL_free(buf); + } } StringVal* ext_val = GetExtensionFromBIO(bio); diff --git a/src/file_analysis/analyzer/x509/functions.bif b/src/file_analysis/analyzer/x509/functions.bif index 217a845aa3..6fdb695ce8 100644 --- a/src/file_analysis/analyzer/x509/functions.bif +++ b/src/file_analysis/analyzer/x509/functions.bif @@ -239,8 +239,6 @@ function x509_get_certificate_string%(cert: opaque of x509, pem: bool &default=F ## x509_get_certificate_string x509_verify function x509_ocsp_verify%(certs: x509_opaque_vector, ocsp_reply: string, root_certs: table_string_of_string, verify_time: time &default=network_time()%): X509::Result %{ - stack_st_X509* ocsp_certs; - RecordVal* rval = 0; X509_STORE* ctx = x509_get_root_store(root_certs->AsTableVal()); if ( ! ctx ) @@ -284,6 +282,7 @@ function x509_ocsp_verify%(certs: x509_opaque_vector, ocsp_reply: string, root_c OCSP_SINGLERESP *single = 0; X509_STORE_CTX *csc = 0; OCSP_CERTID *certid = 0; + stack_st_X509* ocsp_certs = nullptr; int status = -1; int out = -1; int result = -1; @@ -310,7 +309,6 @@ function x509_ocsp_verify%(certs: x509_opaque_vector, ocsp_reply: string, root_c goto x509_ocsp_cleanup; } - // the following code took me _forever_ to get right. // The OCSP_basic_verify command takes a list of certificates. However (which is not immediately // visible or understandable), those are only used to find the signer certificate. They are _not_ @@ -319,21 +317,10 @@ function x509_ocsp_verify%(certs: x509_opaque_vector, ocsp_reply: string, root_c // the lookup. // Yay. - ocsp_certs = sk_X509_dup(OCSP_resp_get0_certs(basic)); - if ( !ocsp_certs ) - { - ocsp_certs = sk_X509_new_null(); - if ( !ocsp_certs ) - { - rval = x509_result_record(-1, "Could not allocate basic x509 stack"); - goto x509_ocsp_cleanup; - } - } - issuer_certificate = 0; for ( int i = 0; i < sk_X509_num(untrusted_certs); i++) { - sk_X509_push(ocsp_certs, X509_dup(sk_X509_value(untrusted_certs, i))); + OCSP_basic_add1_cert(basic, sk_X509_value(untrusted_certs, i)); if ( X509_NAME_cmp(X509_get_issuer_name(cert), X509_get_subject_name(sk_X509_value(untrusted_certs, i))) == 0 ) issuer_certificate = sk_X509_value(untrusted_certs, i); @@ -358,6 +345,14 @@ function x509_ocsp_verify%(certs: x509_opaque_vector, ocsp_reply: string, root_c goto x509_ocsp_cleanup; } + { + auto basic_certs = OCSP_resp_get0_certs(basic); + if ( basic_certs ) + ocsp_certs = sk_X509_dup(basic_certs); + + assert(ocsp_certs); + } + csc = X509_STORE_CTX_new(); X509_STORE_CTX_init(csc, ctx, signer, ocsp_certs); X509_STORE_CTX_set_time(csc, 0, (time_t) verify_time); @@ -383,7 +378,7 @@ function x509_ocsp_verify%(certs: x509_opaque_vector, ocsp_reply: string, root_c // ok, now we verified the OCSP response. This means that we have a valid chain tying it // to a root that we trust and that the signature also hopefully is valid. This does not yet - // mean that the ocsp response actually matches the certificate the server send us or that + // mean that the ocsp response actually matches the certificate the server sent us or that // the OCSP response even says that the certificate is valid. // let's start this out by checking that the response is actually for the certificate we want @@ -457,6 +452,9 @@ function x509_ocsp_verify%(certs: x509_opaque_vector, ocsp_reply: string, root_c x509_ocsp_cleanup: + if ( ocsp_certs ) + sk_X509_free(ocsp_certs); + if ( untrusted_certs ) sk_X509_free(untrusted_certs); @@ -573,12 +571,13 @@ function x509_verify%(certs: x509_opaque_vector, root_certs: table_string_of_str x509_verify_chainerror: + RecordVal* rrecord = x509_result_record(X509_STORE_CTX_get_error(csc), X509_verify_cert_error_string(X509_STORE_CTX_get_error(csc)), chainVector); + X509_STORE_CTX_cleanup(csc); + X509_STORE_CTX_free(csc); sk_X509_free(untrusted_certs); - RecordVal* rrecord = x509_result_record(X509_STORE_CTX_get_error(csc), X509_verify_cert_error_string(X509_STORE_CTX_get_error(csc)), chainVector); - return rrecord; %} @@ -673,7 +672,7 @@ function sct_verify%(cert: opaque of x509, logid: string, log_key: string, signa x->cert_info->enc.modified = 1; cert_length = i2d_X509_CINF(x->cert_info, &cert_out); #else - i2d_re_X509_tbs(x, &cert_out); + cert_length = i2d_re_X509_tbs(x, &cert_out); #endif data.append(reinterpret_cast(issuer_key_hash->Bytes()), issuer_key_hash->Len()); } From 03f98c70223939b651cb5fdc682691a8d7b9e633 Mon Sep 17 00:00:00 2001 From: Johanna Amann Date: Thu, 7 Dec 2017 14:47:56 -0800 Subject: [PATCH 3/3] Fix recently introduced double free in OpenSSL code. --- src/file_analysis/analyzer/x509/X509.cc | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/file_analysis/analyzer/x509/X509.cc b/src/file_analysis/analyzer/x509/X509.cc index 6f3cce23f4..0426b59923 100644 --- a/src/file_analysis/analyzer/x509/X509.cc +++ b/src/file_analysis/analyzer/x509/X509.cc @@ -153,7 +153,13 @@ RecordVal* file_analysis::X509::ParseCertificate(X509Val* cert_val, const char* // key later. Otherwise it will just fail to parse the certificate key. if ( OBJ_obj2nid(algorithm) == NID_md5WithRSAEncryption ) + { + ASN1_OBJECT *copy = OBJ_dup(algorithm); // the next line will destroy the original algorithm. X509_PUBKEY_set0_param(X509_get_X509_PUBKEY(ssl_cert), OBJ_nid2obj(NID_rsaEncryption), 0, NULL, NULL, 0); + algorithm = copy; + // we do not have to worry about freeing algorithm in that case - since it will be re-assigned using + // set0_param and the cert will take ownership. + } else algorithm = 0;