Hello, [ cc'ing also tech@ ] On Mon, Feb 06, 2023 at 06:44:38PM +0300, r...@bh0.amt.ru wrote: > >Synopsis: pf.conf bug > >Category: system > >Environment: > System : OpenBSD 7.2 > Details : OpenBSD 7.2 (GENERIC.MP) #6: Sat Jan 21 01:03:04 MST 2023 > > r...@syspatch-72-amd64.openbsd.org:/usr/src/sys/arch/amd64/compile/GENERIC.MP > > Architecture: OpenBSD.amd64 > Machine : amd64 > >Description: > The pfctl utility incorrectly configures the PF device from > a pf.conf file. The output below shows that instead of loading > the rule > "block drop in quick inet6 proto ipv6-icmp all icmp6-type 255" > the pfctl loads the rule > "block drop in quick inet6 proto ipv6-icmp all". > >How-To-Repeat: > bgps# cat ./pf.conf.test > block in quick inet6 proto icmp6 all icmp6-type {0, 127, 255} > pass quick inet6 proto icmp6 all no state > pass all > bgps# pfctl -f ./pf.conf.test > bgps# pfctl -s rules > block drop in quick inet6 proto ipv6-icmp all icmp6-type 0 > block drop in quick inet6 proto ipv6-icmp all icmp6-type 127 > block drop in quick inet6 proto ipv6-icmp all > pass quick inet6 proto ipv6-icmp all no state > pass all flags S/SA
patch below fixes the issue. We need to change a type for icmp*type and icmp*code from u_int8_t to u_int16_t to make code correct. the thing is that legit values are <0, 256> at the moment. The '0' has a special meaning matching 'any' icmp type/code. Snippet below comes from parse.y which shows how logic works: 3198 icmptype : STRING { 3199 const struct icmptypeent *p; 3200 3201 if ((p = geticmptypebyname($1, AF_INET)) == NULL) { 3202 yyerror("unknown icmp-type %s", $1); 3203 free($1); 3204 YYERROR; 3205 } 3206 $$ = p->type + 1; 3207 free($1); 3208 } 3209 | NUMBER 3210 if ($1 < 0 || $1 > 255) { 3211 yyerror("illegal icmp-type %lld", $1); 3212 YYERROR; 3213 } 3214 $$ = $1 + 1; 3215 } 3216 ; 3217 3218 icmp6type : STRING { if we want to allow firewall administrator to specify a match on icmptype 255 then extending type from uint8_t to uint16_t is the right change. another option is to change logic here to allow matching icmptype in range <0, 254> either way is fine for me. however my preference is leaning towards a type change. note diff also rops pad[2] member to compensate for change of uint8_t to uin16_t for type, code members. I'm not sure about this bit hence I'd like to bring it up here. thanks and regards sashan --------8<---------------8<---------------8<------------------8<-------- diff --git a/sbin/pfctl/parse.y b/sbin/pfctl/parse.y index 2c5a49772ff..898bded8c24 100644 --- a/sbin/pfctl/parse.y +++ b/sbin/pfctl/parse.y @@ -134,8 +134,8 @@ struct node_gid { }; struct node_icmp { - u_int8_t code; - u_int8_t type; + u_int16_t code; /* aux. value 256 is legit */ + u_int16_t type; /* aux. value 256 is legit */ u_int8_t proto; struct node_icmp *next; struct node_icmp *tail; diff --git a/sys/net/pfvar.h b/sys/net/pfvar.h index 1453bc35c04..1efb1b5221c 100644 --- a/sys/net/pfvar.h +++ b/sys/net/pfvar.h @@ -572,8 +572,8 @@ struct pf_rule { u_int8_t keep_state; sa_family_t af; u_int8_t proto; - u_int8_t type; - u_int8_t code; + u_int16_t type; /* aux. value 256 is legit */ + u_int16_t code; /* aux. value 256 is legit */ u_int8_t flags; u_int8_t flagset; u_int8_t min_ttl; @@ -592,7 +592,6 @@ struct pf_rule { u_int8_t set_prio[2]; sa_family_t naf; u_int8_t rcvifnot; - u_int8_t pad[2]; struct { struct pf_addr addr;