NetBSD Problem Report #58543

From www@netbsd.org  Fri Aug  2 23:06:24 2024
Return-Path: <www@netbsd.org>
Received: from mail.netbsd.org (mail.netbsd.org [199.233.217.200])
	(using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)
	 key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256
	 client-signature RSA-PSS (2048 bits) client-digest SHA256)
	(Client CN "mail.NetBSD.org", Issuer "mail.NetBSD.org CA" (not verified))
	by mollari.NetBSD.org (Postfix) with ESMTPS id 5031F1A923C
	for <gnats-bugs@gnats.NetBSD.org>; Fri,  2 Aug 2024 23:06:24 +0000 (UTC)
Message-Id: <20240802230623.01FF81A923E@mollari.NetBSD.org>
Date: Fri,  2 Aug 2024 23:06:22 +0000 (UTC)
From: tnn@nygren.pp.se
Reply-To: tnn@nygren.pp.se
To: gnats-bugs@NetBSD.org
Subject: NPF rule with multiple addresses in "from" disregards source address constraint
X-Send-Pr-Version: www-1.0

>Number:         58543
>Category:       kern
>Synopsis:       NPF rule with multiple addresses in "from" disregards source address constraint
>Confidential:   no
>Severity:       critical
>Priority:       high
>Responsible:    kern-bug-people
>State:          closed
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Fri Aug 02 23:10:00 +0000 2024
>Closed-Date:    Fri Nov 01 15:14:57 +0000 2024
>Last-Modified:  Fri Nov 01 15:14:57 +0000 2024
>Originator:     Tobias Nygren
>Release:        10.99.11
>Organization:
>Environment:
>Description:
Syntactically valid NPF rule does not behave as expected and disregards the configured source address constraint.

---8<---
procedure "log" {
        log: npflog0
}

group "loopback" on lo0 {
        pass in final family inet6 proto tcp from fd42:dead:beef::1 to any port 9000 apply "log"
        pass in final family inet6 proto tcp from {fd42:dead:beef::1} to any port 9001 apply "log"
        pass in final family inet6 proto tcp from {fd42:dead:beef::1, fd42:dead:beef::2} to any port 9002 apply "log"
        block in final family inet6 proto tcp to any port {9000, 9001, 9002} apply "log"
}
group default {
        pass final all
}
---8<---

# tcpdump -n -e -i npflog0 &

## ok packet is blocked
# nc -6 localhost 9000
22:59:56.707978 rule 5.rules.0/0(match): block in on lo0: ::1.65478 > ::1.9000: Flags [S], seq 3480881801, win 32768, options [mss 33564,nop,wscale 3,sackOK,TS val 1 ecr 0], length 0

## ok packet is blocked
root@storage:root> nc -6 localhost 9001
23:00:00.009014 rule 5.rules.0/0(match): block in on lo0: ::1.65477 > ::1.9001: Flags [S], seq 3653343324, win 32768, options [mss 33564,nop,wscale 3,sackOK,TS val 1 ecr 0], length 0

## NOT OK, packet allowed by rule 4 despite wrong source address
# nc -6 localhost 9002
23:00:01.362353 rule 4.rules.0/0(match): pass in on lo0: ::1.65476 > ::1.9002: Flags [S], seq 3710674527, win 32768, options [mss 33564,nop,wscale 3,sackOK,TS val 1 ecr 0], length 0

This is a security problem because it appears to allow any source address when the intent was to only allow two specific ones.

>How-To-Repeat:
See description.
>Fix:
To be investigated.

>Release-Note:

>Audit-Trail:
From: mlelstv@serpens.de (Michael van Elst)
To: gnats-bugs@netbsd.org
Cc: 
Subject: Re: kern/58543: NPF rule with multiple addresses in "from" disregards source address constraint
Date: Sat, 3 Aug 2024 05:34:38 -0000 (UTC)

 tnn@nygren.pp.se writes:

 >Syntactically valid NPF rule does not behave as expected and disregards the configured source address constraint.
 >        pass in final family inet6 proto tcp from {fd42:dead:beef::1, fd42:dead:beef::2} to any port 9002 apply "log"

 The compiler generates bad BPF code for IPv6 (see bin/55403).

 Quick workaround is to use a table instead of a list of addresses.

 I am using this patch that compiles working, but maybe not
 optimal BPF code. Please check if that helps in your case:

 Index: usr.sbin/npf/npfctl/npf_bpf_comp.c
 ===================================================================
 RCS file: /cvsroot/src/usr.sbin/npf/npfctl/npf_bpf_comp.c,v
 retrieving revision 1.16
 diff -p -u -r1.16 npf_bpf_comp.c
 --- usr.sbin/npf/npfctl/npf_bpf_comp.c	30 May 2020 14:16:56 -0000	1.16
 +++ usr.sbin/npf/npfctl/npf_bpf_comp.c	3 Aug 2024 05:31:50 -0000
 @@ -206,12 +206,22 @@ fixup_jumps(npf_bpf_t *ctx, u_int start,
  		 * Fixup the "magic" value.  Swap only the "magic" jumps.
  		 */

 -		if (insn->jt == JUMP_MAGIC) {
 -			insn->jt = fail_off;
 +		if (insn->jf == JUMP_MAGIC) {
 +			if (swap && insn->jt) {
 +				insn->jf = 0;
 +			} else {
 +				insn->jf = fail_off;
 +				insn->jt = 0;
 +			}
  			seen_magic = true;
  		}
 -		if (insn->jf == JUMP_MAGIC) {
 -			insn->jf = fail_off;
 +		if (insn->jt == JUMP_MAGIC) {
 +			if (swap && insn->jf) {
 +				insn->jt = 0;
 +			} else {
 +				insn->jt = fail_off;
 +				insn->jf = 0;
 +			}
  			seen_magic = true;
  		}

 @@ -496,13 +506,14 @@ npfctl_bpf_proto(npf_bpf_t *ctx, unsigne
   * npfctl_bpf_cidr: code block to match IPv4 or IPv6 CIDR.
   *
   * => IP address shall be in the network byte order.
 + *
   */
  void
  npfctl_bpf_cidr(npf_bpf_t *ctx, unsigned opts, sa_family_t af,
      const npf_addr_t *addr, const npf_netmask_t mask)
  {
  	const uint32_t *awords = (const uint32_t *)addr;
 -	unsigned nwords, length, maxmask, off;
 +	unsigned nwords, length, maxmask, off, fail_off;

  	assert(((opts & MATCH_SRC) != 0) ^ ((opts & MATCH_DST) != 0));
  	assert((mask && mask <= NPF_MAX_NETMASK) || mask == NPF_NO_NETMASK);
 @@ -513,14 +524,12 @@ npfctl_bpf_cidr(npf_bpf_t *ctx, unsigned
  		off = (opts & MATCH_SRC) ?
  		    offsetof(struct ip, ip_src) :
  		    offsetof(struct ip, ip_dst);
 -		nwords = sizeof(struct in_addr) / sizeof(uint32_t);
  		break;
  	case AF_INET6:
  		maxmask = 128;
  		off = (opts & MATCH_SRC) ?
  		    offsetof(struct ip6_hdr, ip6_src) :
  		    offsetof(struct ip6_hdr, ip6_dst);
 -		nwords = sizeof(struct in6_addr) / sizeof(uint32_t);
  		break;
  	default:
  		abort();
 @@ -531,6 +540,14 @@ npfctl_bpf_cidr(npf_bpf_t *ctx, unsigned

  	length = (mask == NPF_NO_NETMASK) ? maxmask : mask;

 +	/* Compute number of words to check */
 +	nwords = howmany(length, 32);
 +	/* Compute offset to failure path */
 +	fail_off = length / 32 * 2 + (length % 32 ? 3 : 0) - 1;
 +
 +	if (nwords * 32 > maxmask)
 +		abort();
 +
  	/* CAUTION: BPF operates in host byte-order. */
  	for (unsigned i = 0; i < nwords; i++) {
  		const unsigned woff = i * sizeof(uint32_t);
 @@ -545,8 +562,8 @@ npfctl_bpf_cidr(npf_bpf_t *ctx, unsigned
  			wordmask = 0xffffffff << (32 - length);
  			length = 0;
  		} else {
 -			/* The mask became zero - skip the rest. */
 -			break;
 +			/* The mask became zero - this cannot happen */
 +			abort();
  		}

  		/* A <- IP address (or one word of it) */
 @@ -554,6 +571,7 @@ npfctl_bpf_cidr(npf_bpf_t *ctx, unsigned
  			BPF_STMT(BPF_LD+BPF_W+BPF_ABS, off + woff),
  		};
  		add_insns(ctx, insns_ip, __arraycount(insns_ip));
 +		fail_off--;

  		/* A <- (A & MASK) */
  		if (wordmask) {
 @@ -561,13 +579,17 @@ npfctl_bpf_cidr(npf_bpf_t *ctx, unsigned
  				BPF_STMT(BPF_ALU+BPF_AND+BPF_K, wordmask),
  			};
  			add_insns(ctx, insns_mask, __arraycount(insns_mask));
 +			fail_off--;
  		}

  		/* A == expected-IP-word ? */
  		struct bpf_insn insns_cmp[] = {
  			BPF_JUMP(BPF_JMP+BPF_JEQ+BPF_K, word, 0, JUMP_MAGIC),
  		};
 +		if (i < nwords-1)
 +			insns_cmp[0].jt = fail_off;
  		add_insns(ctx, insns_cmp, __arraycount(insns_cmp));
 +		fail_off--;
  	}

  	uint32_t mwords[] = {

From: Tobias Nygren <tnn@nygren.pp.se>
To: mlelstv@serpens.de (Michael van Elst)
Cc: gnats-bugs@netbsd.org
Subject: Re: kern/58543: NPF rule with multiple addresses in "from"
 disregards source address constraint
Date: Sat, 3 Aug 2024 13:10:09 +0200

 On Sat,  3 Aug 2024 05:40:01 +0000 (UTC)
 mlelstv@serpens.de (Michael van Elst) wrote:

 >  The compiler generates bad BPF code for IPv6 (see bin/55403).
 >  
 >  Quick workaround is to use a table instead of a list of addresses.
 >  
 >  I am using this patch that compiles working, but maybe not
 >  optimal BPF code. Please check if that helps in your case:

 Thanks, the patch seems to work OK on my router box. Other than
 exposing that my full ruleset is incomplete and relies on the broken
 behaviour. Which is a good thing to discover.
 Should I close this PR as duplicate?
 Optimal or not, please ask tech-net@ for review and commit your patch
 with pullups sooner rather than later. Common npf usage patterns such as

 $ext_v6 = inet6(wm0)
 pass stateful out final family inet6 proto tcp from $ext_v6 to any

 will actually expand to a set containing at least two addresses in
 almost all cases, because of the link local scope. Which then risks
 exposing the unsuspecting user's internal services to the Internet.

State-Changed-From-To: open->closed
State-Changed-By: tnn@NetBSD.org
State-Changed-When: Fri, 01 Nov 2024 15:14:57 +0000
State-Changed-Why:
Duplicate of 55403


>Unformatted:

NetBSD Home
NetBSD PR Database Search

(Contact us) $NetBSD: query-full-pr,v 1.47 2022/09/11 19:34:41 kim Exp $
$NetBSD: gnats_config.sh,v 1.9 2014/08/02 14:16:04 spz Exp $
Copyright © 1994-2024 The NetBSD Foundation, Inc. ALL RIGHTS RESERVED.