Alive Connections Being Dropped

[jcyprus]

Summary

Connections are being prematurely severed based on occasionally seen TCP communication gaps
(typically [TCP ACKed unseen segment] or [TCP Retransmission] packets). While the devices autonegotiate
a fix to these gaps and continue with their communication, Zeek has measures in place to alert
parsers of these gaps and leave it up to them on how to handle it. The current implementation is more
in-line with how other Zeek parsers handle these types of packet sequences, however it is resulting in a
high volume of packet loss in this instance.

Motivation and Context

ENIP communications may occasionally send [TCP ACKed unseen segment] or [TCP Retransmission]
packets, which results in a TCP gap being formed. While this doesn’t impact the communication
between devices, Zeek sees this as a failed connection and sets the had_gap variable to true so that
parsers can know that this communication is faulty. The variable will be reset for that communication on
a per parser and per connection basis, meaning that it is up to the parsers to handle the gap and tell
Zeek what to do next.

Lines 37 and 38, if (had_gap) return;, in ENIP.cc handle the error. These lines result in all future events,
for a given TCP connection, being skipped if the parser detected a gap. However, since this flag is being set
during the normal communications of healthy and ongoing connections, a high volume of unrecoverable
packet loss occurs.

As currently implemented, the large amount of packet loss in the final parser output leads to
inconsistent and inaccurate results when compared with communication over the wire. If this condition
is realized, Wireshark will show significantly more traffic than Zeek, as Zeek has stopped logging all
traffic from that connection. This fix would allow for reestablished/retransmitted connections to
continue to be logged. Therefore, anyone who is viewing traffic will have an accurate understanding of
the relationship between assets and the true volume of traffic being sent.

Potential Fixes and Implementation Notes

Comment out or remove the if (had_gap) return; line in ENIP.cc.

Code Comparison Between Current Implementation and Proposed Fixes

Comparing files ENIP.cc and ENIP-WITH-FIXES.CC
***** ENIP.cc
36: assert(TCP());
37: if(had_gap)
38: return;
39:
***** ENIP-WITH-FIXES.CC
36: assert(TCP());
37: //if(had_gap)
38: // return;
39:
*****

[Kleinspider]

Code changes as requested have been tested and pushed into main