Backport Windows security fix #85

dscho · 2023-01-19T12:14:08Z

This change went out with Git for Windows v2.39.1 and is tracked in this security advisory under the label "CVE-2022-41953".

When looking up an executable via the `_which` function, Git GUI imitates the `execlp()` strategy where the environment variable `PATH` is interpreted as a list of paths in which to search. For historical reasons, stemming from the olden times when it was uncommon to download a lot of files from the internet into the current directory, empty elements in this list are treated as if the current directory had been specified. Nowadays, of course, this treatment is highly dangerous as the current directory often contains files that have just been downloaded and not yet been inspected by the user. Unix/Linux users are essentially expected to be very, very careful to simply not add empty `PATH` elements, i.e. not to make use of that feature. On Windows, however, it is quite common for `PATH` to contain empty elements by mistake, e.g. as an unintended left-over entry when an application was installed from the Windows Store and then uninstalled manually. While it would probably make most sense to safe-guard not only Windows users, it seems to be common practice to ignore these empty `PATH` elements _only_ on Windows, but not on other platforms. Sadly, this practice is followed inconsistently between different software projects, where projects with few, if any, Windows-based contributors tend to be less consistent or even "blissful" about it. Here is a non-exhaustive list: Cygwin: It specifically "eats" empty paths when converting path lists to POSIX: cygwin/cygwin@753702223c7d I.e. it follows the common practice. PowerShell: It specifically ignores empty paths when searching the `PATH`. The reason for this is apparently so self-evident that it is not even mentioned here: https://learn.microsoft.com/en-us/powershell/module/microsoft.powershell.core/about/about_environment_variables#path-information I.e. it follows the common practice. CMD: Oh my, CMD. Let's just forget about it, nobody in their right (security) mind takes CMD as inspiration. It is so unsafe by default that we even planned on dropping `Git CMD` from Git for Windows altogether, and only walked back on that plan when we found a super ugly hack, just to keep Git's users secure by default: git-for-windows/MINGW-packages@82172388bb51 So CMD chooses to hide behind the battle cry "Works as Designed!" that all too often leaves users vulnerable. CMD is probably the most prominent project whose lead you want to avoid following in matters of security. Win32 API (`CreateProcess()`) Just like CMD, `CreateProcess()` adheres to the original design of the path lookup in the name of backward compatibility (see https://learn.microsoft.com/en-us/windows/win32/api/processthreadsapi/nf-processthreadsapi-createprocessw for details): If the file name does not contain a directory path, the system searches for the executable file in the following sequence: 1. The directory from which the application loaded. 2. The current directory for the parent process. [...] I.e. the Win32 API itself chooses backwards compatibility over users' safety. Git LFS: There have been not one, not two, but three security advisories about Git LFS executing executables from the current directory by mistake. As part of one of them, a change was introduced to stop treating empty `PATH` elements as equivalent to `.`: git-lfs/git-lfs@7cd7bb0a1f0d I.e. it follows the common practice. Go: Go does not follow the common practice, and you can think about that what you want: https://github.com/golang/go/blob/go1.19.3/src/os/exec/lp_windows.go#L114-L135 https://github.com/golang/go/blob/go1.19.3/src/path/filepath/path_windows.go#L108-L137 Git Credential Manager: It tries to imitate Git LFS, but unfortunately misses the empty `PATH` element handling. As of time of writing, this is in the process of being fixed: git-ecosystem/git-credential-manager#968 So now that we have established that it is a common practice to ignore empty `PATH` elements on Windows, let's assess this commit's change using Schneier's Five-Step Process (https://www.schneier.com/crypto-gram/archives/2002/0415.html#1): Step 1: What problem does it solve? It prevents an entire class of Remote Code Execution exploits via Git GUI's `Clone` functionality. Step 2: How well does it solve that problem? Very well. It prevents the attack vector of luring an unsuspecting victim into cloning an executable into the worktree root directory that Git GUI immediately executes. Step 3: What other security problems does it cause? Maybe non-security problems: If a project (ab-)uses the unsafe `PATH` lookup. That would not only be unsafe, though, but fragile in the first place because it would break when running in a subdirectory. Therefore I would consider this a scenario not worth keeping working. Step 4: What are the costs of this measure? Almost nil, except for the time writing up this commit message ;-) Step 5: Given the answers to steps two through four, is the security measure worth the costs? Yes. Keeping Git's users Secure By Default is worth it. It's a tiny price to pay compared to the damages even a single successful exploit can cost. So let's follow that common practice in Git GUI, too. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>

The `is_Cygwin` function is used, among other things, to determine how executables are discovered in the `PATH` list by the `_which` function. We are about to change the behavior of the `_which` function on Windows (but not Cygwin): On Windows, we want it to ignore empty elements of the `PATH` instead of treating them as referring to the current directory (which is a "legacy feature" according to https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap08.html#tag_08_03, but apparently not explicitly deprecated, the POSIX documentation is quite unclear on that even if the Cygwin project itself considers it to be deprecated: cygwin/cygwin@fc74dbf22f5c). This is important because on Windows, `exec` does something very unsafe by default (unless we're running a Cygwin version of Tcl, which follows Unix semantics). However, we try to `exec` something _inside_ `is_Cygwin` to determine whether we're running within Cygwin or not, i.e. before we determined whether we need to handle `PATH` specially or not. That's a Catch-22. Therefore, and because it is much cleaner anyway, use the `$::tcl_platform(os)` value which is guaranteed to start with `CYGWIN_` when running a Cygwin variant of Tcl/Tk, instead of executing `cygpath --windir`. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>

We need these in `_which` and they should be defined before that function's definition. This commit is best viewed with `--color-moved`. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>

We are about to make use of the `_which` function to address CVE-2022-41953 by overriding Tcl/Tk's unsafe PATH lookup on Windows. In preparation for that, let's move it close to the top of the file to make sure that even early `exec` calls that happen during the start-up of Git GUI benefit from the fix. This commit is best viewed with `--color-moved`. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>

As per https://www.tcl.tk/man/tcl8.6/TclCmd/exec.html#M23, Tcl's `exec` function goes out of its way to imitate the highly dangerous path lookup of `cmd.exe`, but _of course_ only on Windows: If a directory name was not specified as part of the application name, the following directories are automatically searched in order when attempting to locate the application: The directory from which the Tcl executable was loaded. The current directory. The Windows 32-bit system directory. The Windows home directory. The directories listed in the path. The dangerous part is the second item, of course: `exec` _prefers_ executables in the current directory to those that are actually in the `PATH`. It is almost as if people wanted to Windows users vulnerable, specifically. To avoid that, Git GUI already has the `_which` function that does not imitate that dangerous practice when looking up executables in the search path. However, Git GUI currently fails to use that function e.g. when trying to execute `aspell` for spell checking. That is not only dangerous but combined with Tcl's unfortunate default behavior and with the fact that Git GUI tries to spell-check a repository just after cloning, leads to a critical Remote Code Execution vulnerability. Let's override both `exec` and `open` to always use `_which` instead of letting Tcl perform the path lookup, to prevent this attack vector. This addresses CVE-2022-41953. For more details, see GHSA-v4px-mx59-w99c Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>

This topic branch fixes a vulnerability in Git GUI's "clone" feature (tracked as CVE-2022-41953) that was graded with a CVSS Score 8.6/10 (high). These patches were backported to Git GUI in prati0100/git-gui#85 Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>

prati0100 · 2023-01-24T13:19:36Z

Applied. Thanks for working on this.

This topic branch fixes a vulnerability in Git GUI's "clone" feature (tracked as CVE-2022-41953) that was graded with a CVSS Score 8.6/10 (high). These patches were backported to Git GUI in prati0100/git-gui#85 Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>

dscho added 5 commits December 16, 2022 20:06

Move is_<platform> functions to the beginning

db41105

We need these in `_which` and they should be defined before that function's definition. This commit is best viewed with `--color-moved`. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>

prati0100 closed this Jan 24, 2023

dscho deleted the fix-win-rce branch January 24, 2023 14:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Backport Windows security fix #85

Backport Windows security fix #85

dscho commented Jan 19, 2023

prati0100 commented Jan 24, 2023

Backport Windows security fix #85

Backport Windows security fix #85

Conversation

dscho commented Jan 19, 2023

prati0100 commented Jan 24, 2023