Merge remote-tracking branch 'origin/master' into topic/robin/pktsrc

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

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)

VERSION

@@ -1 +1 @@
-2.3-121
+2.3-134

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 <arpa/inet.h> */
+#cmakedefine HAVE_BYTEORDER_64
+
 /* ultrix can't hack const */
 #cmakedefine NEED_ULTRIX_CONST_HACK
 #ifdef NEED_ULTRIX_CONST_HACK

doc/_templates/layout.html

@@ -10,7 +10,7 @@
 {% endblock %}
 
 {% block header %}
-<iframe src="http://www.bro.org/frames/header-no-logo.html" width="100%" height="100px" frameborder="0" marginheight="0" scrolling="no" marginwidth="0">
+<iframe src="//www.bro.org/frames/header-no-logo.html" width="100%" height="100px" frameborder="0" marginheight="0" scrolling="no" marginwidth="0">
 </iframe>
 {% endblock %}
 
@@ -108,6 +108,6 @@
 {% endblock %}
 
 {% block footer %}
-<iframe src="http://www.bro.org/frames/footer.html" width="100%" height="420px" frameborder="0" marginheight="0" scrolling="no" marginwidth="0">
+<iframe src="//www.bro.org/frames/footer.html" width="100%" height="420px" frameborder="0" marginheight="0" scrolling="no" marginwidth="0">
 </iframe>
 {% endblock %}

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;
 				}
 

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

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<int>(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;

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,

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...

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
 

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*.
 ##

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

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

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

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|;
+	}

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