On Fri, Nov 24, 2017 at 02:58:48PM +0100, Alexandr Nedvedicky wrote:
> this diff is part of the 'big patch' [1] to pfctl I've sent while back. The
> pfctl fails to handle nested 'load anchor' statements properly, when ruleset
> is
> being loaded to non-root anchor (e.g. pfctl -a regress ...), see [1] to find
> more details about the issue.
>
> Also I'm piggy backing yet another fix to 'name vs. path'. the bug occurs on
> parse.y, where we handle 'load anchor'. I've just spotted this problem, when
> I was hunting down the problem with '-a'.
>
> OK?
OK bluhm@
> --------8<---------------8<---------------8<------------------8<--------
> diff --git a/sbin/pfctl/parse.y b/sbin/pfctl/parse.y
> index e1dcfbc382f..1016c909ccc 100644
> --- a/sbin/pfctl/parse.y
> +++ b/sbin/pfctl/parse.y
> @@ -939,7 +939,7 @@ anchorrule : ANCHOR anchorname dir quick interface
> af proto fromto
> loadrule : LOAD ANCHOR string FROM string {
> struct loadanchors *loadanchor;
>
> - if (strlen(pf->anchor->name) + 1 +
> + if (strlen(pf->anchor->path) + 1 +
> strlen($3) >= PATH_MAX) {
> yyerror("anchorname %s too long, max %u\n",
> $3, PATH_MAX - 1);
> @@ -954,7 +954,7 @@ loadrule : LOAD ANCHOR string FROM string {
> err(1, "loadrule: malloc");
> if (pf->anchor->name[0])
> snprintf(loadanchor->anchorname, PATH_MAX,
> - "%s/%s", pf->anchor->name, $3);
> + "%s/%s", pf->anchor->path, $3);
> else
> strlcpy(loadanchor->anchorname, $3, PATH_MAX);
> if ((loadanchor->filename = strdup($5)) == NULL)
> diff --git a/sbin/pfctl/pfctl.c b/sbin/pfctl/pfctl.c
> index d6cfc132c0e..044e8767bae 100644
> --- a/sbin/pfctl/pfctl.c
> +++ b/sbin/pfctl/pfctl.c
> @@ -1658,20 +1658,21 @@ pfctl_rules(int dev, char *filename, int opts, int
> optimize,
> free(path);
> path = NULL;
>
> - /* process "load anchor" directives that might have used queues */
> - if (!anchorname[0]) {
> + if (trans == NULL) {
> + /*
> + * process "load anchor" directives that might have used queues
> + */
> if (pfctl_load_anchors(dev, &pf, t) == -1)
> ERRX("load anchors");
> pfctl_clear_queues(&qspecs);
> pfctl_clear_queues(&rootqs);
> - }
>
> - if (trans == NULL && (opts & PF_OPT_NOACTION) == 0) {
> - if (!anchorname[0])
> - if (pfctl_load_options(&pf))
> + if ((opts & PF_OPT_NOACTION) == 0) {
> + if (!anchorname[0] && pfctl_load_options(&pf))
> goto _error;
> - if (pfctl_trans(dev, t, DIOCXCOMMIT, osize))
> - ERR("DIOCXCOMMIT");
> + if (pfctl_trans(dev, t, DIOCXCOMMIT, osize))
> + ERR("DIOCXCOMMIT");
> + }
> }
> return (0);
>
