[RFC PATCH 0/1] libnftnl: Incorrect res_id byte order?

[RFC PATCH 0/1] libnftnl: Incorrect res_id byte order?

[Ian Pilcher]

I am marking this as an RFC and including this cover letter, because
I'm not absolutely 100% sure that the current code is incorrect.  (It
obviously works, or this would have come up before.)

AFAICT, the res_id of struct nfgenmsg is supposed to be in network byte
order, and that isn't happening on little endian systems.
nftnl_batch_begin() and nftnl_batch_end() both pass NFNL_SUBSYS_NFTABLES
(10) to __nftnl_nlmsg_build_hdr(), but without a call to htons(), the
message contains 2560 (when interpretted in network byte order).

Ian Pilcher (1):
  libnftnl: Fix res_id byte order

 src/common.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)


base-commit: 461f36979f4ed2b6cc95f06cf5f9c3c84bdf9e70
-- 
2.37.3

[RFC PATCH 1/1] libnftnl: Fix res_id byte order

[Ian Pilcher]

The res_id member of struct nfgenmsg is supposed to be in network
byte order (big endian).  Call htons() in __nftnl_nlmsg_build_hdr()
to ensure that this is true on little endian systems.

Signed-off-by: Ian Pilcher <arequipeno@gmail.com>
---
 src/common.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/common.c b/src/common.c
index 2d83c12..08572c3 100644
--- a/src/common.c
+++ b/src/common.c
@@ -10,6 +10,7 @@
 #include <stdlib.h>
 #include <sys/socket.h>
 #include <time.h>
+#include <arpa/inet.h>
 #include <linux/netlink.h>
 #include <linux/netfilter/nfnetlink.h>
 #include <linux/netfilter/nf_tables.h>
@@ -37,7 +38,7 @@ static struct nlmsghdr *__nftnl_nlmsg_build_hdr(char *buf, uint16_t type,
 	nfh = mnl_nlmsg_put_extra_header(nlh, sizeof(struct nfgenmsg));
 	nfh->nfgen_family = family;
 	nfh->version = NFNETLINK_V0;
-	nfh->res_id = res_id;
+	nfh->res_id = htons(res_id);
 
 	return nlh;
 }
-- 
2.37.3

Re: [RFC PATCH 1/1] libnftnl: Fix res_id byte order

[Pablo Neira Ayuso]

On Tue, Oct 18, 2022 at 11:45:28AM -0500, Ian Pilcher wrote:
> The res_id member of struct nfgenmsg is supposed to be in network
> byte order (big endian).  Call htons() in __nftnl_nlmsg_build_hdr()
> to ensure that this is true on little endian systems.

LGTM, this is zero all the time at this moment. But it might be useful
in the future to bump it.

Re: [RFC PATCH 1/1] libnftnl: Fix res_id byte order

[Ian Pilcher]

On 10/19/22 01:44, Pablo Neira Ayuso wrote:
> On Tue, Oct 18, 2022 at 11:45:28AM -0500, Ian Pilcher wrote:
>> The res_id member of struct nfgenmsg is supposed to be in network
>> byte order (big endian).  Call htons() in __nftnl_nlmsg_build_hdr()
>> to ensure that this is true on little endian systems.
> 
> LGTM, this is zero all the time at this moment. But it might be useful
> in the future to bump it.

Actually it isn't always zero.  I only noticed this because
nftnl_batch_begin() and nftnl_batch_end() set res_id to
NFNL_SUBSYS_NFTABLES (instead of putting it in the high 8 bits of
nlmsg_type).

It's entirely possible that this is also a bug, as the fact that the
value isn't currently being byte-swapped doesn't seem to make any
difference.

-- 
========================================================================
Google                                      Where SkyNet meets Idiocracy
========================================================================

Re: [RFC PATCH 1/1] libnftnl: Fix res_id byte order

[Pablo Neira Ayuso]

On Thu, Oct 20, 2022 at 10:05:22AM -0500, Ian Pilcher wrote:
> On 10/19/22 01:44, Pablo Neira Ayuso wrote:
> > On Tue, Oct 18, 2022 at 11:45:28AM -0500, Ian Pilcher wrote:
> > > The res_id member of struct nfgenmsg is supposed to be in network
> > > byte order (big endian).  Call htons() in __nftnl_nlmsg_build_hdr()
> > > to ensure that this is true on little endian systems.
> >
> > LGTM, this is zero all the time at this moment. But it might be useful
> > in the future to bump it.
>
> Actually it isn't always zero.  I only noticed this because
> nftnl_batch_begin() and nftnl_batch_end() set res_id to
> NFNL_SUBSYS_NFTABLES (instead of putting it in the high 8 bits of
> nlmsg_type).

Indeed, nfnetlink batch uses this for begin and end message.

> It's entirely possible that this is also a bug, as the fact that the
> value isn't currently being byte-swapped doesn't seem to make any
> difference.

There is code to workaround this issue in the kernel, it was added in 4.3.

commit a9de9777d613500b089a7416f936bf3ae5f070d2
Author: Pablo Neira Ayuso <pablo@netfilter.org>
Date:   Fri Aug 28 21:01:43 2015 +0200

    netfilter: nfnetlink: work around wrong endianess in res_id field

oldest stable kernel is 4.9.

Re: [RFC PATCH 1/1] libnftnl: Fix res_id byte order

[Pablo Neira Ayuso]

On Tue, Oct 18, 2022 at 11:45:28AM -0500, Ian Pilcher wrote:
> The res_id member of struct nfgenmsg is supposed to be in network
> byte order (big endian).  Call htons() in __nftnl_nlmsg_build_hdr()
> to ensure that this is true on little endian systems.

Applied to libnftnl, thanks.