QubesOS / qubes-issues

The Qubes OS Project issue tracker
https://www.qubes-os.org/doc/issue-tracking/
527 stars 46 forks source link

Allow modification of existing nftables `qubes` chains #9056

Open emanruse opened 3 months ago

emanruse commented 3 months ago

The problem you're addressing (if any)

It has been explained that:

Modifying the chains provided by Qubes OS isn’t supported, but should also not be necessary. If it is necessary, please file an issue.

Hence the current issue.

Here is a case, where modifying an existing chain in ip qubes table is necessary.

[The documentation]((https://qubes-os.org/doc/firewall/#where-to-put-firewall-rules) says:

For service qubes supplying networking (sys-firewall and sys-net inclusive), nftables commands should be added to /rw/config/qubes-firewall-user-script.

Suppose one creates a custom sys-dns qube (a network service qube) and wants to use it for resolving instead of 10.139.1.1 and 10.139.1.2. As part of that customization, it makes sense to replace chain dnat-dns in table ip qubes, so that DNS traffic goes to the proper qube:

#!/usr/sbin/nft -f

define sys_dns_addr = <the IP address of sys-dns qube>

add chain ip qubes dnat-dns                                                     
delete chain ip qubes dnat-dns

table ip qubes {
    # ...
    chain dnat-dns {
        type nat hook prerouting priority dstnat; policy accept;
        meta l4proto { tcp, udp } th dport 53 dnat to $sys_dns_addr
    }
}
  1. Place the above snippet in /rw/config/qubes-firewall.d/10-firewall of a firewall qube and make it executable.

  2. Reboot the firewall qube.

Expected:

The overriding should work as expected.

Actual:

qubes-firewall.service starts before qubes-network.service. The latter runs /usr/lib/qubes/init/network-proxy-setup.sh which calls /usr/lib/qubes/qubes-setup-dnat-to-ns, which modifies chain dnat-dns in table ip qubes, thus overriding any potential customization of that chain made through /rw/config/qubes-firewall.d.

The solution is not to do this through /rw/config/rc.local because it is a general network security principle to set up before network start.

The other possible solution (custom table taking priority) can be less efficient as it would result in keeping unused rules.

The solution you'd like

Allow customization of existing qubes chains (perhaps by not modifying firewall rules after the firewall service has been started).

Correct the documentation which currently does not explain what the linked comment explains.

The value to a user, and who that user might be

More efficient firewall customization without ruleset bloat for those who use custom network service qubes.

marmarek commented 3 months ago

You don't need (nor should) modify existing dnat-dns chain. Create a new one (preferably with a custom- prefix) and give it an earlier priority. You can do that using the existing /rw/config/qubes-firewall.d mechanism.

emanruse commented 3 months ago

I know I can and I do exactly that.

But what good is keeping existing chain if it won't be used anyway?

If that is the approach, then we can similarly say there is no need for custom-input/forward, just use another chain with lower priority. Is it not suboptimal to overcomplicate the whole ruleset just to customize a small part of it? What is the benefit of that?

marmarek commented 3 months ago

If that is the approach, then we can similarly say there is no need for custom-input/forward, just use another chain with lower priority

No, there is an important difference here. An earlier "accept" rule (in another chain) won't override later "drop". But an earlier dnat will override later dnat. That's the main (and TBH the only) reason why we provide pre-created custom-input/forward chains. Otherwise a new chains would be preferred for those cases too.

DemiMarie commented 3 months ago

I think there are a few issues here:

  1. There is no obvious way to run something before network goes up, such that the network only goes up if the something succeeds. I say “obvious” because one can do this with a custom systemd unit, but that is way more effort than should be necessary. It’s much simpler to just have a directory for such scripts.
  2. Documentation problems with nftables.
marmarek commented 3 months ago

There is no obvious way to run something before network goes up, such that the network only goes up if the something succeeds. I say “obvious” because one can do this with a custom systemd unit, but that is way more effort than should be necessary. It’s much simpler to just have a directory for such scripts.

IP forwarding is enabled by qubes-network service, which is started after qubes-firewall (which runs scripts from /rw/config/qubes-firewall.d). And input chain is drop by default so you don't need to add rules before enabling network to close something off.

emanruse commented 3 months ago

No, there is a important difference here. An earlier "accept" rule (in another chain) won't override later "drop". But an earlier dnat will override later dnat. That's the main (and TBH the only) reason why we provide pre-created custom-input/forward chains. Otherwise a new chains would be preferred for those cases too.

Unless I am misreading you, that logic applies only to chains with a drop policy or with rules which explicitly drop the same traffic which may have been accepted earlier (in another chain).

Looking at "qubes" tables: The forward chain's policy is accept, not drop. Implicitly, the same applies to the output hook. Everything else is "accept". So, the difference you point out justifies only custom-input's existence, no?

marmarek commented 3 months ago

Unless I am misreading you, that logic applies only to chains with a drop policy or with rules which explicitly drop the same traffic which may have been accepted earlier (in another chain).

Yes

The forward chain's policy is accept

Not really, there is a final drop rule:

# nft list chain ip qubes forward
table ip qubes {
        chain forward {
                type filter hook forward priority filter; policy accept;
                jump custom-forward
                ct state invalid counter packets 17 bytes 1128 drop
                ct state established,related accept
                oifgroup 2 counter packets 0 bytes 0 drop
        }
}

Implicitly, the same applies to the output hook.

This is correct, default "output" is accept.