Use lower-case file extensions in Windows installer path checks#5688
Merged
chrisd8088 merged 1 commit intogit-lfs:mainfrom Mar 26, 2024
Merged
Conversation
In commit a5b751f of PR git-lfs#4925 we introduced an advisory check into the Windows installer for Git LFS which notifies the administrator if a "git.exe" file is found outside of the "C:\Program Files" directory and would, according to the order of elements in the PATH and PATHEXT environment variables, be found and run by Git LFS during the installation process. Git LFS searches for executables using logic imported and modified from the Go standard libraries into our "subprocess" package, because until Go v1.19, the "os/exec" package would search first in the current working directory, which allowed was a security vulnerability. Hence Git LFS explicitly excludes the current working directory when searching for executables. In the Windows version of our "subprocess" package, where we implement this file search, we use an internal findPathExtensions() function which mirrors part of the LookPath() function in the Windows version of the "os/exec" package circa 2020, including the fact that in both implementations, the contents of the PATHEXT environment variable are converted to lower-case: https://github.com/git-lfs/git-lfs/blob/add64b8f91827db7922c8a4f6574b019b7cedfb8/subprocess/path_windows.go#L79 https://github.com/golang/go/blob/7bb721b9384bdd196befeaed593b185f7f2a5589/src/os/exec/lp_windows.go#L64 The current "os/exec" implementation also performs this conversion: https://github.com/golang/go/blob/4c2b1e0feb3d3112da94fa4cd11ebe995003fa89/src/os/exec/lp_windows.go#L119 So long as Windows directories are case-insensitive, this conversion has no particular consequences. However, modern versions of Windows allow for some directories to be made case-sensitive in their handling of filenames, not just case-preserving. In such a directory, Git LFS will only locate a file to execute if it has an all-lowercase extension such as ".exe". However, in the advisory check we perform in our Windows installer, we utilize the file extensions as they are found in the PATH environment variable, which by default contains a list of all-uppercase extensions, e.g., ".COM;.EXE;.BAT;...". Thus if a case-sensitive directory precedes any "C:\Program Files" directory in the PATH environment variable and also contains a file named "git.exe", Git LFS will find and run that file, but the Windows installer will not advise that such a file exists outside of "C:\Program Files". To bring the installer into closer alignment with the behaviour of Git LFS, we therefore want to convert the contents of the PATHEXT environment variable to all-lowercase before we use them to search for a potential Git executable. To reiterate the points we made in PR git-lfs#4925 when we introduced the advisory check into the Windows installer, it is not intended to provide a guarantee of system safety. As we wrote at the time: [W]hen installing Git LFS as an administrator with elevated privileges, final responsibility lies with the administrator to ensure there are no compromised executables in their system PATH. [I]f a Windows administrator runs the "git-lfs.exe install" command manually, the checks we are adding to the Inno Setup script will not be performed, and the situation then is no different than a macOS or Linux user running "sudo git-lfs install" without confidence that the system PATH and installed Git binary are already secure.
enderwert
approved these changes
Mar 25, 2024
bk2204
approved these changes
Mar 26, 2024
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
In commit a5b751f of PR #4925 we introduced an advisory check into the Windows installer for Git LFS which notifies the administrator if a
git.exefile is found outside of theC:\Program Filesdirectory and would, according to the order of elements in thePATHandPATHEXTenvironment variables, be found and run by Git LFS during the installation process.Git LFS searches for executables using logic imported and modified from the Go standard libraries into our
subprocesspackage, because until Go v1.19, theos/execpackage would search first in the current working directory, which allowed was a security vulnerability. Hence Git LFS explicitly excludes the current working directory when searching for executables.In the Windows version of our
subprocesspackage, where we implement this file search, we use an internalfindPathExtensions()function which mirrors part of theLookPath()function in the Windows version of theos/execpackage circa 2020, including the fact that our implementation converts the contents of thePATHEXTenvironment variable to lower-case in the same manner the older Go implementation. The currentos/execimplementation also performs this conversion.So long as Windows directories are case-insensitive, this conversion has no particular consequences. However, modern versions of Windows allow for some directories to be made case-sensitive in their handling of filenames, not just case-preserving. In such a directory, Git LFS will only locate a file to execute if it has an all-lowercase extension such as
.exe.However, in the advisory check we perform in our Windows installer, we utilize the file extensions as they are found in the PATH environment variable, which by default contains a list of all-uppercase extensions, e.g.,
.COM;.EXE;.BAT;.... Thus if a case-sensitive directory precedes anyC:\Program Filesdirectory in thePATHenvironment variable and also contains a file namedgit.exe, Git LFS will find and run that file, but the Windows installer will not advise that such a file exists outside ofC:\Program Files.To bring the installer into closer alignment with the behaviour of Git LFS, we therefore want to convert the contents of the
PATHEXTenvironment variable to all-lowercase before we use them to search for a potential Git executable.To reiterate the points we made in PR #4925 when we introduced the advisory check into the Windows installer, it is not intended to provide a guarantee of system safety. As we wrote at the time: