Re: [regression] Completely editing packet should be allowed.

[Florian Westphal <fw@strlen.de>]

allfox wy <allfoxwy@gmail.com> wrote:
> I have a homebrew cipher program using libnetfilter_queue, which would
> intercept certain IP in IP packets via iptables' NFQUEUE target, edit
> them ( completely ) then put them back to OS for further processing.
> 
> Received packets would be redirected via RAW table ( which would
> happen before CONNTRACK ) , while sending packets would be redirected
> via MANGLE table POSTROUTING chain ( they are leaving the host, no
> longer need to be CONTRACKed ).
>
> The edit is to transform the entire inner packet with a cipher. By
> entire, I mean including the inner header hidden and the original
> length would change.
> The receiving host would decipher it back to normal IP in IP packet in
> RAW table.
> 
> This would form a transparent tunnel and had worked for years.
> 
> However recently I tried to switch from Linux 6.18 LTS to 7.4-rc1 ( in
> favour of tcp_roccet ), my homebrew stopped working.
> There is no error message anywhere, all libnetfilter_queue functions
> return success, I could see the packet, I could edit it, I could send
> the verdict. Just after my edit, the packet would fade into void
> somehow. Like the host never had it.
> 
> So I did a bisect, it comes to
> 112e447d17f78d08485b3076a8baa1dfb9794bd8 is the first bad commit:
> - netfilter: validate L4 headers after userspace packet writes.
>
> I don't really get the idea of the situation here:
> - Freestyle packet via libnetfilter_queue is no longer supported?

It has to be restricted.

> - What is the rule of validation? What can be edited and what can't be touched?

ATM: ip saddr/daddr can change, l4 port numbers can change.
No insertion of extension headers or add/removal of IP options.

Could be supported but requires recomputations of internal data in the
kernel.

> I still feel making a cipher should be a valid use case for
> libnetfilter_queue. ( Which would replace the entire inner packet of
> IP in IP encapsulation, so any validation on the inner content would
> fail )
> The whole point of libnetfilter_queue is giving userspace a way to
> inspect/edit the packet. When all function calls success and the
> userspace say "verdict, allow" then the system silently dropping the
> packet into void does not sound very logical to me.
> 
> So that
> - I vote for reverting 112e447d . As in my view, it would kill a valid
> use case of libnetfilter_queue and NFQUEUE target.

We really can't, no restrictions allow kernel crash.  But its likely
your use case can be made to work again.

Wild guess: you're changing the l4 protocol number and the postrouting
hook reinject fails because of proto mismatch wrt. what conntrack picked
up?

diff --git a/net/netfilter/nfnetlink_queue.c b/net/netfilter/nfnetlink_queue.c
index 90d887546f7d..d2f83004e156 100644
--- a/net/netfilter/nfnetlink_queue.c
+++ b/net/netfilter/nfnetlink_queue.c
@@ -1229,10 +1229,8 @@ static bool nfqnl_validate_l4(const u8 *data, unsigned int data_len,
        const struct nf_conn *ct;

        ct = nf_ct_get(e->skb, &ctinfo);
-       if (ct && !nf_ct_is_template(ct)) {
-               if (fragment || nf_ct_protonum(ct) != proto)
-                       return false;
-       }
+       if (ct && nf_ct_protonum(ct) != proto)
+               nf_reset_ct(e->skb);
 #endif

If that doesn't help, can you either see if you can figure out where the
validation fails, or, alternatively, describe what header modifications
your userspace program is doing?