Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Revert "nixos/firewall: fix reverse path check failures with IPsec" #339393

Merged
merged 1 commit into from
Sep 4, 2024

Conversation

nakato
Copy link
Contributor

@nakato nakato commented Sep 4, 2024

The inclusion of the "meta ipsec" rule in the default reverse path filtering breaks systems not built with specific XFRM kernel config options. Specifically CONFIG_XFRM must be set, which gets selected by CONFIG_NFT_XFRM, which is hidden behind CONFIG_XFRM_USER.

These options are not selected by default in most defconfig's provided by the kernel with the exception of some device-specific defconfigs.

These options are not set by the nix kernel common_config, and I would argue that IPSec support does not belong in a minimal kernel as that elevates its support status above other in-kernel VPN interfaces.

The contributor of this feature does not seem interested in working towards a solution that does not break systems running kernels built with "autoModules = false" while supporting this feature, and as this silently breaks firewalls into an insecure state and poses an immediate security issue I propose this be reverted until a solution that does not break such systems is proposed.
#310857 (comment)

Devices used as firewalls, if they do not have the required kernel config, will fail to load the new firewall rules and will upon boot pass traffic without any filtering into the internal network.

Devices exposed directly to the internet, after reboot, will boot without filtering potentially exposing services not intended to be exposed to the internet, such as databases.

The following platforms in nixpkgs appear to be impacted:

  • pc_simplekernel
  • pogoplug4
  • sheevaplug
  • zero-gravitas
  • zero-sugar
  • utilite
  • guruplug
  • beaglebone
  • fuloong2f_n32

References to hardware without autoModules can be found in nixos-hardware, as well as in active third-party repos on github.

I suspect there are other users impacted that do not have their configurations public, as autoModules = true leads to long compile times when targeting kernels to less standard hardware or hardware with quirks that require patches that cannot be upstreamed.

This reverts commit 3c12ef3.

The inclusion of the "meta ipsec" rule in the default reverse path
filtering breaks systems not built with specific XFRM kernel config
options.  Specifically CONFIG_XFRM must be set, which gets selected
by CONFIG_NFT_XFRM, which is hidden behind CONFIG_XFRM_USER.

These options are not selected by default in most defconfig's provided
by the kernel with the exception of some device-specific defconfigs.

These options are not set by the nix kernel common_config, and I would
argue that IPSec support does not belong in a minimal kernel as that
elevates its support status above other in-kernel VPN interfaces.

The contributor of this feature does not seem interested in working
towards a solution that does not break systems running kernels built
with "autoModules = false" while supporting this feature, and as this
silently breaks firewalls into an insecure state and poses an immediate
security issue I propose this be reverted until a solution that does not
break such systems is proposed.
NixOS#310857 (comment)

Devices used as firewalls, if they do not have the required kernel
config, will fail to load the new firewall rules and will upon boot pass
traffic without any filtering into the internal network.

Devices exposed directly to the internet, after reboot, will boot
without filtering potentially exposing services not intended to be
exposed to the internet, such as databases.

The following platforms in nixpkgs appear to be impacted:
 - pc_simplekernel
 - pogoplug4
 - sheevaplug
 - zero-gravitas
 - zero-sugar
 - utilite
 - guruplug
 - beaglebone
 - fuloong2f_n32

References to hardware without autoModules can be found in
nixos-hardware, as well as in active third-party repos on github.

I suspect there are other users impacted that do not have their configurations
public, as autoModules = true leads to long compile times when targeting
kernels to less standard hardware or hardware with quirks that require
patches that cannot be upstreamed.

This reverts commit 3c12ef3.
@nakato
Copy link
Contributor Author

nakato commented Sep 4, 2024

Ping @mkg20001

Copy link
Member

@mkg20001 mkg20001 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I propose this be reverted until a solution that does not break such systems is proposed.

Let's go with that

@nakato
Copy link
Contributor Author

nakato commented Sep 4, 2024

@mkg20001 I don't have access to merge, are you able to do that or do we need another reviewer?

We should also backport to 24.05 as this was backported there.

@mkg20001 mkg20001 merged commit bf757ce into NixOS:master Sep 4, 2024
25 checks passed
@rnhmjoj
Copy link
Contributor

rnhmjoj commented Sep 5, 2024

The contributor of this feature does not seem interested in working towards a solution that does not break systems running kernels built with "autoModules = false" while supporting this feature, and as this silently breaks firewalls into an insecure state and poses an immediate security issue I propose this be reverted until a solution that does not break such systems is proposed.

It's not that I'm not interested, I think it's simply not possible: to not have IPsec packets fail the reverse path check we must add these rules and the kernel must have the nft_xfrm module to load the firewall with these rules.

The only way I see would be to add the rules conditionally on the kernel having the right modules. You can't inspect the kernel config file because requires IFD, I've found a couple of modules using kernel.features, but I don't understand how that works.

@mkg20001
Copy link
Member

mkg20001 commented Sep 5, 2024

As a temporary solution until a better one is found the rule could be added via networking.firewall.extraReversePathFilterRules by the affected users.

Since this will fail validation we can keep the part sed '/meta ipsec/d' -i ruleset.conf in validation script to enable this to work (or figure out a way to make validation work for this with some magic)

Until we find the right condition to match against in kernel config or other place.

Is this okay?

@rnhmjoj
Copy link
Contributor

rnhmjoj commented Sep 5, 2024

Yes, the problem is that option is for nftables only. For iptables I think the only way is to use networking.firewall.extraCommands if !config.networking.nftables.enable and insert the rule in the right place along the chain. (dealing with two backends is pretty annoying).

Anyway, I'm looking right now into the kernel features thing, hopefully I can make it work.

@nakato
Copy link
Contributor Author

nakato commented Sep 5, 2024

I'm strongly against adding this back in as a default. It needs to go into the ipsec configuration.

Additionally, I've been looking at adding a test to make sure firewall rules don't get broken by additions to the default behaviour, however on x86_64 common_config is very broken which indicates that other platforms are in a worse spot on that front and will silently ignore unknown options in common_config as ignoreConfigErrors = true;, so adding it to common_config still leaves a pretty big door for failure open.

It's a big impact for a router to lose its firewall rules, and they do not tend to be x86_64, so the nix default is such that the operator won't even notice the addition failed. Servers are similarly bad. Both of these are headless use-cases with varying levels of remoteness and ability to get to them to rollback boots.

Not having IPsec is valid, as valid as building our kernels with DRM = no; to drop all the graphics drivers.

@nakato nakato deleted the RevIPSecFW branch September 5, 2024 22:35
@nakato
Copy link
Contributor Author

nakato commented Sep 6, 2024

There's no need to hard-code the sed '/meta ipsec/d' -i ruleset.conf here as networking.nftables.preCheckRuleset is accessible in config in the same way as networking.firewall.extraReversePathFilterRules is.

@rnhmjoj
Copy link
Contributor

rnhmjoj commented Sep 6, 2024

I'm strongly against adding this back in as a default. It needs to go into the ipsec configuration.

Yet, the most appropriate place is the firewall module. Adding it elsewhere requires to maintain a lot of duplicated code sprinkled everywhere: there are at least 5 NixOS modules for IPsec, each needs two rules for iptables and nftables and a switch to toggle them, it's just too much. Also that would not cover other software that doesn't have a module.

It's a big impact for a router to lose its firewall rules, and they do not tend to be x86_64, so the nix default is such that the operator won't even notice the addition failed.

If the firewall fails to load you will see a failed service and the system will show up as degraded in systemctl status. It's hard to not notice.

Not having IPsec is valid, as valid as building our kernels with DRM = no; to drop all the graphics drivers.

Yes, and I think they're both equally unsupported. If you build a custom kernel it's on you to not break enough NixOS modules for you setup to work.
Howver, I do agree that buil-in IPsec support should not be required for a working firewall.

however on x86_64 common_config is very broken which indicates that other platforms are in a worse spot on that front and will silently ignore unknown options in common_config as ignoreConfigErrors = true;, so adding it to common_config still leaves a pretty big door for failure open.

I'm not sure I understand what ignoreConfigErrors has to do with this, but yeah, the kernel configuration in Nixpkgs is a complete mess. I have concluded that there's no fully reliable way to inspect the final configuration: the best I can do is something like this:

diff --git a/nixos/modules/services/networking/firewall-iptables.nix b/nixos/modules/services/networking/firewall-iptables.nix
index 91756f826fe8..28898e1cab2c 100644
--- a/nixos/modules/services/networking/firewall-iptables.nix
+++ b/nixos/modules/services/networking/firewall-iptables.nix
@@ -41,6 +41,7 @@ let
   inherit (config.boot.kernelPackages) kernel;
 
   kernelHasRPFilter = ((kernel.config.isEnabled or (x: false)) "IP_NF_MATCH_RPFILTER") || (kernel.features.netfilterRPFilter or false);
+  kernelHasIPsec = ((kernel.config.isEnabled or (x: false)) "NETFILTER_XT_MATCH_POLICY") || (kernel.features.ipsec or false);
 
   helpers = import ./helpers.nix { inherit config lib; };
 
@@ -126,6 +127,11 @@ let
       # Allows this host to act as a DHCPv4 server
       iptables -t mangle -A nixos-fw-rpfilter -s 0.0.0.0 -d 255.255.255.255 -p udp --sport 68 --dport 67 -j RETURN
 
+      ${optionalString kernelHasIPsec ''
+        # Allows decrypted packets from an IPsec VPN
+        ip46tables -t mangle -A nixos-fw-rpfilter -m policy --dir in --pol ipsec -j RETURN
+      ''}
+
       ${optionalString cfg.logReversePathDrops ''
         ip46tables -t mangle -A nixos-fw-rpfilter -j LOG --log-level info --log-prefix "rpfilter drop: "
       ''}
diff --git a/nixos/modules/services/networking/firewall-nftables.nix b/nixos/modules/services/networking/firewall-nftables.nix
index a5ee7efc3c32..8f8f853b632e 100644
--- a/nixos/modules/services/networking/firewall-nftables.nix
+++ b/nixos/modules/services/networking/firewall-nftables.nix
@@ -15,6 +15,8 @@ let
     ++ map (x: "${toString x.from}-${toString x.to}") portRanges
   );
 
+  kernelHasIPsec = ((kernel.config.isEnabled or (x: false)) "NFT_XFRM") || (kernel.features.ipsec or false);
+
 in
 
 {
@@ -101,6 +103,9 @@ in
         ''}
 
         chain rpfilter-allow {
+          ${optionalString kernelHasIPsec ''
+            meta ipsec exists accept comment "decrypted packets from an IPsec VPN"
+          ''}
           ${cfg.extraReversePathFilterRules}
         }
 
diff --git a/nixos/modules/services/networking/libreswan.nix b/nixos/modules/services/networking/libreswan.nix
index a66ff3065224..c4752c11104c 100644
--- a/nixos/modules/services/networking/libreswan.nix
+++ b/nixos/modules/services/networking/libreswan.nix
@@ -117,6 +117,8 @@ in
   ###### implementation
 
   config = lib.mkIf cfg.enable {
+    # Enable IPsec kernel modules
+    boot.kernel.features.ipsec = true;
 
     # Install package, systemd units, etc.
     environment.systemPackages = [ pkgs.libreswan pkgs.iproute2 ];
diff --git a/nixos/modules/services/networking/networkmanager.nix b/nixos/modules/services/networking/networkmanager.nix
index fda8245ba97d..46ce4f1e18a3 100644
--- a/nixos/modules/services/networking/networkmanager.nix
+++ b/nixos/modules/services/networking/networkmanager.nix
@@ -659,6 +659,10 @@ in
       }
     ];
 
+    # Enable IPsec kernel modules
+    boot.kernel = mkIf cfg.enableStrongSwan
+      { features.ipsec = true; };
+
     boot.kernelModules = [ "ctr" ];
 
     security.polkit.enable = true;
diff --git a/nixos/modules/services/networking/strongswan-swanctl/module.nix b/nixos/modules/services/networking/strongswan-swanctl/module.nix
index e6b5f6ffdeaf..920fc77bedc8 100644
--- a/nixos/modules/services/networking/strongswan-swanctl/module.nix
+++ b/nixos/modules/services/networking/strongswan-swanctl/module.nix
@@ -34,6 +34,8 @@ in  {
   };
 
   config = mkIf cfg.enable {
+    # Enable IPsec kernel modules
+    boot.kernel.features.ipsec = true;
 
     assertions = [
       { assertion = !config.services.strongswan.enable;
diff --git a/nixos/modules/services/networking/strongswan.nix b/nixos/modules/services/networking/strongswan.nix
index 0c04a9c85396..4445c3c5b763 100644
--- a/nixos/modules/services/networking/strongswan.nix
+++ b/nixos/modules/services/networking/strongswan.nix
@@ -144,6 +144,8 @@ in
   in
   mkIf enable
     {
+    # Enable IPsec kernel modules
+    boot.kernel.features.ipsec = true;
 
     # here we should use the default strongswan ipsec.secrets and
     # append to it (default one is empty so not a pb for now)
diff --git a/nixos/tests/libreswan-nat.nix b/nixos/tests/libreswan-nat.nix
index 973e304f9e3a..3f1301b8add9 100644
--- a/nixos/tests/libreswan-nat.nix
+++ b/nixos/tests/libreswan-nat.nix
@@ -88,9 +88,6 @@ in
       networking.firewall.allowedUDPPorts = [ 500 4500 ];
       networking.firewall.allowedTCPPorts = [ 993 ];
 
-      # see https://github.com/NixOS/nixpkgs/pull/310857
-      networking.firewall.checkReversePath = false;
-
       boot.kernel.sysctl = {
         # enable forwarding packets
         "net.ipv6.conf.all.forwarding" = 1;
diff --git a/pkgs/os-specific/linux/kernel/common-config.nix b/pkgs/os-specific/linux/kernel/common-config.nix
index c0bc223b9a05..ed5de4f7ef93 100644
--- a/pkgs/os-specific/linux/kernel/common-config.nix
+++ b/pkgs/os-specific/linux/kernel/common-config.nix
@@ -324,10 +324,6 @@ let
       INET_RAW_DIAG     = lib.mkDefault module;
       INET_DIAG_DESTROY = lib.mkDefault yes;
 
-      # IPsec over TCP
-      INET_ESPINTCP  = whenAtLeast "5.8" yes;
-      INET6_ESPINTCP = whenAtLeast "5.8" yes;
-
       # enable multipath-tcp
       MPTCP           = whenAtLeast "5.6" yes;
       MPTCP_IPV6      = whenAtLeast "5.6" yes;
@@ -353,6 +349,25 @@ let
       MT798X_WMAC = whenAtLeast "6.6" yes;
     };
 
+    ipsec =  {
+      # Basic functionality
+      XFRM = yes;
+      XFRM_OFFLOAD = yes;
+      XFRM_ALGO = module;
+      XFRM_USER = module;
+      XFRM_USER_COMPAT = module;
+      XFRM_INTERFACE = module;
+      XFRM_AH = module;
+      XFRM_ESP = module;
+      XFRM_IPCOMP = module;
+      # For IPsec over TCP
+      INET_ESPINTCP  = whenAtLeast "5.8" yes;
+      INET6_ESPINTCP = whenAtLeast "5.8" yes;
+      # For IPsec firewall rules
+      NFT_XFRM = module;
+      NETFILTER_XT_MATCH_POLICY = module;
+    };
+
     wireless = {
       CFG80211_WEXT               = option yes; # Without it, ipw2200 drivers don't build
       IPW2100_MONITOR             = option yes; # support promiscuous mode

but it would still fail in some cases (eg. custom kernel without common-config and the module explicitly disabled, or the modules blacklisted at boot).
The only sure way is to try load the module at runtime and if it succeed run the command to insert the rule.

@nakato
Copy link
Contributor Author

nakato commented Sep 6, 2024

Adding it elsewhere requires to maintain a lot of duplicated code sprinkled everywhere: there are at least 5 NixOS modules for IPsec, each needs two rules for iptables and nftables and a switch to toggle them, it's just too much.

Maybe there could be a common configuration option, that is disabled by default, that all 5 of these modules enable that then add a common definition to add the rules to networking.firewall.extraReversePathFilterRules and networking.nftables.preCheckRuleset.

services.networking.ipsec.enableReversePathFilterRules = true;

config = mkIf enableReversePathFilterRules {
  networking.firewall.extraReversePathFilterRules = ''
    ...
  '';
  networking.nftables.preCheckRuleset = ''
    sed ...
  '';
}

iptables wise, I think the best way would be to add a rule to the nixos-fw-rpfilter chain in the mangle table to jump to a chain for user-defined reverse-path rules nixos-fw-rpfilter-user which could ACCEPT the traffic (as RETURN would just send it to be dropped). The rules, eg iptables -t mangle -A nixos-fw-rpfilter-user ... -j ACCEPT, could then be added with networking.firewall.extraCommands in the same way as the above nftables ones.

If ACCEPT isn't okay, then it's possible to get fancy with a couple MARK rules, but it wouldn't actually make it any better other than letting it RETURN from the parent chain which would implicitly turn into an ACCEPT.

Also that would not cover other software that doesn't have a module.

They could toggle said common option as well.

If the firewall fails to load you will see a failed service and the system will show up as degraded in systemctl status. It's hard to not notice.

It actually is very easy to not notice, and there are multiple ways in which to have this fail with no error message until AFTER the firewall fails open, potentially exposing private services, the intranet, breaking remote access (and thus rollback methods). If it's a server, there is a very real possibility someone would not notice before a database they were running was scraped by some automated crawler.

I don't see how downplaying the potential impact of the failure is constructive to attempting to find a solution to adding the IPsec rules required to stop the reverse-path filtering from dropping them. Lets focus the issue at hand, adding the IPsec rules in a way that is safe.

Yes, and I think they're both equally unsupported. If you build a custom kernel it's on you to not break enough NixOS modules for you setup to work.

Supported or unsupported is nuanced, however I'd appreciate it if you'd stop attempting to throw anything that doesn't fit the model of x86_64 and an all-module kernel as "unsupported" and any issue stemming from such as invalid. I have built with common_config on a tier-2 platform, it should be supported, and I'm not the only user that does this. Further, if the hardware was tier-1 this issue still would have presented itself.

The only sure way is to try load the module at runtime and if it succeed run the command to insert the rule.

I've mentioned this multiple times. There is not a kernel module to load that will tell you if there is support for that rule or not in nftables.

@rnhmjoj
Copy link
Contributor

rnhmjoj commented Sep 6, 2024

There is not a kernel module to load that will tell you if there is support for that rule or not in nftables.

Yes? NETFILTER_XT_MATCH_POLICY for iptables and NFT_XFRM for nftables.

@nakato
Copy link
Contributor Author

nakato commented Sep 7, 2024

There is not a kernel module to load that will tell you if there is support for that rule or not in nftables.

Yes? NETFILTER_XT_MATCH_POLICY for iptables

I haven't dug into the iptables code.

NFT_XFRM for nftables.

That toggles an ifdef inside a module, it doesn't create a module itself, so the presence of the module does not indicate support.

@rnhmjoj
Copy link
Contributor

rnhmjoj commented Sep 7, 2024

@nakato for testing, can you give me snippet to build a kernel the way you're doing?

@nakato
Copy link
Contributor Author

nakato commented Sep 8, 2024

linuxKernel.packagesFor (linux_6_10.override {
  kernelPatches = [
    ...
  ];

  autoModules = false;

  structuredExtraConfig = with lib.kernel; {
    FB = lib.mkForce no;
    DRM = lib.mkForce no;
    SOUND = no;
    INFINIBAND = lib.mkForce no;
    ACPI = no;
    CAN = no;
    NFC = no;
    NET_9P = no;
    XEN = lib.mkForce no;
    XEN_DOM0 = lib.mkForce no;
    ...

    # Enable device specific drivers
    PCIE_MEDIATEK = yes;
    PCIE_MEDIATEK_GEN3 = yes;
    ...

    NF_TABLES = yes;
    NETFILTER_NETLINK_HOOK = module;
    NETFILTER_NETLINK_ACCT = module;
    NETFILTER_NETLINK_QUEUE = module;
    NETFILTER_NETLINK_LOG = module;
    NETFILTER_NETLINK_OSF = module;
    NF_CONNTRACK_MARK = yes;
    NF_CONNTRACK_LABELS = yes;
    NF_CONNTRACK_AMANDA = module;
    NF_CONNTRACK_FTP = module;
    NF_CONNTRACK_H323 = module;
    NF_CONNTRACK_IRC = module;
    NF_CONNTRACK_NETBIOS_NS = module;
    NF_CONNTRACK_SNMP = module;
    NF_CONNTRACK_PPTP = module;
    NF_CONNTRACK_SANE = module;
    NF_CONNTRACK_SIP = module;
    NF_CONNTRACK_TFTP = module;
    NF_CT_NETLINK = module;
    NF_CT_NETLINK_TIMEOUT = module;
    NFT_NUMGEN = module;
    NFT_CT = module;
    NFT_CONNLIMIT = module;
    NFT_FIB_IPV4 = module;
    NFT_FIB_IPV6 = module;
    NFT_FIB_INET = module;
    NFT_LOG = module;
    NFT_LIMIT = module;
    NFT_MASQ = module;
    NFT_REDIR = module;
    NFT_NAT = module;
    NFT_TUNNEL = module;
    NFT_QUOTA = module;
    NFT_REJECT = module;
    NFT_COMPAT = module;
    NFT_HASH = module;
    NFT_SOCKET = module;
    NFT_OSF = module;
    NFT_TPROXY = module;
    NFT_SYNPROXY = module;
    NF_DUP_NETDEV = module;
    NFT_DUP_NETDEV = module;
    NFT_FWD_NETDEV = module;
    NF_FLOW_TABLE = module;
  };
});

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants