bin/59279: npfctl(8): missing null checks in variable lookups

[campbell+netbsd%mumble.net@localhost]

>Number:         59279
>Category:       bin
>Synopsis:       npfctl(8): missing null checks in variable lookups
>Confidential:   no
>Severity:       serious
>Priority:       medium
>Responsible:    bin-bug-people
>State:          open
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Fri Apr 11 14:00: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