From dde0ce234f4e866a13a626e04fa9c3b518e5fbde Mon Sep 17 00:00:00 2001 From: Jon Siwek Date: Tue, 2 Sep 2014 14:22:26 -0500 Subject: [PATCH 1/4] Fix possible buffer over-read in DNS TSIG parsing --- src/analyzer/protocol/dns/DNS.cc | 28 +++++++++++------- src/analyzer/protocol/dns/DNS.h | 1 + .../scripts.base.protocols.dns.tsig/out | 2 ++ testing/btest/Traces/dns-tsig.trace | Bin 0 -> 294 bytes .../btest/scripts/base/protocols/dns/tsig.bro | 10 +++++++ 5 files changed, 30 insertions(+), 11 deletions(-) create mode 100644 testing/btest/Baseline/scripts.base.protocols.dns.tsig/out create mode 100644 testing/btest/Traces/dns-tsig.trace create mode 100644 testing/btest/scripts/base/protocols/dns/tsig.bro diff --git a/src/analyzer/protocol/dns/DNS.cc b/src/analyzer/protocol/dns/DNS.cc index 1c77fc6b51..8f66d74857 100644 --- a/src/analyzer/protocol/dns/DNS.cc +++ b/src/analyzer/protocol/dns/DNS.cc @@ -701,6 +701,19 @@ int DNS_Interpreter::ParseRR_EDNS(DNS_MsgInfo* msg, 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 +731,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/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/Traces/dns-tsig.trace b/testing/btest/Traces/dns-tsig.trace new file mode 100644 index 0000000000000000000000000000000000000000..9f377b11f7cc509d17f8b0f4e1277b100f52a9d5 GIT binary patch literal 294 zcmca|c+)~A1{MYw`2U}Qff2~L#K#sQ&CSIy9mob@27%ihm)@V)b7I=11sn{n3=Ex0 z`3wvWg4Y6j`<}3J0KuCk22+O8_Egp9K>Z*ifFyfrMPhD2PAYS9elAFmk*hd0xhSgHNOxDd!F=a2#OxI1!NoOufO=r%`D*;M|u<>1D)L{^C+q>WJ zJ(Jmv*Xs;Rb=q&&t3C&51vyhLl#3x8$Od7E10jwDJJVAI=z0UeCz&88f}F}=3UcUj cUDfA}4ImeROu=yG0Un5yo*@HE?2a4d0A!O;yZ`_I literal 0 HcmV?d00001 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|; + } From d57b161c405b34e329da3232023147a9f2b49444 Mon Sep 17 00:00:00 2001 From: Jon Siwek Date: Tue, 2 Sep 2014 16:18:55 -0500 Subject: [PATCH 2/4] Fix a memory leak when bind() fails due to EADDRINUSE. --- src/RemoteSerializer.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/src/RemoteSerializer.cc b/src/RemoteSerializer.cc index 3e46c5a1d2..965e360690 100644 --- a/src/RemoteSerializer.cc +++ b/src/RemoteSerializer.cc @@ -4212,6 +4212,7 @@ bool SocketComm::Listen() safe_close(fd); CloseListenFDs(); listen_next_try = time(0) + bind_retry_interval; + freeaddrinfo(res0); return false; } From 782b4d0eae18f413e44eaf53804cd6167f6ab59c Mon Sep 17 00:00:00 2001 From: Jon Siwek Date: Tue, 2 Sep 2014 16:22:15 -0500 Subject: [PATCH 3/4] Change EDNS parsing code to use rdlength more cautiously. It shouldn't ever be negative, but if it were, using it to modify the data pointer/length isn't appropriate. --- src/analyzer/protocol/dns/DNS.cc | 5 ----- 1 file changed, 5 deletions(-) diff --git a/src/analyzer/protocol/dns/DNS.cc b/src/analyzer/protocol/dns/DNS.cc index 8f66d74857..e551351926 100644 --- a/src/analyzer/protocol/dns/DNS.cc +++ b/src/analyzer/protocol/dns/DNS.cc @@ -692,11 +692,6 @@ int DNS_Interpreter::ParseRR_EDNS(DNS_MsgInfo* msg, data += rdlength; len -= rdlength; } - else - { // no data, move on - data += rdlength; - len -= rdlength; - } return 1; } From ff6173721223807b6cf24a4264fd5d50de3e0250 Mon Sep 17 00:00:00 2001 From: Jon Siwek Date: Tue, 2 Sep 2014 16:29:52 -0500 Subject: [PATCH 4/4] Simplify a conditional with equivalent branches. --- src/Val.cc | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) 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