From 40a35299fa30421de85a56f084f6c59d05ea883e Mon Sep 17 00:00:00 2001 From: pennae Date: Sun, 9 Jan 2022 08:46:55 +0100 Subject: [PATCH] nixos: add functions and documentation for escaping systemd Exec* directives it's really easy to accidentally write the wrong systemd Exec* directive, ones that works most of the time but fails when users include systemd metacharacters in arguments that are interpolated into an Exec* directive. add a few functions analogous to escapeShellArg{,s} and some documentation on how and when to use them. --- .../development/writing-modules.chapter.md | 42 ++++++++++++++++ .../development/writing-modules.chapter.xml | 49 +++++++++++++++++++ nixos/lib/utils.nix | 20 ++++++++ nixos/tests/all-tests.nix | 1 + nixos/tests/empty-file | 0 nixos/tests/systemd-escaping.nix | 45 +++++++++++++++++ 6 files changed, 157 insertions(+) create mode 100644 nixos/tests/empty-file create mode 100644 nixos/tests/systemd-escaping.nix diff --git a/nixos/doc/manual/development/writing-modules.chapter.md b/nixos/doc/manual/development/writing-modules.chapter.md index 2e3c6b34f1f5..0c41cbd3cb75 100644 --- a/nixos/doc/manual/development/writing-modules.chapter.md +++ b/nixos/doc/manual/development/writing-modules.chapter.md @@ -90,6 +90,17 @@ modules: `systemd.services` (the set of all systemd services) and `systemd.timers` (the list of commands to be executed periodically by `systemd`). +Care must be taken when writing systemd services using `Exec*` directives. By +default systemd performs substitution on `%` specifiers in these +directives, expands environment variables from `$FOO` and `${FOO}`, splits +arguments on whitespace, and splits commands on `;`. All of these must be escaped +to avoid unexpected substitution or splitting when interpolating into an `Exec*` +directive, e.g. when using an `extraArgs` option to pass additional arguments to +the service. The functions `utils.escapeSystemdExecArg` and +`utils.escapeSystemdExecArgs` are provided for this, see [Example: Escaping in +Exec directives](#exec-escaping-example) for an example. When using these +functions system environment substitution should *not* be disabled explicitly. + ::: {#locate-example .example} ::: {.title} **Example: NixOS Module for the "locate" Service** @@ -153,6 +164,37 @@ in { ``` ::: +::: {#exec-escaping-example .example} +::: {.title} +**Example: Escaping in Exec directives** +::: +```nix +{ config, lib, pkgs, utils, ... }: + +with lib; + +let + cfg = config.services.echo; + echoAll = pkgs.writeScript "echo-all" '' + #! ${pkgs.runtimeShell} + for s in "$@"; do + printf '%s\n' "$s" + done + ''; + args = [ "a%Nything" "lang=\${LANG}" ";" "/bin/sh -c date" ]; +in { + systemd.services.echo = + { description = "Echo to the journal"; + wantedBy = [ "multi-user.target" ]; + serviceConfig.Type = "oneshot"; + serviceConfig.ExecStart = '' + ${echoAll} ${utils.escapeSystemdExecArgs args} + ''; + }; +} +``` +::: + ```{=docbook} diff --git a/nixos/doc/manual/from_md/development/writing-modules.chapter.xml b/nixos/doc/manual/from_md/development/writing-modules.chapter.xml index e33c24f4f12c..367731eda090 100644 --- a/nixos/doc/manual/from_md/development/writing-modules.chapter.xml +++ b/nixos/doc/manual/from_md/development/writing-modules.chapter.xml @@ -122,6 +122,25 @@ services) and systemd.timers (the list of commands to be executed periodically by systemd). + + Care must be taken when writing systemd services using + Exec* directives. By default systemd performs + substitution on %<char> specifiers in these + directives, expands environment variables from + $FOO and ${FOO}, splits + arguments on whitespace, and splits commands on + ;. All of these must be escaped to avoid + unexpected substitution or splitting when interpolating into an + Exec* directive, e.g. when using an + extraArgs option to pass additional arguments to + the service. The functions + utils.escapeSystemdExecArg and + utils.escapeSystemdExecArgs are provided for + this, see Example: Escaping in + Exec directives for an example. When using these functions + system environment substitution should not be + disabled explicitly. + Example: NixOS Module for the @@ -183,6 +202,36 @@ in { }; }; } + + + + Example: Escaping in Exec + directives + + +{ config, lib, pkgs, utils, ... }: + +with lib; + +let + cfg = config.services.echo; + echoAll = pkgs.writeScript "echo-all" '' + #! ${pkgs.runtimeShell} + for s in "$@"; do + printf '%s\n' "$s" + done + ''; + args = [ "a%Nything" "lang=\${LANG}" ";" "/bin/sh -c date" ]; +in { + systemd.services.echo = + { description = "Echo to the journal"; + wantedBy = [ "multi-user.target" ]; + serviceConfig.Type = "oneshot"; + serviceConfig.ExecStart = '' + ${echoAll} ${utils.escapeSystemdExecArgs args} + ''; + }; +} diff --git a/nixos/lib/utils.nix b/nixos/lib/utils.nix index bbebf8ba35a0..291350241954 100644 --- a/nixos/lib/utils.nix +++ b/nixos/lib/utils.nix @@ -45,6 +45,26 @@ rec { replaceChars ["/" "-" " "] ["-" "\\x2d" "\\x20"] (removePrefix "/" s); + # Quotes an argument for use in Exec* service lines. + # systemd accepts "-quoted strings with escape sequences, toJSON produces + # a subset of these. + # Additionally we escape % to disallow expansion of % specifiers. Any lone ; + # in the input will be turned it ";" and thus lose its special meaning. + # Every $ is escaped to $$, this makes it unnecessary to disable environment + # substitution for the directive. + escapeSystemdExecArg = arg: + let + s = if builtins.isPath arg then "${arg}" + else if builtins.isString arg then arg + else if builtins.isInt arg || builtins.isFloat arg then toString arg + else throw "escapeSystemdExecArg only allows strings, paths and numbers"; + in + replaceChars [ "%" "$" ] [ "%%" "$$" ] (builtins.toJSON s); + + # Quotes a list of arguments into a single string for use in a Exec* + # line. + escapeSystemdExecArgs = concatMapStringsSep " " escapeSystemdExecArg; + # Returns a system path for a given shell package toShellPath = shell: if types.shellPackage.check shell then diff --git a/nixos/tests/all-tests.nix b/nixos/tests/all-tests.nix index 9f3e97ceb13d..01708fe06792 100644 --- a/nixos/tests/all-tests.nix +++ b/nixos/tests/all-tests.nix @@ -459,6 +459,7 @@ in systemd-boot = handleTest ./systemd-boot.nix {}; systemd-confinement = handleTest ./systemd-confinement.nix {}; systemd-cryptenroll = handleTest ./systemd-cryptenroll.nix {}; + systemd-escaping = handleTest ./systemd-escaping.nix {}; systemd-journal = handleTest ./systemd-journal.nix {}; systemd-networkd = handleTest ./systemd-networkd.nix {}; systemd-networkd-dhcpserver = handleTest ./systemd-networkd-dhcpserver.nix {}; diff --git a/nixos/tests/empty-file b/nixos/tests/empty-file new file mode 100644 index 000000000000..e69de29bb2d1 diff --git a/nixos/tests/systemd-escaping.nix b/nixos/tests/systemd-escaping.nix new file mode 100644 index 000000000000..7f93eb5e4f70 --- /dev/null +++ b/nixos/tests/systemd-escaping.nix @@ -0,0 +1,45 @@ +import ./make-test-python.nix ({ pkgs, ... }: + +let + echoAll = pkgs.writeScript "echo-all" '' + #! ${pkgs.runtimeShell} + for s in "$@"; do + printf '%s\n' "$s" + done + ''; + # deliberately using a local empty file instead of pkgs.emptyFile to have + # a non-store path in the test + args = [ "a%Nything" "lang=\${LANG}" ";" "/bin/sh -c date" ./empty-file 4.2 23 ]; +in +{ + name = "systemd-escaping"; + + machine = { pkgs, lib, utils, ... }: { + systemd.services.echo = + assert !(builtins.tryEval (utils.escapeSystemdExecArgs [ [] ])).success; + assert !(builtins.tryEval (utils.escapeSystemdExecArgs [ {} ])).success; + assert !(builtins.tryEval (utils.escapeSystemdExecArgs [ null ])).success; + assert !(builtins.tryEval (utils.escapeSystemdExecArgs [ false ])).success; + assert !(builtins.tryEval (utils.escapeSystemdExecArgs [ (_:_) ])).success; + { description = "Echo to the journal"; + serviceConfig.Type = "oneshot"; + serviceConfig.ExecStart = '' + ${echoAll} ${utils.escapeSystemdExecArgs args} + ''; + }; + }; + + testScript = '' + machine.wait_for_unit("multi-user.target") + machine.succeed("systemctl start echo.service") + # skip the first 'Starting ...' line + logs = machine.succeed("journalctl -u echo.service -o cat").splitlines()[1:] + assert "a%Nything" == logs[0] + assert "lang=''${LANG}" == logs[1] + assert ";" == logs[2] + assert "/bin/sh -c date" == logs[3] + assert "/nix/store/ij3gw72f4n5z4dz6nnzl1731p9kmjbwr-empty-file" == logs[4] + assert "4.2" in logs[5] # toString produces extra fractional digits! + assert "23" == logs[6] + ''; +})