NetBSD Problem Report #59279

From www@netbsd.org  Fri Apr 11 13:57:45 2025
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)
	 client-signature RSA-PSS (2048 bits))
	(Client CN "mail.NetBSD.org", Issuer "mail.NetBSD.org CA" (not verified))
	by mollari.NetBSD.org (Postfix) with ESMTPS id 04A631A9239
	for <gnats-bugs@gnats.NetBSD.org>; Fri, 11 Apr 2025 13:57:45 +0000 (UTC)
Message-Id: <20250411135743.A846C1A923E@mollari.NetBSD.org>
Date: Fri, 11 Apr 2025 13:57:43 +0000 (UTC)
From: campbell+netbsd@mumble.net
Reply-To: campbell+netbsd@mumble.net
To: gnats-bugs@NetBSD.org
Subject: npfctl(8): missing null checks in variable lookups
X-Send-Pr-Version: www-1.0

>Number:         59279
>Category:       bin
>Synopsis:       npfctl(8): missing null checks in variable lookups
>Confidential:   no
>Severity:       serious
>Priority:       medium
>Responsible:    joe
>State:          open
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Fri Apr 11 14:00:01 +0000 2025
>Last-Modified:  Fri Apr 11 14:45:01 +0000 2025
>Originator:     Taylor R Campbell
>Release:        current, 10, 9, ...
>Organization:
The NpfBSD Undefinedation
>Environment:
>Description:
npfvar_lookup returns null for undefined variables, but doesn't give feedback about where they happened.


### filt_addr_element

This case _does_ give feedback but it's not immediately obvious to a reader where -- it happens a couple dozen lines later:

    773 filt_addr_element
...
    784 	| VAR_ID
    785 	{
    786 		npfvar_t *vp = npfvar_lookup($1);
    787 		int type = npfvar_get_type(vp, 0);
    788 		ifnet_addr_t *ifna;
    789 again:
    790 		switch (type) {
...
    808 		case -1:
    809 			yyerror("undefined variable '%s'", $1);

https://nxr.netbsd.org/xref/src/usr.sbin/npf/npfctl/npf_parse.y?r=1.52#773


### port_range

In the syntax of port ranges, there is a null pointer dereference marked by => below:

    841 port_range
...
    850 	| VAR_ID
    851 	{
    852 		npfvar_t *vp = npfvar_lookup($1);
    853 		$$ = npfctl_parse_port_range_variable($1, vp);

https://nxr.netbsd.org/xref/src/usr.sbin/npf/npfctl/npf_parse.y?r=1.52#841

    285 npfvar_t *
    286 npfctl_parse_port_range_variable(const char *v, npfvar_t *vp)
    287 {
    288 	size_t count = npfvar_get_count(vp);
    289 	npfvar_t *pvp = npfvar_create();
    290 	port_range_t *pr;
    291 
    292 	for (size_t i = 0; i < count; i++) {
    293 		int type = npfvar_get_type(vp, i);
    294 		void *data = npfvar_get_data(vp, type, i);

https://nxr.netbsd.org/xref/src/usr.sbin/npf/npfctl/npf_data.c?r=1.30#285

    238 void *
    239 npfvar_get_data(const npfvar_t *vp, unsigned type, size_t idx)
    240 {
    241 	npf_element_t *el = npfvar_get_element(vp, idx, 0);
    242 
    243 	if (el && NPFVAR_TYPE(el->e_type) != NPFVAR_TYPE(type)) {
    244 		yyerror("variable '%s' element %zu "
    245 		    "is of type '%s' rather than '%s'", vp->v_key,
    246 		    idx, npfvar_type(el->e_type), npfvar_type(type));
    247 		return NULL;
    248 	}
 => 249 	return el->e_data;
    250 }

https://nxr.netbsd.org/xref/src/usr.sbin/npf/npfctl/npf_var.c?r=1.13#238


### icmp_type_and_code

Null pointer dereference marked by `=>':

    863 icmp_type_and_code
...
    877 	| ICMPTYPE icmp_type CODE VAR_ID
    878 	{
    879 		char *s = npfvar_expand_string(npfvar_lookup($4));

https://nxr.netbsd.org/xref/src/usr.sbin/npf/npfctl/npf_parse.y?r=1.52#863

    176 char *
    177 npfvar_expand_string(const npfvar_t *vp)
    178 {
    179 	if (npfvar_get_count(vp) != 1) {
 => 180 		yyerror("variable '%s' has multiple elements", vp->v_key);

https://nxr.netbsd.org/xref/src/usr.sbin/npf/npfctl/npf_var.c?r=1.13#176

Note that npfvar_get_count(NULL) returns 0:

    186 size_t
    187 npfvar_get_count(const npfvar_t *vp)
    188 {
    189 	return vp ? vp->v_count : 0;
    190 }

https://nxr.netbsd.org/xref/src/usr.sbin/npf/npfctl/npf_var.c?r=1.13#186

### icmp_type

Same deal as above:

    907 icmp_type
...
    910 	| VAR_ID
    911 	{
    912 		char *s = npfvar_expand_string(npfvar_lookup($1));

https://nxr.netbsd.org/xref/src/usr.sbin/npf/npfctl/npf_parse.y?r=1.52#907

### ifname

Looks fine but the npfvar_lookup call and the error report are again separated by dozens of lines:

    917 ifname
...
    923 	| VAR_ID
    924 	{
    925 		npfvar_t *vp = npfvar_lookup($1);
    926 		const int type = npfvar_get_type(vp, 0);
...
    954 		case -1:
    955 			yyerror("undefined variable '%s' for interface", $1);

https://nxr.netbsd.org/xref/src/usr.sbin/npf/npfctl/npf_parse.y?r=1.52#917
>How-To-Repeat:
write an npf.conf that exercises these cases with undefined variables
>Fix:
1. Add xfail test cases that exercise these issues.
2. Add null checks in appropriate places.

Part of (2) is done already:

Module Name:    src
Committed By:   joe
Date:           Thu Apr 10 18:53:29 UTC 2025

Modified Files:
        src/usr.sbin/npf/npfctl: npf_parse.y

Log Message:
notify us when we use undefined port variable


To generate a diff of this commit:
cvs rdiff -u -r1.52 -r1.53 src/usr.sbin/npf/npfctl/npf_parse.y

>Release-Note:

>Audit-Trail:

Responsible-Changed-From-To: bin-bug-people->joe
Responsible-Changed-By: riastradh@NetBSD.org
Responsible-Changed-When: Fri, 11 Apr 2025 14:06:32 +0000
Responsible-Changed-Why:
Can you please take a look into adding test cases for these?


From: Emmanuel Nyarko <emmankoko519@gmail.com>
To: gnats-bugs@netbsd.org
Cc: Emmanuel <joe@netbsd.org>, gnats-admin@netbsd.org,
 netbsd-bugs@netbsd.org, riastradh@netbsd.org, campbell+netbsd@mumble.net
Subject: Re: bin/59279 (npfctl(8): missing null checks in variable lookups)
Date: Fri, 11 Apr 2025 14:34:58 +0000

 --Apple-Mail-702ABF40-3BCE-446A-8A47-67B03E2C0A4C
 Content-Type: text/plain;
 	charset=utf-8
 Content-Transfer-Encoding: quoted-printable

 =EF=BB=BF

 > On 11 Apr 2025, at 2:06=E2=80=AFPM, riastradh@netbsd.org <riastradh@NetBSD=
 .org> wrote:
 >=20
 > Synopsis: npfctl(8): missing null checks in variable lookups
 >=20
 > Responsible-Changed-From-To: bin-bug-people->joe
 > Responsible-Changed-By: riastradh@NetBSD.org
 > Responsible-Changed-When: Fri, 11 Apr 2025 14:06:32 +0000
 > Responsible-Changed-Why:
 > Can you please take a look into adding test cases for these?


 Emmanuel






 --Apple-Mail-702ABF40-3BCE-446A-8A47-67B03E2C0A4C
 Content-Type: text/html;
 	charset=utf-8
 Content-Transfer-Encoding: quoted-printable

 <html><head><meta http-equiv=3D"content-type" content=3D"text/html; charset=3D=
 utf-8"></head><body dir=3D"auto"><div dir=3D"ltr">=EF=BB=BF<br id=3D"lineBre=
 akAtBeginningOfMessage"><div class=3D"AppleOriginalContents" style=3D"direct=
 ion: ltr;"><br><blockquote type=3D"cite"><div>On 11 Apr 2025, at 2:06=E2=80=AF=
 PM, riastradh@netbsd.org &lt;riastradh@NetBSD.org&gt; wrote:</div><br class=3D=
 "Apple-interchange-newline"><div><div>Synopsis: npfctl(8): missing null chec=
 ks in variable lookups<br><br>Responsible-Changed-From-To: bin-bug-people-&g=
 t;joe<br>Responsible-Changed-By: riastradh@NetBSD.org<br>Responsible-Changed=
 -When: Fri, 11 Apr 2025 14:06:32 +0000<br>Responsible-Changed-Why:<br>Can yo=
 u please take a look into adding test cases for these?<br><br><br><br></div>=
 </div></blockquote></div><br><div>
 <div dir=3D"auto" style=3D"font-family: Helvetica; font-size: 12px; font-sty=
 le: normal; font-variant-caps: normal; font-weight: 400; letter-spacing: nor=
 mal; text-align: start; text-indent: 0px; text-transform: none; white-space:=
  normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration:=
  none; caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0); overflow-wrap: break-=
 word; -webkit-nbsp-mode: space; line-break: after-white-space;"><div dir=3D"=
 auto" style=3D"caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0); letter-spacin=
 g: normal; text-align: start; text-indent: 0px; text-transform: none; white-=
 space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decor=
 ation: none; overflow-wrap: break-word; -webkit-nbsp-mode: space; line-break=
 : after-white-space;"><div class=3D"ApplePlainTextBody"><br class=3D"Apple-i=
 nterchange-newline">Emmanuel</div><div><br></div></div><br class=3D"Apple-in=
 terchange-newline"></div><br class=3D"Apple-interchange-newline" style=3D"ca=
 ret-color: rgb(0, 0, 0); color: rgb(0, 0, 0); font-family: Helvetica; font-s=
 ize: 12px; font-style: normal; font-variant-caps: normal; font-weight: 400; l=
 etter-spacing: normal; text-align: start; text-indent: 0px; text-transform: n=
 one; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px;=
  text-decoration: none;"><br class=3D"Apple-interchange-newline">
 </div>
 <br></div></body></html>=

 --Apple-Mail-702ABF40-3BCE-446A-8A47-67B03E2C0A4C--

From: Emmanuel Nyarko <emmankoko519@gmail.com>
To: gnats-bugs@netbsd.org
Cc: Emmanuel <joe@netbsd.org>,
 gnats-admin@netbsd.org,
 netbsd-bugs@netbsd.org,
 riastradh@netbsd.org,
 campbell+netbsd@mumble.net
Subject: Re: bin/59279 (npfctl(8): missing null checks in variable lookups)
Date: Fri, 11 Apr 2025 14:42:05 +0000

 --Apple-Mail=_B3230F00-ADBF-4C59-800C-AFE868154270
 Content-Transfer-Encoding: quoted-printable
 Content-Type: text/plain;
 	charset=utf-8

 Please ignore unintended reply.

 Alright I will look into it.

 > On 11 Apr 2025, at 2:34=E2=80=AFPM, Emmanuel Nyarko =
 <emmankoko519@gmail.com> wrote:
 >=20
 > =EF=BB=BF
 >=20
 >> On 11 Apr 2025, at 2:06=E2=80=AFPM, riastradh@netbsd.org =
 <riastradh@NetBSD.org> wrote:
 >>=20
 >> Synopsis: npfctl(8): missing null checks in variable lookups
 >>=20
 >> Responsible-Changed-From-To: bin-bug-people->joe
 >> Responsible-Changed-By: riastradh@NetBSD.org
 >> Responsible-Changed-When: Fri, 11 Apr 2025 14:06:32 +0000
 >> Responsible-Changed-Why:
 >> Can you please take a look into adding test cases for these?
 >>=20
 >>=20
 >>=20
 >=20
 >=20
 > Emmanuel
 >=20
 >=20
 >=20
 >=20
 >=20


 Emmanuel






 --Apple-Mail=_B3230F00-ADBF-4C59-800C-AFE868154270
 Content-Transfer-Encoding: quoted-printable
 Content-Type: text/html;
 	charset=utf-8

 <html><head><meta http-equiv=3D"content-type" content=3D"text/html; =
 charset=3Dutf-8"></head><body style=3D"overflow-wrap: break-word; =
 -webkit-nbsp-mode: space; line-break: after-white-space;">Please ignore =
 unintended reply.<div><br></div><div>Alright I will look into it.<br =
 id=3D"lineBreakAtBeginningOfMessage"><div><br><blockquote =
 type=3D"cite"><div>On 11 Apr 2025, at 2:34=E2=80=AFPM, Emmanuel Nyarko =
 &lt;emmankoko519@gmail.com&gt; wrote:</div><br =
 class=3D"Apple-interchange-newline"><div><meta http-equiv=3D"content-type"=
  content=3D"text/html; charset=3Dutf-8"><div dir=3D"auto"><div =
 dir=3D"ltr">=EF=BB=BF<br id=3D"lineBreakAtBeginningOfMessage"><div =
 style=3D"direction: ltr;"><br><blockquote type=3D"cite"><div>On 11 Apr =
 2025, at 2:06=E2=80=AFPM, riastradh@netbsd.org =
 &lt;riastradh@NetBSD.org&gt; wrote:</div><br =
 class=3D"Apple-interchange-newline"><div><div>Synopsis: npfctl(8): =
 missing null checks in variable =
 lookups<br><br>Responsible-Changed-From-To: =
 bin-bug-people-&gt;joe<br>Responsible-Changed-By: =
 riastradh@NetBSD.org<br>Responsible-Changed-When: Fri, 11 Apr 2025 =
 14:06:32 +0000<br>Responsible-Changed-Why:<br>Can you please take a look =
 into adding test cases for =
 these?<br><br><br><br></div></div></blockquote></div><br><div>
 <div dir=3D"auto" style=3D"font-family: Helvetica; font-size: 12px; =
 font-style: normal; font-variant-caps: normal; font-weight: 400; =
 letter-spacing: normal; text-align: start; text-indent: 0px; =
 text-transform: none; white-space: normal; word-spacing: 0px; =
 -webkit-text-stroke-width: 0px; text-decoration: none; caret-color: =
 rgb(0, 0, 0); overflow-wrap: break-word; -webkit-nbsp-mode: space; =
 line-break: after-white-space;"><div dir=3D"auto" style=3D"caret-color: =
 rgb(0, 0, 0); letter-spacing: normal; text-align: start; text-indent: =
 0px; text-transform: none; white-space: normal; word-spacing: 0px; =
 -webkit-text-stroke-width: 0px; text-decoration: none; overflow-wrap: =
 break-word; -webkit-nbsp-mode: space; line-break: =
 after-white-space;"><div><br =
 class=3D"Apple-interchange-newline">Emmanuel</div><div><br></div></div><br=
  class=3D"Apple-interchange-newline"></div><br =
 class=3D"Apple-interchange-newline" style=3D"caret-color: rgb(0, 0, 0); =
 font-family: Helvetica; font-size: 12px; font-style: normal; =
 font-variant-caps: normal; font-weight: 400; letter-spacing: normal; =
 text-align: start; text-indent: 0px; text-transform: none; white-space: =
 normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; =
 text-decoration: none;"><br class=3D"Apple-interchange-newline">
 </div>
 <br></div></div></div></blockquote></div><br><div>
 <div dir=3D"auto" style=3D"font-family: Helvetica; font-size: 12px; =
 font-style: normal; font-variant-caps: normal; font-weight: 400; =
 letter-spacing: normal; text-align: start; text-indent: 0px; =
 text-transform: none; white-space: normal; word-spacing: 0px; =
 -webkit-text-stroke-width: 0px; text-decoration: none; caret-color: =
 rgb(0, 0, 0); color: rgb(0, 0, 0); overflow-wrap: break-word; =
 -webkit-nbsp-mode: space; line-break: after-white-space;"><div =
 dir=3D"auto" style=3D"caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0); =
 letter-spacing: normal; text-align: start; text-indent: 0px; =
 text-transform: none; white-space: normal; word-spacing: 0px; =
 -webkit-text-stroke-width: 0px; text-decoration: none; overflow-wrap: =
 break-word; -webkit-nbsp-mode: space; line-break: =
 after-white-space;"><div><br =
 class=3D"Apple-interchange-newline">Emmanuel</div><div><br></div></div><br=
  class=3D"Apple-interchange-newline"></div><br =
 class=3D"Apple-interchange-newline" style=3D"caret-color: rgb(0, 0, 0); =
 color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; =
 font-style: normal; font-variant-caps: normal; font-weight: 400; =
 letter-spacing: normal; text-align: start; text-indent: 0px; =
 text-transform: none; white-space: normal; word-spacing: 0px; =
 -webkit-text-stroke-width: 0px; text-decoration: none;"><br =
 class=3D"Apple-interchange-newline">
 </div>
 <br></div></body></html>=

 --Apple-Mail=_B3230F00-ADBF-4C59-800C-AFE868154270--

>Unformatted:

Home	PR Database Search