Addressing PR feedback

12 files changed, 215 insertions, 291 deletions

diff --git a/nixos/modules/security/apparmor-suid.nix b/nixos/modules/security/apparmor-suid.nix
index 799f27b6708..e7b870864ee 100644
--- a/nixos/modules/security/apparmor-suid.nix
+++ b/nixos/modules/security/apparmor-suid.nix
@@ -19,7 +19,7 @@ with lib;
   config = mkIf (cfg.confineSUIDApplications) {
     security.apparmor.profiles = [ (pkgs.writeText "ping" ''
       #include <tunables/global>
-      /var/permissions-wrappers/ping {
+      /run/wrappers/ping {
         #include <abstractions/base>
         #include <abstractions/consoles>
         #include <abstractions/nameservice>
@@ -33,7 +33,7 @@ with lib;
         ${pkgs.attr.out}/lib/libattr.so* mr,
 
         ${pkgs.iputils}/bin/ping mixr,
-        /var/permissions-wrappers/ping.real r,
+        /run/wrappers/ping.real r,
 
         #/etc/modules.conf r,
 
diff --git a/nixos/modules/security/chromium-suid-sandbox.nix b/nixos/modules/security/chromium-suid-sandbox.nix
index 6fe25213639..0699fbb728a 100644
--- a/nixos/modules/security/chromium-suid-sandbox.nix
+++ b/nixos/modules/security/chromium-suid-sandbox.nix
@@ -27,10 +27,6 @@ in
 
   config = mkIf cfg.enable {
     environment.systemPackages = [ sandbox ];
-    security.permissionsWrappers.setuid = [
-      { program = sandbox.passthru.sandboxExecutableName;
-        source  = "${sandbox}/bin/${sandbox.passthru.sandboxExecutableName}";
-      }
-    ];
+    security.setuidPrograms = [ sandbox.passthru.sandboxExecutableName ];
   };
 }
diff --git a/nixos/modules/security/duosec.nix b/nixos/modules/security/duosec.nix
index e5b35427015..ee62c34438e 100644
--- a/nixos/modules/security/duosec.nix
+++ b/nixos/modules/security/duosec.nix
@@ -188,16 +188,7 @@ in
 
      environment.systemPackages = [ pkgs.duo-unix ];
 
-     security.permissionsWrappers.setuid =
-     [
-       { program = "login_duo";
-         source  = "${pkgs.duo-unix.out}/bin/login_duo";
-         owner   = "root";
-         group   = "root";
-         setuid  = true;
-       }
-     ];
-
+     security.setuidPrograms = [ "login_duo" ];
      environment.etc = loginCfgFile ++ pamCfgFile;
 
      /* If PAM *and* SSH are enabled, then don't do anything special.
diff --git a/nixos/modules/security/pam.nix b/nixos/modules/security/pam.nix
index c5088b64bb3..3c944acf6cf 100644
--- a/nixos/modules/security/pam.nix
+++ b/nixos/modules/security/pam.nix
@@ -472,33 +472,14 @@ in
       ++ optionals config.security.pam.enableU2F [ pkgs.pam_u2f ]
       ++ optionals config.security.pam.enableEcryptfs [ pkgs.ecryptfs ];
 
-    security.permissionsWrappers.setuid =
-      [
-        { program = "unix_chkpwd";
-          source = "${pkgs.pam}/sbin/unix_chkpwd.orig";
-          owner = "root";
-          group = "root";
-          setuid = true;
-        }
-
-
-          
-      ] ++ (optional config.security.pam.enableEcryptfs 
-        { program = "umount.ecryptfs_private";
-          source  = "${pkgs.ecryptfs.out}/bin/umount.ecryptfs_private";
-          owner   = "root";
-          group   = "root";
-          setuid  = true;
-        }
-      ) ++ (optional config.security.pam.enableEcryptfs
-        { program = "mount.ecryptfs_private";
-          source  = "${pkgs.ecryptfs.out}/bin/mount.ecryptfs_private";
-          owner   = "root";
-          group   = "root";
-          setuid  = true;
-        }
-      );
-        
+    security.setuidPrograms =
+      optionals config.security.pam.enableEcryptfs [ "mount.ecryptfs_private" "umount.ecryptfs_private" ];
+
+    security.wrappers.unix_chkpwd = {
+      source = "${pkgs.pam}/sbin/unix_chkpwd.orig";
+      owner = "root";
+      setuid = true;
+    };
 
     environment.etc =
       mapAttrsToList (n: v: makePAMService v) config.security.pam.services;
diff --git a/nixos/modules/security/pam_usb.nix b/nixos/modules/security/pam_usb.nix
index 53a7921a244..032f8e38d11 100644
--- a/nixos/modules/security/pam_usb.nix
+++ b/nixos/modules/security/pam_usb.nix
@@ -33,22 +33,7 @@ in
   config = mkIf (cfg.enable || anyUsbAuth) {
 
     # Make sure pmount and pumount are setuid wrapped.
-    security.permissionsWrappers.setuid =
-      [
-        { program = "pmount";
-          source  = "${pkgs.pmount.out}/bin/pmount";
-          owner   = "root";
-          group   = "root";
-          setuid  = true;
-        }
-
-        { program = "pumount";
-          source  = "${pkgs.pmount.out}/bin/pumount";
-          owner   = "root";
-          group   = "root";
-          setuid  = true;
-        }
-      ];
+    security.setuidPrograms = [ "pmount" "pumount" ];
 
     environment.systemPackages = [ pkgs.pmount ];
 
diff --git a/nixos/modules/security/permissions-wrappers/default.nix b/nixos/modules/security/permissions-wrappers/default.nix
deleted file mode 100644
index 480bd371040..00000000000
--- a/nixos/modules/security/permissions-wrappers/default.nix
+++ /dev/null
@@ -1,191 +0,0 @@
-{ config, lib, pkgs, ... }:
-let
-
-  inherit (config.security) run-permissionsWrapperDir permissionsWrapperDir;
-
-  isNotNull = v: if v != null then true else false;
-
-  cfg = config.security.permissionsWrappers;
-
-  setcapWrappers = import ./setcap-wrapper-drv.nix {
-    inherit config lib pkgs;
-  };
-
-  setuidWrappers = import ./setuid-wrapper-drv.nix {
-    inherit config lib pkgs;
-  };
-
-  ###### Activation script for the setcap wrappers
-  configureSetcapWrapper =
-    { program
-    , capabilities
-    , source ? null
-    , owner  ? "nobody"
-    , group  ? "nogroup"
-    }: ''
-      cp ${setcapWrappers}/bin/${program}.wrapper $permissionsWrapperDir/${program}
-
-      # Prevent races
-      chmod 0000 $permissionsWrapperDir/${program}
-      chown ${owner}.${group} $permissionsWrapperDir/${program}
-
-      # Set desired capabilities on the file plus cap_setpcap so
-      # the wrapper program can elevate the capabilities set on
-      # its file into the Ambient set.
-      #
-      # Only set the capabilities though if we're being told to
-      # do so.
-      ${pkgs.libcap.out}/bin/setcap "cap_setpcap,${capabilities}" $permissionsWrapperDir/${program}
-
-      # Set the executable bit
-      chmod u+rx,g+x,o+x $permissionsWrapperDir/${program}
-    '';
-
-  ###### Activation script for the setuid wrappers
-  configureSetuidWrapper =
-    { program
-    , source ? null
-    , owner  ? "nobody"
-    , group  ? "nogroup"
-    , setuid ? false
-    , setgid ? false
-    , permissions ? "u+rx,g+x,o+x"
-    }: ''
-      cp ${setuidWrappers}/bin/${program}.wrapper $permissionsWrapperDir/${program}
-
-      # Prevent races
-      chmod 0000 $permissionsWrapperDir/${program}
-      chown ${owner}.${group} $permissionsWrapperDir/${program}
-
-      chmod "u${if setuid then "+" else "-"}s,g${if setgid then "+" else "-"}s,${permissions}" $permissionsWrapperDir/${program}
-    '';
-in
-{
-
-  ###### interface
-
-  options = {
-    security.permissionsWrappers.setcap = lib.mkOption {
-      type    = lib.types.listOf lib.types.attrs;
-      default = [];
-      example =
-        [ { program = "ping";
-            source  = "${pkgs.iputils.out}/bin/ping";
-            owner   = "nobody";
-            group   = "nogroup";
-            capabilities = "cap_net_raw+ep";
-          }
-        ];
-      description = ''
-        This option sets capabilities on a wrapper program that
-        propagates those capabilities down to the wrapped, real
-        program.
-
-        The `program` attribute is the name of the program to be
-        wrapped. If no `source` attribute is provided, specifying the
-        absolute path to the program, then the program will be
-        searched for in the path environment variable.
-
-        NOTE: cap_setpcap, which is required for the wrapper program
-        to be able to raise caps into the Ambient set is NOT raised to
-        the Ambient set so that the real program cannot modify its own
-        capabilities!! This may be too restrictive for cases in which
-        the real program needs cap_setpcap but it at least leans on
-        the side security paranoid vs. too relaxed.
-      '';
-    };
-
-    security.permissionsWrappers.setuid = lib.mkOption {
-      type = lib.types.listOf lib.types.attrs;
-      default = [];
-      example =
-        [ { program = "sendmail";
-            source = "/nix/store/.../bin/sendmail";
-            owner = "nobody";
-            group = "postdrop";
-            setuid = false;
-            setgid = true;
-            permissions = "u+rx,g+x,o+x";
-          }
-        ];
-      description = ''
-        This option allows the ownership and permissions on the setuid
-        wrappers for specific programs to be overridden from the
-        default (setuid root, but not setgid root).
-      '';
-    };
-
-    security.permissionsWrapperDir = lib.mkOption {
-      type        = lib.types.path;
-      default     = "/var/permissions-wrappers";
-      internal    = true;
-      description = ''
-        This option defines the path to the permissions wrappers. It
-        should not be overriden.
-      '';
-    };
-
-    security.run-permissionsWrapperDir = lib.mkOption {
-      type        = lib.types.path;
-      default     = "/run/permissions-wrapper-dirs";
-      internal    = true;
-      description = ''
-        This option defines the run path to the permissions
-        wrappers. It should not be overriden.
-      '';
-    };
-
-  };
-
-
-  ###### implementation
-  
-  config = {
-
-    # Make sure our setcap-wrapper dir exports to the PATH env
-    # variable when initializing the shell
-    environment.extraInit = ''
-    # The permissions wrappers override other bin directories.
-    export PATH="${permissionsWrapperDir}:$PATH"
-    '';
-
-    system.activationScripts.wrapper-dir = ''
-      mkdir -p "${permissionsWrapperDir}"
-    '';
-
-    ###### setcap activation script
-    system.activationScripts.permissions-wrappers =
-      lib.stringAfter [ "users" ]
-        ''
-          # Look in the system path and in the default profile for
-          # programs to be wrapped.
-          PERMISSIONS_WRAPPER_PATH=${config.system.path}/bin:${config.system.path}/sbin
-
-          mkdir -p ${run-permissionsWrapperDir}
-          permissionsWrapperDir=$(mktemp --directory --tmpdir=${run-permissionsWrapperDir} permissions-wrappers.XXXXXXXXXX)
-          chmod a+rx $permissionsWrapperDir
-
-          ${lib.concatMapStrings configureSetcapWrapper (builtins.filter isNotNull cfg.setcap)}
-          ${lib.concatMapStrings configureSetuidWrapper (builtins.filter isNotNull cfg.setuid)}
-
-          if [ -L ${permissionsWrapperDir} ]; then
-            # Atomically replace the symlink
-            # See https://axialcorps.com/2013/07/03/atomically-replacing-files-and-directories/
-            old=$(readlink ${permissionsWrapperDir})
-            ln --symbolic --force --no-dereference $permissionsWrapperDir ${permissionsWrapperDir}-tmp
-            mv --no-target-directory ${permissionsWrapperDir}-tmp ${permissionsWrapperDir}
-            rm --force --recursive $old
-          elif [ -d ${permissionsWrapperDir} ]; then
-            # Compatibility with old state, just remove the folder and symlink
-            rm -f ${permissionsWrapperDir}/*
-            # if it happens to be a tmpfs
-            ${pkgs.utillinux}/bin/umount ${permissionsWrapperDir} || true
-            rm -d ${permissionsWrapperDir}
-            ln -d --symbolic $permissionsWrapperDir ${permissionsWrapperDir}
-          else
-            # For initial setup
-            ln --symbolic $permissionsWrapperDir ${permissionsWrapperDir}
-          fi
-        '';
-  };
-}
diff --git a/nixos/modules/security/polkit.nix b/nixos/modules/security/polkit.nix
index 098319d5ded..547b40cedfd 100644
--- a/nixos/modules/security/polkit.nix
+++ b/nixos/modules/security/polkit.nix
@@ -83,22 +83,8 @@ in
 
     security.pam.services.polkit-1 = {};
 
-    security.permissionsWrappers.setuid = 
-      [
-        { program = "pkexec";
-          source  = "${pkgs.polkit.out}/bin/pkexec";
-          owner   = "root";
-          group   = "root";
-          setuid  = true;
-        }
-
-        { program = "polkit-agent-helper-1";
-          owner = "root";
-          group = "root";
-          setuid = true;
-          source = "${pkgs.polkit.out}/lib/polkit-1/polkit-agent-helper-1";
-        }
-      ];
+    security.setuidPrograms = [ "pkexec" ];
+    security.wrappers."polkit-agent-helper-1".source = "${pkgs.polkit.out}/lib/polkit-1/polkit-agent-helper-1";
 
     system.activationScripts.polkit =
       ''
diff --git a/nixos/modules/security/sudo.nix b/nixos/modules/security/sudo.nix
index 652f23c2938..f5612e1b0c5 100644
--- a/nixos/modules/security/sudo.nix
+++ b/nixos/modules/security/sudo.nix
@@ -81,22 +81,7 @@ in
         ${cfg.extraConfig}
       '';
 
-    security.permissionsWrappers.setuid =
-     [
-       { program = "sudo";
-         source  = "${pkgs.sudo.out}/bin/sudo";
-         owner   = "root";
-         group   = "root";
-         setuid  = true;
-       }
-
-       { program = "sudoedit";
-         source  = "${pkgs.sudo.out}/bin/sudoedit";
-         owner   = "root";
-         group   = "root";
-         setuid  = true;
-       }
-    ];
+    security.setuidPrograms = [ "sudo" "sudoedit" ];
 
     environment.systemPackages = [ sudo ];
 
diff --git a/nixos/modules/security/wrappers/default.nix b/nixos/modules/security/wrappers/default.nix
new file mode 100644
index 00000000000..d12209b375b
--- /dev/null
+++ b/nixos/modules/security/wrappers/default.nix
@@ -0,0 +1,191 @@
+{ config, lib, pkgs, ... }:
+let
+
+  inherit (config.security) wrapperDir;
+
+  isNotNull = v: if v != null || v != "" then true else false;
+
+  cfg = config.security.wrappers;
+
+  setcapWrappers = import ./setcap-wrapper-drv.nix {
+    inherit config lib pkgs;
+  };
+
+  setuidWrappers = import ./setuid-wrapper-drv.nix {
+    inherit config lib pkgs;
+  };
+
+  ###### Activation script for the setcap wrappers
+  mkSetcapProgram =
+    { program
+    , capabilities
+    , source ? null
+    , owner  ? "nobody"
+    , group  ? "nogroup"
+    ...
+    }: ''
+      cp ${setcapWrappers}/bin/${program}.wrapper $wrapperDir/${program}
+
+      # Prevent races
+      chmod 0000 $wrapperDir/${program}
+      chown ${owner}.${group} $wrapperDir/${program}
+
+      # Set desired capabilities on the file plus cap_setpcap so
+      # the wrapper program can elevate the capabilities set on
+      # its file into the Ambient set.
+      #
+      # Only set the capabilities though if we're being told to
+      # do so.
+      ${pkgs.libcap.out}/bin/setcap "cap_setpcap,${capabilities}" $wrapperDir/${program}
+
+      # Set the executable bit
+      chmod u+rx,g+x,o+x $wrapperDir/${program}
+    '';
+
+  ###### Activation script for the setuid wrappers
+  mkSetuidProgram =
+    { program
+    , source ? null
+    , owner  ? "nobody"
+    , group  ? "nogroup"
+    , setuid ? false
+    , setgid ? false
+    , permissions ? "u+rx,g+x,o+x"
+    ...
+    }: ''
+      cp ${setuidWrappers}/bin/${program}.wrapper $wrapperDir/${program}
+
+      # Prevent races
+      chmod 0000 $wrapperDir/${program}
+      chown ${owner}.${group} $wrapperDir/${program}
+
+      chmod "u${if setuid then "+" else "-"}s,g${if setgid then "+" else "-"}s,${permissions}" $wrapperDir/${program}
+    '';
+in
+{
+
+  ###### interface
+
+  options = {
+    security.wrappers.setcap = lib.mkOption {
+      type    = lib.types.listOf lib.types.attrs;
+      default = [];
+      example =
+        [ { program = "ping";
+            source  = "${pkgs.iputils.out}/bin/ping";
+            owner   = "nobody";
+            group   = "nogroup";
+            capabilities = "cap_net_raw+ep";
+          }
+        ];
+      description = ''
+        This option sets capabilities on a wrapper program that
+        propagates those capabilities down to the wrapped, real
+        program.
+
+        The <literal>program</literal> attribute is the name of the
+        program to be wrapped. If no <literal>source</literal>
+        attribute is provided, specifying the absolute path to the
+        program, then the program will be searched for in the path
+        environment variable.
+
+        NOTE: cap_setpcap, which is required for the wrapper program
+        to be able to raise caps into the Ambient set is NOT raised to
+        the Ambient set so that the real program cannot modify its own
+        capabilities!! This may be too restrictive for cases in which
+        the real program needs cap_setpcap but it at least leans on
+        the side security paranoid vs. too relaxed.
+      '';
+    };
+
+    security.setuidPrograms = mkOption {
+      type = types.listOf types.str;
+      default = [];
+      example = ["passwd"];
+      description = ''
+        The Nix store cannot contain setuid/setgid programs directly.
+        For this reason, NixOS can automatically generate wrapper
+        programs that have the necessary privileges.  This option
+        lists the names of programs in the system environment for
+        which setuid root wrappers should be created.
+      '';
+    };
+
+    security.wrappers = lib.mkOption {
+      type = lib.types.attrs;
+      default = {};
+      example = {
+        sendmail.source = "/nix/store/.../bin/sendmail";
+      };
+      description = ''
+        This option allows the ownership and permissions on the setuid
+        wrappers for specific programs to be overridden from the
+        default (setuid root, but not setgid root).
+      '';
+    };
+
+    security.old-wrapperDir = lib.mkOption {
+      type        = lib.types.path;
+      default     = "/var/setuid-wrappers";
+      internal    = true;
+      description = ''
+        This option defines the path to the wrapper programs. It
+        should not be overriden.
+      '';
+    };
+
+    security.wrapperDir = lib.mkOption {
+      type        = lib.types.path;
+      default     = "/run/wrappers";
+      internal    = true;
+      description = ''
+        This option defines the path to the wrapper programs. It
+        should not be overriden.
+      '';
+    };
+  };
+
+  ###### implementation
+  config = {
+    # Make sure our setcap-wrapper dir exports to the PATH env
+    # variable when initializing the shell
+    environment.extraInit = ''
+      # The permissions wrappers override other bin directories.
+      export PATH="${wrapperDir}:$PATH"
+    '';
+
+    ###### setcap activation script
+    system.activationScripts.wrappers =
+      let
+        programs =
+          (map (x: { program = x; owner = "root"; group = "root"; setuid = true; })
+            config.security.setuidPrograms)
+            ++ lib.mapAttrsToList
+                 (n: v: (if v ? "program" then v else v // {program=n;}))
+                 cfg.wrappers;
+
+        wrapperPrograms =
+          builtins.map
+            (s: if (s ? "setuid"  && s.setuid  == true) ||
+                   (s ? "setguid" && s.setguid == true) ||
+                   (s ? "permissions")
+                then mkSetuidProgram s
+                else if (s ? "capabilities")
+                then mkSetcapProgram s
+                else ""
+            ) programs;
+
+      in lib.stringAfter [ "users" ]
+        ''
+          # Look in the system path and in the default profile for
+          # programs to be wrapped.
+          WRAPPER_PATH=${config.system.path}/bin:${config.system.path}/sbin
+
+          mkdir -p ${wrapperDir}
+          wrapperDir=$(mktemp --directory --tmpdir=${wrapperDir} wrappers.XXXXXXXXXX)
+          chmod a+rx $wrapperDir
+
+          ${lib.concatStringsSep "\n" (builtins.filter isNotNull cfg.wrappers)}
+        '';
+  };
+}
diff --git a/nixos/modules/security/permissions-wrappers/permissions-wrapper.c b/nixos/modules/security/wrappers/permissions-wrapper.c
index cb9d8d6b37b..cb9d8d6b37b 100644
--- a/nixos/modules/security/permissions-wrappers/permissions-wrapper.c
+++ b/nixos/modules/security/wrappers/permissions-wrapper.c
diff --git a/nixos/modules/security/permissions-wrappers/setcap-wrapper-drv.nix b/nixos/modules/security/wrappers/setcap-wrapper-drv.nix
index 2ae3067b1b1..03dca5c9f42 100644
--- a/nixos/modules/security/permissions-wrappers/setcap-wrapper-drv.nix
+++ b/nixos/modules/security/wrappers/setcap-wrapper-drv.nix
@@ -1,7 +1,7 @@
 { config, lib, pkgs, ... }:
 
 let  
-     cfg = config.security.permissionsWrappers;
+     cfg = config.security.wrappers;
 
      # Produce a shell-code splice intended to be stitched into one of
      # the build or install phases within the derivation.
@@ -12,7 +12,7 @@ let
          source=/nix/var/nix/profiles/default/bin/${program}
        fi
 
-       gcc -Wall -O2 -DWRAPPER_SETCAP=1 -DSOURCE_PROG=\"$source\" -DWRAPPER_DIR=\"${config.security.run-permissionsWrapperDir}\" \
+       gcc -Wall -O2 -DWRAPPER_SETCAP=1 -DSOURCE_PROG=\"$source\" -DWRAPPER_DIR=\"${config.security.run-wrapperDir}\" \
            -lcap-ng -lcap ${./permissions-wrapper.c} -o $out/bin/${program}.wrapper -L ${pkgs.libcap.lib}/lib -L ${pkgs.libcap_ng}/lib \
            -I ${pkgs.libcap.dev}/include -I ${pkgs.libcap_ng}/include -I ${pkgs.linuxHeaders}/include
      '';
diff --git a/nixos/modules/security/permissions-wrappers/setuid-wrapper-drv.nix b/nixos/modules/security/wrappers/setuid-wrapper-drv.nix
index 53cce2ff48e..e08ae799bf4 100644
--- a/nixos/modules/security/permissions-wrappers/setuid-wrapper-drv.nix
+++ b/nixos/modules/security/wrappers/setuid-wrapper-drv.nix
@@ -1,18 +1,18 @@
 { config, lib, pkgs, ... }:
 
 let  
-     cfg = config.security.permissionsWrappers;
+     cfg = config.security.wrappers;
 
      # Produce a shell-code splice intended to be stitched into one of
      # the build or install phases within the derivation.
      mkSetuidWrapper = { program, source ? null, ...}: ''
-       if ! source=${if source != null then source else "$(readlink -f $(PATH=$PERMISSIONS_WRAPPER_PATH type -tP ${program}))"}; then
-         # If we can't find the program, fall back to the
-         # system profile.
-         source=/nix/var/nix/profiles/default/bin/${program}
+       if ! source=${if source != null then source else "$(readlink -f $(PATH=$WRAPPER_PATH type -tP ${program}))"}; then
+           # If we can't find the program, fall back to the
+           # system profile.
+           source=/nix/var/nix/profiles/default/bin/${program}
        fi
 
-       gcc -Wall -O2 -DWRAPPER_SETUID=1 -DSOURCE_PROG=\"$source\" -DWRAPPER_DIR=\"${config.security.run-permissionsWrapperDir}\" \
+       gcc -Wall -O2 -DWRAPPER_SETUID=1 -DSOURCE_PROG=\"$source\" -DWRAPPER_DIR=\"${config.security.run-wrapperDir}\" \
            -lcap-ng -lcap ${./permissions-wrapper.c} -o $out/bin/${program}.wrapper -L ${pkgs.libcap.lib}/lib -L ${pkgs.libcap_ng}/lib \
            -I ${pkgs.libcap.dev}/include -I ${pkgs.libcap_ng}/include -I ${pkgs.linuxHeaders}/include
      '';