From 385438d47c6ca263f664fe85fc41584ff4ade085 Mon Sep 17 00:00:00 2001 From: Jon Siwek Date: Thu, 1 May 2014 13:04:34 -0500 Subject: [PATCH 1/2] Change X509 extension value parsing to not abort on malloc failures. Also comes with factoring that out in to it's own function and additional error check before using a return value from BIO_pending. --- src/file_analysis/analyzer/x509/X509.cc | 59 +++++++++++++++---- src/file_analysis/analyzer/x509/X509.h | 8 +++ src/file_analysis/analyzer/x509/functions.bif | 14 +---- 3 files changed, 57 insertions(+), 24 deletions(-) diff --git a/src/file_analysis/analyzer/x509/X509.cc b/src/file_analysis/analyzer/x509/X509.cc index c036fca7ed..162e154df9 100644 --- a/src/file_analysis/analyzer/x509/X509.cc +++ b/src/file_analysis/analyzer/x509/X509.cc @@ -159,6 +159,49 @@ RecordVal* file_analysis::X509::ParseCertificate(X509Val* cert_val) return pX509Cert; } +StringVal* file_analysis::X509::GetExtensionFromBIO(BIO* bio) + { + BIO_flush(bio); + ERR_clear_error(); + int length = BIO_pending(bio); + + if ( ERR_peek_error() != 0 ) + { + char tmp[120]; + ERR_error_string(ERR_get_error(), tmp); + reporter->Error("X509::GetExtensionFromBIO: %s", tmp); + BIO_free_all(bio); + return 0; + } + + if ( length == 0 ) + { + BIO_free_all(bio); + return new StringVal(""); + } + + // TODO: see about using regular malloc here, there were unknown problems + // using anything other than OPENSSL_malloc that need investigation. + char* buffer = (char*) OPENSSL_malloc(length); + + if ( ! buffer ) + { + // Just emit an error here and try to continue instead of aborting + // because it's unclear the length value is very reliable. + reporter->Error("X509::GetExtensionFromBIO malloc(%d) failed", length); + BIO_free_all(bio); + return 0; + } + + BIO_read(bio, (void*) buffer, length); + StringVal* ext_val = new StringVal(length, buffer); + + OPENSSL_free(buffer); + BIO_free_all(bio); + + return ext_val; + } + void file_analysis::X509::ParseExtension(X509_EXTENSION* ex) { char name[256]; @@ -178,20 +221,10 @@ void file_analysis::X509::ParseExtension(X509_EXTENSION* ex) if( ! X509V3_EXT_print(bio, ex, 0, 0)) M_ASN1_OCTET_STRING_print(bio,ex->value); - BIO_flush(bio); - int length = BIO_pending(bio); + StringVal* ext_val = GetExtensionFromBIO(bio); - // Use OPENSSL_malloc here. Using new or anything else can lead - // to interesting, hard to debug segfaults. - char *buffer = (char*) OPENSSL_malloc(length); - - if ( ! buffer ) - out_of_memory("X509::ParseExtension"); - - BIO_read(bio, (void*)buffer, length); - StringVal* ext_val = new StringVal(length, buffer); - OPENSSL_free(buffer); - BIO_free_all(bio); + if ( ! ext_val ) + ext_val = new StringVal(0, ""); RecordVal* pX509Ext = new RecordVal(BifType::Record::X509::Extension); pX509Ext->Assign(0, new StringVal(name)); diff --git a/src/file_analysis/analyzer/x509/X509.h b/src/file_analysis/analyzer/x509/X509.h index 8667d870b1..6a3e6393a3 100644 --- a/src/file_analysis/analyzer/x509/X509.h +++ b/src/file_analysis/analyzer/x509/X509.h @@ -37,6 +37,14 @@ public: static file_analysis::Analyzer* Instantiate(RecordVal* args, File* file) { return new X509(args, file); } + /** + * Retrieve an X509 extension value from an OpenSSL BIO to which it was + * written. + * @param bio the OpenSSL BIO to read. + * @return The X509 extension value. + */ + static StringVal* GetExtensionFromBIO(BIO* bio); + protected: X509(RecordVal* args, File* file); diff --git a/src/file_analysis/analyzer/x509/functions.bif b/src/file_analysis/analyzer/x509/functions.bif index e6ed138c74..176df53bdb 100644 --- a/src/file_analysis/analyzer/x509/functions.bif +++ b/src/file_analysis/analyzer/x509/functions.bif @@ -78,18 +78,10 @@ function x509_get_certificate_string%(cert: opaque of x509, pem: bool &default=F else i2d_X509_bio(bio, h->GetCertificate()); - BIO_flush(bio); - int length = BIO_pending(bio); - // use OPENSS_malloc here. Otherwhise, interesting problems will happen. - char *buffer = (char*) OPENSSL_malloc(length); + StringVal* ext_val = file_analysis::X509::GetExtensionFromBIO(bio); - if ( ! buffer ) - out_of_memory("x509_get_certificate_string"); - - BIO_read(bio, (void*) buffer, length); - StringVal* ext_val = new StringVal(length, buffer); - OPENSSL_free(buffer); - BIO_free_all(bio); + if ( ! ext_val ) + ext_val = new StringVal(""); return ext_val; %} From 5b9d190f2c3b7a7c91d877aa98044c59cce16385 Mon Sep 17 00:00:00 2001 From: Jon Siwek Date: Thu, 1 May 2014 14:08:07 -0500 Subject: [PATCH 2/2] Fix missing "irc-dcc-data" service field from IRC DCC connections. --- scripts/base/protocols/irc/dcc-send.bro | 2 +- .../scripts.base.protocols.irc.basic/conn.log | 11 +++++++++++ testing/btest/scripts/base/protocols/irc/basic.test | 1 + 3 files changed, 13 insertions(+), 1 deletion(-) create mode 100644 testing/btest/Baseline/scripts.base.protocols.irc.basic/conn.log diff --git a/scripts/base/protocols/irc/dcc-send.bro b/scripts/base/protocols/irc/dcc-send.bro index d95eb97517..437724004a 100644 --- a/scripts/base/protocols/irc/dcc-send.bro +++ b/scripts/base/protocols/irc/dcc-send.bro @@ -76,7 +76,7 @@ event irc_dcc_message(c: connection, is_orig: bool, dcc_expected_transfers[address, p] = c$irc; } -event expected_connection_seen(c: connection, a: Analyzer::Tag) &priority=10 +event scheduled_analyzer_applied(c: connection, a: Analyzer::Tag) &priority=10 { local id = c$id; if ( [id$resp_h, id$resp_p] in dcc_expected_transfers ) diff --git a/testing/btest/Baseline/scripts.base.protocols.irc.basic/conn.log b/testing/btest/Baseline/scripts.base.protocols.irc.basic/conn.log new file mode 100644 index 0000000000..411e57f8ee --- /dev/null +++ b/testing/btest/Baseline/scripts.base.protocols.irc.basic/conn.log @@ -0,0 +1,11 @@ +#separator \x09 +#set_separator , +#empty_field (empty) +#unset_field - +#path conn +#open 2014-05-01-19-07-07 +#fields ts uid id.orig_h id.orig_p id.resp_h id.resp_p proto service duration orig_bytes resp_bytes conn_state local_orig missed_bytes history orig_pkts orig_ip_bytes resp_pkts resp_ip_bytes tunnel_parents +#types time string addr port addr port enum string interval count count string bool count string count count count count set[string] +1311189318.898709 CjhGID4nQcgTWjvg4c 192.168.1.77 57655 209.197.168.151 1024 tcp irc-dcc-data 2.256935 124 42208 SF - 0 ShAdDaFf 28 1592 43 44452 (empty) +1311189164.064603 CXWv6p3arKYeMETxOg 192.168.1.77 57640 66.198.80.67 6667 tcp irc 178.237017 453 25404 S3 - 0 ShADdaf 63 3761 52 28194 (empty) +#close 2014-05-01-19-07-07 diff --git a/testing/btest/scripts/base/protocols/irc/basic.test b/testing/btest/scripts/base/protocols/irc/basic.test index 32358d12a4..618f4d9079 100644 --- a/testing/btest/scripts/base/protocols/irc/basic.test +++ b/testing/btest/scripts/base/protocols/irc/basic.test @@ -3,6 +3,7 @@ # @TEST-EXEC: bro -r $TRACES/irc-dcc-send.trace %INPUT # @TEST-EXEC: btest-diff irc.log +# @TEST-EXEC: btest-diff conn.log # dcc mime types are irrelevant to this test, so filter it out event bro_init()