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:          open
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Fri Aug 02 23:10:00 +0000 2024
>Last-Modified:  Sat Aug 03 11:15:01 +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.

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

Home	PR Database Search