From nobody Sat Oct 02 16:03:31 2021 X-Original-To: freebsd-pf@mlmmj.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mlmmj.nyi.freebsd.org (Postfix) with ESMTP id 6F8C617E7F3D for ; Sat, 2 Oct 2021 16:03:35 +0000 (UTC) (envelope-from kp@FreeBSD.org) Received: from smtp.freebsd.org (smtp.freebsd.org [96.47.72.83]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (4096 bits) client-digest SHA256) (Client CN "smtp.freebsd.org", Issuer "R3" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4HMBbq2cy9z4gqN; Sat, 2 Oct 2021 16:03:35 +0000 (UTC) (envelope-from kp@FreeBSD.org) Received: from venus.codepro.be (venus.codepro.be [5.9.86.228]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client CN "mx1.codepro.be", Issuer "R3" (verified OK)) (Authenticated sender: kp) by smtp.freebsd.org (Postfix) with ESMTPSA id 0C7C12E917; Sat, 2 Oct 2021 16:03:35 +0000 (UTC) (envelope-from kp@FreeBSD.org) Received: by venus.codepro.be (Postfix, authenticated sender kp) id CDE9329B21; Sat, 2 Oct 2021 18:03:32 +0200 (CEST) From: Kristof Provost To: =?utf-8?q?=C3=96zkan?= KIRIK Cc: freebsd-pf@freebsd.org Subject: Re: pf label $nr macro expand reproducable bug Date: Sat, 02 Oct 2021 18:03:31 +0200 X-Mailer: MailMate (1.14r5818) Message-ID: <424C24F1-D9E2-494E-97C1-6ADC515869D0@FreeBSD.org> In-Reply-To: References: <90E32279-76C0-4D81-B209-BE85A181F874@FreeBSD.org> List-Id: Technical discussion and general questions about packet filter (pf) List-Archive: https://lists.freebsd.org/archives/freebsd-pf List-Help: List-Post: List-Subscribe: List-Unsubscribe: Sender: owner-freebsd-pf@freebsd.org X-BeenThere: freebsd-pf@freebsd.org MIME-Version: 1.0 Content-Type: multipart/alternative; boundary="=_MailMate_66B40783-9029-42AE-B58D-DC806AF51760_=" X-Spam: Yes X-ThisMailContainsUnwantedMimeParts: Y --=_MailMate_66B40783-9029-42AE-B58D-DC806AF51760_= Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: quoted-printable On 22 Sep 2021, at 11:47, =C3=96zkan KIRIK wrote: > Hi Kristof, > > I tried many things and I found the real problem to reproduce the bug. > Tested with the latest stable/12. > And also tested with Live CD without installing > (https://download.freebsd.org/ftp/snapshots/ISO-IMAGES/12.2/FreeBSD-12.= 2-STABLE-amd64-20210916-r370608-disc1.iso). > The result is same. > > My determination is the problem in the rule optimizer of pf. You can > see the difference with / without ruleset optimization. > Without ruleset optimization, $nr macro expanding is true. otherwise = > false. > > if the interface used in the rule, have multiple IP addresses that > rule optimizer removes lines then the rule number expanding fails. ie: > Yeah, that makes sense. It looks like the problem is that we expand the = $nr tag before the ruleset optimisation, so we end up using the wrong = rule number. As a first attempt this also seems to work: diff --git a/sbin/pfctl/parse.y b/sbin/pfctl/parse.y index 89d5f330da47..eb38fd9e344e 100644 --- a/sbin/pfctl/parse.y +++ b/sbin/pfctl/parse.y @@ -330,14 +330,12 @@ int filter_consistent(struct = pfctl_rule *, int); int nat_consistent(struct pfctl_rule *); int rdr_consistent(struct pfctl_rule *); int process_tabledef(char *, struct table_opts *); -void expand_label_str(char *, size_t, const char *, const = char *); void expand_label_if(const char *, char *, size_t, const = char *); void expand_label_addr(const char *, char *, size_t, = u_int8_t, struct node_host *); void expand_label_port(const char *, char *, size_t, struct node_port *); void expand_label_proto(const char *, char *, size_t, = u_int8_t); -void expand_label_nr(const char *, char *, size_t); void expand_label(char *, size_t, const char *, u_int8_t, struct node_host *, struct node_port *, struct = node_host *, struct node_port *, u_int8_t); @@ -5125,17 +5123,6 @@ expand_label_proto(const char *name, char = *label, size_t len, u_int8_t proto) } } -void -expand_label_nr(const char *name, char *label, size_t len) -{ - char n[11]; - - if (strstr(label, name) !=3D NULL) { - snprintf(n, sizeof(n), "%u", pf->anchor->match); - expand_label_str(label, len, name, n); - } -} - void expand_label(char *label, size_t len, const char *ifname, sa_family_t = af, struct node_host *src_host, struct node_port *src_port, @@ -5148,7 +5135,6 @@ expand_label(char *label, size_t len, const char = *ifname, sa_family_t af, expand_label_port("$srcport", label, len, src_port); expand_label_port("$dstport", label, len, dst_port); expand_label_proto("$proto", label, len, proto); - expand_label_nr("$nr", label, len); } int diff --git a/sbin/pfctl/pfctl.c b/sbin/pfctl/pfctl.c index d7bde0012e9b..749d73705f45 100644 --- a/sbin/pfctl/pfctl.c +++ b/sbin/pfctl/pfctl.c @@ -1528,6 +1528,15 @@ pfctl_load_ruleset(struct pfctl *pf, char *path, = struct pfctl_ruleset *rs, while ((r =3D TAILQ_FIRST(rs->rules[rs_num].active.ptr)) !=3D NU= LL) = { TAILQ_REMOVE(rs->rules[rs_num].active.ptr, r, entries); + char n[11]; + snprintf(n, sizeof(n), "%u", r->nr); + for (int i =3D 0; i < PF_RULE_MAX_LABEL_COUNT; i++) { + expand_label_str(r->label[i], = PF_RULE_LABEL_SIZE, + "$nr", n); + } + expand_label_str(r->tagname, PF_TAG_NAME_SIZE, "$nr", = n); + expand_label_str(r->match_tagname, PF_TAG_NAME_SIZE, = "$nr", n); + if ((error =3D pfctl_load_rule(pf, path, r, depth))) goto error; if (r->anchor) { diff --git a/sbin/pfctl/pfctl.h b/sbin/pfctl/pfctl.h index 80ef184fa90f..d725341d1cd7 100644 --- a/sbin/pfctl/pfctl.h +++ b/sbin/pfctl/pfctl.h @@ -139,5 +139,6 @@ struct pfctl_ruleset *pf_find_ruleset(const = char *); struct pfctl_ruleset *pf_find_or_create_ruleset(const char *); const char *pfctl_proto2name(int); +void expand_label_str(char *, size_t, const char *, const char *); #endif /* _PFCTL_H_ */ I think I want to restructure that a bit, but this should already work. Best regards, Kristof --=_MailMate_66B40783-9029-42AE-B58D-DC806AF51760_=--