171 lines
6.1 KiB
Diff
171 lines
6.1 KiB
Diff
From 5ef19b63bf709cf39059bf67d97ab1dd22ef4a59 Mon Sep 17 00:00:00 2001
|
|
From: Johannes Schindelin <johannes.schindelin@gmx.de>
|
|
Date: Wed, 23 Nov 2022 09:12:49 +0100
|
|
Subject: [PATCH] windows: ignore empty `PATH` elements
|
|
|
|
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: https://github.com/cygwin/cygwin/commit/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:
|
|
|
|
https://github.com/git-for-windows/MINGW-packages/commit/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 `.`:
|
|
https://github.com/git-lfs/git-lfs/commit/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:
|
|
https://github.com/GitCredentialManager/git-credential-manager/pull/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>
|
|
---
|
|
git-gui/git-gui.sh | 3 +++
|
|
1 file changed, 3 insertions(+)
|
|
|
|
diff --git a/git-gui/git-gui.sh b/git-gui/git-gui.sh
|
|
index 201524c34e..0cf625ca01 100755
|
|
--- a/git-gui/git-gui.sh
|
|
+++ b/git-gui/git-gui.sh
|
|
@@ -464,6 +464,9 @@ proc _which {what args} {
|
|
regsub -all ";" $gitguidir "\\;" gitguidir
|
|
set env(PATH) "$gitguidir;$env(PATH)"
|
|
set _search_path [split $env(PATH) {;}]
|
|
+ # Skip empty `PATH` elements
|
|
+ set _search_path [lsearch -all -inline -not -exact \
|
|
+ $_search_path ""]
|
|
set _search_exe .exe
|
|
} else {
|
|
set _search_path [split $env(PATH) :]
|
|
--
|
|
2.27.0
|
|
|