From 0dbfebc2f898d8ccba40dc9b5afb9f3f97eecf99 Mon Sep 17 00:00:00 2001 From: Seth Hall Date: Fri, 1 Jul 2011 12:21:50 -0400 Subject: [PATCH] Fixing memory leaks in SSL analyzer. --- src/bro.bif | 9 +++++-- src/ssl-analyzer.pac | 57 ++++++++++++++++++++++---------------------- src/ssl-protocol.pac | 2 +- 3 files changed, 36 insertions(+), 32 deletions(-) diff --git a/src/bro.bif b/src/bro.bif index fa453ec46a..fc7d743c44 100644 --- a/src/bro.bif +++ b/src/bro.bif @@ -3406,7 +3406,7 @@ function x509_verify%(der_cert: string, cert_stack: string_vec, root_certs: tabl BroString* s = convert_index_to_string(root_certs); if ( x509_stores.count(*s) > 0 ) ctx = x509_stores[*s]; - + if ( ! ctx ) // lookup to see if we have this one built already! { ctx = X509_STORE_new(); @@ -3427,10 +3427,12 @@ function x509_verify%(der_cert: string, cert_stack: string_vec, root_certs: tabl } X509_STORE_add_cert(ctx, x); } + delete idxs; // Save the newly constructed certificate store into the cacheing map. x509_stores[*s] = ctx; } + delete s; STACK_OF(X509)* untrusted_certs = sk_X509_new_null(); if ( ! untrusted_certs ) @@ -3445,8 +3447,10 @@ function x509_verify%(der_cert: string, cert_stack: string_vec, root_certs: tabl StringVal *sv = cert_stack_vec->Lookup(i)->AsStringVal(); const uint8 *data = sv->Bytes(); X509* x = d2i_X509_(NULL, &data, sv->Len()); + Unref(sv); if ( ! x ) { + sk_X509_free(untrusted_certs); builtin_run_time(fmt("Untrusted certificate stack creation error: %s", ERR_error_string(ERR_peek_last_error(),NULL))); return new Val((uint64) ERR_get_error(), TYPE_COUNT); } @@ -3470,7 +3474,8 @@ function x509_verify%(der_cert: string, cert_stack: string_vec, root_certs: tabl X509_STORE_CTX_cleanup(&csc); if ( untrusted_certs ) - sk_X509_pop_free(untrusted_certs, X509_free); + sk_X509_free(untrusted_certs); + X509_free(cert); return new Val((uint64) csc.error, TYPE_COUNT); %} diff --git a/src/ssl-analyzer.pac b/src/ssl-analyzer.pac index 1f948aa893..b9986e5ff6 100644 --- a/src/ssl-analyzer.pac +++ b/src/ssl-analyzer.pac @@ -72,22 +72,12 @@ function version_ok(vers : uint16) : bool } %} -function convert_ciphers_uint24(ciph : uint24[]) : int[] - %{ - vector* newciph = new vector(); - - std::transform(ciph->begin(), ciph->end(), - std::back_inserter(*newciph), to_int()); - - return newciph; - %} function convert_ciphers_uint16(ciph : uint16[]) : int[] %{ vector* newciph = new vector(); - std::copy(ciph->begin(), ciph->end(), - std::back_inserter(*newciph)); + std::copy(ciph->begin(), ciph->end(), std::back_inserter(*newciph)); return newciph; %} @@ -141,7 +131,8 @@ refine connection SSL_Conn += { function proc_client_hello(rec: SSLRecord, version : uint16, ts : double, session_id : uint8[], - cipher_suites : int[]) : bool + cipher_suites16 : uint16[], + cipher_suites24 : uint24[]) : bool %{ if ( state_ == STATE_TRACK_LOST ) bro_analyzer()->ProtocolViolation(fmt("unexpected client hello message from %s in state %s", @@ -151,13 +142,15 @@ refine connection SSL_Conn += { if ( ! version_ok(version) ) bro_analyzer()->ProtocolViolation(fmt("unsupported client SSL version 0x%04x", version)); + vector* cipher_suites = new vector(); + if ( cipher_suites16 ) + std::copy(cipher_suites16->begin(), cipher_suites16->end(), std::back_inserter(*cipher_suites)); + else + std::transform(cipher_suites24->begin(), cipher_suites24->end(), std::back_inserter(*cipher_suites), to_int()); + if ( ssl_client_hello ) { - BroType* count_t = base_type(TYPE_COUNT); - TypeList* set_index = new TypeList(count_t); - set_index->Append(count_t); - SetType* s = new SetType(set_index, 0); - TableVal* cipher_set = new TableVal(s); + TableVal* cipher_set = new TableVal(internal_type("count_set")->AsTableType()); for ( unsigned int i = 0; i < cipher_suites->size(); ++i ) { Val* ciph = new Val((*cipher_suites)[i], TYPE_COUNT); @@ -169,6 +162,8 @@ refine connection SSL_Conn += { version, ts, to_string_val(session_id), cipher_set); + + delete cipher_suites; } return true; @@ -177,7 +172,8 @@ refine connection SSL_Conn += { function proc_server_hello(rec: SSLRecord, version : uint16, ts : double, session_id : uint8[], - cipher_suite : uint16, + cipher_suites16 : uint16[], + cipher_suites24 : uint24[], comp_method : uint8) : bool %{ if ( state_ == STATE_TRACK_LOST ) @@ -185,6 +181,13 @@ refine connection SSL_Conn += { orig_label(${rec.is_orig}).c_str(), state_label(old_state_).c_str())); + vector* ciphers = new vector(); + + if ( cipher_suites16 ) + std::copy(cipher_suites16->begin(), cipher_suites16->end(), std::back_inserter(*ciphers)); + else + std::transform(cipher_suites24->begin(), cipher_suites24->end(), std::back_inserter(*ciphers), to_int()); + if ( ! version_ok(version) ) bro_analyzer()->ProtocolViolation(fmt("unsupported server SSL version 0x%04x", version)); @@ -194,9 +197,10 @@ refine connection SSL_Conn += { bro_analyzer()->Conn(), version, ts, to_string_val(session_id), - cipher_suite, comp_method); + ciphers->at(0), comp_method); } + delete ciphers; bro_analyzer()->ProtocolConfirmation(); return true; %} @@ -224,7 +228,6 @@ refine connection SSL_Conn += { if ( x509_certificate ) { - X509* pCert = 0; for ( unsigned int i = 0; i < certificates->size(); ++i ) { const bytestring& cert = (*certificates)[i]; @@ -267,11 +270,6 @@ refine connection SSL_Conn += { // Are there any X509 extensions? if ( x509_extension && X509_get_ext_count(pTemp) > 0 ) { - BroType* count_t = base_type(TYPE_COUNT); - TypeList* set_index = new TypeList(count_t); - set_index->Append(count_t); - SetType* s = new SetType(set_index, 0); - TableVal* x509ex = new TableVal(s); int num_ext = X509_get_ext_count(pTemp); for ( int k = 0; k < num_ext; ++k ) { @@ -296,6 +294,7 @@ refine connection SSL_Conn += { } } } + X509_free(pTemp); } } return true; @@ -397,26 +396,26 @@ refine typeattr ApplicationData += &let { refine typeattr ClientHello += &let { proc : bool = $context.connection.proc_client_hello(rec, client_version, gmt_unix_time, - session_id, convert_ciphers_uint16(csuits)) + session_id, csuits, 0) &requires(state_changed); }; refine typeattr V2ClientHello += &let { proc : bool = $context.connection.proc_client_hello(rec, client_version, 0, - session_id, convert_ciphers_uint24(ciphers)) + session_id, 0, ciphers) &requires(state_changed); }; refine typeattr ServerHello += &let { proc : bool = $context.connection.proc_server_hello(rec, server_version, - gmt_unix_time, session_id, cipher_suite, + gmt_unix_time, session_id, cipher_suite, 0, compression_method) &requires(state_changed); }; refine typeattr V2ServerHello += &let { proc : bool = $context.connection.proc_server_hello(rec, server_version, 0, 0, - convert_ciphers_uint24(ciphers)[0], 0) + 0, ciphers, 0) &requires(state_changed); cert : bool = $context.connection.proc_v2_certificate(rec, cert_data) diff --git a/src/ssl-protocol.pac b/src/ssl-protocol.pac index 42cd920289..88746f2da0 100644 --- a/src/ssl-protocol.pac +++ b/src/ssl-protocol.pac @@ -433,7 +433,7 @@ type ServerHello(rec: SSLRecord) = record { random_bytes : bytestring &length = 28 &transient; session_len : uint8; session_id : uint8[session_len]; - cipher_suite : uint16; + cipher_suite : uint16[1]; compression_method : uint8; } &let { state_changed : bool =