netfilter buglog - Sep 2017 - [Bug 1184] New: disable implicit concatenating of elements of sets with flag interval

https://bugzilla.netfilter.org/show_bug.cgi?id=1184

            Bug ID: 1184
           Summary: disable implicit concatenating of elements of sets
                    with flag interval
           Product: nftables
           Version: unspecified
          Hardware: x86_64
                OS: All
            Status: NEW
          Severity: enhancement
          Priority: P5
         Component: nft
          Assignee: pablo at netfilter.org
          Reporter: karel at unitednetworks.cz

If consequent elements are added to set with flag interval in one command, they
get concatenated. But when they are added with separate commands, they
don't.
This is inconsistent behaviour and needs to be fixed. Listing of such sets and
keeping them in consistence with external data is problematic because of that.

Please disable implicit concatenating for good (maybe except sets with flag
constant) or introduce nft option to let concatenating is not performed for
given operation.

Current (bad) behaviour:

a) adding consequent elements in one command

nft add set t test_set {type ipv4_addr\; flags interval\;}
nft add element t test_set {192.168.0.0, 192.168.0.1}
nft list set t test_set

table ip t {
        set test_set {
                type ipv4_addr
                flags interval
                elements = { 192.168.0.0/31 }
        }
}

b) adding consequent elements in separate commands

nft flush set t test_set
nft add element t test_set {192.168.0.0}
nft add element t test_set {192.168.0.1}
nft list set t test_set

table ip t {
        set test_set {
                type ipv4_addr
                flags interval
                elements = { 192.168.0.0, 192.168.0.1 }
        }
}

-- 
You are receiving this mail because:
You are watching all bug changes.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.netfilter.org/pipermail/netfilter-buglog/attachments/20170921/921f0d5b/attachment.html>

https://bugzilla.netfilter.org/show_bug.cgi?id=1184

--- Comment #1 from Karel Rericha <karel at unitednetworks.cz> ---
Probably the best way would be to concatenate elements internally in kernel
data structure, but let userspace still see separate elements.

Anyway it would probably require significant amount of work and things would
get more complicated. So I will be perfectly ok with nft option :)

Thanks in advance ;)

-- 
You are receiving this mail because:
You are watching all bug changes.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.netfilter.org/pipermail/netfilter-buglog/attachments/20170921/65ba6124/attachment.html>

https://bugzilla.netfilter.org/show_bug.cgi?id=1184

--- Comment #2 from Karel Rericha <karel at unitednetworks.cz> ---
To add insult to injury:

nft add element t test_set {192.168.0.0, 192.168.0.1}
nft delete element t test_set {192.168.0.0}

Error: Could not process rule: No such file or directory
delete element t test_set {192.168.0.0}
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

-- 
You are receiving this mail because:
You are watching all bug changes.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.netfilter.org/pipermail/netfilter-buglog/attachments/20170921/4b6c0a4d/attachment.html>

https://bugzilla.netfilter.org/show_bug.cgi?id=1184

Pablo Neira Ayuso <pablo at netfilter.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED

--- Comment #3 from Pablo Neira Ayuso <pablo at netfilter.org> ---
Hi Karel,

I agree the current behaviour very sloppy.

#1 We can add a no-automerge flag to disable this.
#2 Or, we can just make this the default behaviour, and add a automerge flag.

#2 breaks backward compatibility, but if this behaviour is so
counter-intuitive. Taking into account that set intervals have been broken
until very recently, we can follow this path.

Let me know, thanks for reporting! This sort of feedback is very welcome.

-- 
You are receiving this mail because:
You are watching all bug changes.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.netfilter.org/pipermail/netfilter-buglog/attachments/20170921/6176a588/attachment.html>

https://bugzilla.netfilter.org/show_bug.cgi?id=1184

--- Comment #4 from Karel Rericha <karel at unitednetworks.cz> ---
Hi Pablo,

I would vote for variant #2.

Disable automerge as default and add automerge flag. True it might break some
very specific case when someone is expecting implicit automerge, but I would
say it will be very rare. Much more often people will run into problems not
expecting implicit automerge.

And by getting non-automerge as default, I would guess one can safely enable
timeout flag along with interval flag (considering automerge and timeout flags
mutually exclusive). See

https://bugzilla.netfilter.org/show_bug.cgi?id=1180

-- 
You are receiving this mail because:
You are watching all bug changes.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.netfilter.org/pipermail/netfilter-buglog/attachments/20170921/9f42bd70/attachment.html>

https://bugzilla.netfilter.org/show_bug.cgi?id=1184

--- Comment #5 from Pablo Neira Ayuso <pablo at netfilter.org> ---
Hi Karel,

(In reply to Karel Rericha from comment #4)
> Hi Pablo,
> 
> I would vote for variant #2.
> 
> Disable automerge as default and add automerge flag. True it might break
> some very specific case when someone is expecting implicit automerge, but I
> would say it will be very rare. Much more often people will run into
> problems not expecting implicit automerge.
Agreed.

If we go for this variant, we would need to disable automerge in implicit sets
by default too, eg.

   # nft add rule x y ip saddr { 1.1.1.1, 1.1.1.2, 1.1.1.4-1.1.1.6 }

# nft list ruleset
...
             ip saddr { 1.1.1.1-1.1.1.2, 1.1.1.4-1.1.1.6 }

So we don't automagically do this things. I would say it's better if we
leave
this feature for someone that the user can explicitly request, though global
policy, or through some new nft option to request an explicit ruleset
optimization.

-- 
You are receiving this mail because:
You are watching all bug changes.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.netfilter.org/pipermail/netfilter-buglog/attachments/20171002/03a7cdeb/attachment.html>

https://bugzilla.netfilter.org/show_bug.cgi?id=1184

Jeff Kletsky <netfilter at allycomm.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |netfilter at allycomm.com

--- Comment #6 from Jeff Kletsky <netfilter at allycomm.com> ---
I'm wholeheartedly behind disabling auto-merge of ranges!

Auto-merge is problematic for deletion and I'm glad that it was brought up. 

If I add an element to a set, I should be able to delete that element later.
This allows dynamic behavior, for example by IP address. Just because two IP
addresses or ranges are coincident (or overlapping) doesn't mean that they
can
be treated as a single entity from that point onward. Especially in the case
that two ranges overlap, what does it mean to delete one of them from the set?
Do you keep or remove the intersection?

Auto-merging makes confirming that the ruleset in memory is actually what was
intended. An example of this is the IPv6 address space, where I may enter
ranges that I wish to block based on their assignments (or lack thereof) and
would prefer not to have to manually confirm that the union of my inputs
correspond with the entries shown.

-- 
You are receiving this mail because:
You are watching all bug changes.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.netfilter.org/pipermail/netfilter-buglog/attachments/20171006/02c54261/attachment.html>

https://bugzilla.netfilter.org/show_bug.cgi?id=1184

--- Comment #7 from Karel Rericha <karel at unitednetworks.cz> ---
(In reply to Pablo Neira Ayuso from comment #5)
> Hi Karel,
> 
> (In reply to Karel Rericha from comment #4)
> If we go for this variant, we would need to disable automerge in implicit
> sets by default too, eg.
> 
>    # nft add rule x y ip saddr { 1.1.1.1, 1.1.1.2, 1.1.1.4-1.1.1.6 }
> 
> # nft list ruleset
> ...
>              ip saddr { 1.1.1.1-1.1.1.2, 1.1.1.4-1.1.1.6 }
> 
> So we don't automagically do this things. I would say it's better
if we
> leave this feature for someone that the user can explicitly request, though
> global policy, or through some new nft option to request an explicit
ruleset
> optimization.
Hi Pablo,

I am back from vacation. Regarding anonymous (implicit) sets automerge is good
thing and we can leave it enabled by default, because it doesnt break things,
so let anonymous sets have hidden automerge flag (they have actually already
hidden interval flag).

But in named sets automerge break things for sure. So let it be explicit flag.

And I am against moving this to some kind of global policy or option for few
reasons:
- optimization by my feeling should never break things
- it is perfectly possible to request automerge and non automerge interval
named sets in one ruleset
- disable automerge on anonymous sets brings only one thing: set will appear
different in rule listing than it was entered (and that is inconsistency we can
live with, because it is already broken by hidden interval flag in anonymous
sets, see following ...)

nft add rule x y ip saddr { 1.1.1.0-1.1.1.1 } accept

lists already as

ip saddr { 1.1.1.0/31 } accept


So we can make automerge explicit in anonymous sets somehow too, but it has no
point if we dont make interval flag explicit too.

-- 
You are receiving this mail because:
You are watching all bug changes.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.netfilter.org/pipermail/netfilter-buglog/attachments/20171009/18ffe584/attachment.html>

https://bugzilla.netfilter.org/show_bug.cgi?id=1184

--- Comment #8 from Karel Rericha <karel at unitednetworks.cz> ---
Let me summarize:

Automerge is good thing for performance, but it must be controllable, because
sometimes it can break things.

Anonymous sets:
- they already behave like they would have hidden interval and automerge flags,
so it would be probably easy to let these flags are added in rule parsing

Named sets:
- please disable automerge by default and make it explicit as is interval flag
- the only exception to this could be sets with constant flag, but I would
leave it explicit even in this case

Optionally:
- for consistency with rule listing (and parsing by external program) make
flags in anonymous sets explicit, but it would probably require more work and
it is not something really needed, because people always can use named set, so
I am voting against it

-- 
You are receiving this mail because:
You are watching all bug changes.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.netfilter.org/pipermail/netfilter-buglog/attachments/20171009/519f4464/attachment.html>

https://bugzilla.netfilter.org/show_bug.cgi?id=1184

--- Comment #9 from Jeff Kletsky <netfilter at allycomm.com> ---
As discussed on the nft mailing list on 2018-01-20

Prohibiting overlapping intervals in the same set is problematic on several
levels, including
* Dynamic firewalls, such as behavior-based ones
* Clear rule set definition
* Single-point management of addresses

The changes introduced by

commit 9a4b513014cfdeaad6d247b72a7924b3a536cfe9
Author: Phil Sutter <phil at nwl.cc>
Date:   Wed Jan 10 21:32:04 2018 +0100

    src: Don't merge adjacent/overlapping ranges

    Previously, when adding multiple ranges to a set they were merged if
    overlapping or adjacent. This might cause inconvenience though since it
    is afterwards not easily possible anymore to remove one of the merged
    ranges again while keeping the others in place.

    Since it is not possible to have overlapping ranges, this patch adds a
    check for newly added ranges to make sure they don't overlap if merging
    is turned off.

    [...]


have broken rule sets here. 

The changes break both explicit sets, as well as implicit sets within rules.

I am puzzled as to why "it is not possible to have overlapping ranges"

As long as the left/right of the interval are unique within the set, then the
element is unique.

I don't see any reason why a clear, efficient algorithm could not be
developed
for overlapping intervals. If a user intentionally creates overlapping (or
contained) ranges, that is a conscious decision. Similarly, a dynamically
managed firewall should not have to read out the current state of the set to
decide how to add or remove a host or net block. At least for me, I would not
expect that the implementation would "magically optimize" the tests;
I've
explicitly indicated two tests, I'm not going to be surprised if two
comparisons are done as a result.

-- 
You are receiving this mail because:
You are watching all bug changes.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.netfilter.org/pipermail/netfilter-buglog/attachments/20180121/20641103/attachment.html>

https://bugzilla.netfilter.org/show_bug.cgi?id=1184

--- Comment #10 from Karel Rericha <karel at unitednetworks.cz> ---
Hi Jeff,

I will try to clarify, what IMHO really happened here. Sets behaviour was
inconsistent and Phil's patch just fixed this.

So for now this is fixed. But yes, it can be problematic for some people who
misused this inconsistency in their own ruleset. And for such cases (not only
for them) I proposed to introduce automerge flag. Which should not be hard to
implement and very easy to use in rulesets which require it.

And if I read it right, Phil in his patch states that overlapping ranges are
not allowed if merging is turned off, but so far no mechanism for turning on
automerge exists. So maybe if someone would ask Phil nicely, he would implement
automerge flag and everybody would be happy ;)

-- 
You are receiving this mail because:
You are watching all bug changes.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.netfilter.org/pipermail/netfilter-buglog/attachments/20180122/339c8860/attachment.html>

https://bugzilla.netfilter.org/show_bug.cgi?id=1184

Pablo Neira Ayuso <pablo at netfilter.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |RESOLVED
         Resolution|---                         |FIXED

--- Comment #11 from Pablo Neira Ayuso <pablo at netfilter.org> ---
Fixed in upstream commit:

http://git.netfilter.org/nftables/commit/?id=30f667920601d01107398cbb85da45fdb1237212

-- 
You are receiving this mail because:
You are watching all bug changes.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.netfilter.org/pipermail/netfilter-buglog/attachments/20180202/531bbe7f/attachment-0001.html>