Re: pf label $nr macro expand reproducable bug

From: Özkan KIRIK <ozkan.kirik_at_gmail.com>
Date: Mon, 11 Oct 2021 06:41:58 +0300
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 Özkan KIRIK <ozkan.kirik_at_gmail.com> wrote:
>
> 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_at_freebsd.org> wrote:
> >
> > On 22 Sep 2021, at 11:47, Özkan 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
> > _at__at_ -330,14 +330,12 _at__at_ 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);
> > _at__at_ -5125,17 +5123,6 _at__at_ 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) != 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,
> > _at__at_ -5148,7 +5135,6 _at__at_ 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
> > _at__at_ -1528,6 +1528,15 _at__at_ pfctl_load_ruleset(struct pfctl *pf, char *path, struct pfctl_ruleset *rs,
> >
> >         while ((r = TAILQ_FIRST(rs->rules[rs_num].active.ptr)) != NULL) {
> >                 TAILQ_REMOVE(rs->rules[rs_num].active.ptr, r, entries);
> > +               char n[11];
> > +               snprintf(n, sizeof(n), "%u", r->nr);
> > +               for (int i = 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 = 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
> > _at__at_ -139,5 +139,6 _at__at_ 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
Received on Mon Oct 11 2021 - 03:41:58 UTC

Original text of this message