From nobody Sat Oct 02 17:31:37 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 795C717EE33B for ; Sat, 2 Oct 2021 17:31:54 +0000 (UTC) (envelope-from ozkan.kirik@gmail.com) Received: from mail-vs1-xe34.google.com (mail-vs1-xe34.google.com [IPv6:2607:f8b0:4864:20::e34]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (2048 bits) client-digest SHA256) (Client CN "smtp.gmail.com", Issuer "GTS CA 1O1" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4HMDYk2hLyz4mVg; Sat, 2 Oct 2021 17:31:54 +0000 (UTC) (envelope-from ozkan.kirik@gmail.com) Received: by mail-vs1-xe34.google.com with SMTP id o124so14986689vsc.6; Sat, 02 Oct 2021 10:31:54 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc:content-transfer-encoding; bh=2myFgfvl1aJD1rhY3gadO2cAF/8agtjLtd8FgWrLQ+M=; b=mAS2+xdo+d6KcIFKJMOFkNIc/XMbYQtFwZEnj7NDLwYSN1NsVKoutaMMW9wwVHIKLh GWy84z9wJGAm4s4ohhRop4vBy83Dc7UmSGzBwh+FuenEzb3bnXhJAcexx/XYD/J3OP0C 6Swb1rqGWHt2piVGXOxrh4H5g68HlXz2nesvRolBQf7SXPFNSHd+aLNmSJ9UJeaRdVN+ q/mh+j8DIlDOM1NXzAeGu3PQNYyNetBeIz5SJfu5UfABnrFZZ8u3CXUDvnZJj1FFfZ13 zq8TFRjc6/35owHPm5InBtXeJugutFJ8Pf85HGQQL44H8xiqZ7XPe2JECGmLFIg3KTZh 01Jg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=2myFgfvl1aJD1rhY3gadO2cAF/8agtjLtd8FgWrLQ+M=; b=O8u35OBW5n7rwdY/26uKmZLl2aqPQAondvECrTeqgesm3hFhuuRaB5r5UXTokKqCpN EXhkwfhUeIgW6SUo+US/WyayKllkas257cdsbQ+BxL896lRnC54OPEMObbiBkYMkknnz AxNY/zMOSS2og9Wpx3BVwWd0Bemo44F0myiIDT5RQ3bDVjTScIsu6m+6BwFzNbFULSP/ 7+D4WqTT6G6xAvmceqQV9MCNHrBrR/J9ponhIqR3paKNRlUId4cH+ekZdH5M0PpXd9Kn 3YdlaqpHOmIWOFkvQ+h5u6ZfIrOdCNDn7pVMXMHbCneeJO5j2RuDyCvYZ1UvLUbQ2Gie PC7Q== X-Gm-Message-State: AOAM5318PbLakia9NaZm/IMYy8zp9FAFhyyUz53pCv3JyYJdgP+YWaWY zJeDxVCrz4pZ86YXiYW77PkCoRqVHIwL46REcoeTVHVS/Co= X-Google-Smtp-Source: ABdhPJzPMgz5OCO/U+NtjPFU8rijcvdMY6t/wjyigXV1xljF1VogSoLwESTZ5rhwqBd6OBrlZVLClToWV+3B4HqoUW8= X-Received: by 2002:a05:6102:317a:: with SMTP id l26mr8737323vsm.6.1633195908267; Sat, 02 Oct 2021 10:31:48 -0700 (PDT) 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 References: <90E32279-76C0-4D81-B209-BE85A181F874@FreeBSD.org> <424C24F1-D9E2-494E-97C1-6ADC515869D0@FreeBSD.org> In-Reply-To: <424C24F1-D9E2-494E-97C1-6ADC515869D0@FreeBSD.org> From: =?UTF-8?B?w5Z6a2FuIEtJUklL?= Date: Sat, 2 Oct 2021 20:31:37 +0300 Message-ID: Subject: Re: pf label $nr macro expand reproducable bug To: Kristof Provost Cc: freebsd-pf@freebsd.org Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspamd-Queue-Id: 4HMDYk2hLyz4mVg X-Spamd-Bar: ---- Authentication-Results: mx1.freebsd.org; none X-Spamd-Result: default: False [-4.00 / 15.00]; TAGGED_FROM(0.00)[]; REPLY(-4.00)[] X-ThisMailContainsUnwantedMimeParts: N Thank you! I'll try it. Have a nice day, Best regards, Ozkan On Sat, Oct 2, 2021 at 7:03 PM Kristof Provost 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 fals= e. > > 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 n= umber. > > 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_r= ule *, 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 cha= r *); > 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_h= ost *, > 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 *i= fname, 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, s= truct pfctl_ruleset *rs, > > while ((r =3D TAILQ_FIRST(rs->rules[rs_num].active.ptr)) !=3D NUL= L) { > 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 ch= ar *); > 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