pfctl should allow administrator to flush _anchors

classic Classic list List threaded Threaded
18 messages Options
Reply | Threaded
Open this post in threaded view
|

pfctl should allow administrator to flush _anchors

Alexandr Nedvedicky
Hello,

This issue has been reported by one of our customers.

consider pf.conf comes with rules as follows:

    anchor {
            pass all
            anchor {
                    block all
            }
    }

We load pf.conf to kernel and (pfctl -f pf.conf) and display what got loaded:

    lumpy# ./pfctl -f /tmp/pf.conf
    lumpy# ./pfctl -sr
    anchor all {
      pass all flags S/SA
      anchor all {
        block drop all
      }
    }

so far so good. Now let's flush the rules from kernel:

    lumpy# ./pfctl -Fr
    rules cleared
    lumpy# ./pfctl -sr
    lumpy#

However the underscore anchors are still there:

    lumpy# ./pfctl -vsA
      _1
      _1/_2
    lumpy#

I could not figure out any existing way to remove them, hence I'm proposing
small patch, which allows me to remove those 'underscore' anchors by doing
this:

    lumpy# ./pfctl -a _1/_2 -Fr
    rules cleared
    lumpy# ./pfctl -a _1 -Fr
    rules cleared
    lumpy# ./pfctl -vsA
    lumpy#

Does patch below make sense? Or are there some pitfalls I'm not aware of?

thanks and
regards
sashan

--------8<---------------8<---------------8<------------------8<--------
--- a/sbin/pfctl/pfctl.c
+++ b/sbin/pfctl/pfctl.c
@@ -2445,7 +2445,16 @@ main(int argc, char *argv[])
  warnx("anchors apply to -f, -F, -s, and -T only");
  usage();
  }
+
+ /*
+ * we want enable administrator to flush anonymous anchors,
+ * thus '_' should be allowed for '-Fr' only.  Also make sure
+ * we fail in case of option combination as follows:
+ * pfctl -a _1 -Fr -f /some/rules.conf
+ */
  if (mode == O_RDWR && tblcmdopt == NULL &&
+    (clearopt == NULL || *clearopt != 'r' ||
+    rulesopt != NULL) &&
     (anchoropt[0] == '_' || strstr(anchoropt, "/_") != NULL))
  errx(1, "anchor names beginning with '_' cannot "
     "be modified from the command line");

Reply | Threaded
Open this post in threaded view
|

Re: pfctl should allow administrator to flush _anchors

Klemens Nanni-2
On Fri, Feb 22, 2019 at 01:52:24AM +0100, Alexandr Nedvedicky wrote:
 
> so far so good. Now let's flush the rules from kernel:
>
>     lumpy# ./pfctl -Fr
>     rules cleared
>     lumpy# ./pfctl -sr
>     lumpy#
>
> However the underscore anchors are still there:
Any unreferenced anchor will remain, not just internal ones.  We have no
code/logic that clears them automatically.

>     lumpy# ./pfctl -vsA
>       _1
>       _1/_2
>     lumpy#
That's a known issue, easily visible if you run the pfctl regress tests.
I've been pondering this with different attempts but haven't come up
with a good/safe one yet.

Perhaps we can teach pfctl something like `-F Anchors`, such that it
looks at the ruleset as well as all anchors and removes those which are
not referenced.

The benefits are that we do not require any further
starts-with-underscore quirks and it remains a wilful action done by
root.

> I could not figure out any existing way to remove them, hence I'm proposing
> small patch, which allows me to remove those 'underscore' anchors by doing
> this:
>
>     lumpy# ./pfctl -a _1/_2 -Fr
>     rules cleared
>     lumpy# ./pfctl -a _1 -Fr
>     rules cleared
Did you look at happens with _1/_2 when you flush _1 directly?

> Does patch below make sense? Or are there some pitfalls I'm not aware of?
Given your example, using named anchors seems like the right approach.

> + /*
> + * we want enable administrator to flush anonymous anchors,
> + * thus '_' should be allowed for '-Fr' only.  Also make sure
> + * we fail in case of option combination as follows:
> + * pfctl -a _1 -Fr -f /some/rules.conf
> + */
>   if (mode == O_RDWR && tblcmdopt == NULL &&
> +    (clearopt == NULL || *clearopt != 'r' ||
> +    rulesopt != NULL) &&
I'm not fond of adding an exception to what already is an exception.
The anchor handling code is tricky already and has big potential for
pitfalls and subtle bugs (as seen in the past).

>      (anchoropt[0] == '_' || strstr(anchoropt, "/_") != NULL))
>   errx(1, "anchor names beginning with '_' cannot "
>      "be modified from the command line");

That said, I think the internal semantics should stay purely internal to
avoid another can of worms.

Reply | Threaded
Open this post in threaded view
|

Re: pfctl should allow administrator to flush _anchors

Alexandr Nedvedicky
Hello,

On Fri, Feb 22, 2019 at 10:55:17AM +0100, Klemens Nanni wrote:

> On Fri, Feb 22, 2019 at 01:52:24AM +0100, Alexandr Nedvedicky wrote:
>  
> > so far so good. Now let's flush the rules from kernel:
> >
> >     lumpy# ./pfctl -Fr
> >     rules cleared
> >     lumpy# ./pfctl -sr
> >     lumpy#
> >
> > However the underscore anchors are still there:
> Any unreferenced anchor will remain, not just internal ones.  We have no
> code/logic that clears them automatically.

    yes, that's what I thought. We have a kind 'service' on Solaris, which
    wraps pfctl to manage firewall. If firewall is being enabled, the service
    cleans up all rules (anchors). We basically dump the rulesets (pfctl -vsA)
    and then traverse from leaves to root to clean it up.
>
> >     lumpy# ./pfctl -vsA
> >       _1
> >       _1/_2
> >     lumpy#
> That's a known issue, easily visible if you run the pfctl regress tests.
> I've been pondering this with different attempts but haven't come up
> with a good/safe one yet.

    unlike named anchors, the Solaris service can't remove the 'underscore'
    anchors.  my patch solves that glitch for Solaris.
    I was looking for the smallest change possible, which will fix Solaris
    without introducing too much risk/changes in upstream.

>
> Perhaps we can teach pfctl something like `-F Anchors`, such that it
> looks at the ruleset as well as all anchors and removes those which are
> not referenced.

    so if I understand you right, your scenario for ruleset from my
    first email works as follows:
        pfctl -Fr # makes the anchors _1 and _1/_2 unreferenced
                        # (they are not reachable from root any more)

        pfctl -FAnchors # purge all unreferenced anchors.

    the 'unreferenced' means the anchor is not reachable by any packet.
    like there is no path for packet between main ruleset and that particular
    anchor (and all its descendants).
>
> The benefits are that we do not require any further
> starts-with-underscore quirks and it remains a wilful action done by
> root.

    sure, I agree, adding -FAnchors options i more systemic approach, though
    such change is more complex. I think I can give it a try to prototype it.

>
> > I could not figure out any existing way to remove them, hence I'm proposing
> > small patch, which allows me to remove those 'underscore' anchors by doing
> > this:
> >
> >     lumpy# ./pfctl -a _1/_2 -Fr
> >     rules cleared
> >     lumpy# ./pfctl -a _1 -Fr
> >     rules cleared
> Did you look at happens with _1/_2 when you flush _1 directly?

    yes, the ruleset becomes empty, but it still hanging around,
    because its descendant (_1/_2) contains the block rule.
    the terminal looks as follows:

        lumpy# pfctl -f /tmp/pf.conf  
        lumpy# ./pfctl -Fr                                                            
        rules cleared
        lumpy# ./pfctl -a _1 -Fr
        rules cleared
        lumpy# ./pfctl -vsA                                                            
          _1
          _1/_2
        lumpy# ./pfctl -a _1 -sr
        lumpy# ./pfctl -a _1/_2 -sr
        block drop all
        lumpy#

>
> > Does patch below make sense? Or are there some pitfalls I'm not aware of?
> Given your example, using named anchors seems like the right approach.
>
> > + /*
> > + * we want enable administrator to flush anonymous anchors,
> > + * thus '_' should be allowed for '-Fr' only.  Also make sure
> > + * we fail in case of option combination as follows:
> > + * pfctl -a _1 -Fr -f /some/rules.conf
> > + */
> >   if (mode == O_RDWR && tblcmdopt == NULL &&
> > +    (clearopt == NULL || *clearopt != 'r' ||
> > +    rulesopt != NULL) &&
> I'm not fond of adding an exception to what already is an exception.
> The anchor handling code is tricky already and has big potential for
> pitfalls and subtle bugs (as seen in the past).

    I completely agree here.

>
> >      (anchoropt[0] == '_' || strstr(anchoropt, "/_") != NULL))
> >   errx(1, "anchor names beginning with '_' cannot "
> >      "be modified from the command line");
>
> That said, I think the internal semantics should stay purely internal to
> avoid another can of worms.

    I partially agree here. As I don't think the current semantics is purely
    internal. IMO the underscore just denotes a kind of namespace, which
    requires special handling. I read it as 'name created by PF'. And as such
    is too weak to constitute a 'purely internal', because the rules contained
    in _1 anchor are coming from administrator not from PF itself.

    I agree with 'can of worms'. So let's see what else I'm going to find
    when I'll try to prototype '-FAnchors'.

thanks and
regards
sashan

Reply | Threaded
Open this post in threaded view
|

Re: pfctl should allow administrator to flush _anchors

Klemens Nanni-2
On Fri, Feb 22, 2019 at 12:42:02PM +0100, Alexandr Nedvedicky wrote:
>     yes, that's what I thought. We have a kind 'service' on Solaris, which
>     wraps pfctl to manage firewall. If firewall is being enabled, the service
>     cleans up all rules (anchors). We basically dump the rulesets (pfctl -vsA)
>     and then traverse from leaves to root to clean it up.
That's probaly how I'd approach `-F Anchors'.

>     so if I understand you right, your scenario for ruleset from my
>     first email works as follows:
> pfctl -Fr # makes the anchors _1 and _1/_2 unreferenced
> # (they are not reachable from root any more)
>
> pfctl -FAnchors # purge all unreferenced anchors.
>
>     the 'unreferenced' means the anchor is not reachable by any packet.
>     like there is no path for packet between main ruleset and that particular
>     anchor (and all its descendants).
Yes.  With the regress suite for example, the following should leave no
trace of regress anchors or rules:

        make
        pfctl -f /etc/pf.conf
        pfctl -F Anchors

>     sure, I agree, adding -FAnchors options i more systemic approach, though
>     such change is more complex. I think I can give it a try to prototype it.
Cool! I'm happy to help here.

Reply | Threaded
Open this post in threaded view
|

Re: pfctl should allow administrator to flush _anchors

Alexandr Nedvedicky
Hello Klemens,

I just need to clarify some details.

</snip>
> >     the 'unreferenced' means the anchor is not reachable by any packet.
> >     like there is no path for packet between main ruleset and that particular
> >     anchor (and all its descendants).
> Yes.  With the regress suite for example, the following should leave no
> trace of regress anchors or rules:
>
> make
> pfctl -f /etc/pf.conf
> pfctl -F Anchors

    so the option '-F Anchors' will also perform a '-Fr' on main ruleset, is
    that correct?

    And also one more thing, which comes to my mind. How '-F Anchors' should
    treat tables attached to anchors?

    the firewall service (which is a kind of rc-script in fact)  on Solaris,
    kills (removes) all tables first, then it removes anchors.

    What shall we do in case of '-F Anchors'? do we want '-F Anchors' to
    kill attached tables too? Or should it just report "anchor can't be
    removed, because table is still attached?"

    It looks like '-F Anchors' shifts pfctl(8) from simple tool, which does
    exactly what it's told to do, to advanced tool, which does more things at
    one step.

    The simple tools just seem to be more friendly for scripts, while advanced
    tool is easier to use by human.

thanks and
regards
sasha

Reply | Threaded
Open this post in threaded view
|

Re: pfctl should allow administrator to flush _anchors

Klemens Nanni-2
On Fri, Feb 22, 2019 at 03:02:07PM +0100, Alexandr Nedvedicky wrote:
>     so the option '-F Anchors' will also perform a '-Fr' on main ruleset, is
>     that correct?
No, my `-f /etc/pf.conf' is the equivalent to your `-F rules' here.

>     And also one more thing, which comes to my mind. How '-F Anchors' should
>     treat tables attached to anchors?
If an unreferenced anchor contains a table, either destroy that table
as well since it cannot be referenced or abort garbage collecting
anchor completely, possibly prompting the user to clean tables in
besaid anchors as well.

I tend towards the first option since anchors may contain tables that
were automatically created due to ruleset optimization.  That is to say,
while enforcing manual removal of tables as well, this may become
annoying for big optimized rulesets where administrators would have to
wield `-F Tables' and or `-a aname -T kill -t tname' before having
`-F Anchors' run cleanly.

Does that make sense?

>     the firewall service (which is a kind of rc-script in fact)  on Solaris,
>     kills (removes) all tables first, then it removes anchors.
>
>     What shall we do in case of '-F Anchors'? do we want '-F Anchors' to
>     kill attached tables too? Or should it just report "anchor can't be
>     removed, because table is still attached?"
See above.  What this "service" roughly seems like what I have in mind.

>     It looks like '-F Anchors' shifts pfctl(8) from simple tool, which does
>     exactly what it's told to do, to advanced tool, which does more things at
>     one step.
But isn't that a perfect job for pfctl?  It manipulates everything, it's
the single interface for firewall related and that's a good.  Cleaning
things up after work is done is a necessity; this is about adding the
missing peaces, imho.

>     The simple tools just seem to be more friendly for scripts, while advanced
>     tool is easier to use by human.
pfctl is well suited for both, and it always should be.

Reply | Threaded
Open this post in threaded view
|

Re: pfctl should allow administrator to flush _anchors

Alexandr Nedvedicky
Hello,

I'm giving up on identifying unreferenced/orphaned rulesets. It seems to me
like too much work/complexity, which invites more troubles. I prefer to keep
things simple, hence I'm proposing new modifier for Flush option:

    -F Nuke       Flush all rulesets and tables.

The option is meant to be used when things will get too messy.  It removes all
tables and rulesets from kernel allowing admin to quickly start from scratch
without doing reboot.

so here is my first question:

    does proposal for the 'Nuke' flush modifier sound good?
    or is it completely off?

Proposed change below splits current pfctl_show_anchors() to two functions:

    pfctl_walk_anchors(), which implements ruleset/anchor traversal

    pfctl_walk_show(), which prints full path to every anchor, while
        anchors are traversed. the pfctl_walk_show() is passed
        as argument to pfctl_walk_anchors()

The implementation of Nuke modifier comes with its own callback
pfctl_walk_get(), which collects anchors to SLIST during anchor tree
traversal. As soon as all anchors/rulesets are collected
we effectively apply:
    pfctl -a ... -FT
    pfctl -a ... -Fr
to every anchor/ruleset found in the list.

If you think the change is too big to be done in one step, then
I can split the diff to two steps:

    split pfctl_show_anchors() in the first step

    then send an updated diff, which just adds 'Nuke' modifier to F
    option.

thanks and
regards
sashan

--------8<---------------8<---------------8<------------------8<--------
diff --git a/sbin/pfctl/pfctl.8 b/sbin/pfctl/pfctl.8
index 48b2893cfcd..06e76e35b68 100644
--- a/sbin/pfctl/pfctl.8
+++ b/sbin/pfctl/pfctl.8
@@ -199,6 +199,8 @@ Flush the tables.
 Flush the passive operating system fingerprints.
 .It Fl F Cm all
 Flush all of the above.
+.It Fl F Cm Nuke
+Flush all rulesets and tables.
 .El
 .It Fl f Ar file
 Replace the current ruleset with
diff --git a/sbin/pfctl/pfctl.c b/sbin/pfctl/pfctl.c
index 493ff47af2f..fc909ade507 100644
--- a/sbin/pfctl/pfctl.c
+++ b/sbin/pfctl/pfctl.c
@@ -63,7 +63,7 @@ int pfctl_disable(int, int);
 void pfctl_clear_queues(struct pf_qihead *);
 void pfctl_clear_stats(int, const char *, int);
 void pfctl_clear_interface_flags(int, int);
-void pfctl_clear_rules(int, int, char *);
+int pfctl_clear_rules(int, int, char *);
 void pfctl_clear_src_nodes(int, int);
 void pfctl_clear_states(int, const char *, int);
 struct addrinfo *
@@ -105,6 +105,13 @@ int pfctl_load_rule(struct pfctl *, char *, struct pf_rule *, int);
 const char *pfctl_lookup_option(char *, const char **);
 void pfctl_state_store(int, const char *);
 void pfctl_state_load(int, const char *);
+int pfctl_walk_show(int, struct pfioc_ruleset *, void *);
+int pfctl_walk_get(int, struct pfioc_ruleset *, void *);
+int pfctl_walk_anchors(int, int, char *, void *,
+    int(*)(int, struct pfioc_ruleset *, void *));
+struct pfr_anchors *
+ pfctl_get_anchors(int, int);
+int pfctl_nuke_anchors(int, int);
 
 const char *clearopt;
 char *rulesopt;
@@ -205,7 +212,8 @@ static const struct {
 };
 
 static const char *clearopt_list[] = {
- "rules", "Sources", "states", "info", "Tables", "osfp", "all", NULL
+ "rules", "Sources", "states", "info", "Tables", "osfp", "all",
+ "Nuke", NULL
 };
 
 static const char *showopt_list[] = {
@@ -232,7 +240,6 @@ static const char *optiopt_list[] = {
 struct pf_qihead qspecs = TAILQ_HEAD_INITIALIZER(qspecs);
 struct pf_qihead rootqs = TAILQ_HEAD_INITIALIZER(rootqs);
 
-
 __dead void
 usage(void)
 {
@@ -315,19 +322,29 @@ pfctl_clear_interface_flags(int dev, int opts)
  }
 }
 
-void
+int
 pfctl_clear_rules(int dev, int opts, char *anchorname)
 {
- struct pfr_buffer t;
+ struct pfr_buffer t;
+ int rv = 0;
 
  memset(&t, 0, sizeof(t));
  t.pfrb_type = PFRB_TRANS;
  if (pfctl_add_trans(&t, PF_TRANS_RULESET, anchorname) ||
     pfctl_trans(dev, &t, DIOCXBEGIN, 0) ||
-    pfctl_trans(dev, &t, DIOCXCOMMIT, 0))
- err(1, "pfctl_clear_rules");
- if ((opts & PF_OPT_QUIET) == 0)
+    pfctl_trans(dev, &t, DIOCXCOMMIT, 0)) {
+ if (opts & PF_OPT_IGNFAIL) {
+ if ((opts & PF_OPT_QUIET) == 0) {
+ warn("%s", __func__);
+ }
+ rv = 1;
+ } else {
+ err(1, "%s", __func__);
+ }
+ } else if ((opts & PF_OPT_QUIET) == 0)
  fprintf(stderr, "rules cleared\n");
+
+ return (rv);
 }
 
 void
@@ -2107,13 +2124,59 @@ pfctl_debug(int dev, u_int32_t level, int opts)
 }
 
 int
-pfctl_show_anchors(int dev, int opts, char *anchorname)
+pfctl_walk_show(int opts, struct pfioc_ruleset *pr, void *warg)
+{
+ if (pr->path[0]) {
+ if (pr->path[0] != '_' || (opts & PF_OPT_VERBOSE))
+ printf("  %s/%s\n", pr->path, pr->name);
+ } else if (pr->name[0] != '_' || (opts & PF_OPT_VERBOSE))
+ printf("  %s\n", pr->name);
+
+ return (0);
+}
+
+int
+pfctl_walk_get(int opts, struct pfioc_ruleset *pr, void *warg)
+{
+ struct pfr_anchoritem *pfra;
+ unsigned int len;
+ struct {
+ struct pfr_anchoritem *pfra;
+ } *tail_pfra = warg;
+
+ pfra = malloc(sizeof(*pfra));
+ if (pfra == NULL)
+ err(1, "%s", __func__);
+
+ len = strlen(pr->path) + 1;
+ len += strlen(pr->name) + 1;
+ pfra->pfra_anchorname = malloc(len);
+ if (pfra->pfra_anchorname == NULL)
+ err(1, "%s", __func__);
+
+ if (pr->path[0])
+ snprintf(pfra->pfra_anchorname, len, "%s/%s",
+    pr->path, pr->name);
+ else
+ snprintf(pfra->pfra_anchorname, len, "%s", pr->name);
+
+ if (tail_pfra->pfra != NULL)
+ SLIST_INSERT_AFTER(tail_pfra->pfra, pfra, pfra_sle);
+
+ tail_pfra->pfra = pfra;
+
+ return (0);
+}
+
+int
+pfctl_walk_anchors(int dev, int opts, char *anchorname, void *warg,
+    int(walkf)(int, struct pfioc_ruleset *, void *))
 {
  struct pfioc_ruleset pr;
  u_int32_t mnr, nr;
 
  memset(&pr, 0, sizeof(pr));
- memcpy(pr.path, anchorname, sizeof(pr.path));
+ strlcpy(pr.path, anchorname, sizeof(pr.path));
  if (ioctl(dev, DIOCGETRULESETS, &pr)) {
  if (errno == EINVAL)
  fprintf(stderr, "Anchor '%s' not found.\n",
@@ -2132,19 +2195,91 @@ pfctl_show_anchors(int dev, int opts, char *anchorname)
  if (!strcmp(pr.name, PF_RESERVED_ANCHOR))
  continue;
  sub[0] = '\0';
- if (pr.path[0]) {
- strlcat(sub, pr.path, sizeof(sub));
- strlcat(sub, "/", sizeof(sub));
- }
- strlcat(sub, pr.name, sizeof(sub));
- if (sub[0] != '_' || (opts & PF_OPT_VERBOSE))
- printf("  %s\n", sub);
- if ((opts & PF_OPT_VERBOSE) && pfctl_show_anchors(dev, opts, sub))
+
+ if (walkf(opts, &pr, warg))
  return (-1);
+
+ if (opts & PF_OPT_VERBOSE) {
+ char sub[PATH_MAX];
+
+ if (pr.path[0])
+ snprintf(sub, sizeof(sub), "%s/%s",
+    pr.path, pr.name);
+ else
+ snprintf(sub, sizeof(sub), "%s",
+    pr.name);
+ if (pfctl_walk_anchors(dev, opts, sub, warg, walkf))
+ return (-1);
+ }
  }
  return (0);
 }
 
+int
+pfctl_show_anchors(int dev, int opts, char *anchorname)
+{
+ int rv;
+
+ rv = pfctl_walk_anchors(dev, opts, anchorname, NULL, pfctl_walk_show);
+ return (rv);
+}
+
+struct pfr_anchors *
+pfctl_get_anchors(int dev, int opts)
+{
+ struct pfioc_ruleset pr;
+ struct {
+ struct pfr_anchoritem *pfra;
+ } pfra;
+ static struct pfr_anchors anchors;
+
+ SLIST_INIT(&anchors);
+
+ memset(&pr, 0, sizeof(pr));
+ pfra.pfra = NULL;
+ pfctl_walk_get(opts, &pr, &pfra);
+ if (pfra.pfra == NULL) {
+ fprintf(stderr, "%s failed to allocate main anchor\n",
+    __func__);
+ exit(1);
+ }
+ SLIST_INSERT_HEAD(&anchors, pfra.pfra, pfra_sle);
+
+ opts |= PF_OPT_VERBOSE;
+ if (pfctl_walk_anchors(dev, opts, "", &pfra, pfctl_walk_get)) {
+ fprintf(stderr,
+    "%s failed to retreive list of anchors, can't continue\n",
+    __func__);
+ exit(1);
+ }
+
+ return (&anchors);
+}
+
+int
+pfctl_nuke_anchors(int dev, int opts)
+{
+ int rv = 0;
+ struct pfr_anchors *anchors;
+ struct pfr_anchoritem *pfra, *pfra_save;
+
+ anchors = pfctl_get_anchors(dev, opts);
+ /*
+ * don't let pfctl_clear_*() function to fail with exit
+ */
+ opts |= PF_OPT_IGNFAIL;
+ SLIST_FOREACH_SAFE(pfra, anchors, pfra_sle, pfra_save) {
+ rv |= (pfctl_clear_tables(pfra->pfra_anchorname, opts) == -1) ?
+    1 : 0;
+ rv |= pfctl_clear_rules(dev, opts, pfra->pfra_anchorname);
+ SLIST_REMOVE(anchors, pfra, pfr_anchoritem, pfra_sle);
+ free(pfra->pfra_anchorname);
+ free(pfra);
+ }
+
+ return (rv);
+}
+
 const char *
 pfctl_lookup_option(char *cmd, const char **list)
 {
@@ -2566,6 +2701,14 @@ main(int argc, char *argv[])
  case 'T':
  pfctl_clear_tables(anchorname, opts);
  break;
+ case 'N':
+ if (*anchorname) {
+ warnx("-a can not be used with -FN");
+ usage();
+ /* NOTREACHED */
+ }
+ pfctl_nuke_anchors(dev, opts);
+ break;
  }
  }
  if (state_killers) {
diff --git a/sbin/pfctl/pfctl.h b/sbin/pfctl/pfctl.h
index 7981cf66fdb..796c894c335 100644
--- a/sbin/pfctl/pfctl.h
+++ b/sbin/pfctl/pfctl.h
@@ -48,6 +48,14 @@ struct pfr_buffer {
     (var) != NULL; \
     (var) = pfr_buf_next((buf), (var)))
 
+
+struct pfr_anchoritem {
+ SLIST_ENTRY(pfr_anchoritem) pfra_sle;
+ char *pfra_anchorname;
+};
+
+SLIST_HEAD(pfr_anchors, pfr_anchoritem);
+
 int pfr_get_fd(void);
 int pfr_clr_tables(struct pfr_table *, int *, int);
 int pfr_add_tables(struct pfr_table *, int, int *, int);
@@ -75,7 +83,7 @@ int pfi_get_ifaces(const char *, struct pfi_kif *, int *);
 int pfi_clr_istats(const char *, int *, int);
 
 void pfctl_print_title(char *);
-void pfctl_clear_tables(const char *, int);
+int pfctl_clear_tables(const char *, int);
 void pfctl_show_tables(const char *, int);
 int pfctl_table(int, char *[], char *, const char *, char *,
     const char *, int);
diff --git a/sbin/pfctl/pfctl_parser.h b/sbin/pfctl/pfctl_parser.h
index be0ccee1897..993b1fe1c77 100644
--- a/sbin/pfctl/pfctl_parser.h
+++ b/sbin/pfctl/pfctl_parser.h
@@ -36,21 +36,22 @@
 
 #define PF_OSFP_FILE "/etc/pf.os"
 
-#define PF_OPT_DISABLE 0x0001
-#define PF_OPT_ENABLE 0x0002
-#define PF_OPT_VERBOSE 0x0004
-#define PF_OPT_NOACTION 0x0008
-#define PF_OPT_QUIET 0x0010
-#define PF_OPT_CLRRULECTRS 0x0020
-#define PF_OPT_USEDNS 0x0040
-#define PF_OPT_VERBOSE2 0x0080
-#define PF_OPT_DUMMYACTION 0x0100
-#define PF_OPT_DEBUG 0x0200
-#define PF_OPT_SHOWALL 0x0400
-#define PF_OPT_OPTIMIZE 0x0800
-#define PF_OPT_NODNS 0x1000
-#define PF_OPT_RECURSE 0x4000
-#define PF_OPT_PORTNAMES 0x8000
+#define PF_OPT_DISABLE 0x00001
+#define PF_OPT_ENABLE 0x00002
+#define PF_OPT_VERBOSE 0x00004
+#define PF_OPT_NOACTION 0x00008
+#define PF_OPT_QUIET 0x00010
+#define PF_OPT_CLRRULECTRS 0x00020
+#define PF_OPT_USEDNS 0x00040
+#define PF_OPT_VERBOSE2 0x00080
+#define PF_OPT_DUMMYACTION 0x00100
+#define PF_OPT_DEBUG 0x00200
+#define PF_OPT_SHOWALL 0x00400
+#define PF_OPT_OPTIMIZE 0x00800
+#define PF_OPT_NODNS 0x01000
+#define PF_OPT_RECURSE 0x04000
+#define PF_OPT_PORTNAMES 0x08000
+#define PF_OPT_IGNFAIL 0x10000
 
 #define PF_TH_ALL 0xFF
 
diff --git a/sbin/pfctl/pfctl_table.c b/sbin/pfctl/pfctl_table.c
index 6ed4024da4e..922bd78aae4 100644
--- a/sbin/pfctl/pfctl_table.c
+++ b/sbin/pfctl/pfctl_table.c
@@ -101,11 +101,17 @@ static const char *istats_text[2][2][2] = {
  table.pfrt_flags &= ~PFR_TFLAG_PERSIST; \
  } while(0)
 
-void
+int
 pfctl_clear_tables(const char *anchor, int opts)
 {
- if (pfctl_table(0, NULL, NULL, "-F", NULL, anchor, opts) == -1)
- exit(1);
+ int rv;
+
+ if ((rv = pfctl_table(0, NULL, NULL, "-F", NULL, anchor, opts)) == -1) {
+ if ((opts & PF_OPT_IGNFAIL) == 0)
+ exit(1);
+ }
+
+ return (rv);
 }
 
 void

Reply | Threaded
Open this post in threaded view
|

Re: pfctl should allow administrator to flush _anchors

Alexandr Nedvedicky
Hello,

I received a feedback suggesting to come up with better name than 'Nuke'

> not sure about the name.
>
> i've often want a similar operation for network interfaces, which
> returns them to 'raw' state.
>
> and there's also been people who want the same for the routing
> table
>
> if we ever do such a thing for all of these things, i think we should
> use the same word
>
> 'nuke' sounds like the wrong word, it is a 'destroy' operation rather
> than 'return to unitialized'.

I think all the above calls for a new standalone option, which I named as
'Unconfigure'.  Patch below suggest unconfigure behavior for PF.
Doing 'pfctl -U' will bring PF back to its initial state (e.g. right before
pf.conf got processed during the system boot). In case of PF the proposed -U
will do following:
    - remove all rulesets and tables
    - remove all states and source nodes
    - remove all OS fingerprints
    - set all limits, timeouts and options to their defaults

thanks and
regards
sashan

--------8<---------------8<---------------8<------------------8<--------
diff --git a/sbin/pfctl/pfctl.8 b/sbin/pfctl/pfctl.8
index 48b2893cfcd..562e03b3477 100644
--- a/sbin/pfctl/pfctl.8
+++ b/sbin/pfctl/pfctl.8
@@ -644,6 +644,9 @@ This flag is set when per-address counters are enabled on the table.
 .El
 .It Fl t Ar table
 Specify the name of the table.
+.It Fl U
+Unconfigure firewall. All rules, states and tables are removed. All limits
+and timeouts are reset to default values.
 .It Fl V Ar rdomain
 Select the routing domain to be used to kill states by host or by label.
 The rdomain of a state is displayed in parentheses before the host by
diff --git a/sbin/pfctl/pfctl.c b/sbin/pfctl/pfctl.c
index 493ff47af2f..19f63db56f2 100644
--- a/sbin/pfctl/pfctl.c
+++ b/sbin/pfctl/pfctl.c
@@ -54,6 +54,8 @@
 
 #include <syslog.h>
 
+#include <stdarg.h>
+
 #include "pfctl_parser.h"
 #include "pfctl.h"
 
@@ -63,7 +65,7 @@ int pfctl_disable(int, int);
 void pfctl_clear_queues(struct pf_qihead *);
 void pfctl_clear_stats(int, const char *, int);
 void pfctl_clear_interface_flags(int, int);
-void pfctl_clear_rules(int, int, char *);
+int pfctl_clear_rules(int, int, char *);
 void pfctl_clear_src_nodes(int, int);
 void pfctl_clear_states(int, const char *, int);
 struct addrinfo *
@@ -105,6 +107,14 @@ int pfctl_load_rule(struct pfctl *, char *, struct pf_rule *, int);
 const char *pfctl_lookup_option(char *, const char **);
 void pfctl_state_store(int, const char *);
 void pfctl_state_load(int, const char *);
+int pfctl_walk_show(int, struct pfioc_ruleset *, void *);
+int pfctl_walk_get(int, struct pfioc_ruleset *, void *);
+int pfctl_walk_anchors(int, int, char *, void *,
+    int(*)(int, struct pfioc_ruleset *, void *));
+struct pfr_anchors *
+ pfctl_get_anchors(int, int);
+int pfctl_nuke_anchors(int, int);
+void pfctl_set_defaults(int, int);
 
 const char *clearopt;
 char *rulesopt;
@@ -124,6 +134,7 @@ char *state_kill[2];
 int dev = -1;
 int first_title = 1;
 int labels = 0;
+int exit_val = 0;
 
 #define INDENT(d, o) do { \
  if (o) { \
@@ -232,7 +243,6 @@ static const char *optiopt_list[] = {
 struct pf_qihead qspecs = TAILQ_HEAD_INITIALIZER(qspecs);
 struct pf_qihead rootqs = TAILQ_HEAD_INITIALIZER(rootqs);
 
-
 __dead void
 usage(void)
 {
@@ -249,6 +259,40 @@ usage(void)
  exit(1);
 }
 
+void
+pfctl_err(int opts, int eval, const char *fmt, ...)
+{
+ va_list ap;
+
+ va_start(ap, fmt);
+
+ if ((opts & PF_OPT_IGNFAIL) == 0)
+ verr(1, fmt, ap);
+ else
+ vwarn(fmt, ap);
+
+ va_end(ap);
+
+ exit_val = eval;
+}
+
+void
+pfctl_errx(int opts, int eval, const char *fmt, ...)
+{
+ va_list ap;
+
+ va_start(ap, fmt);
+
+ if ((opts & PF_OPT_IGNFAIL) == 0)
+ verrx(1, fmt, ap);
+ else
+ vwarnx(fmt, ap);
+
+ va_end(ap);
+
+ exit_val = eval;
+}
+
 int
 pfctl_enable(int dev, int opts)
 {
@@ -287,10 +331,10 @@ pfctl_clear_stats(int dev, const char *iface, int opts)
  memset(&pi, 0, sizeof(pi));
  if (iface != NULL && strlcpy(pi.pfiio_name, iface,
     sizeof(pi.pfiio_name)) >= sizeof(pi.pfiio_name))
- errx(1, "invalid interface: %s", iface);
+ pfctl_errx(opts, 1, "invalid interface: %s", iface);
 
  if (ioctl(dev, DIOCCLRSTATUS, &pi))
- err(1, "DIOCCLRSTATUS");
+ pfctl_err(opts, 1, "DIOCCLRSTATUS");
  if ((opts & PF_OPT_QUIET) == 0) {
  fprintf(stderr, "pf: statistics cleared");
  if (iface != NULL)
@@ -309,32 +353,36 @@ pfctl_clear_interface_flags(int dev, int opts)
  pi.pfiio_flags = PFI_IFLAG_SKIP;
 
  if (ioctl(dev, DIOCCLRIFFLAG, &pi))
- err(1, "DIOCCLRIFFLAG");
+ pfctl_err(opts, 1, "DIOCCLRIFFLAG");
  if ((opts & PF_OPT_QUIET) == 0)
  fprintf(stderr, "pf: interface flags reset\n");
  }
 }
 
-void
+int
 pfctl_clear_rules(int dev, int opts, char *anchorname)
 {
- struct pfr_buffer t;
+ struct pfr_buffer t;
+ int rv = 0;
 
  memset(&t, 0, sizeof(t));
  t.pfrb_type = PFRB_TRANS;
  if (pfctl_add_trans(&t, PF_TRANS_RULESET, anchorname) ||
     pfctl_trans(dev, &t, DIOCXBEGIN, 0) ||
-    pfctl_trans(dev, &t, DIOCXCOMMIT, 0))
- err(1, "pfctl_clear_rules");
- if ((opts & PF_OPT_QUIET) == 0)
+    pfctl_trans(dev, &t, DIOCXCOMMIT, 0)) {
+ pfctl_err(opts, 1, "%s", __func__);
+ rv = 1;
+ } else if ((opts & PF_OPT_QUIET) == 0)
  fprintf(stderr, "rules cleared\n");
+
+ return (rv);
 }
 
 void
 pfctl_clear_src_nodes(int dev, int opts)
 {
  if (ioctl(dev, DIOCCLRSRCNODES))
- err(1, "DIOCCLRSRCNODES");
+ pfctl_err(opts, 1, "DIOCCLRSRCNODES");
  if ((opts & PF_OPT_QUIET) == 0)
  fprintf(stderr, "source tracking entries cleared\n");
 }
@@ -347,10 +395,10 @@ pfctl_clear_states(int dev, const char *iface, int opts)
  memset(&psk, 0, sizeof(psk));
  if (iface != NULL && strlcpy(psk.psk_ifname, iface,
     sizeof(psk.psk_ifname)) >= sizeof(psk.psk_ifname))
- errx(1, "invalid interface: %s", iface);
+ pfctl_errx(opts, 1, "invalid interface: %s", iface);
 
  if (ioctl(dev, DIOCCLRSTATES, &psk))
- err(1, "DIOCCLRSTATES");
+ pfctl_err(opts, 1, "DIOCCLRSTATES");
  if ((opts & PF_OPT_QUIET) == 0)
  fprintf(stderr, "%d states cleared\n", psk.psk_killed);
 }
@@ -2107,13 +2155,59 @@ pfctl_debug(int dev, u_int32_t level, int opts)
 }
 
 int
-pfctl_show_anchors(int dev, int opts, char *anchorname)
+pfctl_walk_show(int opts, struct pfioc_ruleset *pr, void *warg)
+{
+ if (pr->path[0]) {
+ if (pr->path[0] != '_' || (opts & PF_OPT_VERBOSE))
+ printf("  %s/%s\n", pr->path, pr->name);
+ } else if (pr->name[0] != '_' || (opts & PF_OPT_VERBOSE))
+ printf("  %s\n", pr->name);
+
+ return (0);
+}
+
+int
+pfctl_walk_get(int opts, struct pfioc_ruleset *pr, void *warg)
+{
+ struct pfr_anchoritem *pfra;
+ unsigned int len;
+ struct {
+ struct pfr_anchoritem *pfra;
+ } *tail_pfra = warg;
+
+ pfra = malloc(sizeof(*pfra));
+ if (pfra == NULL)
+ err(1, "%s", __func__);
+
+ len = strlen(pr->path) + 1;
+ len += strlen(pr->name) + 1;
+ pfra->pfra_anchorname = malloc(len);
+ if (pfra->pfra_anchorname == NULL)
+ err(1, "%s", __func__);
+
+ if (pr->path[0])
+ snprintf(pfra->pfra_anchorname, len, "%s/%s",
+    pr->path, pr->name);
+ else
+ snprintf(pfra->pfra_anchorname, len, "%s", pr->name);
+
+ if (tail_pfra->pfra != NULL)
+ SLIST_INSERT_AFTER(tail_pfra->pfra, pfra, pfra_sle);
+
+ tail_pfra->pfra = pfra;
+
+ return (0);
+}
+
+int
+pfctl_walk_anchors(int dev, int opts, char *anchorname, void *warg,
+    int(walkf)(int, struct pfioc_ruleset *, void *))
 {
  struct pfioc_ruleset pr;
  u_int32_t mnr, nr;
 
  memset(&pr, 0, sizeof(pr));
- memcpy(pr.path, anchorname, sizeof(pr.path));
+ strlcpy(pr.path, anchorname, sizeof(pr.path));
  if (ioctl(dev, DIOCGETRULESETS, &pr)) {
  if (errno == EINVAL)
  fprintf(stderr, "Anchor '%s' not found.\n",
@@ -2132,19 +2226,91 @@ pfctl_show_anchors(int dev, int opts, char *anchorname)
  if (!strcmp(pr.name, PF_RESERVED_ANCHOR))
  continue;
  sub[0] = '\0';
- if (pr.path[0]) {
- strlcat(sub, pr.path, sizeof(sub));
- strlcat(sub, "/", sizeof(sub));
- }
- strlcat(sub, pr.name, sizeof(sub));
- if (sub[0] != '_' || (opts & PF_OPT_VERBOSE))
- printf("  %s\n", sub);
- if ((opts & PF_OPT_VERBOSE) && pfctl_show_anchors(dev, opts, sub))
+
+ if (walkf(opts, &pr, warg))
  return (-1);
+
+ if (opts & PF_OPT_VERBOSE) {
+ char sub[PATH_MAX];
+
+ if (pr.path[0])
+ snprintf(sub, sizeof(sub), "%s/%s",
+    pr.path, pr.name);
+ else
+ snprintf(sub, sizeof(sub), "%s",
+    pr.name);
+ if (pfctl_walk_anchors(dev, opts, sub, warg, walkf))
+ return (-1);
+ }
  }
  return (0);
 }
 
+int
+pfctl_show_anchors(int dev, int opts, char *anchorname)
+{
+ int rv;
+
+ rv = pfctl_walk_anchors(dev, opts, anchorname, NULL, pfctl_walk_show);
+ return (rv);
+}
+
+struct pfr_anchors *
+pfctl_get_anchors(int dev, int opts)
+{
+ struct pfioc_ruleset pr;
+ struct {
+ struct pfr_anchoritem *pfra;
+ } pfra;
+ static struct pfr_anchors anchors;
+
+ SLIST_INIT(&anchors);
+
+ memset(&pr, 0, sizeof(pr));
+ pfra.pfra = NULL;
+ pfctl_walk_get(opts, &pr, &pfra);
+ if (pfra.pfra == NULL) {
+ fprintf(stderr, "%s failed to allocate main anchor\n",
+    __func__);
+ exit(1);
+ }
+ SLIST_INSERT_HEAD(&anchors, pfra.pfra, pfra_sle);
+
+ opts |= PF_OPT_VERBOSE;
+ if (pfctl_walk_anchors(dev, opts, "", &pfra, pfctl_walk_get)) {
+ fprintf(stderr,
+    "%s failed to retreive list of anchors, can't continue\n",
+    __func__);
+ exit(1);
+ }
+
+ return (&anchors);
+}
+
+int
+pfctl_nuke_anchors(int dev, int opts)
+{
+ int rv = 0;
+ struct pfr_anchors *anchors;
+ struct pfr_anchoritem *pfra, *pfra_save;
+
+ anchors = pfctl_get_anchors(dev, opts);
+ /*
+ * don't let pfctl_clear_*() function to fail with exit
+ */
+ opts |= PF_OPT_IGNFAIL;
+ SLIST_FOREACH_SAFE(pfra, anchors, pfra_sle, pfra_save) {
+ rv |= (pfctl_clear_tables(pfra->pfra_anchorname, opts) == -1) ?
+    1 : 0;
+ rv |= pfctl_clear_rules(dev, opts, pfra->pfra_anchorname);
+ SLIST_REMOVE(anchors, pfra, pfr_anchoritem, pfra_sle);
+ free(pfra->pfra_anchorname);
+ free(pfra);
+ }
+
+ return (rv);
+}
+
 const char *
 pfctl_lookup_option(char *cmd, const char **list)
 {
@@ -2232,6 +2398,39 @@ pfctl_state_load(int dev, const char *file)
  fclose(f);
 }
 
+void
+pfctl_set_defaults(int dev, int opts)
+{
+ struct pfctl pf;
+ struct pfr_buffer t;
+ int i;
+
+ pf.dev = dev;
+ pfctl_init_options(&pf);
+
+ pf.debug_set = 1;
+ pf.reass_set = 1;
+ pf.syncookieswat_set = 1;
+ pf.ifname_set = 1;
+
+ memset(&t, 0, sizeof(t));
+ t.pfrb_type = PFRB_TRANS;
+ if (pfctl_trans(dev, &t, DIOCXBEGIN, 0))
+ warn("%s, DIOCXBEGIN", __func__);
+
+
+ for (i = 0; pf_limits[i].name; i++)
+ pf.limit_set[pf_limits[i].index] = 1;
+
+ for (i = 0; pf_timeouts[i].name; i++)
+ pf.timeout_set[pf_timeouts[i].timeout] = 1;
+
+ pfctl_load_options(&pf);
+
+ if (pfctl_trans(dev, &t, DIOCXCOMMIT, 0))
+ warn("%s, DIOCXCOMMIT", __func__);
+}
+
 int
 main(int argc, char *argv[])
 {
@@ -2248,12 +2447,13 @@ main(int argc, char *argv[])
  char *lfile = NULL, *sfile = NULL;
  const char *errstr;
  long shownr = -1;
+ int unconfigure = 0;
 
  if (argc < 2)
  usage();
 
  while ((ch = getopt(argc, argv,
-    "a:dD:eqf:F:ghi:k:K:L:Nno:Pp:R:rS:s:t:T:vV:x:z")) != -1) {
+    "a:dD:eqf:F:ghi:k:K:L:Nno:Pp:R:rS:s:t:T:UvV:x:z")) != -1) {
  switch (ch) {
  case 'a':
  anchoropt = optarg;
@@ -2388,6 +2588,10 @@ main(int argc, char *argv[])
  mode = O_RDWR;
  lfile = optarg;
  break;
+ case 'U':
+ unconfigure = 1;
+ mode = O_RDWR;
+ break;
  case 'h':
  /* FALLTHROUGH */
  default:
@@ -2399,6 +2603,15 @@ main(int argc, char *argv[])
  if ((opts & PF_OPT_NODNS) && (opts & PF_OPT_USEDNS))
  errx(1, "-N and -r are mutually exclusive");
 
+ if (unconfigure) {
+ if ((tblcmdopt || tableopt || anchoropt || clearopt ||
+    rdomain || showopt || optiopt || src_node_killers ||
+    lfile || sfile || (opts & PF_OPT_DISABLE) ||
+    (opts & PF_OPT_ENABLE) || (opts & PF_OPT_CLRRULECTRS)))
+ errx(1,
+    "-U option can be combined with -x and -v only");
+ }
+
  if (tblcmdopt == NULL ^ tableopt == NULL)
  usage();
 
@@ -2454,6 +2667,22 @@ main(int argc, char *argv[])
  clearopt = showopt = debugopt = NULL;
  }
 
+ if (unconfigure) {
+ exit_val = pfctl_nuke_anchors(dev, opts);
+ /*
+ * don't let pfctl_clear_*() functions to fail with exit
+ */
+ opts |= PF_OPT_IGNFAIL;
+ pfctl_clear_states(dev, NULL, opts);
+ pfctl_clear_src_nodes(dev, opts);
+ pfctl_clear_stats(dev, NULL, opts);
+ pfctl_clear_fingerprints(dev, opts);
+ pfctl_clear_interface_flags(dev, opts);
+ pfctl_set_defaults(dev, opts);
+
+ exit(exit_val);
+ }
+
  if (opts & PF_OPT_DISABLE)
  if (pfctl_disable(dev, opts))
  error = 1;
diff --git a/sbin/pfctl/pfctl.h b/sbin/pfctl/pfctl.h
index 7981cf66fdb..8709c90d710 100644
--- a/sbin/pfctl/pfctl.h
+++ b/sbin/pfctl/pfctl.h
@@ -48,6 +48,14 @@ struct pfr_buffer {
     (var) != NULL; \
     (var) = pfr_buf_next((buf), (var)))
 
+
+struct pfr_anchoritem {
+ SLIST_ENTRY(pfr_anchoritem) pfra_sle;
+ char *pfra_anchorname;
+};
+
+SLIST_HEAD(pfr_anchors, pfr_anchoritem);
+
 int pfr_get_fd(void);
 int pfr_clr_tables(struct pfr_table *, int *, int);
 int pfr_add_tables(struct pfr_table *, int, int *, int);
@@ -75,7 +83,7 @@ int pfi_get_ifaces(const char *, struct pfi_kif *, int *);
 int pfi_clr_istats(const char *, int *, int);
 
 void pfctl_print_title(char *);
-void pfctl_clear_tables(const char *, int);
+int pfctl_clear_tables(const char *, int);
 void pfctl_show_tables(const char *, int);
 int pfctl_table(int, char *[], char *, const char *, char *,
     const char *, int);
@@ -96,5 +104,7 @@ u_int32_t
 int pfctl_trans(int, struct pfr_buffer *, u_long, int);
 
 int pfctl_show_queues(int, const char *, int, int);
+void pfctl_err(int, int, const char *, ...);
+void pfctl_errx(int, int, const char *, ...);
 
 #endif /* _PFCTL_H_ */
diff --git a/sbin/pfctl/pfctl_osfp.c b/sbin/pfctl/pfctl_osfp.c
index 9c51d7462eb..e54b113d7e0 100644
--- a/sbin/pfctl/pfctl_osfp.c
+++ b/sbin/pfctl/pfctl_osfp.c
@@ -260,7 +260,7 @@ void
 pfctl_clear_fingerprints(int dev, int opts)
 {
  if (ioctl(dev, DIOCOSFPFLUSH))
- err(1, "DIOCOSFPFLUSH");
+ pfctl_err(opts, 1, "DIOCOSFPFLUSH");
 }
 
 /* flush pfctl's view of the fingerprints */
diff --git a/sbin/pfctl/pfctl_parser.h b/sbin/pfctl/pfctl_parser.h
index be0ccee1897..993b1fe1c77 100644
--- a/sbin/pfctl/pfctl_parser.h
+++ b/sbin/pfctl/pfctl_parser.h
@@ -36,21 +36,22 @@
 
 #define PF_OSFP_FILE "/etc/pf.os"
 
-#define PF_OPT_DISABLE 0x0001
-#define PF_OPT_ENABLE 0x0002
-#define PF_OPT_VERBOSE 0x0004
-#define PF_OPT_NOACTION 0x0008
-#define PF_OPT_QUIET 0x0010
-#define PF_OPT_CLRRULECTRS 0x0020
-#define PF_OPT_USEDNS 0x0040
-#define PF_OPT_VERBOSE2 0x0080
-#define PF_OPT_DUMMYACTION 0x0100
-#define PF_OPT_DEBUG 0x0200
-#define PF_OPT_SHOWALL 0x0400
-#define PF_OPT_OPTIMIZE 0x0800
-#define PF_OPT_NODNS 0x1000
-#define PF_OPT_RECURSE 0x4000
-#define PF_OPT_PORTNAMES 0x8000
+#define PF_OPT_DISABLE 0x00001
+#define PF_OPT_ENABLE 0x00002
+#define PF_OPT_VERBOSE 0x00004
+#define PF_OPT_NOACTION 0x00008
+#define PF_OPT_QUIET 0x00010
+#define PF_OPT_CLRRULECTRS 0x00020
+#define PF_OPT_USEDNS 0x00040
+#define PF_OPT_VERBOSE2 0x00080
+#define PF_OPT_DUMMYACTION 0x00100
+#define PF_OPT_DEBUG 0x00200
+#define PF_OPT_SHOWALL 0x00400
+#define PF_OPT_OPTIMIZE 0x00800
+#define PF_OPT_NODNS 0x01000
+#define PF_OPT_RECURSE 0x04000
+#define PF_OPT_PORTNAMES 0x08000
+#define PF_OPT_IGNFAIL 0x10000
 
 #define PF_TH_ALL 0xFF
 
diff --git a/sbin/pfctl/pfctl_table.c b/sbin/pfctl/pfctl_table.c
index 6ed4024da4e..922bd78aae4 100644
--- a/sbin/pfctl/pfctl_table.c
+++ b/sbin/pfctl/pfctl_table.c
@@ -101,11 +101,17 @@ static const char *istats_text[2][2][2] = {
  table.pfrt_flags &= ~PFR_TFLAG_PERSIST; \
  } while(0)
 
-void
+int
 pfctl_clear_tables(const char *anchor, int opts)
 {
- if (pfctl_table(0, NULL, NULL, "-F", NULL, anchor, opts) == -1)
- exit(1);
+ int rv;
+
+ if ((rv = pfctl_table(0, NULL, NULL, "-F", NULL, anchor, opts)) == -1) {
+ if ((opts & PF_OPT_IGNFAIL) == 0)
+ exit(1);
+ }
+
+ return (rv);
 }
 
 void

Reply | Threaded
Open this post in threaded view
|

Re: pfctl should allow administrator to flush _anchors

Denis Fondras
On Sun, Mar 24, 2019 at 09:24:34AM +0100, Alexandr Nedvedicky wrote:

> I think all the above calls for a new standalone option, which I named as
> 'Unconfigure'.  Patch below suggest unconfigure behavior for PF.
> Doing 'pfctl -U' will bring PF back to its initial state (e.g. right before
> pf.conf got processed during the system boot). In case of PF the proposed -U
> will do following:
>     - remove all rulesets and tables
>     - remove all states and source nodes
>     - remove all OS fingerprints
>     - set all limits, timeouts and options to their defaults
>

Isn't -U pretty close to -Fall ?

Reply | Threaded
Open this post in threaded view
|

Re: pfctl should allow administrator to flush _anchors

Alexandr Nedvedicky
On Sun, Mar 24, 2019 at 09:51:13AM +0100, Denis Fondras wrote:

> On Sun, Mar 24, 2019 at 09:24:34AM +0100, Alexandr Nedvedicky wrote:
> > I think all the above calls for a new standalone option, which I named as
> > 'Unconfigure'.  Patch below suggest unconfigure behavior for PF.
> > Doing 'pfctl -U' will bring PF back to its initial state (e.g. right before
> > pf.conf got processed during the system boot). In case of PF the proposed -U
> > will do following:
> >     - remove all rulesets and tables
> >     - remove all states and source nodes
> >     - remove all OS fingerprints
> >     - set all limits, timeouts and options to their defaults
> >
>
> Isn't -U pretty close to -Fall ?
>

    it is, however -Fall operates on main ruleset only. -Fall also does
    not reset limits and timeouts. Hence my first idea was to introduce
    '-FNuke', which kills all rulesets and tables.

    I don't want to change behaviour of existing option ('-Fall'), therefore
    I'm in favor to introduce a new option. Either '-FNuke' or '-U' works
    for me. I'm the most concerned about flushing all rulesets.

    Also making "pfctl -a '_1/_2' -Fr" to remove PF 'private' rulesets works
    for me. Actually this is the most important thing I'd like to achieve.

sashan

Reply | Threaded
Open this post in threaded view
|

Re: pfctl should allow administrator to flush _anchors

Theo de Raadt-2
Alexandr Nedvedicky <[hidden email]> wrote:

> On Sun, Mar 24, 2019 at 09:51:13AM +0100, Denis Fondras wrote:
> > On Sun, Mar 24, 2019 at 09:24:34AM +0100, Alexandr Nedvedicky wrote:
> > > I think all the above calls for a new standalone option, which I named as
> > > 'Unconfigure'.  Patch below suggest unconfigure behavior for PF.
> > > Doing 'pfctl -U' will bring PF back to its initial state (e.g. right before
> > > pf.conf got processed during the system boot). In case of PF the proposed -U
> > > will do following:
> > >     - remove all rulesets and tables
> > >     - remove all states and source nodes
> > >     - remove all OS fingerprints
> > >     - set all limits, timeouts and options to their defaults
> > >
> >
> > Isn't -U pretty close to -Fall ?
> >
>
>     it is, however -Fall operates on main ruleset only. -Fall also does
>     not reset limits and timeouts. Hence my first idea was to introduce
>     '-FNuke', which kills all rulesets and tables.
>
>     I don't want to change behaviour of existing option ('-Fall'), therefore
>     I'm in favor to introduce a new option. Either '-FNuke' or '-U' works
>     for me. I'm the most concerned about flushing all rulesets.
>
>     Also making "pfctl -a '_1/_2' -Fr" to remove PF 'private' rulesets works
>     for me. Actually this is the most important thing I'd like to achieve.

whatever gets done here, the initial-raw-state-forcing should be 1 operation.
not multiple operations acting on aspects of pf.

I think if it is multiple operations, people won't ever get comfortable
using it.

Reply | Threaded
Open this post in threaded view
|

Re: pfctl should allow administrator to flush _anchors

Sebastian Benoit-3
Theo de Raadt([hidden email]) on 2019.03.24 10:22:25 -0600:

> Alexandr Nedvedicky <[hidden email]> wrote:
>
> > On Sun, Mar 24, 2019 at 09:51:13AM +0100, Denis Fondras wrote:
> > > On Sun, Mar 24, 2019 at 09:24:34AM +0100, Alexandr Nedvedicky wrote:
> > > > I think all the above calls for a new standalone option, which I named as
> > > > 'Unconfigure'.  Patch below suggest unconfigure behavior for PF.
> > > > Doing 'pfctl -U' will bring PF back to its initial state (e.g. right before
> > > > pf.conf got processed during the system boot). In case of PF the proposed -U
> > > > will do following:
> > > >     - remove all rulesets and tables
> > > >     - remove all states and source nodes
> > > >     - remove all OS fingerprints
> > > >     - set all limits, timeouts and options to their defaults
> > > >
> > >
> > > Isn't -U pretty close to -Fall ?
> > >
> >
> >     it is, however -Fall operates on main ruleset only. -Fall also does
> >     not reset limits and timeouts. Hence my first idea was to introduce
> >     '-FNuke', which kills all rulesets and tables.
> >
> >     I don't want to change behaviour of existing option ('-Fall'), therefore
> >     I'm in favor to introduce a new option. Either '-FNuke' or '-U' works
> >     for me. I'm the most concerned about flushing all rulesets.
> >
> >     Also making "pfctl -a '_1/_2' -Fr" to remove PF 'private' rulesets works
> >     for me. Actually this is the most important thing I'd like to achieve.
>
> whatever gets done here, the initial-raw-state-forcing should be 1 operation.
> not multiple operations acting on aspects of pf.
>
> I think if it is multiple operations, people won't ever get comfortable
> using it.

Not sure about that: I wont be comfortable anyway, as it can cause all sorts
of problems on a running system.

When i reset things to the boot state, i would expect thats not a simple
thing and not without issues.

I consider this as a cleanup op, most useful for regress tests, developers
testing stuff etc. In normal sysadmin work i never needed it.

Reply | Threaded
Open this post in threaded view
|

Re: pfctl should allow administrator to flush _anchors

Alexandr Nedvedicky
Hello,

> > > > Isn't -U pretty close to -Fall ?
> > > >
> > >
> > >     it is, however -Fall operates on main ruleset only. -Fall also does
> > >     not reset limits and timeouts. Hence my first idea was to introduce
> > >     '-FNuke', which kills all rulesets and tables.
> > >
> > >     I don't want to change behaviour of existing option ('-Fall'), therefore
> > >     I'm in favor to introduce a new option. Either '-FNuke' or '-U' works
> > >     for me. I'm the most concerned about flushing all rulesets.
> > >
> > >     Also making "pfctl -a '_1/_2' -Fr" to remove PF 'private' rulesets works
> > >     for me. Actually this is the most important thing I'd like to achieve.
> >
> > whatever gets done here, the initial-raw-state-forcing should be 1 operation.
> > not multiple operations acting on aspects of pf.
> >
> > I think if it is multiple operations, people won't ever get comfortable
> > using it.
>
> Not sure about that: I wont be comfortable anyway, as it can cause all sorts
> of problems on a running system.
>
> When i reset things to the boot state, i would expect thats not a simple
> thing and not without issues.
>
> I consider this as a cleanup op, most useful for regress tests, developers
> testing stuff etc. In normal sysadmin work i never needed it.

    I think this is a good point. I don't expect experienced admins, who
    maintain production systems to use an unconfigure operation. It's indeed
    more useful for regression tests and people who are configuring their PF
    in sandbox/testing environment.

    how about making the '-U' (or whatever name we agree) undocumented. We can
    also make the option available if pfctl will get compiled with 'DEBUG'
    option (assuming we are doing regress on debug bits anyway).

sashan

Reply | Threaded
Open this post in threaded view
|

Re: pfctl should allow administrator to flush _anchors

Theo de Raadt-2
Alexandr Nedvedicky <[hidden email]> wrote:

>     how about making the '-U' (or whatever name we agree) undocumented. We can
>     also make the option available if pfctl will get compiled with 'DEBUG'
>     option (assuming we are doing regress on debug bits anyway).

no, it should be documented.

but few people will use it, and that's fine

Reply | Threaded
Open this post in threaded view
|

Re: pfctl should allow administrator to flush _anchors

Ted Unangst-6
In reply to this post by Alexandr Nedvedicky
Alexandr Nedvedicky wrote:
>     it is, however -Fall operates on main ruleset only. -Fall also does
>     not reset limits and timeouts. Hence my first idea was to introduce
>     '-FNuke', which kills all rulesets and tables.
>
>     I don't want to change behaviour of existing option ('-Fall'), therefore
>     I'm in favor to introduce a new option. Either '-FNuke' or '-U' works
>     for me. I'm the most concerned about flushing all rulesets.

Is the existing behavior intentional or an oversight? I don't know when I
would want to use -Fall, but keep the old timeouts, and depend on that. I'd
guess most people using -Fall are keeping old timeout only by happen stance,
and not because they desire that.

In any case, if you're seeking input on the name, something like -Freset says
to me that it resets pf back to its initial state.

Reply | Threaded
Open this post in threaded view
|

Re: pfctl should allow administrator to flush _anchors

Alexandr Nedvedicky
On Mon, Mar 25, 2019 at 10:28:40PM -0400, Ted Unangst wrote:

> Alexandr Nedvedicky wrote:
> >     it is, however -Fall operates on main ruleset only. -Fall also does
> >     not reset limits and timeouts. Hence my first idea was to introduce
> >     '-FNuke', which kills all rulesets and tables.
> >
> >     I don't want to change behaviour of existing option ('-Fall'), therefore
> >     I'm in favor to introduce a new option. Either '-FNuke' or '-U' works
> >     for me. I'm the most concerned about flushing all rulesets.
>
> Is the existing behavior intentional or an oversight? I don't know when I
> would want to use -Fall, but keep the old timeouts, and depend on that. I'd
> guess most people using -Fall are keeping old timeout only by happen stance,
> and not because they desire that.

    I had similar question on my mind when I came to PF for the first time.
    my expectations about '-Fall' were the option removes all rules (and tables)
    from kernel module. But it is not the case it acts on main ruleset only.
    Given '-Fall' works like that for ages, I see changing '-Fall' to remove
    all rules as disturbing (hence I'm in favor to introduce a new option). On
    the other hand if there will be consensus to fix '-Fall' so it will remove
    all rules (not just main ruleset), then we can forget about '-U'.

    With '-Fall' changed, we can further fix pfctl. The proposed '-U', will
    be achieved by combination of various '-F' modifiers:
        pfctl -FA -FS -Fs -Freset
    command above should revert PF driver state back to initial.

>
> In any case, if you're seeking input on the name, something like -Freset says
> to me that it resets pf back to its initial state.

    I like the '-Fresst' to reset all PF settings (variables modified by 'set')
    back to defaults.

So how people feel about changing '-Fa' to kill all rules and tables, not just
those, which are attached to main ruleset (root)?

thanks and
regards
sashan

Reply | Threaded
Open this post in threaded view
|

Re: pfctl should allow administrator to flush _anchors

Stuart Henderson
On 2019/03/26 09:38, Alexandr Nedvedicky wrote:

> On Mon, Mar 25, 2019 at 10:28:40PM -0400, Ted Unangst wrote:
> > Alexandr Nedvedicky wrote:
> > >     it is, however -Fall operates on main ruleset only. -Fall also does
> > >     not reset limits and timeouts. Hence my first idea was to introduce
> > >     '-FNuke', which kills all rulesets and tables.
> > >
> > >     I don't want to change behaviour of existing option ('-Fall'), therefore
> > >     I'm in favor to introduce a new option. Either '-FNuke' or '-U' works
> > >     for me. I'm the most concerned about flushing all rulesets.
> >
> > Is the existing behavior intentional or an oversight? I don't know when I
> > would want to use -Fall, but keep the old timeouts, and depend on that. I'd
> > guess most people using -Fall are keeping old timeout only by happen stance,
> > and not because they desire that.
>
>     I had similar question on my mind when I came to PF for the first time.
>     my expectations about '-Fall' were the option removes all rules (and tables)
>     from kernel module. But it is not the case it acts on main ruleset only.
>     Given '-Fall' works like that for ages, I see changing '-Fall' to remove
>     all rules as disturbing (hence I'm in favor to introduce a new option). On
>     the other hand if there will be consensus to fix '-Fall' so it will remove
>     all rules (not just main ruleset), then we can forget about '-U'.
>
>     With '-Fall' changed, we can further fix pfctl. The proposed '-U', will
>     be achieved by combination of various '-F' modifiers:
> pfctl -FA -FS -Fs -Freset
>     command above should revert PF driver state back to initial.
>
> >
> > In any case, if you're seeking input on the name, something like -Freset says
> > to me that it resets pf back to its initial state.
>
>     I like the '-Fresst' to reset all PF settings (variables modified by 'set')
>     back to defaults.
>
> So how people feel about changing '-Fa' to kill all rules and tables, not just
> those, which are attached to main ruleset (root)?
>
> thanks and
> regards
> sashan
>

IMHO this is a needed feature, but I agree with your hesitation about
using -Fa. This would be convenient to type, but the current documentation
for pfctl -a says:

    "In addition to the main ruleset, pfctl can load and manipulate
   additional rulesets by name, called anchors. The main ruleset is the
   default anchor."

The wording is slightly awkward but I read this as saying the current
behaviour is intended.

There's an obvious alternative user interface for this. Currently
 -a '*' is only described in conjunction with -s, but it would feel
natural to allow this to be used with -F as well, e.g.

   # pfctl -a '*' -Fa

Reply | Threaded
Open this post in threaded view
|

Re: pfctl should allow administrator to flush _anchors

Alexandr Nedvedicky
Hello,
</snip>

> >
> > So how people feel about changing '-Fa' to kill all rules and tables, not just
> > those, which are attached to main ruleset (root)?
> >
> > thanks and
> > regards
> > sashan
> >
>
> IMHO this is a needed feature, but I agree with your hesitation about
> using -Fa. This would be convenient to type, but the current documentation
> for pfctl -a says:
>
>     "In addition to the main ruleset, pfctl can load and manipulate
>    additional rulesets by name, called anchors. The main ruleset is the
>    default anchor."
>
> The wording is slightly awkward but I read this as saying the current
> behaviour is intended.
>
> There's an obvious alternative user interface for this. Currently
>  -a '*' is only described in conjunction with -s, but it would feel
> natural to allow this to be used with -F as well, e.g.
>
>    # pfctl -a '*' -Fa
>

    I like this idea to interpret "-a '*'" option in conjunction with '-F...'
    in the same way we do it for "-s" already.

    I also like tedu's idea to introduce a '-Freset'. I'll try to cook up some
    diffs. One diff will deal with "-a '*' -F..." the other will bring '-Freset'.

thanks and
regards
sashan