Re: [PATCH] netfilter: nft_socket: socket expressions for GID & UID

[Pablo Neira Ayuso <pablo@netfilter.org>]

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

On Wed, Apr 27, 2022 at 06:00:49PM +0300, Topi Miettinen wrote:
> On 27.4.2022 10.01, Pablo Neira Ayuso wrote:
> > On Wed, Apr 27, 2022 at 07:48:20AM +0200, Florian Westphal wrote:
> > > Topi Miettinen <toiwoton@gmail.com> wrote:
> > > > On 26.4.2022 1.34, Florian Westphal wrote:
> > > > > Topi Miettinen <toiwoton@gmail.com> wrote:
> > > > > > On 20.4.2022 21.54, Topi Miettinen wrote:
> > > > > > > Add socket expressions for checking GID or UID of the originating
> > > > > > > socket. These work also on input side, unlike meta skuid/skgid.
> > > > > > 
> > > > > > Unfortunately, there's a reproducible kernel BUG when closing a local
> > > > > > connection:
> > > > > > 
> > > > > > Apr 25 21:18:13 kernel:
> > > > > > ==================================================================
> > > > > > Apr 25 21:18:13 kernel: BUG: KASAN: null-ptr-deref in
> > > > > > nf_sk_lookup_slow_v6+0x45b/0x590 [nf_socket_ipv6]
> > > > > 
> > > > > You can pass this to scripts/faddr2line to get the location of the null deref.
> > > > 
> > > > Didn't work,
> > > 
> > > ?
> > > 
> > > You pass the object file and the nf_sk_lookup_slow_v6+0x45b/0x590 info.
> > > I can't do it for you because I lack the object file and the exact
> > > source code.
> > > 
> 
> $ faddr2line nf_socket_ipv6.ko nf_sk_lookup_slow_v6+0x45b/0x590
> bad symbol size: base: 0x0000000000000000 end: 0x0000000000000000
> $ faddr2line nf_socket_ipv6.o nf_sk_lookup_slow_v6+0x45b/0x590
> bad symbol size: base: 0x0000000000000000 end: 0x0000000000000000
> $ faddr2line nf_socket_ipv6.mod nf_sk_lookup_slow_v6+0x45b/0x590
> readelf: Error: nf_socket_ipv6.mod: Failed to read file header
> size: nf_socket_ipv6.mod: file format not recognized
> nm: nf_socket_ipv6.mod: file format not recognized
> size: nf_socket_ipv6.mod: file format not recognized
> nm: nf_socket_ipv6.mod: file format not recognized
> no match for nf_sk_lookup_slow_v6+0x45b/0x590
> $ faddr2line nf_socket_ipv6.mod.o nf_sk_lookup_slow_v6+0x45b/0x590
> no match for nf_sk_lookup_slow_v6+0x45b/0x590
> $ faddr2line vmlinux nf_sk_lookup_slow_v6+0x45b/0x590
> no match for nf_sk_lookup_slow_v6+0x45b/0x590
>
> > > > net/ipv6/netfilter/nf_socket_ipv6.c:
> > > > 
> > > > static struct sock *
> > > > nf_socket_get_sock_v6(struct net *net, struct sk_buff *skb, int doff,
> > > >                        const u8 protocol,
> > > >                        const struct in6_addr *saddr, const struct in6_addr
> > > > *daddr,
> > > >                        const __be16 sport, const __be16 dport,
> > > >                        const struct net_device *in)
> > > > {
> > > >          switch (protocol) {
> > > >          case IPPROTO_TCP:
> > > >                  return inet6_lookup(net, &tcp_hashinfo, skb, doff,
> > > >                                      saddr, sport, daddr, dport,
> > > >                                      in->ifindex);
> > > 
> > > What does that rule look like?  Seems like no input interface is
> > > available, seems like a bug in existing code?
> > 
> > nft_socket_eval() assumes it always run from input path.
> > 
> > @Topi: How does you test ruleset look like?
> 
> Here's a reproducer:
> #!/usr/sbin/nft -f
> 
> table inet mangle {
>         chain output {
>                 type route hook output priority mangle; policy accept;
> 
>                 socket uid != 0 reject with icmpx type admin-prohibited
>         }
> }
> 
> Start nc -6l 1 as root
> 
> Try 'telnet ::1 1' as root, press enter and close the connection. After 1-3
> tries, system hangs and Caps Lock starts blinking.

Looks like skb->sk is NULL? Patch attached.

[-- Attachment #2: fix-nft-socket.patch --]
[-- Type: text/x-diff, Size: 2057 bytes --]

diff --git a/net/netfilter/nft_socket.c b/net/netfilter/nft_socket.c
index 6d9e8e0a3a7d..d6da68a3b739 100644
--- a/net/netfilter/nft_socket.c
+++ b/net/netfilter/nft_socket.c
@@ -59,21 +59,27 @@ static void nft_socket_eval(const struct nft_expr *expr,
 			    const struct nft_pktinfo *pkt)
 {
 	const struct nft_socket *priv = nft_expr_priv(expr);
+	u32 *dest = &regs->data[priv->dreg];
 	struct sk_buff *skb = pkt->skb;
+	const struct net_device *dev;
 	struct sock *sk = skb->sk;
-	u32 *dest = &regs->data[priv->dreg];
 
 	if (sk && !net_eq(nft_net(pkt), sock_net(sk)))
 		sk = NULL;
 
-	if (!sk)
+	if (nft_hook(pkt) == NF_INET_LOCAL_OUT)
+		dev = nft_out(pkt);
+	else
+		dev = nft_in(pkt);
+
+	if (!sk) {
 		switch(nft_pf(pkt)) {
 		case NFPROTO_IPV4:
-			sk = nf_sk_lookup_slow_v4(nft_net(pkt), skb, nft_in(pkt));
+			sk = nf_sk_lookup_slow_v4(nft_net(pkt), skb, dev);
 			break;
 #if IS_ENABLED(CONFIG_NF_TABLES_IPV6)
 		case NFPROTO_IPV6:
-			sk = nf_sk_lookup_slow_v6(nft_net(pkt), skb, nft_in(pkt));
+			sk = nf_sk_lookup_slow_v6(nft_net(pkt), skb, dev);
 			break;
 #endif
 		default:
@@ -81,6 +87,7 @@ static void nft_socket_eval(const struct nft_expr *expr,
 			regs->verdict.code = NFT_BREAK;
 			return;
 		}
+	}
 
 	if (!sk) {
 		regs->verdict.code = NFT_BREAK;
@@ -184,6 +191,15 @@ static int nft_socket_init(const struct nft_ctx *ctx,
 					NULL, NFT_DATA_VALUE, len);
 }
 
+static int nft_socket_validate(const struct nft_ctx *ctx,
+			       const struct nft_expr *expr,
+			       const struct nft_data **data)
+{
+	return nft_chain_validate_hooks(ctx->chain, (1 << NF_INET_PRE_ROUTING) |
+						    (1 << NF_INET_LOCAL_IN) |
+						    (1 << NF_INET_LOCAL_OUT));
+}
+
 static int nft_socket_dump(struct sk_buff *skb,
 			   const struct nft_expr *expr)
 {
@@ -230,6 +246,7 @@ static const struct nft_expr_ops nft_socket_ops = {
 	.size		= NFT_EXPR_SIZE(sizeof(struct nft_socket)),
 	.eval		= nft_socket_eval,
 	.init		= nft_socket_init,
+	.validate	= nft_socket_validate,
 	.dump		= nft_socket_dump,
 	.reduce		= nft_socket_reduce,
 };