netfilter buglog - Oct 2023 - [Bug 1715] New: __netlink_gen_concat_key assertion raised by attempt to expand set-defining variable as a set key

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

            Bug ID: 1715
           Summary: __netlink_gen_concat_key assertion raised by attempt
                    to expand set-defining variable as a set key
           Product: nftables
           Version: 1.0.x
          Hardware: x86_64
                OS: All
            Status: NEW
          Severity: normal
          Priority: P5
         Component: nft
          Assignee: pablo at netfilter.org
          Reporter: kfm at plushkava.net

Consider the following ruleset.

define ext_if = { "eth0", "eth1" }
table ip filter {
    chain c {
        iifname . tcp dport { $ext_if . 22 } accept
    }
}

Attempting to load this ruleset results in SIGABRT.

BUG: invalid expression type 'set' in setnft: netlink.c:304:
__netlink_gen_concat_key: Assertion `0' failed.
Aborted (core dumped)

I presume that this is a consequence of ntables not knowing how to treat such a
variable, which implies the generation of multiple elements. Ideally, nftables
would be able to handle this and commpose the set as { "eth0" . 22,
"eth1" . 22
}. Otherwise, it should instead print an informative diagnostic message and
exit, rather than die to a signal.

-- 
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/20231025/789a432c/attachment.html>

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

kfm at plushkava.net changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Blocks|                            |1461

-- 
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/20231025/d657eb3e/attachment.html>

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

--- Comment #1 from kfm at plushkava.net ---
My mistake. It's only now that I realise that the crash is partly a
side-effect
of having forgotten to specify the table name in the sample ruleset. Still, for
the process to abort is not a good outcome. I shall amend the summary
accordingly.

-- 
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/20231025/0f41e197/attachment.html>

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

kfm at plushkava.net changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
            Summary|__netlink_gen_concat_key    |__netlink_gen_concat_key
                   |assertion raised by attempt |assertion raised by
                   |to expand set-defining      |expanding set-defining
                   |variable as a set key       |variable as a set key with
                   |                            |table name missing

-- 
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/20231025/cc276735/attachment.html>

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

--- Comment #2 from kfm at plushkava.net ---
Please disregard comment #1. I was fatigued and not thinking clearly. Of
course, the table name isn't missing and the initial report stands.

-- 
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/20231025/d3018c65/attachment.html>

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

kfm at plushkava.net changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
            Summary|__netlink_gen_concat_key    |__netlink_gen_concat_key
                   |assertion raised by         |assertion raised by
                   |expanding set-defining      |expanding set-defining
                   |variable as a set key with  |variable as a component of
                   |table name missing          |a set key

-- 
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/20231025/31b1b30b/attachment.html>

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

Phil Sutter <phil at nwl.cc> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |phil at nwl.cc

--- Comment #3 from Phil Sutter <phil at nwl.cc> ---
Just for the record: This is neither a crash nor "dying to a signal".
It's
merely an assert() call triggering because the parser constructed something the
remaining code can't handle. Effectively this is a case of missing error
handling (or insufficient parser strictness), not a bug.

I guess the given ruleset works if you pull the second concat part into the
defined variable like so:

define ext_if = { "eth0" . 22, "eth1" . 22 }
[...]
    iifname .tcp dport $ext_if accept
[...]

right?

-- 
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/20231025/85cd2af3/attachment.html>

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

--- Comment #4 from Pablo Neira Ayuso <pablo at netfilter.org> ---
There is code missing in the evaluation path to reject this, so a nice error
report is provided, e.g. "cannot use set as tuple in concatenation".

Another possibility would be to support for expansions, but I am not sure I
want to enter that path, so I would start by simply fixing this late bug report
coming in the bytecode generation step.

-- 
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/20231025/addbc548/attachment.html>

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

--- Comment #5 from kfm at plushkava.net ---
(In reply to Phil Sutter from comment #3)
> Just for the record: This is neither a crash nor "dying to a
signal". It's
> merely an assert() call triggering because the parser constructed something
> the remaining code can't handle. Effectively this is a case of missing
error
> handling (or insufficient parser strictness), not a bug.
Firstly, my ability to write and work with C is rather modest, so my
terminology might not be strictly correct. Still, if I run something in the
shell and see "Aborted (core dumped)" and can also see that the value
of $? is
134, it hardly seems inaccurate to say that the process did, in fact, die as a
consequence of SIGABRT (134 - 126 being 6). Clearly, nft did not get the
opportunity to formally exit. So, if it did not die as a consequence of the
ABRT signal, how would you describe what happened?

I understand that the use of assert(3) is wilful and why you would take issue
with using "crash" as a term.

I do consider this as to be a genuine bug because I do not consider assert(3)
to be an appropriate means of conveying an error to the end-user, even if it be
unintentional. I wouldn't think this to be a contentious perspective. While
the
assertion served a purpose - and is better than nothing - I am certain that the
end-user would prefer to see a formal diagnostic message, with nft having the
opportunity to exit with a non-zero status value. As such, I always report
assertions caused by the parser wherever I encounter them. I have reason to
believe that such reports are welcome as prior reports in this vein have been
attended to, for which I am grateful.
> 
> I guess the given ruleset works if you pull the second concat part into the
> defined variable like so:
> 
> define ext_if = { "eth0" . 22, "eth1" . 22 }
> [...]
>     iifname .tcp dport $ext_if accept
> [...]
> 
> right?
Yes, it does.

-- 
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/20231025/10c10801/attachment.html>

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

--- Comment #6 from Pablo Neira Ayuso <pablo at netfilter.org> ---
(In reply to kfm from comment #5)
> I do consider this as to be a genuine bug because I do not consider
> assert(3) to be an appropriate means of conveying an error to the end-user,
> even if it be unintentional.
Agreed, looking into this to fix 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/20231025/b1eacb9a/attachment-0001.html>

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

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

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

--- Comment #7 from Pablo Neira Ayuso <pablo at netfilter.org> ---
Proposed patch:

https://patchwork.ozlabs.org/project/netfilter-devel/patch/20231025141210.124123-1-pablo
at netfilter.org/

-- 
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/20231025/ba6a2de0/attachment.html>

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

--- Comment #8 from kfm at plushkava.net ---
Thank you, as ever.

-- 
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/20231025/84ad985b/attachment.html>

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

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

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

-- 
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/20231026/6b948195/attachment.html>

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

--- Comment #9 from Pablo Neira Ayuso <pablo at netfilter.org> ---
http://git.netfilter.org/nftables/commit/?id=4b6a4ad9134fa71277c2ff7f92776e1faeb83000

-- 
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/20231026/39f8e365/attachment.html>