From nobody Mon Oct 11 03:41:58 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 0774817E293D for ; Mon, 11 Oct 2021 03:42:17 +0000 (UTC) (envelope-from ozkan.kirik@gmail.com) Received: from mail-vk1-xa2d.google.com (mail-vk1-xa2d.google.com [IPv6:2607:f8b0:4864:20::a2d]) (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 4HSPkJ2VcRz4Vk7; Mon, 11 Oct 2021 03:42:16 +0000 (UTC) (envelope-from ozkan.kirik@gmail.com) Received: by mail-vk1-xa2d.google.com with SMTP id j38so4682391vkd.10; Sun, 10 Oct 2021 20:42:16 -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=6FIWY5CUd9aEo5M1Dvh97KT37gPJDEh8vLN8nR7Xmg0=; b=p8xIkhaP6DyLeLCK/BBhWN5s/d+w1p6+2pl+Xr7pEImmXt2cnLfndFuf9pYcSLALZ9 PMHt/5xOOr55nG1hlPwEY3n8/TYWQf149lvNk8++/vlXqBuZAzsWUfGSruPe38q9LGyg VztIkrRwHY4Lg4bucKbIMzPlde/EJrNi8Zxzpd9HkY+1kr0/Y2UElX84YCFWEEuDLy4U k0nMQKIX1D0qVmoHJTbQIUpjdYnDXBLbx1oCW+jxH+JusANE8zT4V0my6R/SjW3bnDj1 U4oIAMvz0FmbkSEA+bvaR08gRwZrN5uYiLMI2Q+jmLfZR+Myk2SxBIvW/jTLcq7Wf5RY HoAQ== 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=6FIWY5CUd9aEo5M1Dvh97KT37gPJDEh8vLN8nR7Xmg0=; b=LR4vOuUQFxXC586YuJNEh6w2f5cxQeumq0gIsKjoZwo+trqpmJIaFD8MHUkmo8PCO9 LEk085GA16YiGJmCuAO1MJ2ccNJFuDO+seyUhpNe4yXWkWqLNHJoa6U8Izfq9I2hUVJX ZMNp+c0nTH5rn08hZaYwabWQ11KlNdJjXVixtndEEjlXyVCkSxeceQuKlOk18E+ZAenG 1Rl9S8IbVJSoB87G9xlPbfBlFetjTLOuDx0FeOjXKvv6KUpWMVMmQTJ4Fh2qHQfL3k76 J9zYmzNP/T5KBar1RoZwlgRWbtkkXR28RzZABB+sa6jHbttfsZIpFoJSOUF84LNWcv3R nY3w== X-Gm-Message-State: AOAM5338QJwGlt+vRXZTuCFljsao9j7O3JnyWASf6mK/AFAHYKJLH1Px 7QkH5e2muDwjLMXvMwX1W8/srG5Rq6iohnaMpsj5OIzJiug= X-Google-Smtp-Source: ABdhPJwIpYDYxbpJGFtMn9+w/4WnKVAfOfaCZQH1tEbaDQWHiN8nwsOoW5ZNNq7i10YFmPo5pZz3s4WGV+uVMgBbxAY= X-Received: by 2002:a05:6122:20a7:: with SMTP id i39mr5953992vkd.15.1633923729852; Sun, 10 Oct 2021 20:42:09 -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: From: =?UTF-8?B?w5Z6a2FuIEtJUklL?= Date: Mon, 11 Oct 2021 06:41:58 +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: 4HSPkJ2VcRz4Vk7 X-Spamd-Bar: - Authentication-Results: mx1.freebsd.org; dkim=pass header.d=gmail.com header.s=20210112 header.b=p8xIkhaP; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (mx1.freebsd.org: domain of ozkankirik@gmail.com designates 2607:f8b0:4864:20::a2d as permitted sender) smtp.mailfrom=ozkankirik@gmail.com X-Spamd-Result: default: False [-1.02 / 15.00]; RCVD_TLS_ALL(0.00)[]; ARC_NA(0.00)[]; R_DKIM_ALLOW(-0.20)[gmail.com:s=20210112]; NEURAL_HAM_MEDIUM(-1.00)[-1.000]; FROM_HAS_DN(0.00)[]; TO_DN_SOME(0.00)[]; FREEMAIL_FROM(0.00)[gmail.com]; TO_MATCH_ENVRCPT_ALL(0.00)[]; MIME_GOOD(-0.10)[text/plain]; SUBJECT_HAS_CURRENCY(1.00)[]; NEURAL_HAM_LONG(-1.00)[-1.000]; R_SPF_ALLOW(-0.20)[+ip6:2607:f8b0:4000::/36]; MID_RHS_MATCH_FROMTLD(0.00)[]; NEURAL_SPAM_SHORT(0.98)[0.982]; DKIM_TRACE(0.00)[gmail.com:+]; RCPT_COUNT_TWO(0.00)[2]; DMARC_POLICY_ALLOW(-0.50)[gmail.com,none]; RCVD_IN_DNSWL_NONE(0.00)[2607:f8b0:4864:20::a2d:from]; FROM_EQ_ENVFROM(0.00)[]; MIME_TRACE(0.00)[0:+]; FREEMAIL_ENVFROM(0.00)[gmail.com]; RCVD_COUNT_TWO(0.00)[2]; ASN(0.00)[asn:15169, ipnet:2607:f8b0::/32, country:US]; TAGGED_FROM(0.00)[]; DWL_DNSWL_NONE(0.00)[gmail.com:dkim] X-ThisMailContainsUnwantedMimeParts: N 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 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 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