core: NFT sets by topimiettinen · Pull Request #22587 · systemd/systemd

topimiettinen · 2022-02-21T22:19:51Z

New directives ControlGroupNFTSet=, DynamicUserNFTSet=, IPv4NFTSet= and
IPv6NFTSet= provide methods for integrating configuration of control groups,
dynamic users and dynamic networks into firewall rules with NFT sets.

Examples:
ControlGroupNFTSet:

table inet filter {
        set timesyncd {
                type cgroupsv2
        }

        chain ntp_output {
                socket cgroupv2 != @timesyncd counter drop
                accept
        }
}

/etc/systemd/system/systemd-timesyncd.service.d/override.conf

[Service]
ControlGroupNFTSet=inet:filter:timesyncd

$ sudo nft list set inet filter timesyncd
table inet filter {
        set timesyncd {
                type cgroupsv2
                elements = { "system.slice/systemd-timesyncd.service" }
        }
}

DynamicUser:

table inet filter {
        set u {
                typeof meta skuid
        }

        chain service_output {
                meta skuid != @u drop
                accept
        }
}

/etc/systemd/system/dunft.service

[Service]
ExecStart=/bin/sleep 1000
DynamicUser=yes
DynamicUserNFTSet=inet:filter:u

[Install]
WantedBy=multi-user.target

$ sudo nft list set inet filter u
table inet filter {
        set u {
                typeof meta skuid
                elements = { 64864 }
        }
}
$ ps -n --format user,group,pid,command -p `pgrep sleep`
    USER    GROUP     PID COMMAND
   64864    64864   55158 /bin/sleep 1000

Network:
/etc/systemd/network/lan.network

[Network]
...
IPv4NFTSet=netdev:filter:lan-ipv4-address
IPv6NFTSet=netdev:filter:lan-ipv6-address

table netdev filter {
        set lan-ipv4-address {
                type ipv4_addr
                flags interval
        }
        chain lan_ingress {
                type filter hook ingress device "lan" priority filter; policy drop;
                ip saddr != @lan-ipv4-address drop
                accept
        }
}

$ sudo nft list set netdev filter lan-ipv4-address
table netdev filter {
        set lan-ipv4-address {
                type ipv4_addr
                flags interval
                elements = { 10.0.0.0/24 }
        }
}

Closes: #22527.

src/core/dbus-execute.c

src/core/execute.c

src/core/load-fragment.c

src/core/service.c

src/shared/bus-unit-util.c

poettering · 2022-02-22T10:59:39Z

src/shared/firewall-util-nft.c

+
+DEFINE_STRING_TABLE_LOOKUP(nfproto, int);
+
+#define NFT_SET_MSGS 3


why a macro for this? why not just specify the size literally in the variable declaration, and then use ELEMENTSOF to reference the size when you need it?

I have no preference, but similar #defines are used elsewhere in this file.

I agree that there seems to be little reason to make a define for this, which survives beyond the end of the function, and only use it once in the function.

src/test/test-nft-set.c

arianvp · 2022-02-23T21:29:00Z

Hmm it seems that in more recent nft you can actually match on Cgroup paths directly? Wouldn't that be the way to go?

e.g.:
https://man.archlinux.org/man/nft.8.en#SOCKET_EXPRESSION

table inet x {
  chain y {
    type filter hook input priority filter; policy accept;
    socket cgroupv2 level 2 "system.slice/systemd-network.service" counter
  }
}

topimiettinen · 2022-02-23T21:56:12Z

I was wondering why the filter wouldn't work as expected even though the cgroup numbers are correctly inserted to the set. Looking at here, here and here, it seems that meta cgroup operates with net_cls cgroup v1 controller.

The cgroupv2 version, socket cgroupv2 level x "path" has the same problems as cgroupv1 ID had, the path (/sys/fs/cgroup/path) is resolved immediately when the rules are loaded and this fails since the cgroups don't exist yet. This is exactly what https://github.com/mk-fg/systemd-cgroup-nftables-policy-manager tries to solve. The NFT set approach can't work since strings can't be used in sets.

Maybe cgroupv2 could be made to work with bit more invasive approach: PID1 would install filters which match cgroups (once they are realized). The filter would then set socket marks. Firewall rules would then be able to use the socket marks as they please.

topimiettinen · 2022-02-23T22:00:04Z

Hmm it seems that in more recent nft you can actually match on Cgroup paths directly? Wouldn't that be the way to go?

The path is resolved when the firewall rules are loaded and if the cgroup path doesn't exist (since the service hasn't been started yet), it will fail. This makes them equally useless directly.

Perhaps there's a way (PID1 would manage cgroupv2 rules which use socket marks and store the marks in sets so that rules can reference them).

arianvp · 2022-02-24T11:00:13Z

Yeh sounds to me like upstreaming something along the lines of https://github.com/mk-fg/systemd-cgroup-nftables-policy-manager seems to be the way to go. Also if it turns out that cgroup mark only works for net_cls (which doesn't exist in cgroups v2)

topimiettinen · 2022-02-24T12:21:34Z

Yeh sounds to me like upstreaming something along the lines of https://github.com/mk-fg/systemd-cgroup-nftables-policy-manager seems to be the way to go.

It works by loading arbitrary NFT filters when a service starts. I think this level of complexity was rejected earlier, leading to simpler IPAddressAllow=/IPAddressDeny= features. Maybe this could be implemented with ExecStartPre=+nft -f /etc/nftables-%p.conf with no changes, if the cgroup is realized and unchanging at that point. If not, perhaps that could be ensured with a new flag (like [+-@:!] now). Also ExecStartPost=+nft ... may work, though the starting order may leave a vulnerable window depending on the firewall design.

Perhaps network namespace support could be beefed up so that interfaces would be available inside the namespace without ExecStartPre= stuff.

Also if it turns out that cgroup mark only works for net_cls (which doesn't exist in cgroups v2)

This is indeed the case.

topimiettinen · 2022-02-24T22:35:18Z

The ExecStartPre=/ExecStartPost= version seems to be flaky, don't know why but the filter isn't accepted since the cgroup doesn't exist. It works if I start the service and then manually load the rules.

/etc/nftables-systemd-timesyncd.conf
#!/usr/sbin/nft -f

flush table inet io.systemd.systemd-timesyncd.service

table inet io.systemd.systemd-timesyncd.service {
        chain systemd-timesyncd_input {
                udp dport 123 counter accept
                counter drop
        }

        chain systemd-timesyncd_output {
                udp dport 123 counter accept
                counter reject with icmpx type admin-prohibited
        }

        chain input {
                type filter hook input priority filter; policy drop;

                socket cgroupv2 level 2 "system.slice/systemd-timesyncd.service" counter jump systemd-timesyncd_input
                # nothing else may use NTP
                udp dport 123 counter drop
                counter accept
        }

        chain output {
                type filter hook output priority filter; policy drop;

                socket cgroupv2 level 2 "system.slice/systemd-timesyncd.service" counter jump systemd-timesyncd_output
                # nothing else may use NTP
                udp dport 123 counter reject with icmpx type admin-prohibited
                counter accept
        }
}

/etc/systemd/system/systemd-timesyncd.service.d/nftables.conf 
[Service]
ExecStartPre=+/usr/sbin/nft -f /etc/nftables-%p.conf

$ sudo nft list ruleset|grep cgroup
                socket cgroupv2 level 2 30152 counter packets 0 bytes 0 jump systemd-timesyncd_input
                socket cgroupv2 level 2 30152 counter packets 10 bytes 760 jump systemd-timesyncd_output
$ ls -lid /sys/fs/cgroup/system.slice/systemd-timesyncd.service
30152 drwxr-xr-x. 2 root root 0 Feb 24 21:32 /sys/fs/cgroup/system.slice/systemd-timesyncd.service/

topimiettinen · 2022-03-05T20:14:44Z

Updated with DynamicUserNFTSets= directive for automatic insertion of UID of the dynamic user into NFT set.

topimiettinen · 2022-04-21T17:16:20Z

/cc @poettering

Removed also GitHub review request.

topimiettinen · 2022-05-23T17:16:13Z

Split commits for easier review, updated from lessons learned of #23269 and rebased to it.

topimiettinen · 2022-05-25T18:56:58Z

mkosi / ci (arch, rolling) fails with

‣ Error: Workspace command bootctl install returned non-zero exit code 1.

Unit tests / build (CLANG, gcrypt):

384/1394 systemd / test-repart                                                                                TIMEOUT         30.02s   killed by signal 15 SIGTERM

and CentOS CI (Arch Linux):

TEST-60-MOUNT-RATELIMIT

I don't think these are related (nothing to do with networking). @mrc0mmand do they look familiar?

mrc0mmand · 2022-05-25T19:03:28Z

Hah, you've hit the bingo of known issues:

mkosi / ci (arch, rolling) fails with
‣ Error: Workspace command bootctl install returned non-zero exit code 1.

That's #23485.

Unit tests / build (CLANG, gcrypt):

384/1394 systemd / test-repart                                                                                TIMEOUT         30.02s   killed by signal 15 SIGTERM

Spurious, but known timeout.

and CentOS CI (Arch Linux):
TEST-60-MOUNT-RATELIMIT

Aaand that's #23424.

I don't think these are related (nothing to do with networking). @mrc0mmand do they look familiar?

I'll retrigger the jobs and will cross my fingers for better luck :-)

topimiettinen · 2022-05-26T06:22:46Z

@mrc0mmand All green!

@yuwata @poettering please review.

topimiettinen · 2022-05-29T08:25:14Z

I converted my firewalls to use cgroups for examining packets: first by cgroup level 1 (system.slice / user.slice), then by L4 protocol and port, and finally again by cgroup level 2 (for system services). The order could be equally cgroup levels 1 & 2 first and then by L4 protocol and port for the service.

table inet filter {
        set system_slice {
                type cgroupsv2
        }

        set user_slice {
                type cgroupsv2
        }

        set systemd_networkd {
                type cgroupsv2
        }
...
        chain dhcpd_output {
                meta skuid != systemd-network reject with icmpx type admin-prohibited
....
                accept
        }
...
        chain tcp_output_sys {
                tcp dport { 547, 548, 647, 847, 7911 } socket cgroupv2 level 2 @systemd_networkd jump dhcpd_output
...
                reject with icmpx type admin-prohibited
        }
...
        chain system_slice_out {
                meta l4proto vmap { tcp : jump tcp_output_sys, udp : jump udp_output_sys }
                reject with icmpx type admin-prohibited
        }
...
        chain input {
                type filter hook input priority filter; policy drop;
...
                ct state new socket cgroupv2 level 1 @system_slice jump system_slice_in
                ct state new socket cgroupv2 level 1 @user_slice jump user_slice_in
...
        }
...
        chain output {
                type filter hook output priority filter; policy drop;
...
                ct state new socket cgroupv2 level 1 @system_slice jump system_slice_out
                ct state new socket cgroupv2 level 1 @user_slice jump user_slice_out
...
        }
...
}

This kind of setup allows very fine firewall control over individual services since all NFT features can be used at that point. This can be contrasted with IPAddressDeny=, which also gives a simple firewall for services, or more powerful BPFProgram= which isn't very easy to use.

The feature doesn't work well for user services since the cgroups are realized by user manager, which doesn't have privileges to fill in the NFT sets. That doesn't matter much for server systems. I suppose a trusted SUID program like Firejail could manage the sets for desktop apps.

Flushing the NFT rulesets also clears the NFT sets. Then PID1 only repopulates them when the cgroup is restarted and that isn't ever going to happen for system.slice / user.slice, so the filters may become invalid. A workaround could be that PID1 would also repopulate the sets on for example daemon-reload or a DBus call, but it could be also argued that there could be a feature on NFT side to keep the sets, the admin should be careful when flushing rulesets, or that kernel should use cgroup paths instead of cgroup IDs.

New directives `NFTSet=`, `IPv4NFTSet=` and `IPv6NFTSet=` provide a method for integrating configuration of dynamic networks into firewall rules with NFT sets. /etc/systemd/network/eth.network ``` [DHCPv4] ... NFTSet=netdev:filter:eth_ipv4_address ``` ``` table netdev filter { set eth_ipv4_address { type ipv4_addr flags interval } chain eth_ingress { type filter hook ingress device "eth0" priority filter; policy drop; ip saddr != @eth_ipv4_address drop accept } } ``` ``` sudo nft list set netdev filter eth_ipv4_address table netdev filter { set eth_ipv4_address { type ipv4_addr flags interval elements = { 10.0.0.0/24 } } } ```

New directive `ControlGroupNFTSet=` provides a method for integrating services into firewall rules with NFT sets. Example: ``` table inet filter { ... set timesyncd { type cgroupsv2 } chain ntp_output { socket cgroupv2 != @timesyncd counter drop accept } ... } ``` /etc/systemd/system/systemd-timesyncd.service.d/override.conf ``` [Service] ControlGroupNFTSet=inet:filter:timesyncd ``` ``` $ sudo nft list set inet filter timesyncd table inet filter { set timesyncd { type cgroupsv2 elements = { "system.slice/systemd-timesyncd.service" } } } ```

New directive `DynamicUserNFTSet=` provides a method for integrating configuration of dynamic users into firewall rules with NFT sets. Example: ``` table inet filter { set u { typeof meta skuid } chain service_output { meta skuid != @U drop accept } } ``` ``` /etc/systemd/system/dunft.service [Service] DynamicUser=yes DynamicUserNFTSet=inet:filter:u ExecStart=/bin/sleep 1000 [Install] WantedBy=multi-user.target ``` ``` $ sudo nft list set inet filter u table inet filter { set u { typeof meta skuid elements = { 64864 } } } $ ps -n --format user,group,pid,command -p `pgrep sleep` USER GROUP PID COMMAND 64864 64864 55158 /bin/sleep 1000 ```

daandemeyer · 2022-06-08T16:29:12Z

@topimiettinen Did this get another look by @poettering or @yuwata?

topimiettinen · 2022-06-08T18:44:14Z

@topimiettinen Did this get another look by @poettering or @yuwata?

Not that I know of.

yuwata · 2022-06-09T03:18:08Z

Same here. This needs final review. Temporary setting 'merged-but-needs-fixing' label.

keszybz · 2022-06-09T07:40:52Z

man/systemd.network.xml

+          <para>These settings provide a method for integrating dynamic network configuration into firewall
+          rules with NFT sets. These options expect a whitespace separated list of NFT set definitions. Each


Please link to some explanation of NFT.

keszybz · 2022-06-09T07:43:55Z

src/basic/parse-util.c

+        if ((c >= 'a' && c <= 'z') ||
+            (c >= 'A' && c <= 'Z') ||
+            (c >= '0' && c <= '9') ||
+            c == '/' || c == '\\' || c == '_' || c == '.')
+                return false;
+        return true;


Maybe call this next_char_good() (positive conditions are easier to grok, we generally prefer positive booleans and checks), and just write

return (c >= 'a' && c <= 'z') || (c >= 'A' && c <= 'Z') || ...;

keszybz · 2022-06-09T07:44:58Z

src/shared/firewall-util-nft.c

+        r = nft_set_element_op_in_addr(nfnl, table, set,
+                                       add, nfproto, af, address, prefixlen);
+
+        (void) in_addr_prefix_to_string(af, address, prefixlen, &addr_str);
+
+        log_debug("%s NFT family %s table %s set %s IP addresss %s", add? "Added" : "Deleted",
+                  nfproto_to_string(nfproto), table, set, strna(addr_str));


Hmm, if r < 0, this debug stmt is wrong.

keszybz · 2022-06-09T07:45:24Z

src/shared/firewall-util-nft.c

+        [NFPROTO_ARP] = "arp",
+        [NFPROTO_BRIDGE] = "bridge",
+        [NFPROTO_INET] = "inet",
+        [NFPROTO_IPV4] = "ip",
+        [NFPROTO_IPV6] = "ip6",
+        [NFPROTO_NETDEV] = "netdev",


Please align the strings to the same columns.

keszybz · 2022-06-09T07:46:16Z

src/shared/firewall-util-nft.c

+
+DEFINE_STRING_TABLE_LOOKUP(nfproto, int);
+
+#define NFT_SET_MSGS 3


I agree that there seems to be little reason to make a define for this, which survives beyond the end of the function, and only use it once in the function.

keszybz · 2022-06-09T07:51:02Z

src/shared/firewall-util-nft.c

+        ++tsize;
+        assert(tsize < ELEMENTSOF(transaction));
+        r = sd_nfnl_message_batch_end(nfnl, &transaction[tsize]);
+        if (r < 0)
+                return r;
+
+        ++tsize;


Why is tsize incremented twice?

See similar uses in for example above line 1091, tsize is used to keep track of used space in the array and assert()s guard that the array size isn't exceeded at any point.

keszybz · 2022-06-09T07:53:00Z

src/shared/firewall-util-nft.c

+                }
+
+                if (nft_identifier_bad(table))
+                        return log_syntax(unit, LOG_WARNING, filename, line, 0, "Invalid table name %s, ignoring", table);


Here too, cunescape probably needs to be used.

keszybz · 2022-06-09T07:53:04Z

src/shared/firewall-util-nft.c

+                        return log_syntax(unit, LOG_WARNING, filename, line, 0, "Invalid table name %s, ignoring", table);
+
+                if (nft_identifier_bad(set))
+                        return log_syntax(unit, LOG_WARNING, filename, line, 0, "Invalid set name %s, ignoring", set);


Add links to documentation to NetLabel, LSM, Netlabel Fallback Peer Labeling and NFT. Use a positive boolean function nft_identifier_good() to help grok the checks more easily than nft_identifier_bad(). Escape logged invalid items. Add an ASSERT_PTR() for 'data' in config_parse_netlabel(). Use a different debug message when NFT operation fails. Align nfproto_table strings. Remove NFT_SET_MSGS, it's only used once.

topimiettinen · 2022-06-11T15:39:27Z

Created #23712 for the fixes.

This reverts PR systemd#22587 and its follow-up commit. More specifically, 2299b1c (partially), e176f85, ceb46a3, and 51bb907. The PR was merged without final approval, and has several issues: - OSS fuzz reported issues in the conf parser, - It calls synchrnous netlink call, it should not be especially in PID1, - The importance of NFTSet for CGroup and DynamicUser may be questionable, at least, there was no justification PID1 should support it. - For networkd, it should be implemented with Request object, - There is no test for the feature. Fixes systemd#23711. Fixes systemd#23717. Fixes systemd#23719. Fixes systemd#23720. Fixes systemd#23721. Fixes systemd#23759.

poettering

Hmpf, this doesn't look ready to me yet.

Is there a test for this?

poettering · 2022-08-09T09:08:51Z

man/systemd.network.xml

+
+      <varlistentry>
+        <term><varname>IPv4NFTSet=</varname><replaceable>family</replaceable>:<replaceable>table</replaceable>:<replaceable>set</replaceable></term>
+        <term><varname>IPv6NFTSet=</varname><replaceable>family</replaceable>:<replaceable>table</replaceable>:<replaceable>set</replaceable></term>


BTW, I think it would make sense to implicitly use the network file name as the set name, if not specified, to push people to a clear nomenclature where each .network file gets its own set. i.e. if NFTSet= is set to just "netdev:filter" in a file foobar.network then this would be equivalent to "netdev:filter:foobar"

BTW, does it really make sense to have two distinct settings for IPv4 and IPv6 here? why is this in the lvalue instead of the rvalue? Can you explain?

Can't the two kind of addresses be added to the same set? (no nft pro here, as you might guess).

it appears icky to me to allow ipv4 and ipv configuration here to be entirely orthogonal.

poettering · 2022-08-09T09:13:42Z

man/systemd.resource-control.xml

        </listitem>
      </varlistentry>
+      <varlistentry>
+        <term><varname>ControlGroupNFTSet=</varname><replaceable>family</replaceable>:<replaceable>table</replaceable>:<replaceable>set</replaceable></term>


As in the .network patch: I think the set part should be optional, and we should imply the unit name there, so that set names are by default derived from the unit names.

And I am not sure this should carry "Controlgroup" in the name really. that is an implementation detail I think → #24255

poettering · 2022-08-09T09:15:15Z

src/core/cgroup.c

+                                 nfproto_to_string(s->nfproto),
+                                 s->table,
+                                 s->set,
+                                 u->cgroup_id);


you must validate u->cgroup_id first here, it's not guaranteed to be initialized, as we require privs for that, we might not have

poettering · 2022-08-09T09:15:21Z

src/core/cgroup.c

+                                 nfproto_to_string(s->nfproto),
+                                 s->table,
+                                 s->set,
+                                 u->cgroup_id);


poettering · 2022-08-09T09:17:36Z

src/basic/parse-util.c

+            (c >= 'A' && c <= 'Z'))
+                return false;
+        return true;
+}


this is weird. In all other cases where we validate strings we keep lists of what is good, not what is bad, and the functions hence are called xyz_valid() or xyz_is_valid() or valid_xyz() or so. This here is the only one that is inverted. Could you please swap this around to match the general style?

poettering · 2022-08-09T09:20:14Z

src/core/cgroup.c

+        CGroupContext *c;
+
+        assert(u);
+


this should not even be attempted if we aren't in cgroupv2 unified mode

poettering · 2022-08-09T09:23:06Z

man/systemd.exec.xml

+    <variablelist class='unit-directives'>
+
+      <varlistentry>
+        <term><varname>DynamicUserNFTSet=</varname><replaceable>family</replaceable>:<replaceable>table</replaceable>:<replaceable>set</replaceable></term>


this sounds awfully specific to me. I mean, I can see the usefulness, but why limit this to dynamic users? Why not do that for any UID here? it's really unnecessarily restricting to just say dynamic users.

Given this leaks into the unit file syntax, this really should be fixed before 252

Also, as in the other case I am sure the third part of the expression should be derived from the unit name automatically, if not specified, to push people to a "standardized" name of sets. And the examples provided should always make use of these default namings, instead of making up their own

poettering · 2022-08-09T09:23:59Z

src/shared/bus-unit-util.c

+                if (l < 0)
+                        return log_error_errno(l, "Failed to unescape %s= value: %s", field, eq);
+
+                r = sd_bus_message_append(m, "(iss)", family, word, eq);


missing error handling

poettering · 2022-08-09T09:24:42Z

src/shared/bus-unit-util.c

+                        return log_error_errno(r, "Failed to parse %s: %m", field);
+                if (isempty(word) || isempty(eq)) {
+                        log_error("Failed to parse %s", field);
+                        return 0;


return 0?

should be return log_error_errno(SYNTHETIC_ERRNO(EINVAL), …

(similar above)

poettering · 2022-08-09T09:27:06Z

src/core/execute.c

                if (dcreds->user)
                        username = dcreds->user->name;

+                exec_add_dynamic_user_nft_set(context, uid);


please don't do this from the child, but PID 1, i.e. we send the resolved UID/GID to PID 1 further down, look for send_user_lookup(). Put the registration in the NFT tables at the receiving end of that.

topimiettinen added network resolve pid1 do-not-merge 💣 labels Feb 21, 2022