From 0aecca979e830d0ee8f6524c4dee3fe83cfc3c4c Mon Sep 17 00:00:00 2001 From: Jon Siwek Date: Tue, 29 May 2012 17:29:11 -0500 Subject: [PATCH] Remove unnecessary assert in ICMP analyzer (addresses #822). The ICMP/ICMPv6 analyzers function correctly when full packets have not been captured, but everything up to and including the ICMP header is there (e.g. the functions that inspect ICMP error message context correctly check the caplen to see if more info can be extracted). The "Should have been caught earlier already." comment may have referred to NetSessions::CheckHeaderTrunc, which works as intended to catch cases where the ICMP header is not there in full, but then the assert was still not correctly formulated for that... Also changed the ICMP checksum calculation to not occur when the full packet has not been captured, which seems consistent with what the UDP analysis does. --- src/ICMP.cc | 4 +--- testing/btest/Baseline/core.truncation/output | 8 ++++++++ testing/btest/Traces/trunc/icmp-header-trunc.pcap | Bin 0 -> 136 bytes .../btest/Traces/trunc/icmp-payload-trunc.pcap | Bin 0 -> 408 bytes testing/btest/core/truncation.test | 13 +++++++++++++ 5 files changed, 22 insertions(+), 3 deletions(-) create mode 100644 testing/btest/Traces/trunc/icmp-header-trunc.pcap create mode 100644 testing/btest/Traces/trunc/icmp-payload-trunc.pcap diff --git a/src/ICMP.cc b/src/ICMP.cc index 05a6b67dff..b06c6440e1 100644 --- a/src/ICMP.cc +++ b/src/ICMP.cc @@ -49,9 +49,7 @@ void ICMP_Analyzer::DeliverPacket(int len, const u_char* data, const struct icmp* icmpp = (const struct icmp*) data; - assert(caplen >= len); // Should have been caught earlier already. - - if ( ! ignore_checksums ) + if ( ! ignore_checksums && caplen >= len ) { int chksum = 0; diff --git a/testing/btest/Baseline/core.truncation/output b/testing/btest/Baseline/core.truncation/output index f3d64b8b28..95d9073648 100644 --- a/testing/btest/Baseline/core.truncation/output +++ b/testing/btest/Baseline/core.truncation/output @@ -22,3 +22,11 @@ #fields ts uid id.orig_h id.orig_p id.resp_h id.resp_p name addl notice peer #types time string addr port addr port string string bool string 1334094648.590126 - - - - - truncated_IP - F bro +#separator \x09 +#set_separator , +#empty_field (empty) +#unset_field - +#path weird +#fields ts uid id.orig_h id.orig_p id.resp_h id.resp_p name addl notice peer +#types time string addr port addr port string string bool string +1338328954.078361 - - - - - internally_truncated_header - F bro diff --git a/testing/btest/Traces/trunc/icmp-header-trunc.pcap b/testing/btest/Traces/trunc/icmp-header-trunc.pcap new file mode 100644 index 0000000000000000000000000000000000000000..5765cf288605f05f9344d27334bb26258b467ebf GIT binary patch literal 136 zcmca|c+)~A1{MYw`2U}Qff2~5azE-XX~f8&0c0nEBn57FTzU^;Ffed1xH2$=L>^&a zaA4HF#Rb%GfI*v!gW=}w%=;jH^IMVhL9~E%L-g&M>%j1X@zQ^g9*|xJhKE3X04E?J Al>h($ literal 0 HcmV?d00001 diff --git a/testing/btest/Traces/trunc/icmp-payload-trunc.pcap b/testing/btest/Traces/trunc/icmp-payload-trunc.pcap new file mode 100644 index 0000000000000000000000000000000000000000..13607dd50cd5344a6d6706b56d8070474a5e3cee GIT binary patch literal 408 zcmca|c+)~A1{MYw`2U}Qff2}AYkt%}p^J+l0LV@PNebNVxbz;xU|`^2aAjZ!d3%6? z!GST{kqfB7tM;KL2ZKuQ&CLu9{zuK%GH|uqb8vET^YHTV3kV7ci-?MeOGrvd%gD;f zD<~={tEhs^-!v1+e6Yz7TOj7o*vi0Q!MMc-WH!hS1_m7x%-;YspE(7|d<^&J&jq<( zj1gq^0S0X@4hGxzk0I{gz`&J!3%~n8=C^M{G9SbJITsulelRMW1(^+Tzbgsm0{}2B BQ#$|v literal 0 HcmV?d00001 diff --git a/testing/btest/core/truncation.test b/testing/btest/core/truncation.test index ee8bdd5bf9..3406879183 100644 --- a/testing/btest/core/truncation.test +++ b/testing/btest/core/truncation.test @@ -6,4 +6,17 @@ # @TEST-EXEC: cat weird.log >> output # @TEST-EXEC: bro -r $TRACES/trunc/ip6-ext-trunc.pcap # @TEST-EXEC: cat weird.log >> output + +# If an ICMP packet's payload is truncated due to too small snaplen, +# the checksum calculation is bypassed (and Bro doesn't crash, of course). + +# @TEST-EXEC: rm -f weird.log +# @TEST-EXEC: bro -r $TRACES/trunc/icmp-payload-trunc.pcap +# @TEST-EXEC: test ! -e weird.log + +# If an ICMP packet has the ICMP header truncated due to too small snaplen, +# an internally_truncated_header weird gets generated. + +# @TEST-EXEC: bro -r $TRACES/trunc/icmp-header-trunc.pcap +# @TEST-EXEC: cat weird.log >> output # @TEST-EXEC: btest-diff output