On Thu, Jun 12, 2025 at 05:50:39PM +0200, Dion Bosschieter wrote:
> Upon VM bootstrapping (start,restore,incoming migration)
> iptablesCreateBaseChainsFW is called and unconditionally deletes and
> reinserts top-level firewall chain jumps (e.g. INPUT, FORWARD rules).
> This briefly opens a hole in the firewall, allowing packets through
> until the insertions complete.
>
> This commit ensures that the base chains are only created once per layer
> (IPV4/IPV6) and checks whether the expected rules already exist using
> `iptables -C`. If they do, no delete/insert operations are performed.
>
> This eliminates the short window where packets could bypass filters during
> VM lifecycle operations.
>
> Signed-off-by: Dion Bosschieter <dionbosschie...@gmail.com>
> ---
> src/nwfilter/nwfilter_ebiptables_driver.c | 79 ++++++++++++++---------
> 1 file changed, 47 insertions(+), 32 deletions(-)
>
> diff --git a/src/nwfilter/nwfilter_ebiptables_driver.c
> b/src/nwfilter/nwfilter_ebiptables_driver.c
> index 067df6e612..42a0133159 100644
> --- a/src/nwfilter/nwfilter_ebiptables_driver.c
> +++ b/src/nwfilter/nwfilter_ebiptables_driver.c
> @@ -131,6 +131,14 @@ static char chainprefixes_host_temp[3] = {
> 0
> };
>
> +typedef struct {
> + const char *chain;
> + const char *position;
> + const char *targetChain;
> +} iptablesBaseChainFW;
> +
> +static bool baseChainFWDefined[VIR_FIREWALL_LAYER_LAST] = { false };
> +
> static int
> printVar(virNWFilterVarCombIter *vars,
> char *buf, int bufsize,
> @@ -403,38 +411,45 @@ static void
> iptablesCreateBaseChainsFW(virFirewall *fw,
> virFirewallLayer layer)
> {
> - virFirewallAddCmdFull(fw, layer,
> - true, NULL, NULL,
> - "-N", VIRT_IN_CHAIN, NULL);
> - virFirewallAddCmdFull(fw, layer,
> - true, NULL, NULL,
> - "-N", VIRT_OUT_CHAIN, NULL);
> - virFirewallAddCmdFull(fw, layer,
> - true, NULL, NULL,
> - "-N", VIRT_IN_POST_CHAIN, NULL);
> - virFirewallAddCmdFull(fw, layer,
> - true, NULL, NULL,
> - "-N", HOST_IN_CHAIN, NULL);
> - virFirewallAddCmdFull(fw, layer,
> - true, NULL, NULL,
> - "-D", "FORWARD", "-j", VIRT_IN_CHAIN, NULL);
> - virFirewallAddCmdFull(fw, layer,
> - true, NULL, NULL,
> - "-D", "FORWARD", "-j", VIRT_OUT_CHAIN, NULL);
> - virFirewallAddCmdFull(fw, layer,
> - true, NULL, NULL,
> - "-D", "FORWARD", "-j", VIRT_IN_POST_CHAIN, NULL);
> - virFirewallAddCmdFull(fw, layer,
> - true, NULL, NULL,
> - "-D", "INPUT", "-j", HOST_IN_CHAIN, NULL);
> - virFirewallAddCmd(fw, layer,
> - "-I", "FORWARD", "1", "-j", VIRT_IN_CHAIN, NULL);
> - virFirewallAddCmd(fw, layer,
> - "-I", "FORWARD", "2", "-j", VIRT_OUT_CHAIN, NULL);
> - virFirewallAddCmd(fw, layer,
> - "-I", "FORWARD", "3", "-j", VIRT_IN_POST_CHAIN, NULL);
> - virFirewallAddCmd(fw, layer,
> - "-I", "INPUT", "1", "-j", HOST_IN_CHAIN, NULL);
> + iptablesBaseChainFW fw_chains[] = {
> + {"FORWARD", "1", VIRT_IN_CHAIN},
> + {"FORWARD", "2", VIRT_OUT_CHAIN},
> + {"FORWARD", "3", VIRT_IN_POST_CHAIN},
> + {"INPUT", "1", HOST_IN_CHAIN},
> + };
> + size_t i;
> +
> + // iptablesCreateBaseChainsFW already ran once for this layer,
> + // we don't have to recreate the base chains on every firewall update
> + if (baseChainFWDefined[layer])
> + return;
> +
> + // set defined state so we skip the following logic next run
> + baseChainFWDefined[layer] = true;
> +
> + virFirewallStartTransaction(fw, 0);
> +
> + for (i = 0; i < G_N_ELEMENTS(fw_chains); i++)
> + virFirewallAddCmd(fw, layer,
> + "-C", fw_chains[i].chain,
> + "-j", fw_chains[i].targetChain, NULL);
> +
> + if (virFirewallApply(fw) == 0)
> + // rules already in place
> + return;
> +
> + for (i = 0; i < G_N_ELEMENTS(fw_chains); i++) {
> + virFirewallAddCmdFull(fw, layer,
> + true, NULL, NULL,
> + "-N", fw_chains[i].targetChain, NULL);
> + virFirewallAddCmdFull(fw, layer,
> + true, NULL, NULL,
> + "-D", fw_chains[i].chain, "-j",
> + fw_chains[i].targetChain, NULL);
> + virFirewallAddCmd(fw, layer,
> + "-I", fw_chains[i].chain, fw_chains[i].position,
> + "-j", fw_chains[i].targetChain, NULL);
> + }
> }
If I purge all iptables rules
iptables -F
iptables -X
and then restart virtnwfilterd and attempt to create a VM I get a fatal
error
# virsh start --console f42
error: Failed to start domain 'f42'
error: internal error: Failed to run firewall command iptables -w -C FORWARD -j
libvirt-in: iptables v1.8.11 (nf_tables): Chain 'libvirt-in' does not exist
Try `iptables -h' or 'iptables --help' for more information.
the 'iptables -w -C FORWARD -j libvirt-in' bit is from this patch IIUC.
The problem here is that you cannot simply call 'virFirewallApply' in
this context.
This method needs to be exclusively adding rules to the virFirewall
object. The call to virFirewallApply happens later on in the code
paths, and currently that later call is picking up the faiiled
transaction your early virFirewallApply call triggers.
To make this work you need to change to use virFirewallAddCmdFull
with '-L' to query what rules currently exist. Pass in a callback
function to receive the output of '-L', and in that callback you
can then use 'virFirwallAddCmd to create the base chains if they
were missing.
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
