Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix two remaining Windows untrusted search path cases #1792

Merged
merged 16 commits into from
Jan 10, 2024

Commits on Dec 26, 2023

  1. Show "not from cwd" test is broken for shell=True

    This adds a test_it_executes_git_not_from_cwd case for shell=True.
    (This case also gives the command as a string, so the test need not
    be further special-cased for non-Windows systems, where argument
    lists aren't accepted with shell=True.)
    
    The test did not attempt to cover the shell=True case before,
    because I had erroneously assumed it worked similarity. It is
    actually very different, because when a shell is used, both the
    shell and the command the shell runs must be found and executed,
    and because the process creation GitPython performs is that of the
    shell process, with the state of the shell process being what is
    relevant to how the path search is done for the git (or other)
    command.
    
    The code change here does not itself demonstrate that the test is
    broken for shell=True, because that case passes. However, manually
    undoing the fix in cmd.py for CVE-2023-40590, which as expected
    causes the preexisting (implicitly shell=False case) to fail, does
    *not* cause the new shell=True case to fail. That case passes!
    
    That passing result in the absence of a fix for CVE-2023-40590 is
    erroneous, because the cmd.exe shell does search the CWD first when
    nothing has been done to prevent it.
    EliahKagan committed Dec 26, 2023
    Configuration menu
    Copy the full SHA
    1c65efb View commit details
    Browse the repository at this point in the history
  2. Correct the "not from cwd" test and add more cases

    This shows that CVE-2023-40590 is only partially patched. This
    commit does not fix the vulnerbility, only the test.
    
    The problem is that, when shell=True, the environment of the shell
    subprocess, instead of the GitPython process, determines whether
    searching for the "git" command will use the current directory.
    Currently NoDefaultCurrentDirectoryInExePath is set only in the
    current process, and this is done after the environment for the
    subprocess (env) is computed. So when git is an indirect subprocess
    due to shell=True, Windows still checks a CWD when looking it up.
    
    (Note that this should not be a problem for indirect subprocesses
    started by git itself. When a standard git command is implemented
    as an external executable, when git runs a custom command, and when
    one git command delegates some of its work to another -- e.g.
    "git clone" running git-remote-https -- Git for Windows already
    does not search the current directory. Custom git commands that
    start their own git subprocesses could have an analogous path
    search bug, but this would be separate from the bug in GitPython.)
    
    This is an exploitable vulnerability in GitPython. Although
    shell=True is rarer and inherently less secure than the default of
    shell=False, it still ought to avoid automatically running an
    executable that may exist only due to having been cloned as part of
    an untrusted repository. In addition, historically programs on
    Windows had sometimes used shell=True as a workaround for console
    windows being displayed for subprocesses, and some such code may
    still be in use.
    
    Furthermore, even when GitPython's current working directory is
    outside the repository being worked on, the Git object in a Repo
    instance's "git" attribute holds the repository directory as its
    "_working_dir" attribute, which Git.execute consults to determine
    the value of "cwd" to pass to subprocess.Popen. When the created
    direct subprocess is really a shell, this is the CWD where git.exe
    is looked for before searching PATH directories.
    
    This is also why previous, more limited testing (including
    accompanying manual testing with shell=True) didn't discover the
    bug. Even when modified to test with shell=True, the old test had
    the "impostor" git.exe in the GitPython process's own current
    directory (which was changed to a temporary directory for the test,
    where the "impostor" was created, but this was separate from the
    working tree of the self.git repository). This was effective for
    the shell=False case, but not the shell=True case where the
    impostor would be found and run in the repository directory even
    when it differs from the GitPython process's CWD.
    EliahKagan committed Dec 26, 2023
    Configuration menu
    Copy the full SHA
    2b47933 View commit details
    Browse the repository at this point in the history
  3. Use SystemRoot instead of WINDIR, to fix tox

    In most cases they will be the same, but WINDIR may be absent (or
    have a different value?) in rare cases.
    
    The practical reason it matters in GitPython's tests is that tox
    automatically passes SystemRoot through to environments on Windows.
    EliahKagan committed Dec 26, 2023
    Configuration menu
    Copy the full SHA
    c1f6c17 View commit details
    Browse the repository at this point in the history
  4. Configuration menu
    Copy the full SHA
    06bd2c7 View commit details
    Browse the repository at this point in the history
  5. Refactor "not from cwd" test for readability

    By using pathlib.Path instead of os.path functions.
    EliahKagan committed Dec 26, 2023
    Configuration menu
    Copy the full SHA
    7da9c3b View commit details
    Browse the repository at this point in the history

Commits on Jan 9, 2024

  1. Further expand "not from cwd" test re: cmd.exe

    On Python versions for which python/cpython#101283 is not patched,
    using Popen with shell=True can find cmd.exe in the current
    directory on Windows, in the rare case that the ComSpec environment
    variable is not defined.
    
    This is not necessarily worth addressing, because it is a bug in
    CPython rahter than GitPython, because that bug has been patched,
    and because it is very rare that ComSpec is undefined. However:
    
    - Changing the code to avoid it would also make that code simpler.
    - Patched versions of Python <=3.9 don't have python.org builds.
    
    This commit just expands the test to add cases where a repository
    also has a file cmd.exe and where ComSpec is undefined, showing
    that this case is not covered.
    EliahKagan committed Jan 9, 2024
    Configuration menu
    Copy the full SHA
    865c6e8 View commit details
    Browse the repository at this point in the history
  2. Make Git.execute a bit simpler and very slightly more robust

    This covers the rare unpatched python/cpython#101283 case the
    previous commit added tests for, that only applies in the unusual
    situation that the ComSpec environment variable is unset and an old
    build (but this includes downloadable builds and current
    actions/setup-python builds) of Python <=3.9 for Windows is in use.
    
    The main benefit of this change is really to slightly simplify the
    code under test. (It might even be justified to remove the
    use_shell_impostor=True test cases at some point.)
    EliahKagan committed Jan 9, 2024
    Configuration menu
    Copy the full SHA
    d2506c7 View commit details
    Browse the repository at this point in the history
  3. Start on test_hook_uses_shell_not_from_cwd

    This shows that run_commit_hook is vulnerable to an untrusted
    search path bug on Windows, when running script hooks: bash.exe is
    run without setting NoDefaultCurrentDirectoryInExePath or otherwise
    excluding the current directory from the path search.
    
    The new test uses a non-bare repo, even though the surrounding
    tests use bare repos. Although the test also works if the repo is
    initialized with `Repo.init(rw_dir, bare=True)`, using a bare repo
    would obscure how the bug this test reveals would typically be
    exploited, where a command that uses a hook is run after a
    malicious bash.exe is checked out into the working tree from an
    untrusted branch.
    
    Running hooks that are themselves provided by an untrusted
    repository or branch is of course never safe. If an attacker can
    deceive a user into doing that, then this vulnerability is not
    needed. Instead, an attack that leverages this untrusted search
    path vulnerability would most likely be of roughly this form:
    
    1. The user clones a trusted repository and installs hooks.
    2. A malicious party offers a contribution to the project.
    3. The user checks out the malicious party's untrusted branch.
    4. The user performs an action that runs a hook.
    
    The hook the user runs is still trusted, but it runs with the
    malicious bash.exe found in the current directory (which is the
    working tree or perhaps some subdirectory of it).
    
    The test added in this commit should, if possible, be improved to
    be able to run and detect the bug (or its absence) even when bash
    is absent from the Windows system and, preferably, also even when
    the WSL bash.exe is present but no WSL distribution is installed.
    EliahKagan committed Jan 9, 2024
    Configuration menu
    Copy the full SHA
    61b4dda View commit details
    Browse the repository at this point in the history
  4. Omit CWD in search for bash.exe to run hooks on Windows

    This uses the same NoDefaultCurrentDirectoryInExePath technique for
    the Popen call in git.index.fun.run_commit_hook on Windows as is
    already used in git.cmd.Git.execute.
    
    The code is simpler in run_commit_hook because a shell is never
    used to run bash.exe. (bash.exe is itself a shell, but we never
    run it *via* a shell by passing shell=True to Popen.) Nonetheless,
    it may make sense to extract out a helper function both can call.
    This commit does not do that, so there is some code duplication.
    EliahKagan committed Jan 9, 2024
    Configuration menu
    Copy the full SHA
    66ff4c1 View commit details
    Browse the repository at this point in the history
  5. Extract venv management from test_installation

    - Create and use a test.lib.helper.VirtualEnvironment class.
    - Import and use venv module instead of running "python -m venv".
    
    These changes make no significant difference in speed or clarity
    for the existing test in test_installation. The reason for this
    change is instead to support the use of new per-test virtual
    environments in at least one other test.
    EliahKagan committed Jan 9, 2024
    Configuration menu
    Copy the full SHA
    7751436 View commit details
    Browse the repository at this point in the history
  6. Cover absent/no-distro bash.exe in hooks "not from cwd" test

    See comments for details on the test's new implementation and what
    it achieves.
    EliahKagan committed Jan 9, 2024
    Configuration menu
    Copy the full SHA
    a42ea0a View commit details
    Browse the repository at this point in the history
  7. Avoid spurious "location may have moved" on Windows

    The main case where this happens is when tempfile.gettempdir() has
    a component in it that uses an 8.3-encoded path, e.g.,
    C:\Users\Administrator\... -> C:\Users\ADMINI~1\...
    
    This is a workaround for python/cpython#90329. I call realpath only
    once, when the venv is created, and not on any paths inside the
    venv, to make it less likely this masks the problems the warning is
    meant for. (For example, if Scripts, or python.exe in it, produced
    this even with the venv created as it is now, then that may indicte
    an actual problem.)
    
    Note that copying python.exe from Scripts to one level up in the
    venv, and changing its name to bash.exe to use it to simulate the
    bash.exe impostor, as is done in test_hook_uses_shell_not_from_cwd,
    should not (and does not) produce this warning. If that ever starts
    to do so, then that should be examined as a sign of brittleness.
    EliahKagan committed Jan 9, 2024
    Configuration menu
    Copy the full SHA
    f44524a View commit details
    Browse the repository at this point in the history
  8. Configuration menu
    Copy the full SHA
    15ebb25 View commit details
    Browse the repository at this point in the history
  9. Extract shared logic for using Popen safely on Windows

    This creates git.util.safer_popen that, on non-Windows systems, is
    bound to subprocess.Popen (to avoid introducing unnecessary
    latency). On Windows, it is a function that wraps subprocess.Popen,
    consolidating two pieces of logic that had previously been
    duplicated:
    
    1. Temporarily setting NoDefaultCurrentDirectoryInExePath in the
       calling environment and, when shell=True is used, setting it in
       the subprocess environment as well. This prevents executables
       specified as single names (which are mainly "git" and, for
       hooks, "bash.exe") from being searched for in the current
       working directory of GitPython or, when a shell is used, the
       current working directory of the shell used to run them.
    
    2. Passing the CREATE_NO_WINDOW and CREATE_NEW_PROCESS_GROUP flags
       as creationflags. This is not a security measure. It is
       indirectly related to safety in that CREATE_NO_WINDOW eliminated
       at least some, and possibly all, cases where calling Git.execute
       (directly, or indirectly via a dynamic method) with shell=True
       conferred an advantage over the inherently more secure default
       of shell=False; and CREATE_NEW_PROCESS facilitates some ways of
       terminating subprocesses that would otherwise be unavailable,
       thereby making resource exhaustion less likely. But really the
       reason I included creationflags here is that it seems it should
       always be used in the same situations as preventing the current
       directory from being searched (and always was), and including it
       further reduces code duplication and simplifies calling code.
    
    This commit does not improve security or robustness, because these
    features were already present. Instead, this moves them to a single
    location. It also documents them by giving the function bound to
    safer_popen on Windows, _safer_popen_windows, a detailed docstring.
    
    Because there would otherwise be potential for confusion on the
    different ways to perform or customize path searches, I have also
    added a doctring to py_where noting its limited use case and its
    relationship to shutil.which and non-shell search.
    
    (The search in _safer_popen_windows is typically a non-shell
    search, which is why it cannot be reimplemented to do its own
    lookup by calling an only slightly modified version of
    shutil.which, without a risk of breaking some currently working
    uses. It may, however, be possible to fix the race condition by
    doing something analogous for Windows non-shell search behavior,
    which is largely but not entirely described in the documentation
    for CreateProcessW.)
    EliahKagan committed Jan 9, 2024
    Configuration menu
    Copy the full SHA
    c551e91 View commit details
    Browse the repository at this point in the history
  10. Move safer_popen from git.util to git.cmd

    I had originally written it in git.util because it is used from two
    separate modules (git.cmd and git.index.fun) and is arguably the
    same sort of thing as remove_password_if_present, in that both
    relate to running processes (and to security) and both are used
    from multiple modules yet are not meant for use outside GitPython.
    
    However, all of this is also true of git.cmd.handle_process_output,
    which this really has more in common with: it's a utility related
    *only* to the use of subprocesses, while remove_password_if_present
    can be used for other sanitization. In addition, it also replaces
    git.cmd.PROC_CREATIONFLAGS (also imported in git.index.fun) by
    passing that to Popen on Windows (since the situations where a
    custom value of creationflags should be used are the same as those
    where safer_popen should be called for its primary benefit of
    avoiding an untrusted path search when creating the subprocess).
    
    safer_popen and its Windows implementation _safer_popen_windows are
    thus moved from git/util.py to git/cmd.py, with related changes,
    such as to imports, done everywhere they are needed.
    EliahKagan committed Jan 9, 2024
    Configuration menu
    Copy the full SHA
    3eb7c2a View commit details
    Browse the repository at this point in the history
  11. Configuration menu
    Copy the full SHA
    1f3caa3 View commit details
    Browse the repository at this point in the history