t/{t-path,testenv}.sh: refactor and fix path tests

chrisd8088 · chrisd8088 · commit 11092ef2b17e · 2022-04-19T09:45:20.000-07:00
In commits 74d5f23 and 10c4ffc the t/t-path.sh tests were added to validate the remediations of the security issues from CVE-2020-27955 and CVE-2021-21237, respectively. On Windows, both of these tests make use of a "git.bat" script which stands in for Git, and if the script is executed instead of the real Git during a test, then that indicates that a security problem still exists. However, in a previous commit we have now added a new helper program, lfstest-badpathcheck, which can be used for the same purpose, and which has the advantage of having a .exe extension on Windows. We will make use of this helper program in a new test accompanying the remediation of CVE-2022-24826. The existence of this new helper also means we can now use it as a fake "malicious" binary named "git.exe" in our existing t/t-path.sh tests. This will help ensure that these tests are robust against unexpected values of the Windows PATHEXT environment variable or future changes to the set of executable file extensions we support in Git LFS. Using this new helper program instead of a "git.bat" script does mean we need to be careful how and when it might run, because it mirrors the name of the real Git executable. We therefore try to keep it out of any possible execution path until we run our final concluding checks in each test. In other words, rather than try to manipulate PATH (and PATHEXT on Windows), we take steps to keep our "malicious" executable out of any possible search path prior to when we want to explicitly test Git LFS's behaviour in a known state relating to one of the CVEs. (One challenge with manipulating PATH to try to remove paths that resolve to the current directory is that it must be done iteratively to deal with pathological cases like ":.:.:" and ":::". The "sed" regular expressions in our existing tests would also need to use "\." to match only the "." character, as well as additional expressions to match "." at the beginning and end of PATH.) It is easier, therefore, to simply avoid putting our "malicious" executable into the current directory until the last moment. Also, in the second test where we want to add it to a repository which we will then clone, we can remove it as soon as we've run "git add", and we can make sure to run the real Git (i.e., whatever is found first using the extant PATH and PATHEXT variables) rather than ours to perform that "git add". When we reach the end of each test, where we want to explicitly run Git LFS and see if it incorrectly searches in the current directory even when not asked to, we now reset PATH and PATHEXT just for those specific invocations of Git LFS. For PATH we use either BINPATH, which contains only our compiled "git-lfs" binary, or BINPATH plus the path to the real Git as returned by "command -v". For PATHEXT we just use the primary executable file extension (if any) for the current operating system. To determine that primary executable file extension we add an X variable which we set in t/testenv.sh and which parallels the one set in the main Makefile. On Windows, we set X to contain ".exe", and on Unix we set it to the empty string. We can then use this X variable throughout our tests wherever we want to refer to a specific executable's full filename. With these changes, even when PATH includes "." as the first directory to be searched, both of our tests should now always reach their concluding checks and should function as expected at that point. Specifically, they should fail by detecting the output of our "malicious" Git program when run without the Git LFS code changes corresponding to their respective CVEs, and should succeed otherwise. (Technically, the second test will fail for a different reason if the remediation for CVE-2020-27955 is not in effect than if only the remediation for CVE-2021-21237 is not in effect. But the test will fail at the same point in both cases, i.e., in its concluding final check.) In the final checks in both tests we search for the text string "exploit" in the output log file captured after running a Git LFS command, using the shell command "! grep -q 'exploit' output.log". The "!" reverses the exit code from "grep", so if the word "exploit" is found, the test should fail. This works in the first test because the command is the last one in the test, so the inverted exit code from "grep" is returned as the exit code from whole test's subshell. However, in the second test several other commands follow this command, and because "set -e" (which is standard at the top of every test) ignores commands' exit codes when they are inverted with "!", the test proceeds even if the word "exploit" is seen in the output log. To resolve this problem we instead use a command pipeline and ensure that when the "grep" succeeds, the exit code from the final command in the pipeline is generated by "false". This successfully causes the test to fail immediately when the word "exploit" is seen in the output log file. Moreover, in both tests we now follow the "grep" check with checks for the presence of a file named "exploit"; this provides a second level of assurance that our "malicious" Git program has not executed. Finally, we add detailed comments regarding specific steps in both tests where the intention and purpose may not be clear just from the context.
diff --git a/t/t-path.sh b/t/t-path.sh
@@ -9,16 +9,18 @@ begin_test "does not look in current directory for git"
   reponame="$(basename "$0" ".sh")"
   git init "$reponame"
   cd "$reponame"
-  export PATH="$(echo "$PATH" | sed -e "s/:.:/:/g" -e "s/::/:/g")"
 
-  printf "#!/bin/sh\necho exploit >&2\n" > git
-  chmod +x git || true
-  printf "echo exploit 1>&2\n" > git.bat
+  cp "$BINPATH/lfstest-badpathcheck$X" "git$X"
 
-  # This needs to succeed.  If it fails, that could be because our malicious
-  # "git" is broken but got invoked anyway.
-  git lfs env > output.log 2>&1
-  ! grep -q 'exploit' output.log
+  # This should always succeed, even if git-lfs is incorrectly searching for
+  # executables in the current directory first, because the "git-lfs env"
+  # command ignores all errors when it runs "git config".  So we should always
+  # pass this step and then, if our malicious Git was executed, detect
+  # its output below.  If this command does fail, something else is wrong.
+  PATH="$BINPATH" PATHEXT="$X" "git-lfs$X" env >output.log 2>&1
+
+  grep "exploit" output.log && false
+  [ ! -f exploit ]
 )
 end_test
 
@@ -30,32 +32,88 @@ begin_test "does not look in current directory for git with credential helper"
   setup_remote_repo "$reponame"
 
   clone_repo "$reponame" credentials-1
-  export PATH="$(echo "$PATH" | sed -e "s/:.:/:/g" -e "s/::/:/g")"
-
-  printf "#!/bin/sh\necho exploit >&2\ntouch exploit\n" > git
-  chmod +x git || true
-  printf "echo exploit 1>&2\r\necho >exploit" > git.bat
 
   git lfs track "*.dat"
   printf abc > z.dat
   git add z.dat
   git add .gitattributes
-  git add git git.bat
-  git commit -m "Add files"
 
+  GITPATH="$(dirname "$(command -v git)")"
+
+  # We add our malicious Git to the index and then remove it from the
+  # work tree so it is not found early, before we perform our key test.
+  # Specifically, our "git push" below will run git-lfs, which then runs
+  # "git credential", so if we are looking for Git in the current directory
+  # first when running a credential helper, we will fail at that point
+  # because our malicious Git will be found first.
+  #
+  # We prefer to check for this behavior during our "git-lfs pull" further
+  # below when we are populating LFS objects into a clone of this repo
+  # (which contains the malicious Git), so for now we remove the malicious
+  # Git as soon as possible.
+  cp "$BINPATH/lfstest-badpathcheck$X" "git$X"
+  PATH="$BINPATH:$GITPATH" "$GITPATH/git$X" add "git$X"
+  rm "git$X"
+
+  git commit -m "Add files"
   git push origin HEAD
   cd ..
 
   unset GIT_ASKPASS SSH_ASKPASS
 
-  # This needs to succeed.  If it fails, that could be because our malicious
-  # "git" is broken but got invoked anyway.
-  GIT_LFS_SKIP_SMUDGE=1 clone_repo "$reponame" credentials-2
+  # When we call "git clone" below, it will run git-lfs as a smudge filter
+  # during the post-clone checkout phase, and specifically will run git-lfs
+  # in the newly cloned repository directory which contains a copy of our
+  # malicious Git.  So, if we are looking for Git in the current directory
+  # first in most cases (and not just when running a credential helper),
+  # then when git-lfs runs "git config" we will fail at that point because
+  # our malicious Git will be found first.  This occurs even if we specify
+  # GIT_LFS_SKIP_SMUDGE=1 because git-lfs will still run "git config".
+  #
+  # We could ignore errors from clone_repo() and then search for the output
+  # of our malicious Git in the t-path-credentials-2 directory; however,
+  # this may be somewhat fragile as clone_repo() performs other steps such
+  # as changing the current working directory to the new repo clone and
+  # attempting to run "git config" there.
+  #
+  # Instead, since our key check of "git-lfs pull" below will also detect
+  # the general failure case where we are looking for Git in the current
+  # directory first when running most commands, we temporarily uninstall
+  # Git LFS so no smudge filter will execute when "git clone" checks out the
+  # repository.
+  #
+  # We also remove any "exploit" file potentially created by our malicious
+  # Git in case it was run anywhere in clone_repo(), which may happen if
+  # PATH contains the "." directory already.  Note that we reset PATH
+  # to contain only the necessary directories in our key "git-lfs pull"
+  # check below.
+  git lfs uninstall
+  clone_repo "$reponame" t-path-credentials-2
+  rm -f exploit
+  pushd ..
+    git lfs install
+  popd
 
-  git lfs pull | tee output.log
+  # As noted, if we are looking for Git in the current directory first
+  # only when running a credential helper, then when this runs
+  # "git credential", it will find our malicious Git in the current directory
+  # and execute it.
+  #
+  # If we are looking for Git in the current directory first when running
+  # most commands (and not just when running a credential helper), then this
+  # will also find our malicious Git.  However, in this case it will find it
+  # earlier when we try to run "git config" rather than later when we try
+  # to run "git credential".
+  #
+  # We use a pipeline with "tee" here so as to avoid an early failure in the
+  # case that our "git-lfs pull" command executes our malicious Git.
+  # Unlike "git-lfs env" in the other tests, "git-lfs pull" will halt when
+  # it does not receive the normal output from Git.  This in turn halts
+  # our test due to our use of the "set -e" option, unless we terminate a
+  # pipeline with successful command like "tee".
+  PATH="$BINPATH:$GITPATH" PATHEXT="$X" "git-lfs$X" pull 2>&1 | tee output.log
 
-  ! grep -q 'exploit' output.log
-  [ ! -f ../exploit ]
+  grep "exploit" output.log && false
   [ ! -f exploit ]
 )
 end_test
diff --git a/t/testenv.sh b/t/testenv.sh
@@ -6,12 +6,14 @@ set -e
 UNAME=$(uname -s)
 IS_WINDOWS=0
 IS_MAC=0
+X=""
 SHASUM="shasum -a 256"
 PATH_SEPARATOR="/"
 
 if [[ $UNAME == MINGW* || $UNAME == MSYS* || $UNAME == CYGWIN* ]]
 then
   IS_WINDOWS=1
+  X=".exe"
 
   # Windows might be MSYS2 which does not have the shasum Perl wrapper
   # script by default, so use sha256sum directly. MacOS on the other hand
