From 904f68fb0fc01cf4072c1215416eb4e2b9fc4e56 Mon Sep 17 00:00:00 2001 From: rnhmjoj Date: Wed, 9 Jun 2021 01:41:26 +0200 Subject: [PATCH] nixos/security/wrappers: make well-typed The security.wrappers option is morally a set of submodules but it's actually (un)typed as a generic attribute set. This is bad for several reasons: 1. Some of the "submodule" option are not document; 2. the default values are not documented and are chosen based on somewhat bizarre rules (issue #23217); 3. It's not possible to override an existing wrapper due to the dumb types.attrs.merge strategy; 4. It's easy to make mistakes that will go unnoticed, which is really bad given the sensitivity of this module (issue #47839). This makes the option a proper set of submodule and add strict types and descriptions to every sub-option. Considering it's not yet clear if the way the default values are picked is intended, this reproduces the current behavior, but it's now documented explicitly. --- nixos/modules/security/wrappers/default.nix | 176 +++++++++++++------- 1 file changed, 119 insertions(+), 57 deletions(-) diff --git a/nixos/modules/security/wrappers/default.nix b/nixos/modules/security/wrappers/default.nix index 1e65f4515155..74dfd86b86af 100644 --- a/nixos/modules/security/wrappers/default.nix +++ b/nixos/modules/security/wrappers/default.nix @@ -5,23 +5,108 @@ let parentWrapperDir = dirOf wrapperDir; - programs = - (lib.mapAttrsToList - (n: v: (if v ? program then v else v // {program=n;})) - wrappers); - securityWrapper = pkgs.callPackage ./wrapper.nix { inherit parentWrapperDir; }; + fileModeType = + let + # taken from the chmod(1) man page + symbolic = "[ugoa]*([-+=]([rwxXst]*|[ugo]))+|[-+=][0-7]+"; + numeric = "[-+=]?[0-7]{0,4}"; + mode = "((${symbolic})(,${symbolic})*)|(${numeric})"; + in + lib.types.strMatching mode + // { description = "file mode string"; }; + + wrapperType = lib.types.submodule ({ name, config, ... }: { + options.source = lib.mkOption + { type = lib.types.path; + description = "The absolute path to the program to be wrapped."; + }; + options.program = lib.mkOption + { type = with lib.types; nullOr str; + default = name; + description = '' + The name of the wrapper program. Defaults to the attribute name. + ''; + }; + options.owner = lib.mkOption + { type = lib.types.str; + default = with config; + if (capabilities != "") || !(setuid || setgid || permissions != null) + then "root" + else "nobody"; + description = '' + The owner of the wrapper program. Defaults to root + if any capability is set and setuid/setgid/permissions are not, otherwise to + nobody. + ''; + }; + options.group = lib.mkOption + { type = lib.types.str; + default = with config; + if (capabilities != "") || !(setuid || setgid || permissions != null) + then "root" + else "nogroup"; + description = '' + The group of the wrapper program. Defaults to root + if any capability is set and setuid/setgid/permissions are not, + otherwise to nogroup. + ''; + }; + options.permissions = lib.mkOption + { type = lib.types.nullOr fileModeType; + default = null; + example = "u+rx,g+x,o+x"; + apply = x: if x == null then "u+rx,g+x,o+x" else x; + description = '' + The permissions of the wrapper program. The format is that of a + symbolic or numeric file mode understood by chmod. + ''; + }; + options.capabilities = lib.mkOption + { type = lib.types.commas; + default = ""; + description = '' + A comma-separated list of capabilities to be given to the wrapper + program. For capabilities supported by the system check the + + capabilities + 7 + + manual page. + + + 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. + + ''; + }; + options.setuid = lib.mkOption + { type = lib.types.bool; + default = false; + description = "Whether to add the setuid bit the wrapper program."; + }; + options.setgid = lib.mkOption + { type = lib.types.bool; + default = false; + description = "Whether to add the setgid bit the wrapper program."; + }; + }); + ###### Activation script for the setcap wrappers mkSetcapProgram = { program , capabilities , source - , owner ? "nobody" - , group ? "nogroup" - , permissions ? "u+rx,g+x,o+x" + , owner + , group + , permissions , ... }: assert (lib.versionAtLeast (lib.getVersion config.boot.kernelPackages.kernel) "4.3"); @@ -46,11 +131,11 @@ let mkSetuidProgram = { program , source - , owner ? "nobody" - , group ? "nogroup" - , setuid ? false - , setgid ? false - , permissions ? "u+rx,g+x,o+x" + , owner + , group + , setuid + , setgid + , permissions , ... }: '' @@ -66,24 +151,11 @@ let mkWrappedPrograms = builtins.map - (s: if (s ? capabilities) - then mkSetcapProgram - ({ owner = "root"; - group = "root"; - } // s) - else if - (s ? setuid && s.setuid) || - (s ? setgid && s.setgid) || - (s ? permissions) - then mkSetuidProgram s - else mkSetuidProgram - ({ owner = "root"; - group = "root"; - setuid = true; - setgid = false; - permissions = "u+rx,g+x,o+x"; - } // s) - ) programs; + (opts: + if opts.capabilities != "" + then mkSetcapProgram opts + else mkSetuidProgram opts + ) (lib.attrValues wrappers); in { imports = [ @@ -95,7 +167,7 @@ in options = { security.wrappers = lib.mkOption { - type = lib.types.attrs; + type = lib.types.attrsOf wrapperType; default = {}; example = lib.literalExample '' @@ -109,31 +181,11 @@ in } ''; 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). - - - The sub-attribute source is mandatory, - it must be the absolute path to the program to be wrapped. - - - The sub-attribute program is optional and - can give the wrapper program a new name. The default name is the same - as the attribute name itself. - - Additionally, this option can set capabilities on a - wrapper program that propagates those capabilities down to the - wrapped, real program. - - 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. - + This option effectively allows adding setuid/setgid bits, capabilities, + changing file ownership and permissions of a program without directly + modifying it. This works by creating a wrapper program under the + directory, which is then added to + the shell PATH. ''; }; @@ -151,6 +203,16 @@ in ###### implementation config = { + assertions = lib.mapAttrsToList + (name: opts: + { assertion = opts.setuid || opts.setgid -> opts.capabilities == ""; + message = '' + The security.wrappers.${name} wrapper is not valid: + setuid/setgid and capabilities are mutually exclusive. + ''; + } + ) wrappers; + security.wrappers = { # These are mount related wrappers that require the +s permission. fusermount.source = "${pkgs.fuse}/bin/fusermount";