ARP: implement decoder and logger v10 #11060
Conversation
This adds a decoder for ARP. Ticket OISF#6827
This change exposes 'JSONFormatAndAddMACAddr' as a public function, allowing it to be reused across modules, such as the ARP logger, for logging MAC addresses extracted from ARP packets.
This commit enhances the JSON output by introducing a feature for conditional port logging. Now, port logging is dependent on the underlying protocol (such as TCP, UDP, or SCTP), where port information is pertinent, while it avoids unnecessary logging for protocols where a port is not utilized (e.g. ARP). Furthermore, this update ensures that IP addresses and the protocol have meaningful values set, rather than being logged as empty strings. These changes will make each log entry more precise, eliminating cases where 5-tuple fields are empty or set to zero, indicating the absence of a field.
This adds a logger for ARP, disabled by default. Ticket OISF#6827
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #11060 +/- ##
==========================================
- Coverage 83.63% 83.62% -0.02%
==========================================
Files 922 925 +3
Lines 250321 250425 +104
==========================================
+ Hits 209349 209406 +57
- Misses 40972 41019 +47
Flags with carried forward coverage won't be shown. Click here to find out more. |
| return TM_ECODE_FAILED; | ||
|
|
||
| if (SCNtohs(arph->hw_type) != ARP_HW_TYPE_ETHERNET) { | ||
| ENGINE_SET_INVALID_EVENT(p, ARP_UNSUPPORTED_HARDWARE); |
There was a problem hiding this comment.
should call PacketClearL3(p); to "unset" the ARP header in the packet.
There was a problem hiding this comment.
same for other failure paths
| #ifndef SURICATA_DECODE_ARP_H | ||
| #define SURICATA_DECODE_ARP_H | ||
|
|
||
| #define ARP_HEADER_MIN_LEN 2 |
There was a problem hiding this comment.
should be 4? 2 times 16 bits are read (hw_type, proto_type) before more checks are done
| } | ||
|
|
||
| if (unlikely(len != ARP_HEADER_LEN)) { | ||
| ENGINE_SET_INVALID_EVENT(p, ARP_UNSUPPORTED_PKT); |
There was a problem hiding this comment.
if we get here, we know that packet is arp, hw type ethernet, proto type ip. So is a size mismatch here a "invalid" or an "unsupported" packet? I think invalid seems more appropriate?
There was a problem hiding this comment.
in fact, I would think < ARP_HEADER_LEN => invalid
and > ARP_HEADER_LEN "trailing data" or something?
In general, it might be good to validate the fixed size part of the header, so the first 8 bytes
uint16_t hw_type;
uint16_t proto_type;
uint8_t hw_size;
uint8_t proto_size;
uint16_t opcode;+ the sum of the hw_size and proto_size, and see if it matches the input. Shorter is bad/invalid+event, longer is suspicious/valid+event?
There was a problem hiding this comment.
if we get here, we know that packet is arp, hw type ethernet, proto type ip. So is a size mismatch here a "invalid" or an "unsupported" packet? I think invalid seems more appropriate?
Yes, I think invalid is more appropriate.
There was a problem hiding this comment.
in fact, I would think < ARP_HEADER_LEN => invalid and > ARP_HEADER_LEN "trailing data" or something?
In general, it might be good to validate the fixed size part of the header, so the first 8 bytes
uint16_t hw_type; uint16_t proto_type; uint8_t hw_size; uint8_t proto_size; uint16_t opcode;
+the sum of the hw_size and proto_size, and see if it matches the input. Shorter is bad/invalid+event, longer is suspicious/valid+event?
I've found a pcap where after the ARP pkt, 18 bytes are added as padding for Ethernet (which can be filtered with eth.padding). Here is how the packet looks:
ARP:
00 01 08 00 06 04 00 03 00 00 a1 12 dd 88 00 00
00 00 00 00 a1 12 dd 88 00 00 00 00
Ethernet padding:
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
00 00
To summarize, if the length is shorter than ARP_HEADER_LEN, it is invalid (and an event is set).
If it is longer, it is valid, but I wonder if we should set an event, as it can be more noisy than helpful if padding is often added at the end of the packet.
|
Replaced with #11085 |
Make sure these boxes are signed before submitting your Pull Request -- thank you.
https://docs.suricata.io/en/latest/devguide/contributing/contribution-process.html
https://suricata.io/about/contribution-agreement/ (note: this is only required once)
https://redmine.openinfosecfoundation.org/projects/suricata/issues
(if applicable)
Link to ticket: https://redmine.openinfosecfoundation.org/issues/
https://redmine.openinfosecfoundation.org/issues/6827
Describe changes:
SV_BRANCH=OISF/suricata-verify#1807