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 via email to