From e04682434fb0714812aa2e2106596887518704c0 Mon Sep 17 00:00:00 2001 From: Arne Welzel Date: Mon, 21 Oct 2024 15:51:05 +0200 Subject: [PATCH] Merge branch 'modbus-fixes' of https://github.com/zambo99/zeek * 'modbus-fixes' of https://github.com/zambo99/zeek: Prevent non-Modbus on port 502 to be reported as Modbus (cherry picked from commit 4763282f36d13808b58948cc378a7df00201c9f5) --- CHANGES | 15 ++++++++++++ VERSION | 2 +- .../protocol/modbus/modbus-protocol.pac | 4 ++-- .../modbus.log | 10 ++++---- .../analyzer.log | 20 ++++++++++++++++ .../conn.log | 18 ++++++++++++++ .../modbus.log | 22 ++++++++++++++++++ .../modbus/modbus-and-non-modbus-p502.pcap | Bin 0 -> 7414 bytes .../modbus_and_non_modbus_on_port_502.test | 7 ++++++ 9 files changed, 90 insertions(+), 8 deletions(-) create mode 100644 testing/btest/Baseline/scripts.base.protocols.modbus.modbus_and_non_modbus_on_port_502/analyzer.log create mode 100644 testing/btest/Baseline/scripts.base.protocols.modbus.modbus_and_non_modbus_on_port_502/conn.log create mode 100644 testing/btest/Baseline/scripts.base.protocols.modbus.modbus_and_non_modbus_on_port_502/modbus.log create mode 100644 testing/btest/Traces/modbus/modbus-and-non-modbus-p502.pcap create mode 100644 testing/btest/scripts/base/protocols/modbus/modbus_and_non_modbus_on_port_502.test diff --git a/CHANGES b/CHANGES index 5dd3828a38..fe9b32b2dd 100644 --- a/CHANGES +++ b/CHANGES @@ -1,3 +1,18 @@ +6.0.8-4 | 2024-11-14 13:51:17 -0700 + + * GH-3962: Prevent non-Modbus on port 502 to be reported as Modbus (Emmanuele Zambon) + + This commit prevents most non-Modbus TCP traffic on port 502 to be + reported as Modbus in conn.log as well as in modbus.log. + To do so, we have introduced two &enforce checks in the Modbus + protocol definition that checks that some specific fields of the + (supposedly) Modbus header are compatible with values specified in + the specs. + + (cherry picked from commit 4763282f36d13808b58948cc378a7df00201c9f5) + + * Minor clang-format fix (Tim Wojtulewicz, Corelight) + 6.0.8-2 | 2024-11-14 13:49:22 -0700 * GH-3957: input/Raw: Rework GetLine() (Arne Welzel, Corelight) diff --git a/VERSION b/VERSION index e6a110eb58..86beecd0f1 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -6.0.8-2 +6.0.8-4 diff --git a/src/analyzer/protocol/modbus/modbus-protocol.pac b/src/analyzer/protocol/modbus/modbus-protocol.pac index fba0a9cd3a..5e73e5d554 100644 --- a/src/analyzer/protocol/modbus/modbus-protocol.pac +++ b/src/analyzer/protocol/modbus/modbus-protocol.pac @@ -70,8 +70,8 @@ type ModbusTCP_PDU(is_orig: bool) = record { type ModbusTCP_TransportHeader = record { tid: uint16; # Transaction identifier - pid: uint16; # Protocol identifier - len: uint16; # Length of everything after this field + pid: uint16 &enforce(pid == 0); # Protocol identifier + len: uint16 &enforce(len >= 2); # Length of everything after this field uid: uint8; # Unit identifier (previously 'slave address') fc: uint8; # MODBUS function code (see function_codes enum) } &byteorder=bigendian, &let { diff --git a/testing/btest/Baseline/scripts.base.protocols.modbus.exception_handling/modbus.log b/testing/btest/Baseline/scripts.base.protocols.modbus.exception_handling/modbus.log index 9298bf4eeb..07518ad4fc 100644 --- a/testing/btest/Baseline/scripts.base.protocols.modbus.exception_handling/modbus.log +++ b/testing/btest/Baseline/scripts.base.protocols.modbus.exception_handling/modbus.log @@ -11,15 +11,15 @@ XXXXXXXXXX.XXXXXX CHhAvVGS1DHFjwGM9 192.168.66.235 2582 166.161.16.230 502 0 1 u XXXXXXXXXX.XXXXXX CHhAvVGS1DHFjwGM9 192.168.66.235 2582 166.161.16.230 502 0 1 unknown-29 REQ - XXXXXXXXXX.XXXXXX CHhAvVGS1DHFjwGM9 192.168.66.235 2582 166.161.16.230 502 0 1 unknown-160 RESP - XXXXXXXXXX.XXXXXX CHhAvVGS1DHFjwGM9 192.168.66.235 2582 166.161.16.230 502 0 1 unknown-33 REQ - -XXXXXXXXXX.XXXXXX CHhAvVGS1DHFjwGM9 192.168.66.235 2582 166.161.16.230 502 0 0 WRITE_SINGLE_REGISTER REQ - -XXXXXXXXXX.XXXXXX CHhAvVGS1DHFjwGM9 192.168.66.235 2582 166.161.16.230 502 0 1 unknown-162 RESP - XXXXXXXXXX.XXXXXX CHhAvVGS1DHFjwGM9 192.168.66.235 2582 166.161.16.230 502 21504 1 unknown-35 REQ - XXXXXXXXXX.XXXXXX CHhAvVGS1DHFjwGM9 192.168.66.235 2582 166.161.16.230 502 0 1 unknown-36 REQ - XXXXXXXXXX.XXXXXX CHhAvVGS1DHFjwGM9 192.168.66.235 2582 166.161.16.230 502 0 1 unknown-37 REQ - XXXXXXXXXX.XXXXXX CHhAvVGS1DHFjwGM9 192.168.66.235 2582 166.161.16.230 502 0 0 unknown-38 REQ - XXXXXXXXXX.XXXXXX CHhAvVGS1DHFjwGM9 192.168.66.235 2582 166.161.16.230 502 0 1 unknown-175 RESP - XXXXXXXXXX.XXXXXX CHhAvVGS1DHFjwGM9 192.168.66.235 2582 166.161.16.230 502 0 1 unknown-179 RESP - -XXXXXXXXXX.XXXXXX CHhAvVGS1DHFjwGM9 192.168.66.235 2582 166.161.16.230 502 12032 0 unknown-0 REQ - -XXXXXXXXXX.XXXXXX CHhAvVGS1DHFjwGM9 192.168.66.235 2582 166.161.16.230 502 0 0 unknown-0 REQ - -XXXXXXXXXX.XXXXXX CHhAvVGS1DHFjwGM9 192.168.66.235 2582 166.161.16.230 502 0 1 unknown-165 RESP - +XXXXXXXXXX.XXXXXX CHhAvVGS1DHFjwGM9 192.168.66.235 2582 166.161.16.230 502 0 1 unknown-54 REQ - +XXXXXXXXXX.XXXXXX CHhAvVGS1DHFjwGM9 192.168.66.235 2582 166.161.16.230 502 37 1 unknown-71 REQ - +XXXXXXXXXX.XXXXXX CHhAvVGS1DHFjwGM9 192.168.66.235 2582 166.161.16.230 502 0 1 unknown-63 REQ - +XXXXXXXXXX.XXXXXX CHhAvVGS1DHFjwGM9 192.168.66.235 2582 166.161.16.230 502 0 1 unknown-65 REQ - +XXXXXXXXXX.XXXXXX CHhAvVGS1DHFjwGM9 192.168.66.235 2582 166.161.16.230 502 0 1 unknown-71 REQ - #close XXXX-XX-XX-XX-XX-XX diff --git a/testing/btest/Baseline/scripts.base.protocols.modbus.modbus_and_non_modbus_on_port_502/analyzer.log b/testing/btest/Baseline/scripts.base.protocols.modbus.modbus_and_non_modbus_on_port_502/analyzer.log new file mode 100644 index 0000000000..ab8cb41827 --- /dev/null +++ b/testing/btest/Baseline/scripts.base.protocols.modbus.modbus_and_non_modbus_on_port_502/analyzer.log @@ -0,0 +1,20 @@ +### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63. +#separator \x09 +#set_separator , +#empty_field (empty) +#unset_field - +#path analyzer +#open XXXX-XX-XX-XX-XX-XX +#fields ts cause analyzer_kind analyzer_name uid fuid id.orig_h id.orig_p id.resp_h id.resp_p failure_reason failure_data +#types time string string string string string addr port addr port string string +XXXXXXXXXX.XXXXXX violation protocol MODBUS ClEkJM2Vm5giqnMf4h - 87.236.176.106 38129 192.168.10.111 502 Binpac exception: binpac exception: &enforce violation : ModbusTCP_TransportHeader:pid - +XXXXXXXXXX.XXXXXX violation protocol MODBUS ClEkJM2Vm5giqnMf4h - 87.236.176.106 38129 192.168.10.111 502 Binpac exception: binpac exception: &enforce violation : ModbusTCP_TransportHeader:pid - +XXXXXXXXXX.XXXXXX violation protocol MODBUS C4J4Th3PJpwUYZZ6gc - 87.236.176.96 60175 192.168.10.111 502 Binpac exception: binpac exception: &enforce violation : ModbusTCP_TransportHeader:pid - +XXXXXXXXXX.XXXXXX violation protocol MODBUS C4J4Th3PJpwUYZZ6gc - 87.236.176.96 60175 192.168.10.111 502 Binpac exception: binpac exception: &enforce violation : ModbusTCP_TransportHeader:pid - +XXXXXXXXXX.XXXXXX violation protocol MODBUS CtPZjS20MLrsMUOJi2 - 66.175.213.4 58380 192.168.10.111 502 Binpac exception: binpac exception: &enforce violation : ModbusTCP_TransportHeader:pid - +XXXXXXXXXX.XXXXXX violation protocol MODBUS CtPZjS20MLrsMUOJi2 - 66.175.213.4 58380 192.168.10.111 502 Binpac exception: binpac exception: &enforce violation : ModbusTCP_TransportHeader:pid - +XXXXXXXXXX.XXXXXX violation protocol MODBUS CP5puj4I8PtEU4qzYg - 159.203.208.13 33752 192.168.10.113 502 Binpac exception: binpac exception: &enforce violation : ModbusTCP_TransportHeader:pid - +XXXXXXXXXX.XXXXXX violation protocol MODBUS CP5puj4I8PtEU4qzYg - 159.203.208.13 33752 192.168.10.113 502 Binpac exception: binpac exception: &enforce violation : ModbusTCP_TransportHeader:pid - +XXXXXXXXXX.XXXXXX violation protocol MODBUS C37jN32gN3y3AZzyf6 - 62.122.184.123 7488 192.168.10.111 502 Binpac exception: binpac exception: &enforce violation : ModbusTCP_TransportHeader:pid - +XXXXXXXXXX.XXXXXX violation protocol MODBUS C37jN32gN3y3AZzyf6 - 62.122.184.123 7488 192.168.10.111 502 Binpac exception: binpac exception: &enforce violation : ModbusTCP_TransportHeader:pid - +#close XXXX-XX-XX-XX-XX-XX diff --git a/testing/btest/Baseline/scripts.base.protocols.modbus.modbus_and_non_modbus_on_port_502/conn.log b/testing/btest/Baseline/scripts.base.protocols.modbus.modbus_and_non_modbus_on_port_502/conn.log new file mode 100644 index 0000000000..945c873d56 --- /dev/null +++ b/testing/btest/Baseline/scripts.base.protocols.modbus.modbus_and_non_modbus_on_port_502/conn.log @@ -0,0 +1,18 @@ +### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63. +#separator \x09 +#set_separator , +#empty_field (empty) +#unset_field - +#path conn +#open XXXX-XX-XX-XX-XX-XX +#fields ts uid id.orig_h id.orig_p id.resp_h id.resp_p proto service duration orig_bytes resp_bytes conn_state local_orig local_resp missed_bytes history orig_pkts orig_ip_bytes resp_pkts resp_ip_bytes tunnel_parents +#types time string addr port addr port enum string interval count count string bool bool count string count count count count set[string] +XXXXXXXXXX.XXXXXX CHhAvVGS1DHFjwGM9 10.0.0.9 3082 10.0.0.3 502 tcp modbus 177.095534 72 69 SF T T 0 ShADdFaf 16 720 9 437 - +XXXXXXXXXX.XXXXXX ClEkJM2Vm5giqnMf4h 87.236.176.106 38129 192.168.10.111 502 tcp dce_rpc 5.102604 72 9 SF F T 0 ShADadFf 6 392 4 225 - +XXXXXXXXXX.XXXXXX C4J4Th3PJpwUYZZ6gc 87.236.176.96 60175 192.168.10.111 502 tcp - 5.052092 44 9 SF F T 0 ShADadFf 6 364 4 225 - +XXXXXXXXXX.XXXXXX CtPZjS20MLrsMUOJi2 66.175.213.4 58380 192.168.10.111 502 tcp ssl 59.999857 138 9 SF F T 0 ShADadFf 9 610 7 377 - +XXXXXXXXXX.XXXXXX CUM0KZ3MLUfNB0cl11 198.74.56.135 60293 192.168.10.111 502 tcp - 0.117322 0 0 RSTO F T 0 ShR 2 80 1 44 - +XXXXXXXXXX.XXXXXX CmES5u32sYpV7JYN 198.74.56.135 60293 192.168.10.111 502 tcp - 0.000054 109 0 RSTRH F T 0 Dr 1 149 1 40 - +XXXXXXXXXX.XXXXXX CP5puj4I8PtEU4qzYg 159.203.208.13 33752 192.168.10.113 502 tcp - 0.470159 24 9 SF F T 0 ShADadFf 6 344 4 225 - +XXXXXXXXXX.XXXXXX C37jN32gN3y3AZzyf6 62.122.184.123 7488 192.168.10.111 502 tcp - 30.159557 43 9 SF F T 0 ShADadFf 6 295 4 181 - +#close XXXX-XX-XX-XX-XX-XX diff --git a/testing/btest/Baseline/scripts.base.protocols.modbus.modbus_and_non_modbus_on_port_502/modbus.log b/testing/btest/Baseline/scripts.base.protocols.modbus.modbus_and_non_modbus_on_port_502/modbus.log new file mode 100644 index 0000000000..0bf4ca545b --- /dev/null +++ b/testing/btest/Baseline/scripts.base.protocols.modbus.modbus_and_non_modbus_on_port_502/modbus.log @@ -0,0 +1,22 @@ +### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63. +#separator \x09 +#set_separator , +#empty_field (empty) +#unset_field - +#path modbus +#open XXXX-XX-XX-XX-XX-XX +#fields ts uid id.orig_h id.orig_p id.resp_h id.resp_p tid unit func pdu_type exception +#types time string addr port addr port count count string string string +XXXXXXXXXX.XXXXXX CHhAvVGS1DHFjwGM9 10.0.0.9 3082 10.0.0.3 502 1 10 READ_COILS REQ - +XXXXXXXXXX.XXXXXX CHhAvVGS1DHFjwGM9 10.0.0.9 3082 10.0.0.3 502 1 10 READ_COILS RESP - +XXXXXXXXXX.XXXXXX CHhAvVGS1DHFjwGM9 10.0.0.9 3082 10.0.0.3 502 1 10 READ_COILS REQ - +XXXXXXXXXX.XXXXXX CHhAvVGS1DHFjwGM9 10.0.0.9 3082 10.0.0.3 502 1 10 READ_COILS RESP - +XXXXXXXXXX.XXXXXX CHhAvVGS1DHFjwGM9 10.0.0.9 3082 10.0.0.3 502 1 10 READ_HOLDING_REGISTERS REQ - +XXXXXXXXXX.XXXXXX CHhAvVGS1DHFjwGM9 10.0.0.9 3082 10.0.0.3 502 1 10 READ_HOLDING_REGISTERS RESP - +XXXXXXXXXX.XXXXXX CHhAvVGS1DHFjwGM9 10.0.0.9 3082 10.0.0.3 502 1 10 WRITE_SINGLE_COIL REQ - +XXXXXXXXXX.XXXXXX CHhAvVGS1DHFjwGM9 10.0.0.9 3082 10.0.0.3 502 1 10 WRITE_SINGLE_COIL RESP - +XXXXXXXXXX.XXXXXX CHhAvVGS1DHFjwGM9 10.0.0.9 3082 10.0.0.3 502 1 10 WRITE_SINGLE_COIL REQ - +XXXXXXXXXX.XXXXXX CHhAvVGS1DHFjwGM9 10.0.0.9 3082 10.0.0.3 502 1 10 WRITE_SINGLE_COIL RESP - +XXXXXXXXXX.XXXXXX CHhAvVGS1DHFjwGM9 10.0.0.9 3082 10.0.0.3 502 1 10 WRITE_SINGLE_REGISTER REQ - +XXXXXXXXXX.XXXXXX CHhAvVGS1DHFjwGM9 10.0.0.9 3082 10.0.0.3 502 1 10 WRITE_SINGLE_REGISTER RESP - +#close XXXX-XX-XX-XX-XX-XX diff --git a/testing/btest/Traces/modbus/modbus-and-non-modbus-p502.pcap b/testing/btest/Traces/modbus/modbus-and-non-modbus-p502.pcap new file mode 100644 index 0000000000000000000000000000000000000000..a3b1ea28f848db3fbb49fdeae33b59d7bba2acbd GIT binary patch literal 7414 zcmaKx3sh9q9>({X83r7gWKdy@R3IwkZH~#sMP9@n7iqigg38#A3OQh^7pETSX)yN=|=6^^y%i zATF#3?Cwrl)0b9IC$R~*5_J!4k=nIfTezTYA&9}YfR4*$m2?`lwNMsl7{rqCnz{$e zqTR$7r43Yb+rU{yuPk%Nx%_*8akOmeUGySdS#hAPUPPOjO>y9#kqFiC{gnIl z6EuDazc%=7EE`8&sl`KGagyrI>ND=Q(<{r|(U`9XMjl^1CiK7r5YOQd)gX0`32k99 z-0xDsuMK_=!f)g34xA+r6YQ$Dt3BMWUeI_DerLmPLs>Klp>_|{EzqiF_c>?zcx9Qu zijqk(%cN7V#F33;vD~S+1kHxiW?MjaW6=mDE7eDp6~|ex3aQB~6IjEQtef=;mblPH zY8~98PVm@_ys~*-qg`jbCpUHo9K zqux-y@`~jAaMr%R({HZ9qCF70XwlYb{0!SKzI%#}OIocjkz18y|52}hWv5S>#20X^ z!&YY17tJIp(Iw}~E2T9*bb$WMn!p>2BJXySv$m!fiS8o5_O;GE136Qn*|NHXYd$Mz zeg^bTdw}Fjn+p*V>3we~kspKB_N6{-2}sm=Q>O6D>bacwx>urEa_WY-h`SgOQ7GB~ zV5XzfRNon}liu>z^waP8_fL(?-fyZ~@5iEttm)9=GhOzgC9SvYH@*D}js;B5)!-N@ z3w_DQ?Nv!~>_m$D;I(UfpsXxw>#XSQ7k6>uoiF>a`$4CU-1?5o#ACUf zc+o4-EOm{l?j^<$k-Rc3vr8nlFiYS$n+PRM9r$3SpQ zF^Jog_C1MZoOsGB(Ih#?U@IyQwd0VBI2-npL89(q-*POE+i&Y@|ELauHrY*5SM*~p z`ww5=e$jD=G+xeBbv=)3JSk{wf@>dg9i8OT_=PrW>%-S;L2L~6RVhjv7Q8rp5W3TpN}_5G%YAQIvxYxroKriD+jmvSwk!eLXAOl7MO6|H!6{2XT&A?|NyM5A;%i=s zD7=`DT*QiRUBof4uM%hjxBrn~eEb@hd}uGX^@xTVgUJ4eP6uf1bsG5Q5*$?yN5Bb{gcCFhNpMoeAC zmZR_%zu_*Aj81Jd9Vcq0k}4KmB4gw{>n|b_S#qBB-|%AkagcMDqv%o_-PHD|XiI`$DA?CwY6Mprl2(!#Hlj+i_(8W1)_C zM}i~T5p$saz-Gr_#{kC=hk?Dp-emD?tRs-ovvML98z20SX23r`Hk<`BGaJSRG7SzJ z*-gw}C+W04l8;s+X|&xE%79@7ByXFPX^CTCiTRnvAUZ@CgA*8o3-*An8>I_nZ|zZ2 zA}BE;=Kt!aQqD2|<3y8JqPgqFtZ#dXDMX~S_&Q4HxDO5PQX#*}fv{X#&xbZ$7>ElP z+7=A$`FY1(1ECua!6}D9{9unNaf8>1{i};OX0nSo>8OkNg_7tQ=t*vWm|(xf6=;0V z4Wd1&CNrZ`H~thfev)*Pn(K{R<1E2-uf|V%h#8&LvxrK>14^Q&##3CQxj&6s0jv7mkI;>wo@=xT8s*tz>NtP+7njCMOWUK`T8Ow(N%YishHDJ+qOsL(>grlm z-=i@)_NI)^OG}A(JXiJRh1C@~J}78x#e6)68Gi1ydtDlT)3k%SWkr4AA3x~bUlNET z6G;bR^!v_#bzkC(q_eeFa?&wsHcfZ;)qlvZ86#HAQ9Z-9jCYScVhrOq?20qQAs%)~ zwMSl9YQ_mtZeYjd+Hm=XO{58H@Dzm)sPm?*WT+yaFNmseC!i;5#;#(sEw|lZg%oHQ zZCR-Ereyt-j=*8H;823HVR-DjJmLr*YNw@##)nQ%Pq)R7jvH+>OfRo|A}MtAT?ui2 z8y7b&A#RMtXqa43FsA}XqvE4nZ(^s-FDNf8nO~F?T2x(9VKih^&RGyUxoFOO{N4lQ z)g@(RdGTZ85<;ts7Ubo}C&WEvG#CveITeyoMZ9|i$?aS973YN@hmjSszv?G^DYs#B z;iF@9{cVSlY$+ZBV)dJ{-~Q&15v0eT|50)_We&k6n6|?U0_J|_UO%PqgO}(yZCZ)E zQb`skHpa|~RiS%7QHd@&pZ{@0T!TO8S7|%^+fp}qa9!Z|5Pi&QaltubD^O5 zD(FUg0BgCqS)SlhcykDmNoy5|k3c(h-ju^XqjWwe-tU!YW-WIQ>Pvj!o*6TzPR&t_ z{ara@6Yez7l@=+ri>X*(#g%8k@#_wYl&>Nxv(bhFPj!u(ERf;e<)mJO2)y<-@I#^P+TW z76oqqGL*f&Q!WKee?R&Q4iP(*L=V@D7uQpbL?f!?9Bz$_`Qv6#)9kp5HYD^AL8GD~ z;;}5nUvb2-j;yNX8Yc=G(?AWe1xT#rPdbGfu>v2G7-Ha^A5_kpw5o-iW%9~0ORV5o z{^%He`$>N8l=AY@k~vAC^D3XHEY7Peo|soSuVlUvyH-EigYpaBiVOCCFO;yUv+Aox z3UP!Wjv9z}o;@s6cCVAz-5^r;xTB7iu#p30s8{YP!#kT!Cg!v&iW2