From 392b99b2fa4b7bdda267eca55d4cc57d85e88641 Mon Sep 17 00:00:00 2001 From: Jon Siwek Date: Tue, 18 Sep 2012 16:52:12 -0500 Subject: [PATCH] Fix construction of ip6_ah (Authentication Header) record values. Authentication Headers with a Payload Len field set to zero would cause a crash due to invalid memory allocation because the previous code assumed Payload Len would always be great enough to contain all mandatory fields of the header. This changes it so the length of the header is explicitly checked before attempting to extract fields located past the minimum length (8 bytes) of an Authentication Header. Crashes due to this are only possible when handling script-layer events ipv6_ext_headers, new_packet, esp_packet, or teredo_*. Or also when implementing one of the discarder_check_* family of functions. Otherwise, Bro correctly parses past such a header. --- scripts/base/init-bare.bro | 8 ++++---- src/IP.cc | 11 ++++++++--- .../btest/Baseline/core.ipv6_zero_len_ah/output | 2 ++ testing/btest/Traces/ipv6_zero_len_ah.trace | Bin 0 -> 1320 bytes testing/btest/core/ipv6_zero_len_ah.test | 11 +++++++++++ 5 files changed, 25 insertions(+), 7 deletions(-) create mode 100644 testing/btest/Baseline/core.ipv6_zero_len_ah/output create mode 100644 testing/btest/Traces/ipv6_zero_len_ah.trace create mode 100644 testing/btest/core/ipv6_zero_len_ah.test diff --git a/scripts/base/init-bare.bro b/scripts/base/init-bare.bro index ec75c76beb..cc3a40f54b 100644 --- a/scripts/base/init-bare.bro +++ b/scripts/base/init-bare.bro @@ -1135,10 +1135,10 @@ type ip6_ah: record { rsv: count; ## Security Parameter Index. spi: count; - ## Sequence number. - seq: count; - ## Authentication data. - data: string; + ## Sequence number, unset in the case that *len* field is zero. + seq: count &optional; + ## Authentication data, unset in the case that *len* field is zero. + data: string &optional; }; ## Values extracted from an IPv6 ESP extension header. diff --git a/src/IP.cc b/src/IP.cc index 45afd593a9..398aacf1ee 100644 --- a/src/IP.cc +++ b/src/IP.cc @@ -148,9 +148,14 @@ RecordVal* IPv6_Hdr::BuildRecordVal(VectorVal* chain) const rv->Assign(1, new Val(((ip6_ext*)data)->ip6e_len, TYPE_COUNT)); rv->Assign(2, new Val(ntohs(((uint16*)data)[1]), TYPE_COUNT)); rv->Assign(3, new Val(ntohl(((uint32*)data)[1]), TYPE_COUNT)); - rv->Assign(4, new Val(ntohl(((uint32*)data)[2]), TYPE_COUNT)); - uint16 off = 3 * sizeof(uint32); - rv->Assign(5, new StringVal(new BroString(data + off, Length() - off, 1))); + if ( Length() >= 12 ) + { + // Sequence Number and ICV fields can only be extracted if + // Payload Len was non-zero for this header. + rv->Assign(4, new Val(ntohl(((uint32*)data)[2]), TYPE_COUNT)); + uint16 off = 3 * sizeof(uint32); + rv->Assign(5, new StringVal(new BroString(data + off, Length() - off, 1))); + } } break; diff --git a/testing/btest/Baseline/core.ipv6_zero_len_ah/output b/testing/btest/Baseline/core.ipv6_zero_len_ah/output new file mode 100644 index 0000000000..d8db6a4c48 --- /dev/null +++ b/testing/btest/Baseline/core.ipv6_zero_len_ah/output @@ -0,0 +1,2 @@ +[orig_h=2000:1300::1, orig_p=128/icmp, resp_h=2000:1300::2, resp_p=129/icmp] +[ip=, ip6=[class=0, flow=0, len=166, nxt=51, hlim=255, src=2000:1300::1, dst=2000:1300::2, exts=[[id=51, hopopts=, dstopts=, routing=, fragment=, ah=[nxt=58, len=0, rsv=0, spi=0, seq=, data=], esp=, mobility=]]], tcp=, udp=, icmp=] diff --git a/testing/btest/Traces/ipv6_zero_len_ah.trace b/testing/btest/Traces/ipv6_zero_len_ah.trace new file mode 100644 index 0000000000000000000000000000000000000000..7c3922525c26f97d870d6c2c3aa0462e82315b4a GIT binary patch literal 1320 zcmca|c+)~A1{MYw`2U}Qff2~DHt-7wNoHd31F}JwgF$_t(qt|Mbs)R#ZUT^Gkg)o% zz#t4_!2lx~pQ(W%X{Sk8#RNw*05ZL?kclA-s1t;ZjX~Bz?0}lCfMGh*e$dHILq%IdTG28*V0EDslVVN<(c(4NM1c3&IU(bM){NMzjko*MnD-SRM zf-shlyoQ-7&_j}i@whn9k8BA*f?-&NjZp<6m0?K-6y`@?B-62kJf`VP=pm0Q4Lbni zb=oRI`S4!@D8j%g{Qo~7jb=JiJHr+^kUY9LBQzg^Y`G4!1#dn?&nZmkwstV6svp2& F3jp^*vyA`% literal 0 HcmV?d00001 diff --git a/testing/btest/core/ipv6_zero_len_ah.test b/testing/btest/core/ipv6_zero_len_ah.test new file mode 100644 index 0000000000..dc3acf8443 --- /dev/null +++ b/testing/btest/core/ipv6_zero_len_ah.test @@ -0,0 +1,11 @@ +# @TEST-EXEC: bro -r $TRACES/ipv6_zero_len_ah.trace %INPUT >output +# @TEST-EXEC: btest-diff output + +# Shouldn't crash, but we also won't have seq and data fields set of the ip6_ah +# record. + +event ipv6_ext_headers(c: connection, p: pkt_hdr) + { + print c$id; + print p; + }