From df475092e92b9dab9642c48f2216d49027a457a1 Mon Sep 17 00:00:00 2001 From: aszlig Date: Mon, 20 Jun 2016 18:31:49 +0200 Subject: [PATCH] lib: Make escapeShellArg more robust Quoting various characters that the shell *may* interpret specially is a very fragile thing to do. I've used something more robust all over the place in various Nix expression I've written just because I didn't trust escapeShellArg. Here is a proof of concept showing that I was indeed right in distrusting escapeShellArg: with import {}; let payload = runCommand "payload" {} '' # \x00 is not allowed for Nix strings, so let's begin at 1 for i in $(seq 1 255); do echo -en "\\x$(printf %02x $i)" done > "$out" ''; escapers = with lib; { current = escapeShellArg; better = arg: let backslashEscapes = stringToCharacters "\"\\ ';$`()|<>\r\t*[]&!~#"; search = backslashEscapes ++ [ "\n" ]; replace = map (c: "\\${c}") backslashEscapes ++ [ "'\n'" ]; in replaceStrings search replace (toString arg); best = arg: "'${replaceStrings ["'"] ["'\\''"] (toString arg)}'"; }; testWith = escaper: let escaped = escaper (builtins.readFile payload); in runCommand "test" {} '' if ! r="$(bash -c ${escapers.best "echo -nE ${escaped}"} 2> /dev/null)" then echo bash eval error > "$out" exit 0 fi if echo -n "$r" | cmp -s "${payload}"; then echo success > "$out" else echo failed > "$out" fi ''; in runCommand "results" {} '' echo "Test results:" ${lib.concatStrings (lib.mapAttrsToList (name: impl: '' echo " ${name}: $(< "${testWith impl}")" '') escapers)} exit 1 '' The resulting output is the following: Test results: best: success better: success current: bash eval error I did the "better" implementation just to illustrate that the method of quoting only "harmful" characters results in madness in terms of implementation and performance. Signed-off-by: aszlig Cc: @edolstra, @zimbatm --- lib/strings.nix | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/lib/strings.nix b/lib/strings.nix index 04376a5f2fb..5e5f7b37866 100644 --- a/lib/strings.nix +++ b/lib/strings.nix @@ -203,20 +203,19 @@ rec { */ escape = list: replaceChars list (map (c: "\\${c}") list); - /* Escape all characters that have special meaning in the Bourne shell. + /* Quote string to be used safely within the Bourne shell. Example: - escapeShellArg "so([<>])me" - => "so\\(\\[\\<\\>\\]\\)me" + escapeShellArg "esc'ape\nme" + => "'esc'\\''ape\nme'" */ - escapeShellArg = arg: - lib.escape (stringToCharacters "\\ ';$`()|<>\t*[]") (toString arg); + escapeShellArg = arg: "'${replaceStrings ["'"] ["'\\''"] (toString arg)}'"; - /* Escape all arguments to be passed to the Bourne shell. + /* Quote all arguments to be safely passed to the Bourne shell. Example: - escapeShellArgs ["one" "two three"] - => "one two\\ three" + escapeShellArgs ["one" "two three" "four'five"] + => "'one' 'two three' 'four'\\''five'" */ escapeShellArgs = concatMapStringsSep " " escapeShellArg;