From 9e1592d5c49961d038f82a848c7752455046d2ea Mon Sep 17 00:00:00 2001 From: Robin Sommer Date: Thu, 7 Nov 2024 13:01:43 +0100 Subject: [PATCH] Spicy: Do not raise an analyzer error when a connection is missing a regular tear-down. So far, when Zeek didn't see a connection's regular tear-down (e.g., because its state timed-out before we got to the end), we'd still signal a regular end-of-data to Spicy parsers. As a result, they would then typically raise a parse error because they were probably still expecting data and would now declare it missing. That's not very useful because semantically it's not really a protocol issue if the data just doesn't make it over to us; it's a transport-layer issue that Zeek already handles elsewhere. So we now switch to signaling end-of-data to Spicy analyzers only if the connection indeed shuts down regularly. This is also matches how BinPAC handles it. This also comes with a test exercising various combinations of end-of-data behavior so that we ensure consistent/desired behavior. Closes #4007. --- src/spicy/protocol-analyzer.cc | 19 +--- .../spicy.tcp-eod-behavior/output-1024-fins | 3 + .../output-1024-no-fins | 2 + .../spicy.tcp-eod-behavior/output-136-fins | 3 + .../spicy.tcp-eod-behavior/output-136-no-fins | 3 + .../spicy.tcp-eod-behavior/output-16-fins | 3 + .../spicy.tcp-eod-behavior/output-16-no-fins | 3 + .../spicy.tcp-eod-behavior/output-eod-fins | 3 + .../spicy.tcp-eod-behavior/output-eod-no-fins | 2 + .../btest/Traces/http/get-without-fins.trace | Bin 0 -> 6089 bytes testing/btest/spicy/tcp-eod-behavior.zeek | 107 ++++++++++++++++++ 11 files changed, 134 insertions(+), 14 deletions(-) create mode 100644 testing/btest/Baseline/spicy.tcp-eod-behavior/output-1024-fins create mode 100644 testing/btest/Baseline/spicy.tcp-eod-behavior/output-1024-no-fins create mode 100644 testing/btest/Baseline/spicy.tcp-eod-behavior/output-136-fins create mode 100644 testing/btest/Baseline/spicy.tcp-eod-behavior/output-136-no-fins create mode 100644 testing/btest/Baseline/spicy.tcp-eod-behavior/output-16-fins create mode 100644 testing/btest/Baseline/spicy.tcp-eod-behavior/output-16-no-fins create mode 100644 testing/btest/Baseline/spicy.tcp-eod-behavior/output-eod-fins create mode 100644 testing/btest/Baseline/spicy.tcp-eod-behavior/output-eod-no-fins create mode 100644 testing/btest/Traces/http/get-without-fins.trace create mode 100644 testing/btest/spicy/tcp-eod-behavior.zeek diff --git a/src/spicy/protocol-analyzer.cc b/src/spicy/protocol-analyzer.cc index 05b64895e3..274c29643a 100644 --- a/src/spicy/protocol-analyzer.cc +++ b/src/spicy/protocol-analyzer.cc @@ -40,10 +40,7 @@ ProtocolAnalyzer::~ProtocolAnalyzer() {} void ProtocolAnalyzer::Init() {} -void ProtocolAnalyzer::Done() { - Finish(true); - Finish(false); -} +void ProtocolAnalyzer::Done() {} void ProtocolAnalyzer::Process(bool is_orig, int len, const u_char* data) { auto* endp = is_orig ? &_originator : &_responder; @@ -162,16 +159,7 @@ void TCP_Analyzer::Undelivered(uint64_t seq, int len, bool is_orig) { Process(is_orig, len, nullptr); } -void TCP_Analyzer::EndOfData(bool is_orig) { - analyzer::tcp::TCP_ApplicationAnalyzer::EndOfData(is_orig); - - if ( TCP() && TCP()->IsPartial() ) { - STATE_DEBUG_MSG(is_orig, "skipping end-of-data delivery on partial TCP connection"); - return; - } - - Finish(is_orig); -} +void TCP_Analyzer::EndOfData(bool is_orig) { analyzer::tcp::TCP_ApplicationAnalyzer::EndOfData(is_orig); } void TCP_Analyzer::FlipRoles() { analyzer::tcp::TCP_ApplicationAnalyzer::FlipRoles(); @@ -211,6 +199,9 @@ void UDP_Analyzer::Init() { void UDP_Analyzer::Done() { analyzer::Analyzer::Done(); ProtocolAnalyzer::Done(); + + Finish(true); + Finish(false); } void UDP_Analyzer::DeliverPacket(int len, const u_char* data, bool is_orig, uint64_t seq, const IP_Hdr* ip, diff --git a/testing/btest/Baseline/spicy.tcp-eod-behavior/output-1024-fins b/testing/btest/Baseline/spicy.tcp-eod-behavior/output-1024-fins new file mode 100644 index 0000000000..9505b5bca0 --- /dev/null +++ b/testing/btest/Baseline/spicy.tcp-eod-behavior/output-1024-fins @@ -0,0 +1,3 @@ +### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63. +=== Not enough data, regular FINs (expect analyzer error) +violation expected 1024 bytes (136 available) (<...>/test.spicy:12:5-12:23) diff --git a/testing/btest/Baseline/spicy.tcp-eod-behavior/output-1024-no-fins b/testing/btest/Baseline/spicy.tcp-eod-behavior/output-1024-no-fins new file mode 100644 index 0000000000..a2d03b81be --- /dev/null +++ b/testing/btest/Baseline/spicy.tcp-eod-behavior/output-1024-no-fins @@ -0,0 +1,2 @@ +### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63. +=== Not enough data, missing FINs (expect no output) diff --git a/testing/btest/Baseline/spicy.tcp-eod-behavior/output-136-fins b/testing/btest/Baseline/spicy.tcp-eod-behavior/output-136-fins new file mode 100644 index 0000000000..6f98953895 --- /dev/null +++ b/testing/btest/Baseline/spicy.tcp-eod-behavior/output-136-fins @@ -0,0 +1,3 @@ +### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63. +=== Exact data, regular FINs (expect event output) +event foo() diff --git a/testing/btest/Baseline/spicy.tcp-eod-behavior/output-136-no-fins b/testing/btest/Baseline/spicy.tcp-eod-behavior/output-136-no-fins new file mode 100644 index 0000000000..9cf286944f --- /dev/null +++ b/testing/btest/Baseline/spicy.tcp-eod-behavior/output-136-no-fins @@ -0,0 +1,3 @@ +### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63. +=== Exact data, missing FINs (expect event output) +event foo() diff --git a/testing/btest/Baseline/spicy.tcp-eod-behavior/output-16-fins b/testing/btest/Baseline/spicy.tcp-eod-behavior/output-16-fins new file mode 100644 index 0000000000..62ef9d5df0 --- /dev/null +++ b/testing/btest/Baseline/spicy.tcp-eod-behavior/output-16-fins @@ -0,0 +1,3 @@ +### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63. +=== Too much data, regular FINs (expect event output) +event foo() diff --git a/testing/btest/Baseline/spicy.tcp-eod-behavior/output-16-no-fins b/testing/btest/Baseline/spicy.tcp-eod-behavior/output-16-no-fins new file mode 100644 index 0000000000..78b655c416 --- /dev/null +++ b/testing/btest/Baseline/spicy.tcp-eod-behavior/output-16-no-fins @@ -0,0 +1,3 @@ +### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63. +=== Too much data, missing FINs (expect event output) +event foo() diff --git a/testing/btest/Baseline/spicy.tcp-eod-behavior/output-eod-fins b/testing/btest/Baseline/spicy.tcp-eod-behavior/output-eod-fins new file mode 100644 index 0000000000..205da6bca1 --- /dev/null +++ b/testing/btest/Baseline/spicy.tcp-eod-behavior/output-eod-fins @@ -0,0 +1,3 @@ +### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63. +=== Until EOD, regular FINs (expect event output) +event foo() diff --git a/testing/btest/Baseline/spicy.tcp-eod-behavior/output-eod-no-fins b/testing/btest/Baseline/spicy.tcp-eod-behavior/output-eod-no-fins new file mode 100644 index 0000000000..fd610216b3 --- /dev/null +++ b/testing/btest/Baseline/spicy.tcp-eod-behavior/output-eod-no-fins @@ -0,0 +1,2 @@ +### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63. +=== Until EOD, missing FINs (expect no output) diff --git a/testing/btest/Traces/http/get-without-fins.trace b/testing/btest/Traces/http/get-without-fins.trace new file mode 100644 index 0000000000000000000000000000000000000000..f146132b8838619660500ea8ac63778e36b90b37 GIT binary patch literal 6089 zcma)AO>87b6`pl=lQ5FQiUZ0iRGftMI_~-NjDLEwNxc5s-K@RF9w*{LQtqyqsko=R zsjl{TR!H~=Vo8udIdI7jM9KjXgt(DIqzEa35c0z%0u+#tP>@h04oF;BzE|BnJAV$W z)Y_SG*Q;0G``-87tKYx%^Oq-br*qlATeot#Q~2`J&n-Ex0cI2{_%5<=N`Ya{o!|?`|FuEU-;$2pS+jb{?Gjtn_m9e#I0Kw-pu7D zPM`V3sXOjCed^TR`7d7m6QjSKo=B`;tKXBmitqF!c9T7f^+&Klu6XCy8f2Mv$Zdb} z-m9;_`zUs}@X{9+zgO5!te>8^J74_vrLU58>~t6Tt@iH8U4m75M*Ob6^CJ0u{O*w7 z7hwIdPMdu@>)T51&3Bw79WWXGJ&Y z_+G%>;>zXat&O$a!oCVD9v>C-k!F|c_3dK0P@c^1MM7E2P2p>sJ=GMNeyy@;m#dES z%auZrYJPSGg88tGD7=R$5@2T;VU}IaVp#)j1o#Y;4vi z^SeSF3T3n9kUK3=tQ4w+IX1m6+(2=_b&W^b+6-LTkirG7!kuN6CAJ(l$ykL|=IrW{ zT}h3t)p^rqQ{~Eh*{V8=3#GZ*BA;(KrKt>fYlr(y5drkB7LkEmBVyJy;Wu^5X4QpK zZ8Cowe4A<67D22p)@E6oA6+b$N+$;j0W{X?-4G|&;z$=mk4ygob6Q+QLSNjguUm^0 z(R`^;nP=ZHr?5(kR;9|yHM?B0OI2o-7D}ajp0OFWE{~+&WO3*MObb@X`&rg$NvFk} zAog4qO6P#-mS7zf;8h1csK8?l>4^fH-U;@l&vt`$Td4B`HaE66SR|W1*^eA0Lp}OB zoUHXZSt_&goL#Hg)pvNK}ZOCSm z`9r%`3iCvz{tXDxW!JrZVtUUNp3s5?fU*&LUe^LrO&Hu)+y^D2v8c>hm7b?%3m^fP zO4SBH2xdPnoYLEJVx1niEux4wDKe|vm%>AS@U96BPCwUYvka^&c6r{$hd!J2j^GCo z`K{nL2NFr_K#I@O>!I*3 z?XI#Y4#PkZX@(*0o%jgZK;eK}KG2m%gyygZclh0g&M;~-QMBIpxk zh~y5KOvXIv3*>>of({FL1V~ych?^~AiZSj{3cwI{Ty_;uhqVXhYYPQR0>&)1Big~C zp+e9WY-4LrMs3zmL7VZ;K`$dfm)ncydW-QhC=!Gu)&^;k2?m`A5osjcUVVl3y}fpR zad>U=ZgSX2gj{hAbW@Yo`rz{FQg3-G3E7K|4QRYBi|7KoRt#Ig^SR{89Kjc;BXUB?9q% z=}Fxk;t^;vQ{E@$ zE3CX|Q^pO54a}SHwq*oD5?i!rheli}%!9hzBh9etn!}X)50mUxvgatvdTM`7YV9F& zvrPe!HJIjOq#vX+nPKGLa9i`{XaLBEo(ClhC9}L>S0S#oeh*SO(+oZrv6$3~rK)Ap zn5$;oM$JqG4H0*djAM4BkY9?MbejJkDq1vzDrGq8RDe7*=`VG}P*_~);=f3EjnTtq zY=iouQ^4*d9YbbMs(G|FQ!v_^dmY}zjy`Ia=NajJFjNA^DpG0P@3LnMm(v%X8Dbil zyKZ#CEamRAafE=8TtH3RTpNC+8;&~(Lx~WmQVkw^X*soU^2jLPmEG&9nSy!Eh<~C= zQ`Rbbq|w%oP8m`}p>U{+q7vc21?XI)Rf8OvK5s?3OT=NkI!b!QW5 z!4=^jO0*h=1BYBmzK^AscCJ6YvR8k)er0oQeY4IS2mu-%1AJgPurHTM{ED1U!an@B>07 zRj?EK56X|*E=I+nEX5$zYibf+Tb#NdbO-1HbYOr$Iv&aZ-4AsKkOtX6Wr-pQViwqH znmK94_`nzIr(;tV1NcgGGiCxbG?CQed={_4zB!H6W^hA#9-L5=$R=KC_|nD`Q4>cd zh7qYl6z@NasSfcR6Q$IqDBUC=}|bX$Z&nqW^;nq{c9f_Ltn`>|DyMa|{&^{+J+2S6~upbd88WPe`27 z+)Q-f+`iEKoH0Q!OAWOg07`?swZr z&G7`!^vw)|u!7nYwSo@v4}3w_Zw4pOf;BLehv@jSa-5knTMX#iZfXlO!>~f*;mY=2 zQvHw`bgM!Nfm$Jb!Xq*g4MqhGV=J9)GW&!1i1Au+2PDO{Nd`Dk#1$N68gnO`DKu}G zUTw?}N0F*aALy9b+9Y1ml7kN1LvyjRj7A#SMZ=S!%kJ(qlh(#H^A2ib;KRFJ0WdBD zrgX$kk(^3<0=g}l0!cAfB5wU-L8XG#!_s{M)q1a)ayE9b_ucK4m8lJr&#{S^8M6g+cj09Nd5FV`=RzW-F9 z4mhSg8Y-a#>q-ckNRul7nkjHS0G$D|TtN+B!{g0p)Xa|PD9Am?X3}V=@|@HkmI@dc zvSy()ZrCUZo`jnDjV?7bum9V%+Cm7q=y;67+Km zd-ry(T67&VZ&wy=Oz?d>;}!~Ouj%+n`3O}&aUqHuh{jN^r!7vS*lH9VZp|dKFx8h# z_L3HJXKi_PbB(pQsmlTIL+n*jL9s&v<&TciCKirQ+>N33YpjZ1@uimN(PY~M6fr`# zSVSTve(gTF#neJ+c(5a)e-lKfOHz79u7Iom`W0OPKl$3w74TcHzW&kk$7jEEC~qc2 l$0g}cXuu_lFMg)Q&%Z+!FTOHr@h|TlpS|y&{QW_T{{>output-16-fins +# @TEST-EXEC: rm -f analyzer.log && zeek -b -r ${TRACES}/http/get.trace Zeek::Spicy foo-16.hlto %INPUT >>output-16-fins +# @TEST-EXEC: test '!' -f analyzer.log +# @TEST-EXEC: btest-diff output-16-fins + +# @TEST-EXEC: echo "=== Too much data, missing FINs (expect event output)" >>output-16-no-fins +# @TEST-EXEC: rm -f analyzer.log && zeek -b -r ${TRACES}/http/get-without-fins.trace Zeek::Spicy foo-16.hlto %INPUT >>output-16-no-fins +# @TEST-EXEC: test '!' -f analyzer.log +# @TEST-EXEC: btest-diff output-16-no-fins + +# @TEST-EXEC: echo "=== Exact data, regular FINs (expect event output)" >>output-136-fins +# @TEST-EXEC: rm -f analyzer.log && zeek -b -r ${TRACES}/http/get.trace Zeek::Spicy foo-136.hlto %INPUT >>output-136-fins +# @TEST-EXEC: test '!' -f analyzer.log +# @TEST-EXEC: btest-diff output-136-fins + +# @TEST-EXEC: echo "=== Exact data, missing FINs (expect event output)" >>output-136-no-fins +# @TEST-EXEC: rm -f analyzer.log && zeek -b -r ${TRACES}/http/get-without-fins.trace Zeek::Spicy foo-136.hlto %INPUT >>output-136-no-fins +# @TEST-EXEC: test '!' -f analyzer.log +# @TEST-EXEC: btest-diff output-136-no-fins + +# @TEST-EXEC: echo "=== Not enough data, regular FINs (expect analyzer error)" >>output-1024-fins +# @TEST-EXEC: rm -f analyzer.log && zeek -b -r ${TRACES}/http/get.trace Zeek::Spicy foo-1024.hlto %INPUT >>output-1024-fins +# @TEST-EXEC: test -f analyzer.log && zeek-cut cause failure_reason >output-1024-fins +# @TEST-EXEC: btest-diff output-1024-fins + +# @TEST-EXEC: echo "=== Not enough data, missing FINs (expect no output)" >>output-1024-no-fins +# @TEST-EXEC: rm -f analyzer.log && zeek -b -r ${TRACES}/http/get-without-fins.trace Zeek::Spicy foo-1024.hlto %INPUT >>output-1024-no-fins +# @TEST-EXEC: test '!' -f analyzer.log +# @TEST-EXEC: btest-diff output-1024-no-fins + +# @TEST-EXEC: echo "=== Until EOD, regular FINs (expect event output)" >>output-eod-fins +# @TEST-EXEC: rm -f analyzer.log && zeek -b -r ${TRACES}/http/get.trace Zeek::Spicy foo-eod.hlto %INPUT >>output-eod-fins +# @TEST-EXEC: test '!' -f analyzer.log +# @TEST-EXEC: btest-diff output-eod-fins + +# @TEST-EXEC: echo "=== Until EOD, missing FINs (expect no output)" >>output-eod-no-fins +# @TEST-EXEC: rm -f analyzer.log && zeek -b -r ${TRACES}/http/get-without-fins.trace Zeek::Spicy foo-eod.hlto %INPUT >>output-eod-no-fins +# @TEST-EXEC: test '!' -f analyzer.log +# @TEST-EXEC: btest-diff output-eod-no-fins + +event Test::foo() { + print "event foo()"; + } + +# @TEST-START-FILE test.spicy +module Test; + +public type Foo16 = unit { + : bytes &size=16; +}; + +public type Foo136 = unit { + : bytes &size=136; +}; + +public type Foo1024 = unit { + : bytes &size=1024; +}; + +public type FooEOD = unit { + : bytes &eod; +}; + +# @TEST-END-FILE + +# @TEST-START-FILE foo-16.evt + +protocol analyzer spicy::Foo over TCP: + parse originator with Test::Foo16, + port 80/tcp; + +on Test::Foo16 -> event Test::foo(); +# @TEST-END-FILE + +# @TEST-START-FILE foo-136.evt +protocol analyzer spicy::Foo over TCP: + parse originator with Test::Foo136, + port 80/tcp; + +on Test::Foo136 -> event Test::foo(); +# @TEST-END-FILE + +# @TEST-START-FILE foo-1024.evt +protocol analyzer spicy::Foo over TCP: + parse originator with Test::Foo1024, + port 80/tcp; + +on Test::Foo1024 -> event Test::foo(); +# @TEST-END-FILE + +# @TEST-START-FILE foo-eod.evt +protocol analyzer spicy::Foo over TCP: + parse originator with Test::FooEOD, + port 80/tcp; + +on Test::FooEOD -> event Test::foo(); +# @TEST-END-FILE +