diff --git a/CHANGES b/CHANGES index 18d42005b1..b1c5f03a1f 100644 --- a/CHANGES +++ b/CHANGES @@ -1,4 +1,36 @@ +2.3-134 | 2014-09-04 16:16:36 -0700 + + * Fixed a number of issues with OCSP reply validation. Addresses + BIT-1212. (Johanna Amann) + + * Fix null pointer dereference in OCSP verification code in case no + certificate is sent as part as the ocsp reply. Addresses BIT-1212. + (Johanna Amann) + +2.3-131 | 2014-09-04 16:10:32 -0700 + + * Make links in documentation templates protocol relative. (Johanna + Amann) + +2.3-129 | 2014-09-02 17:21:21 -0700 + + * Simplify a conditional with equivalent branches. (Jon Siwek) + + * Change EDNS parsing code to use rdlength more cautiously. (Jon + Siwek) + + * Fix a memory leak when bind() fails due to EADDRINUSE. (Jon Siwek) + + * Fix possible buffer over-read in DNS TSIG parsing. (Jon Siwek) + +2.3-124 | 2014-08-26 09:24:19 -0500 + + * Better documentation for sub_bytes (Jimmy Jones) + + * BIT-1234: Fix build on systems that already have ntohll/htonll + (Jon Siwek) + 2.3-121 | 2014-08-22 15:22:15 -0700 * Detect functions that try to bind variables from an outer scope diff --git a/CMakeLists.txt b/CMakeLists.txt index 080b731875..22d63a89d5 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -148,6 +148,8 @@ set(brodeps include(TestBigEndian) test_big_endian(WORDS_BIGENDIAN) +include(CheckSymbolExists) +check_symbol_exists(htonll arpa/inet.h HAVE_BYTEORDER_64) include(OSSpecific) include(CheckTypes) diff --git a/VERSION b/VERSION index 99c4783780..0154ba2563 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -2.3-121 +2.3-134 diff --git a/config.h.in b/config.h.in index d3889a2d90..755a9eee98 100644 --- a/config.h.in +++ b/config.h.in @@ -129,6 +129,9 @@ /* whether words are stored with the most significant byte first */ #cmakedefine WORDS_BIGENDIAN +/* whether htonll/ntohll is defined in */ +#cmakedefine HAVE_BYTEORDER_64 + /* ultrix can't hack const */ #cmakedefine NEED_ULTRIX_CONST_HACK #ifdef NEED_ULTRIX_CONST_HACK diff --git a/doc/_templates/layout.html b/doc/_templates/layout.html index 2f8ea02aff..3df56a12ff 100644 --- a/doc/_templates/layout.html +++ b/doc/_templates/layout.html @@ -10,7 +10,7 @@ {% endblock %} {% block header %} - {% endblock %} @@ -108,6 +108,6 @@ {% endblock %} {% block footer %} - {% endblock %} diff --git a/src/RemoteSerializer.cc b/src/RemoteSerializer.cc index 3392179d28..e08ccb1c6d 100644 --- a/src/RemoteSerializer.cc +++ b/src/RemoteSerializer.cc @@ -4211,6 +4211,7 @@ bool SocketComm::Listen() safe_close(fd); CloseListenFDs(); listen_next_try = time(0) + bind_retry_interval; + freeaddrinfo(res0); return false; } diff --git a/src/Val.cc b/src/Val.cc index 5f605a178e..7c83830bf9 100644 --- a/src/Val.cc +++ b/src/Val.cc @@ -465,10 +465,7 @@ void Val::Describe(ODesc* d) const d->SP(); } - if ( d->IsReadable() ) - ValDescribe(d); - else - Val::ValDescribe(d); + ValDescribe(d); } void Val::DescribeReST(ODesc* d) const diff --git a/src/analyzer/protocol/dns/DNS.cc b/src/analyzer/protocol/dns/DNS.cc index 1c77fc6b51..e551351926 100644 --- a/src/analyzer/protocol/dns/DNS.cc +++ b/src/analyzer/protocol/dns/DNS.cc @@ -692,15 +692,23 @@ int DNS_Interpreter::ParseRR_EDNS(DNS_MsgInfo* msg, data += rdlength; len -= rdlength; } - else - { // no data, move on - data += rdlength; - len -= rdlength; - } return 1; } +void DNS_Interpreter::ExtractOctets(const u_char*& data, int& len, + BroString** p) + { + uint16 dlen = ExtractShort(data, len); + dlen = min(len, static_cast(dlen)); + + if ( p ) + *p = new BroString(data, dlen, 0); + + data += dlen; + len -= dlen; + } + int DNS_Interpreter::ParseRR_TSIG(DNS_MsgInfo* msg, const u_char*& data, int& len, int rdlength, const u_char* msg_start) @@ -718,24 +726,17 @@ int DNS_Interpreter::ParseRR_TSIG(DNS_MsgInfo* msg, uint32 sign_time_sec = ExtractLong(data, len); unsigned int sign_time_msec = ExtractShort(data, len); unsigned int fudge = ExtractShort(data, len); - - u_char request_MAC[16]; - memcpy(request_MAC, data, sizeof(request_MAC)); - - // Here we adjust the size of the requested MAC + u_int16_t - // for length. See RFC 2845, sec 2.3. - int n = sizeof(request_MAC) + sizeof(u_int16_t); - data += n; - len -= n; - + BroString* request_MAC; + ExtractOctets(data, len, &request_MAC); unsigned int orig_id = ExtractShort(data, len); unsigned int rr_error = ExtractShort(data, len); + ExtractOctets(data, len, 0); // Other Data msg->tsig = new TSIG_DATA; msg->tsig->alg_name = new BroString(alg_name, alg_name_end - alg_name, 1); - msg->tsig->sig = new BroString(request_MAC, sizeof(request_MAC), 1); + msg->tsig->sig = request_MAC; msg->tsig->time_s = sign_time_sec; msg->tsig->time_ms = sign_time_msec; msg->tsig->fudge = fudge; diff --git a/src/analyzer/protocol/dns/DNS.h b/src/analyzer/protocol/dns/DNS.h index 569a4ee53a..2d95d979b8 100644 --- a/src/analyzer/protocol/dns/DNS.h +++ b/src/analyzer/protocol/dns/DNS.h @@ -180,6 +180,7 @@ protected: uint16 ExtractShort(const u_char*& data, int& len); uint32 ExtractLong(const u_char*& data, int& len); + void ExtractOctets(const u_char*& data, int& len, BroString** p); int ParseRR_Name(DNS_MsgInfo* msg, const u_char*& data, int& len, int rdlength, diff --git a/src/file_analysis/analyzer/x509/functions.bif b/src/file_analysis/analyzer/x509/functions.bif index 9a8a8e78b7..216f4c69cc 100644 --- a/src/file_analysis/analyzer/x509/functions.bif +++ b/src/file_analysis/analyzer/x509/functions.bif @@ -104,6 +104,39 @@ STACK_OF(X509)* x509_get_untrusted_stack(VectorVal* certs_vec) return untrusted_certs; } +// We need this function to be able to identify the signer certificate of an +// OCSP request out of a list of possible certificates. +X509* x509_get_ocsp_signer(STACK_OF(X509) *certs, OCSP_RESPID *rid) + { + // We support two lookup types - either by response id or by key. + if ( rid->type == V_OCSP_RESPID_NAME ) + return X509_find_by_subject(certs, rid->value.byName); + + // There only should be name and type - but let's be sure... + if ( rid->type != V_OCSP_RESPID_KEY ) + return 0; + + // Just like OpenSSL, we just support SHA-1 lookups and bail out otherwhise. + if ( rid->value.byKey->length != SHA_DIGEST_LENGTH ) + return 0; + + unsigned char* key_hash = rid->value.byKey->data; + for ( int i = 0; i < sk_X509_num(certs); ++i ) + { + unsigned char digest[SHA_DIGEST_LENGTH]; + X509* cert = sk_X509_value(certs, i); + if ( ! X509_pubkey_digest(cert, EVP_sha1(), digest, NULL) ) + // digest failed for this certificate, try with next + continue; + + if ( memcmp(digest, key_hash, SHA_DIGEST_LENGTH) == 0 ) + // keys match, return certificate + return cert; + } + + return 0; + } + %%} ## Parses a certificate into an X509::Certificate structure. @@ -221,6 +254,7 @@ function x509_ocsp_verify%(certs: x509_opaque_vector, ocsp_reply: string, root_c int out = -1; int result = -1; X509* issuer_certificate = 0; + X509* signer = 0; OCSP_RESPONSE *resp = d2i_OCSP_RESPONSE(NULL, &start, ocsp_reply->Len()); if ( ! resp ) { @@ -250,19 +284,47 @@ function x509_ocsp_verify%(certs: x509_opaque_vector, ocsp_reply: string, root_c // inject the certificates in the certificate list of the OCSP reply, they actually are used during // the lookup. // Yay. + + if ( ! basic->certs ) + { + basic->certs = sk_X509_new_null(); + if ( ! basic->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(basic->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))) ) + 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); } // Because we actually want to be able to give nice error messages that show why we were // not able to verify the OCSP response - do our own verification logic first. + signer = x509_get_ocsp_signer(basic->certs, basic->tbsResponseData->responderId); + + /* + Do this perhaps - OpenSSL also cannot do it, so I do not really feel bad about it. + Needs a different lookup because the root store is no stack of X509 certs + + 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); + */ + + if ( ! signer ) + { + rval = x509_result_record(-1, "Could not find OCSP responder certificate"); + goto x509_ocsp_cleanup; + } + csc = X509_STORE_CTX_new(); - X509_STORE_CTX_init(csc, ctx, sk_X509_value(basic->certs, 0), basic->certs); + X509_STORE_CTX_init(csc, ctx, signer, basic->certs); X509_STORE_CTX_set_time(csc, 0, (time_t) verify_time); X509_STORE_CTX_set_purpose(csc, X509_PURPOSE_OCSP_HELPER); @@ -281,7 +343,6 @@ function x509_ocsp_verify%(certs: x509_opaque_vector, ocsp_reply: string, root_c goto x509_ocsp_cleanup; } - // 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 @@ -322,7 +383,7 @@ function x509_ocsp_verify%(certs: x509_opaque_vector, ocsp_reply: string, root_c goto x509_ocsp_cleanup; } - if ( ! OCSP_id_cmp(certid, single->certId) ) + if ( OCSP_id_cmp(certid, single->certId) != 0 ) return x509_result_record(-1, "OCSP reply is not for host certificate"); // next - check freshness of proof... diff --git a/src/net_util.h b/src/net_util.h index 0f34335267..d68a7110ce 100644 --- a/src/net_util.h +++ b/src/net_util.h @@ -180,8 +180,11 @@ extern uint32 extract_uint32(const u_char* data); inline double ntohd(double d) { return d; } inline double htond(double d) { return d; } + +#ifndef HAVE_BYTEORDER_64 inline uint64 ntohll(uint64 i) { return i; } inline uint64 htonll(uint64 i) { return i; } +#endif #else @@ -207,6 +210,7 @@ inline double ntohd(double d) inline double htond(double d) { return ntohd(d); } +#ifndef HAVE_BYTEORDER_64 inline uint64 ntohll(uint64 i) { u_char c; @@ -224,6 +228,7 @@ inline uint64 ntohll(uint64 i) } inline uint64 htonll(uint64 i) { return ntohll(i); } +#endif #endif diff --git a/src/strings.bif b/src/strings.bif index f50eb1f89b..4a30ca2aa4 100644 --- a/src/strings.bif +++ b/src/strings.bif @@ -308,7 +308,8 @@ function edit%(arg_s: string, arg_edit_char: string%): string ## ## s: The string to obtain a substring from. ## -## start: The starting position of the substring in *s* +## start: The starting position of the substring in *s*, where 1 is the first +## character. As a special case, 0 also represents the first character. ## ## n: The number of characters to extract, beginning at *start*. ## diff --git a/testing/btest/Baseline/scripts.base.protocols.dns.tsig/out b/testing/btest/Baseline/scripts.base.protocols.dns.tsig/out new file mode 100644 index 0000000000..ddeb775ec8 --- /dev/null +++ b/testing/btest/Baseline/scripts.base.protocols.dns.tsig/out @@ -0,0 +1,2 @@ +[query=secret-key, qtype=3, alg_name=hmac-md5.sig-alg.reg.int, sig=F\xbd\xbf1\xef^B6\xb8\xeb\xae1u,\x87\xdb^?, time_signed=21513.794, fudge=300.0, orig_id=9703, rr_error=0, is_query=1] +16 diff --git a/testing/btest/Baseline/scripts.policy.protocols.ssl.validate-ocsp/ssl-digicert.log b/testing/btest/Baseline/scripts.policy.protocols.ssl.validate-ocsp/ssl-digicert.log new file mode 100644 index 0000000000..bb0a25ac0c --- /dev/null +++ b/testing/btest/Baseline/scripts.policy.protocols.ssl.validate-ocsp/ssl-digicert.log @@ -0,0 +1,10 @@ +#separator \x09 +#set_separator , +#empty_field (empty) +#unset_field - +#path ssl +#open 2014-09-04-19-17-18 +#fields ts uid id.orig_h id.orig_p id.resp_h id.resp_p version cipher curve server_name session_id last_alert established cert_chain_fuids client_cert_chain_fuids subject issuer client_subject client_issuer ocsp_status +#types time string addr port addr port string string string string string string bool vector[string] vector[string] string string string string string +1404148886.994021 CXWv6p3arKYeMETxOg 192.168.4.149 51293 72.21.91.29 443 TLSv10 TLS_ECDHE_RSA_WITH_RC4_128_SHA secp256r1 - - - T FhwjYM0FkbvVCvMf2,Fajs2d2lipsadwoK1h (empty) CN=www.digicert.com,O=DigiCert\, Inc.,L=Lehi,ST=Utah,C=US,postalCode=84043,street=2600 West Executive Parkway,street=Suite 500,serialNumber=5299537-0142,1.3.6.1.4.1.311.60.2.1.2=#130455746168,1.3.6.1.4.1.311.60.2.1.3=#13025553,businessCategory=Private Organization CN=DigiCert SHA2 Extended Validation Server CA,OU=www.digicert.com,O=DigiCert Inc,C=US - - good +#close 2014-09-04-19-17-18 diff --git a/testing/btest/Baseline/scripts.policy.protocols.ssl.validate-ocsp/ssl-twimg.log b/testing/btest/Baseline/scripts.policy.protocols.ssl.validate-ocsp/ssl-twimg.log new file mode 100644 index 0000000000..4806744a5c --- /dev/null +++ b/testing/btest/Baseline/scripts.policy.protocols.ssl.validate-ocsp/ssl-twimg.log @@ -0,0 +1,10 @@ +#separator \x09 +#set_separator , +#empty_field (empty) +#unset_field - +#path ssl +#open 2014-09-04-19-17-14 +#fields ts uid id.orig_h id.orig_p id.resp_h id.resp_p version cipher curve server_name session_id last_alert established cert_chain_fuids client_cert_chain_fuids subject issuer client_subject client_issuer ocsp_status +#types time string addr port addr port string string string string string string bool vector[string] vector[string] string string string string string +1409786981.016881 CXWv6p3arKYeMETxOg 192.168.4.149 53106 93.184.216.146 443 TLSv10 TLS_ECDHE_RSA_WITH_RC4_128_SHA secp256r1 - - - T FtaZVlJfywdNmVFr1,FoILekwkdtTuZtlVa (empty) CN=si0.twimg.com,O=Twitter\, Inc.,L=San Francisco,ST=California,C=US CN=DigiCert High Assurance CA-3,OU=www.digicert.com,O=DigiCert Inc,C=US - - good +#close 2014-09-04-19-17-14 diff --git a/testing/btest/Traces/dns-tsig.trace b/testing/btest/Traces/dns-tsig.trace new file mode 100644 index 0000000000..9f377b11f7 Binary files /dev/null and b/testing/btest/Traces/dns-tsig.trace differ diff --git a/testing/btest/Traces/tls/ocsp-stapling-digicert.trace b/testing/btest/Traces/tls/ocsp-stapling-digicert.trace new file mode 100644 index 0000000000..982249c0f5 Binary files /dev/null and b/testing/btest/Traces/tls/ocsp-stapling-digicert.trace differ diff --git a/testing/btest/Traces/tls/ocsp-stapling-twimg.trace b/testing/btest/Traces/tls/ocsp-stapling-twimg.trace new file mode 100644 index 0000000000..f53762f6e0 Binary files /dev/null and b/testing/btest/Traces/tls/ocsp-stapling-twimg.trace differ diff --git a/testing/btest/scripts/base/protocols/dns/tsig.bro b/testing/btest/scripts/base/protocols/dns/tsig.bro new file mode 100644 index 0000000000..79de4cf9f1 --- /dev/null +++ b/testing/btest/scripts/base/protocols/dns/tsig.bro @@ -0,0 +1,10 @@ +# @TEST-EXEC: bro -r $TRACES/dns-tsig.trace %INPUT >out +# @TEST-EXEC: btest-diff out + +redef dns_skip_all_addl = F; + +event dns_TSIG_addl(c: connection, msg: dns_msg, ans: dns_tsig_additional) + { + print ans; + print |ans$sig|; + } diff --git a/testing/btest/scripts/policy/protocols/ssl/validate-ocsp.bro b/testing/btest/scripts/policy/protocols/ssl/validate-ocsp.bro index b0392f9c27..e7e3c3ff8e 100644 --- a/testing/btest/scripts/policy/protocols/ssl/validate-ocsp.bro +++ b/testing/btest/scripts/policy/protocols/ssl/validate-ocsp.bro @@ -1,4 +1,10 @@ # @TEST-EXEC: bro -C -r $TRACES/tls/ocsp-stapling.trace %INPUT # @TEST-EXEC: btest-diff ssl.log +# @TEST-EXEC: bro -C -r $TRACES/tls/ocsp-stapling-twimg.trace %INPUT +# @TEST-EXEC: mv ssl.log ssl-twimg.log +# @TEST-EXEC: btest-diff ssl-twimg.log +# @TEST-EXEC: bro -C -r $TRACES/tls/ocsp-stapling-digicert.trace %INPUT +# @TEST-EXEC: mv ssl.log ssl-digicert.log +# @TEST-EXEC: btest-diff ssl-digicert.log @load protocols/ssl/validate-ocsp