Date: Mon, 11 Oct 2021 06:41:58 +0300 From: =?UTF-8?B?w5Z6a2FuIEtJUklL?= <ozkan.kirik@gmail.com> To: Kristof Provost <kp@freebsd.org> Cc: freebsd-pf@freebsd.org Subject: Re: pf label $nr macro expand reproducable bug Message-ID: <CAAcX-AFFmnWnuofp=P_KAaGrnQxtqSWFNrw4JpChYfZHVVqJOQ@mail.gmail.com> In-Reply-To: <CAAcX-AFUEfO_GzwEXoMH_HU61jLc0spBZm7-_%2BjSxn6KYvXi4w@mail.gmail.com> References: <CAAcX-AFmFwyEK4uzK66LoBK2e6W0_-8ZkJju5jAfSYE7wmSjXA@mail.gmail.com> <90E32279-76C0-4D81-B209-BE85A181F874@FreeBSD.org> <CAAcX-AF_S5WrU%2Bhy6WCzuot33%2Bp_LtP6_7HPTFNMSxoevqM35g@mail.gmail.com> <424C24F1-D9E2-494E-97C1-6ADC515869D0@FreeBSD.org> <CAAcX-AFUEfO_GzwEXoMH_HU61jLc0spBZm7-_%2BjSxn6KYvXi4w@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
Hello Kristof, I didn't have time to test the patch before. But the patch couldn't be applied successfully. # cd /usr/src # git branch main * stable/12 # git pull Already up to date. # patch < pf.patch Hmm... Looks like a unified diff to me... The text leading up to this was: -------------------------- |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 -------------------------- Patching file sbin/pfctl/parse.y using Plan A... Hunk #1 failed at 330. Hunk #2 failed at 5123. Hunk #3 failed at 5135. 3 out of 3 hunks failed--saving rejects to sbin/pfctl/parse.y.rej Hmm... The next patch looks like a unified diff to me... The text leading up to this was: -------------------------- |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 -------------------------- Patching file sbin/pfctl/pfctl.c using Plan A... Hunk #1 failed at 1528. 1 out of 1 hunks failed--saving rejects to sbin/pfctl/pfctl.c.rej Hmm... The next patch looks like a unified diff to me... The text leading up to this was: -------------------------- |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 -------------------------- Patching file sbin/pfctl/pfctl.h using Plan A... Hunk #1 succeeded at 139 with fuzz 1. done On Sat, Oct 2, 2021 at 8:31 PM =C3=96zkan KIRIK <ozkan.kirik@gmail.com> wro= te: > > Thank you! I'll try it. > > Have a nice day, > Best regards, > Ozkan > > On Sat, Oct 2, 2021 at 7:03 PM Kristof Provost <kp@freebsd.org> wrote: > > > > 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 fa= lse. > > > > 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 c= har *); > > void expand_label_if(const char *, char *, size_t, const ch= ar *); > > 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_int= 8_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 N= ULL) { > > 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_SIZ= E, > > + "$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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAAcX-AFFmnWnuofp=P_KAaGrnQxtqSWFNrw4JpChYfZHVVqJOQ>