Merge pull request #22034 from mayflower/conntrack-helpers

Disable conntrack helper autoloading by default

4 files changed, 54 insertions, 26 deletions

diff --git a/nixos/doc/manual/release-notes/rl-1703.xml b/nixos/doc/manual/release-notes/rl-1703.xml
index aa864b7a757a..177010e2a322 100644
--- a/nixos/doc/manual/release-notes/rl-1703.xml
+++ b/nixos/doc/manual/release-notes/rl-1703.xml
@@ -133,6 +133,19 @@ following incompatible changes:</para>
 
     </para>
   </listitem>
+
+  <listitem>
+    <para>
+      Autoloading connection tracking helpers is now disabled by default.
+      This default was also changed in the Linux kernel and is considered
+      insecure if not configured properly in your firewall. If you need
+      connection tracking helpers (i.e. for active FTP) please enable
+      <literal>networking.firewall.autoLoadConntrackHelpers</literal> and
+      tune <literal>networking.firewall.connectionTrackingModules</literal>
+      to suit your needs.
+    </para>
+  </listitem>
+
 </itemizedlist>
 
 
diff --git a/nixos/modules/services/networking/firewall.nix b/nixos/modules/services/networking/firewall.nix
index c251b52e03fd..34b731ad35c9 100644
--- a/nixos/modules/services/networking/firewall.nix
+++ b/nixos/modules/services/networking/firewall.nix
@@ -41,7 +41,6 @@ let
   kernelPackages = config.boot.kernelPackages;
 
   kernelHasRPFilter = kernelPackages.kernel.features.netfilterRPFilter or false;
-  kernelCanDisableHelpers = kernelPackages.kernel.features.canDisableNetfilterConntrackHelpers or false;
 
   helpers =
     ''
@@ -426,7 +425,7 @@ in
 
     networking.firewall.connectionTrackingModules = mkOption {
       type = types.listOf types.str;
-      default = [ "ftp" ];
+      default = [ ];
       example = [ "ftp" "irc" "sane" "sip" "tftp" "amanda" "h323" "netbios_sn" "pptp" "snmp" ];
       description =
         ''
@@ -435,9 +434,11 @@ in
 
           As helpers can pose as a security risk, it is advised to
           set this to an empty list and disable the setting
-          networking.firewall.autoLoadConntrackHelpers
+          networking.firewall.autoLoadConntrackHelpers unless you
+          know what you are doing. Connection tracking is disabled
+          by default.
 
-          Loading of helpers is recommended to be done through the new
+          Loading of helpers is recommended to be done through the
           CT target.  More info:
           https://home.regit.org/netfilter-en/secure-use-of-helpers/
         '';
@@ -445,7 +446,7 @@ in
 
     networking.firewall.autoLoadConntrackHelpers = mkOption {
       type = types.bool;
-      default = true;
+      default = false;
       description =
         ''
           Whether to auto-load connection-tracking helpers.
@@ -505,15 +506,14 @@ in
 
     environment.systemPackages = [ pkgs.iptables ] ++ cfg.extraPackages;
 
-    boot.kernelModules = map (x: "nf_conntrack_${x}") cfg.connectionTrackingModules;
-    boot.extraModprobeConfig = optionalString (!cfg.autoLoadConntrackHelpers) ''
-      options nf_conntrack nf_conntrack_helper=0
+    boot.kernelModules = (optional cfg.autoLoadConntrackHelpers "nf_conntrack")
+      ++ map (x: "nf_conntrack_${x}") cfg.connectionTrackingModules;
+    boot.extraModprobeConfig = optionalString cfg.autoLoadConntrackHelpers ''
+      options nf_conntrack nf_conntrack_helper=1
     '';
 
     assertions = [ { assertion = (cfg.checkReversePath != false) || kernelHasRPFilter;
                      message = "This kernel does not support rpfilter"; }
-                   { assertion = cfg.autoLoadConntrackHelpers || kernelCanDisableHelpers;
-                     message = "This kernel does not support disabling conntrack helpers"; }
                  ];
 
     systemd.services.firewall = {
diff --git a/nixos/release.nix b/nixos/release.nix
index dfa9b67654fb..2d78a4db9736 100644
--- a/nixos/release.nix
+++ b/nixos/release.nix
@@ -273,6 +273,7 @@ in rec {
   tests.mysql = callTest tests/mysql.nix {};
   tests.mysqlReplication = callTest tests/mysql-replication.nix {};
   tests.nat.firewall = callTest tests/nat.nix { withFirewall = true; };
+  tests.nat.firewall-conntrack = callTest tests/nat.nix { withFirewall = true; withConntrackHelpers = true; };
   tests.nat.standalone = callTest tests/nat.nix { withFirewall = false; };
   tests.networking.networkd = callSubTests tests/networking.nix { networkd = true; };
   tests.networking.scripted = callSubTests tests/networking.nix { networkd = false; };
diff --git a/nixos/tests/nat.nix b/nixos/tests/nat.nix
index 4fbf64462682..74e20bff8d81 100644
--- a/nixos/tests/nat.nix
+++ b/nixos/tests/nat.nix
@@ -3,34 +3,47 @@
 # client on the inside network, a server on the outside network, and a
 # router connected to both that performs Network Address Translation
 # for the client.
-import ./make-test.nix ({ pkgs, withFirewall, ... }:
+import ./make-test.nix ({ pkgs, lib, withFirewall, withConntrackHelpers ? false, ... }:
   let
     unit = if withFirewall then "firewall" else "nat";
   in
   {
-    name = "nat${if withFirewall then "WithFirewall" else "Standalone"}";
-  meta = with pkgs.stdenv.lib.maintainers; {
+    name = "nat" + (if withFirewall then "WithFirewall" else "Standalone")
+                 + (lib.optionalString withConntrackHelpers "withConntrackHelpers");
+    meta = with pkgs.stdenv.lib.maintainers; {
       maintainers = [ eelco chaoflow rob wkennington ];
     };
 
     nodes =
       { client =
           { config, pkgs, nodes, ... }:
-          { virtualisation.vlans = [ 1 ];
-            networking.firewall.allowPing = true;
-            networking.defaultGateway =
-              (pkgs.lib.head nodes.router.config.networking.interfaces.eth2.ip4).address;
-          };
+          lib.mkMerge [
+            { virtualisation.vlans = [ 1 ];
+              networking.firewall.allowPing = true;
+              networking.defaultGateway =
+                (pkgs.lib.head nodes.router.config.networking.interfaces.eth2.ip4).address;
+            }
+            (lib.optionalAttrs withConntrackHelpers {
+              networking.firewall.connectionTrackingModules = [ "ftp" ];
+              networking.firewall.autoLoadConntrackHelpers = true;
+            })
+          ];
 
         router =
           { config, pkgs, ... }:
-          { virtualisation.vlans = [ 2 1 ];
-            networking.firewall.enable = withFirewall;
-            networking.firewall.allowPing = true;
-            networking.nat.enable = true;
-            networking.nat.internalIPs = [ "192.168.1.0/24" ];
-            networking.nat.externalInterface = "eth1";
-          };
+          lib.mkMerge [
+            { virtualisation.vlans = [ 2 1 ];
+              networking.firewall.enable = withFirewall;
+              networking.firewall.allowPing = true;
+              networking.nat.enable = true;
+              networking.nat.internalIPs = [ "192.168.1.0/24" ];
+              networking.nat.externalInterface = "eth1";
+            }
+            (lib.optionalAttrs withConntrackHelpers {
+              networking.firewall.connectionTrackingModules = [ "ftp" ];
+              networking.firewall.autoLoadConntrackHelpers = true;
+            })
+          ];
 
         server =
           { config, pkgs, ... }:
@@ -66,7 +79,8 @@ import ./make-test.nix ({ pkgs, withFirewall, ... }:
         $client->succeed("curl -v ftp://server/foo.txt >&2");
 
         # Test whether active FTP works.
-        $client->succeed("curl -v -P - ftp://server/foo.txt >&2");
+        $client->${if withConntrackHelpers then "succeed" else "fail"}(
+          "curl -v -P - ftp://server/foo.txt >&2");
 
         # Test ICMP.
         $client->succeed("ping -c 1 router >&2");