diff --git a/aux/binpac b/aux/binpac index 3034da8f08..dd1a3a95f0 160000 --- a/aux/binpac +++ b/aux/binpac @@ -1 +1 @@ -Subproject commit 3034da8f082b61157e234237993ffd7a95be6e62 +Subproject commit dd1a3a95f07082efcd5274b21104a038d523d132 diff --git a/aux/bro-aux b/aux/bro-aux index f53bcb2b49..a59b35bdad 160000 --- a/aux/bro-aux +++ b/aux/bro-aux @@ -1 +1 @@ -Subproject commit f53bcb2b492cb0db3dd288384040abc2ab711767 +Subproject commit a59b35bdada8f70fb1a59bf7bb2976534c86d378 diff --git a/aux/broccoli b/aux/broccoli index a08ca90727..612e95ac62 160000 --- a/aux/broccoli +++ b/aux/broccoli @@ -1 +1 @@ -Subproject commit a08ca90727c5c4b90aa8633106ec33a5cf7378d4 +Subproject commit 612e95ac62a06b32b2e9e627f30527012a89a12c diff --git a/aux/broctl b/aux/broctl index 954538514d..66e9e87bee 160000 --- a/aux/broctl +++ b/aux/broctl @@ -1 +1 @@ -Subproject commit 954538514d71983e7ef3f0e109960466096e1c1d +Subproject commit 66e9e87beebce983fa0f479b0284d5690b0290d4 diff --git a/cmake b/cmake index 2cc1055770..550ab2c8d9 160000 --- a/cmake +++ b/cmake @@ -1 +1 @@ -Subproject commit 2cc105577044a2d214124568f3f2496ed2ccbb34 +Subproject commit 550ab2c8d95b1d3e18e40a903152650e6c7a3c45 diff --git a/scripts/base/init-bare.bro b/scripts/base/init-bare.bro index b3c997a750..7b1b304405 100644 --- a/scripts/base/init-bare.bro +++ b/scripts/base/init-bare.bro @@ -933,11 +933,7 @@ const ICMP_UNREACH_ADMIN_PROHIB = 13; ##< Adminstratively prohibited. # Definitions for access to packet headers. Currently only used for # discarders. # todo::these should go into an enum to make them autodoc'able -const IPPROTO_IP = 0; ##< Dummy for IP. [Robin] Rename to IPPROTO_IP4? -# [Jon] I'd say leave it be or remove it because from -# IPPROTO_IPV4 can actually be the same as IPPROTO_IPIP (4)... -# IPPROTO_IP seems to be just for use with the socket API and not -# actually identifying protocol numbers in packet headers +const IPPROTO_IP = 0; ##< Dummy for IP. const IPPROTO_ICMP = 1; ##< Control message protocol. const IPPROTO_IGMP = 2; ##< Group management protocol. const IPPROTO_IPIP = 4; ##< IP encapsulation in IP. @@ -947,14 +943,6 @@ const IPPROTO_IPV6 = 41; ##< IPv6 header. const IPPROTO_RAW = 255; ##< Raw IP packet. # Definitions for IPv6 extension headers. -# [Robin] Do we need a constant for unknown extensions? -# [Jon] I don't think so, these constants are just conveniences to improve -# script readability, but they also identify the actual assigned protocol -# number of the header type. If the core were to actually pass to the -# script-layer a next-header value of something we don't know about yet, -# that value would be the actual value seen in the packet, not something -# we should make up. We could provide a "KNOWN_PROTOCOLS" set for -# convenience that one could check membership against. const IPPROTO_HOPOPTS = 0; ##< IPv6 hop-by-hop-options header. const IPPROTO_ROUTING = 43; ##< IPv6 routing header. const IPPROTO_FRAGMENT = 44; ##< IPv6 fragment header. @@ -1068,15 +1056,6 @@ type ip6_esp: record { ## ## .. bro:see:: pkt_hdr ip4_hdr ip6_hopopts ip6_dstopts ip6_routing ip6_fragment ## ip6_ah ip6_esp -# -# [Robin] What happens to unknown extension headers? We should keep them too so that -# one can at least identify what one can't analyze. -# [Jon] Currently, they show up as "unknown_protocol" weirds and those packets -# are skipped before any "new_packet" or "ipv6_ext_headers" events are -# raised as those depend on a connection parameter which can't be -# created since we can't parse past unknown extension headers to get -# at the upper layer protocol. Does that seem reasonable for at -# being able to identify things that couldn't be analyzed? type ip6_ext_hdr: record { ## The RFC 1700 et seq. IANA assigned number identifying the type of ## the extension header. @@ -1170,11 +1149,6 @@ type icmp_hdr: record { ## A packet header, consisting of an IP header and transport-layer header. ## ## .. bro:see:: new_packet -# -# [Robin] Add flags saying whether it's v4/v6, tcp/udp/icmp? The day will come where -# we can't infer that from the connection anymore (tunnels). -# [Jon] I'm not sure what you mean, doesn't checking result of ?$ operator -# always work for finding out protocols involved? type pkt_hdr: record { ip: ip4_hdr &optional; ##< The IPv4 header if an IPv4 packet. ip6: ip6_hdr &optional; ##< The IPv6 header if an IPv6 packet. diff --git a/src/Frag.cc b/src/Frag.cc index 5fcad35560..a744526921 100644 --- a/src/Frag.cc +++ b/src/Frag.cc @@ -33,10 +33,6 @@ FragReassembler::FragReassembler(NetSessions* arg_s, s = arg_s; key = k; - // [Robin] Can't we merge these two cases now? - // [Jon] I think we'll always have to check v4 versus v6 to get the correct - // proto_hdr_len unless IP_Hdr::HdrLen itself makes a special case for - // IPv6 fragments (but that seems more confusing to me) const struct ip* ip4 = ip->IP4_Hdr(); if ( ip4 ) { diff --git a/src/IP.h b/src/IP.h index a989b04d76..f3e8272080 100644 --- a/src/IP.h +++ b/src/IP.h @@ -14,22 +14,6 @@ #include #include -// [Robin] I'm concerced about the virtual methods here. These methods will -// be called *a lot* and that may add to some significant overhead I'm afraid -// (at least eventually as IPv6 is picking up). -// -// [Robin] Similar concern for the vector and ip6_hdrs data -// members: we're creating/allocating those for every IPv6 packet, right? -// -// Any idea how to avoid these? -// -// [Jon] Seems fair enough to just remove the virtual method concern at this -// point by replacing the class hierarchy with some inline functions that -// do switch statements. I don't know what to do about the -// vector and ip6_hdrs data members being allocated for every -// IPv6 packet, maybe it's too early to try to optimize before we know -// the frequency at which extension headers appear in real IPv6 traffic? - /** * Base class for IPv6 header/extensions. */ diff --git a/src/Net.cc b/src/Net.cc index c92545cb87..35c3b383f6 100644 --- a/src/Net.cc +++ b/src/Net.cc @@ -42,7 +42,6 @@ extern int select(int, fd_set *, fd_set *, fd_set *, struct timeval *); PList(PktSrc) pkt_srcs; // FIXME: We should really merge PktDumper and PacketDumper. -// It's on my to-do [Robin]. PktDumper* pkt_dumper = 0; int reading_live = 0; diff --git a/src/Sessions.cc b/src/Sessions.cc index 9e91fdc304..4f31d29346 100644 --- a/src/Sessions.cc +++ b/src/Sessions.cc @@ -430,13 +430,6 @@ void NetSessions::DoNextPacket(double t, const struct pcap_pkthdr* hdr, if ( discarder && discarder->NextPacket(ip_hdr, len, caplen) ) return; - // [Robin] dump_this_packet = 1 for non-ICMP/UDP/TCP removed here. Why? - // [Jon] The default case of the "switch ( proto )" calls Weird() which - // should set dump_this_packet = 1. The old code also returned - // at this point for non-ICMP/UDP/TCP, but for IPv6 fragments - // we need to do the reassembly first before knowing for sure what - // upper-layer protocol it is. - FragReassembler* f = 0; if ( ip_hdr->IsFragment() ) @@ -472,10 +465,8 @@ void NetSessions::DoNextPacket(double t, const struct pcap_pkthdr* hdr, len -= ip_hdr_len; // remove IP header caplen -= ip_hdr_len; - // [Robin] Does ESP need to be the last header? - // [Jon] In terms of what we try to parse, yes, we can't go any further - // in parsing a header chain once we reach an ESP one since - // encrypted payload immediately follows. + // We stop building the chain when seeing IPPROTO_ESP so if it's + // there, it's always the last. if ( ip_hdr->LastHeader() == IPPROTO_ESP ) { dump_this_packet = 1; @@ -498,16 +489,6 @@ void NetSessions::DoNextPacket(double t, const struct pcap_pkthdr* hdr, return; } - // [Robin] The Remove(f) used to be here, while it's now before every - // return statement. I'm not seeing why? - // [Jon] That Remove(f) is still here above in the CheckHeaderTrunc() - // conditional that's just a refactoring of the old code. - // The reason why it's not done unconditionally after the reassembly - // is because doing that could cause the object that ip_hdr points - // to to be freed when we still need to use that below. - // I added Remove(f)'s before other "abnormal" return points that - // looked like they'd otherwise leak the memory. - const u_char* data = ip_hdr->Payload(); ConnID id; @@ -553,7 +534,7 @@ void NetSessions::DoNextPacket(double t, const struct pcap_pkthdr* hdr, } default: - Weird(fmt("unknown_protocol %d", proto), hdr, pkt); + Weird(fmt("unknown_protocol_%d", proto), hdr, pkt); Remove(f); return; }