From 46f7dd436f4b10d9c6cdde737d4da3ffce8e88be Mon Sep 17 00:00:00 2001
From: aszlig <aszlig@nix.build>
Date: Thu, 14 Mar 2019 19:07:03 +0100
Subject: nixos/confinement: Allow to configure /bin/sh

Another thing requested by @edolstra in [1]:

  We should not provide a different /bin/sh in the chroot, that's just
  asking for confusion and random shell script breakage. It should be
  the same shell (i.e. bash) as in a regular environment.

While I personally would even go as far to even have a very restricted
shell that is not even a shell and basically *only* allows "/bin/sh -c"
with only *very* minimal parsing of shell syntax, I do agree that people
expect /bin/sh to be bash (or the one configured by environment.binsh)
on NixOS.

So this should make both others and me happy in that I could just use
confinement.binSh = "${pkgs.dash}/bin/dash" for the services I confine.

[1]: https://github.com/NixOS/nixpkgs/pull/57519#issuecomment-472855704

Signed-off-by: aszlig <aszlig@nix.build>
---
 nixos/modules/security/systemd-confinement.nix | 35 +++++++++++++++++---------
 nixos/tests/systemd-confinement.nix            | 26 +++++++++++++++++++
 2 files changed, 49 insertions(+), 12 deletions(-)

diff --git a/nixos/modules/security/systemd-confinement.nix b/nixos/modules/security/systemd-confinement.nix
index dc53bbc4dbb..a8367ca5eed 100644
--- a/nixos/modules/security/systemd-confinement.nix
+++ b/nixos/modules/security/systemd-confinement.nix
@@ -1,6 +1,7 @@
 { config, pkgs, lib, ... }:
 
 let
+  toplevelConfig = config;
   inherit (lib) types;
   inherit (import ../system/boot/systemd-lib.nix {
     inherit config pkgs lib;
@@ -44,12 +45,15 @@ in {
         '';
       };
 
-      options.confinement.withBinSh = lib.mkOption {
-        type = types.bool;
-        default = true;
+      options.confinement.binSh = lib.mkOption {
+        type = types.nullOr types.path;
+        default = toplevelConfig.environment.binsh;
+        defaultText = "config.environment.binsh";
+        example = lib.literalExample "\${pkgs.dash}/bin/dash";
         description = ''
-          Whether to symlink <command>dash</command> as
-          <filename>/bin/sh</filename> to the chroot.
+          The program to make available as <filename>/bin/sh</filename> inside
+          the chroot. If this is set to <literal>null</literal>, no
+          <filename>/bin/sh</filename> is provided at all.
 
           This is useful for some applications, which for example use the
           <citerefentry>
@@ -81,15 +85,14 @@ in {
         '';
       };
 
-      config = lib.mkIf config.confinement.enable {
-        serviceConfig = let
-          rootName = "${mkPathSafeName name}-chroot";
-        in {
+      config = let
+        rootName = "${mkPathSafeName name}-chroot";
+        inherit (config.confinement) binSh;
+      in lib.mkIf config.confinement.enable {
+        serviceConfig = {
           RootDirectory = pkgs.runCommand rootName {} "mkdir \"$out\"";
           TemporaryFileSystem = "/";
           MountFlags = lib.mkDefault "private";
-        } // lib.optionalAttrs config.confinement.withBinSh {
-          BindReadOnlyPaths = [ "${pkgs.dash}/bin/dash:/bin/sh" ];
         } // lib.optionalAttrs (config.confinement.mode == "full-apivfs") {
           MountAPIVFS = true;
           PrivateDevices = true;
@@ -108,7 +111,7 @@ in {
           execPkgs = lib.concatMap (opt: let
             isSet = config.serviceConfig ? ${opt};
           in lib.optional isSet config.serviceConfig.${opt}) execOpts;
-        in execPkgs ++ lib.optional config.confinement.withBinSh pkgs.dash;
+        in execPkgs ++ lib.optional (binSh != null) binSh;
       };
     }));
   };
@@ -146,6 +149,14 @@ in {
 
       echo '[Service]' > "$serviceFile"
 
+      # /bin/sh is special here, because the option value could contain a
+      # symlink and we need to properly resolve it.
+      ${lib.optionalString (cfg.confinement.binSh != null) ''
+        binsh=${lib.escapeShellArg cfg.confinement.binSh}
+        realprog="$(readlink -e "$binsh")"
+        echo "BindReadOnlyPaths=$realprog:/bin/sh" >> "$serviceFile"
+      ''}
+
       while read storePath; do
         if [ -L "$storePath" ]; then
           # Currently, systemd can't cope with symlinks in Bind(ReadOnly)Paths,
diff --git a/nixos/tests/systemd-confinement.nix b/nixos/tests/systemd-confinement.nix
index 448d34ec30b..63cb074d7ca 100644
--- a/nixos/tests/systemd-confinement.nix
+++ b/nixos/tests/systemd-confinement.nix
@@ -106,6 +106,32 @@ import ./make-test.nix {
           $machine->succeed('test ! -e /tmp/canary');
         '';
       }
+      { description = "check if /bin/sh works";
+        testScript = ''
+          $machine->succeed(
+            'chroot-exec test -e /bin/sh',
+            'test "$(chroot-exec \'/bin/sh -c "echo bar"\')" = bar',
+          );
+        '';
+      }
+      { description = "check if suppressing /bin/sh works";
+        config.confinement.binSh = null;
+        testScript = ''
+          $machine->succeed(
+            'chroot-exec test ! -e /bin/sh',
+            'test "$(chroot-exec \'/bin/sh -c "echo foo"\')" != foo',
+          );
+        '';
+      }
+      { description = "check if we can set /bin/sh to something different";
+        config.confinement.binSh = "${pkgs.hello}/bin/hello";
+        testScript = ''
+          $machine->succeed(
+            'chroot-exec test -e /bin/sh',
+            'test "$(chroot-exec /bin/sh -g foo)" = foo',
+          );
+        '';
+      }
     ];
 
     options.__testSteps = lib.mkOption {
-- 
cgit 1.4.1