RE: PATCH ulogd2 filter BASE ARP packet IP addresses

["Robert O'Brien" <robrien@foxtrot-research.com>]

[-- Attachment #1: Type: text/plain, Size: 955 bytes --]

I am developing for an embedded target and just recently deployed
libnetfilter and ulogd2 for logging packets which are rejected by rules in
ebtables. While performing this effort I discovered a bug which generates
incorrect values in the arp.saddr and arp.daddr fields in the OPRINT and
GPRINT outputs. I created a patch to resolve this issue in my deployment and
I believe it is a candidate for integration into the repository. The files
that this patch modifies have not changed in many years so I'm thinking that
the bug appeared due to changes in another codebase but I'm not sure. Please
review and provide feedback.

P.S. I could not find a way to submit a patch via Patchwork so I am writing
this email and attaching the patch. If there is a better way to submit a
patch, please tell me and I will re-submit it that way.

Robert O'Brien
Foxtrot Research
6201 Johns Road, Suite 3
Tampa, FL 33634
mailto:robrien@foxtrot-research.com - 813-501-7961


[-- Attachment #2: 0002-filter-BASE-Fixed-IP-addresses-in-ARP-packets.patch --]
[-- Type: application/octet-stream, Size: 2673 bytes --]

From b9820800820dcefadf912f16c009e506a87a91dd Mon Sep 17 00:00:00 2001
From: Robert O'Brien <robrien@foxtrot-research.com>
Date: Thu, 10 Nov 2022 10:53:52 -0500
Subject: [PATCH] filter: BASE: Fixed IP addresses in ARP packets

I noticed that the source and target IP addresses in the ARP header that
were printed by the GPRINT plugin were incorrect. I traced this down to
a type mismatch between the KEY_ARP_SPA and KEY_ARP_TPA keys (names
arp.saddr and arp.daddr) in ulogd_raw2packet_BASE.c:_interp_arp()
function and the ULOGD_RET_IPADDR key type. The _interp_arp function in
the BASE filter plugin was storing the ARP header IP addresses as a
pointer but all the plugins which use this key expect a u32 value.

I updated the _interp_arp() function to store the value using the
okey_set_u32() macro instead of *_ptr() and changed the cast to handle
the u8[] type that the value is stored as in struct
ether_arp.arp_spa/tpa. I have a feeling that at one point the type in
struct ether_arp.arp_spa/tpa changed from a u32 to a u8[] but I couldn't
find a commit to prove this.

I also updated the output plugin OPRINT as it was interpreting this
value in little endian when it should be big endian/network order.
---
 filter/raw2packet/ulogd_raw2packet_BASE.c | 4 ++--
 output/ulogd_output_OPRINT.c              | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/filter/raw2packet/ulogd_raw2packet_BASE.c b/filter/raw2packet/ulogd_raw2packet_BASE.c
index 9117d27..9210131 100644
--- a/filter/raw2packet/ulogd_raw2packet_BASE.c
+++ b/filter/raw2packet/ulogd_raw2packet_BASE.c
@@ -905,9 +905,9 @@ static int _interp_arp(struct ulogd_pluginstance *pi, uint32_t len)
 	okey_set_u16(&ret[KEY_ARP_OPCODE], ntohs(arph->arp_op));
 
 	okey_set_ptr(&ret[KEY_ARP_SHA], (void *)&arph->arp_sha);
-	okey_set_ptr(&ret[KEY_ARP_SPA], (void *)&arph->arp_spa);
+	okey_set_u32(&ret[KEY_ARP_SPA], *(uint32_t *)arph->arp_spa);
 	okey_set_ptr(&ret[KEY_ARP_THA], (void *)&arph->arp_tha);
-	okey_set_ptr(&ret[KEY_ARP_TPA], (void *)&arph->arp_tpa);
+	okey_set_u32(&ret[KEY_ARP_TPA], *(uint32_t *)arph->arp_tpa);
 
 	return ULOGD_IRET_OK;
 }
diff --git a/output/ulogd_output_OPRINT.c b/output/ulogd_output_OPRINT.c
index 6fde445..4850a76 100644
--- a/output/ulogd_output_OPRINT.c
+++ b/output/ulogd_output_OPRINT.c
@@ -85,7 +85,7 @@ static int oprint_interp(struct ulogd_pluginstance *upi)
 				break;
 			case ULOGD_RET_IPADDR:
 				fprintf(opi->of, "%u.%u.%u.%u\n", 
-					HIPQUAD(ret->u.value.ui32));
+					NIPQUAD(ret->u.value.ui32));
 				break;
 			case ULOGD_RET_NONE:
 				fprintf(opi->of, "<none>\n");
-- 
2.25.1