RE: PATCH ulogd2 filter BASE ARP packet IP addresses

RE: PATCH ulogd2 filter BASE ARP packet IP addresses

[Robert O'Brien]

[-- 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

Re: PATCH ulogd2 filter BASE ARP packet IP addresses

[Pablo Neira Ayuso]

On Thu, Nov 10, 2022 at 01:28:53PM -0500, Robert O'Brien wrote:
> 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.

Could you post an example ulogd configuration file to reproduce the
issue?

> 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.

For patches to show up in patchwork, you have to use the git
format-patch and git send-email tools.

Thanks.

RE: PATCH ulogd2 filter BASE ARP packet IP addresses

[Robert O'Brien]

I will create a bug report and attach an example ulogd configuration file that demonstrates the issue.

I will send a patch using git send-email and mention it in my bug report. What is the email address to where I should send the patch?

-----Original Message-----
From: Pablo Neira Ayuso <pablo@netfilter.org> 
Sent: Friday, November 11, 2022 6:09 AM
To: Robert O'Brien <robrien@foxtrot-research.com>
Cc: netfilter-devel@vger.kernel.org
Subject: Re: PATCH ulogd2 filter BASE ARP packet IP addresses

On Thu, Nov 10, 2022 at 01:28:53PM -0500, Robert O'Brien wrote:
> 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.

Could you post an example ulogd configuration file to reproduce the issue?

> 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.

For patches to show up in patchwork, you have to use the git format-patch and git send-email tools.

Thanks.

Re: PATCH ulogd2 filter BASE ARP packet IP addresses

[Pablo Neira Ayuso]

On Mon, Nov 14, 2022 at 12:47:52PM -0500, Robert O'Brien wrote:
> I will create a bug report and attach an example ulogd configuration
> file that demonstrates the issue.

OK.

> I will send a patch using git send-email and mention it in my bug
> report. What is the email address to where I should send the patch?

Please use netfilter-devel@vger.kernel.org so patchwork catches this
patch.

Thanks.

Re: PATCH ulogd2 filter BASE ARP packet IP addresses

[Jeremy Sowden]

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

On 2022-11-14, at 19:12:58 +0100, Pablo Neira Ayuso wrote:
> On Mon, Nov 14, 2022 at 12:47:52PM -0500, Robert O'Brien wrote:
> > I will create a bug report and attach an example ulogd configuration
> > file that demonstrates the issue.
>
> OK.
>
> > I will send a patch using git send-email and mention it in my bug
> > report. What is the email address to where I should send the patch?
>
> Please use netfilter-devel@vger.kernel.org so patchwork catches this
> patch.

I think there are at least a couple more places where IP addresses are not
correctly handled:

  https://git.netfilter.org/ulogd2/tree/output/sqlite3/ulogd_output_SQLITE3.c?h=ulogd-2.0.8&id=79aa980f2df9dda0c097e8f883a62f414b9e5138#n176
  https://git.netfilter.org/ulogd2/tree/filter/raw2packet/ulogd_raw2packet_BASE.c?h=ulogd-2.0.8&id=79aa980f2df9dda0c097e8f883a62f414b9e5138#n647

I'll see if I can find any others.

J.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: PATCH ulogd2 filter BASE ARP packet IP addresses

[Jeremy Sowden]

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

On 2022-11-21, at 22:52:18 +0000, Jeremy Sowden wrote:
> On 2022-11-14, at 19:12:58 +0100, Pablo Neira Ayuso wrote:
> > On Mon, Nov 14, 2022 at 12:47:52PM -0500, Robert O'Brien wrote:
> > > I will create a bug report and attach an example ulogd
> > > configuration file that demonstrates the issue.
> >
> > OK.
> >
> > > I will send a patch using git send-email and mention it in my bug
> > > report. What is the email address to where I should send the
> > > patch?
> >
> > Please use netfilter-devel@vger.kernel.org so patchwork catches this
> > patch.
>
> I think there are at least a couple more places where IP addresses are
> not correctly handled:
>
>   https://git.netfilter.org/ulogd2/tree/output/sqlite3/ulogd_output_SQLITE3.c?h=ulogd-2.0.8&id=79aa980f2df9dda0c097e8f883a62f414b9e5138#n176
>   https://git.netfilter.org/ulogd2/tree/filter/raw2packet/ulogd_raw2packet_BASE.c?h=ulogd-2.0.8&id=79aa980f2df9dda0c097e8f883a62f414b9e5138#n647
>
> I'll see if I can find any others.

Didn't find any other endianness bugs, but there are assumptions in some
of the output plug-ins that all `ULOGD_RET_IPADDR` values are IPv4
addresses that can be treated as uint32_t values, which is not valid.
One possibility would be to handle all IP addresses internally as in
IPv6 format (i.e., convert IPv4 addresses to `::ffff:w.x.y.z` -- this is
what the IP2BIN filter does) and then convert the IPv4 addresses back on
output.

J.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]