Hello,
[ cc'ing also tech@ ]
On Mon, Feb 06, 2023 at 06:44:38PM +0300, [email protected] 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
>
> [email protected]:/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;
