-
Notifications
You must be signed in to change notification settings - Fork 2.2k
add Inno Setup check of Git install paths and remove old uninstaller checks #4925
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
Merged
Conversation
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
In PR git-lfs#875 in 2016 the Windows installer was changed from NSIS to Inno Setup, and so logic was added in commit 1651e55 to silently try to run the old NSIS git-lfs-uninstaller.exe binary if it existed in the same directory as git.exe, as found using the normal PATH environment variables. As all users of Git LFS should have upgraded to a recent version installed with the Inno Setup installer, we can simply drop this additional logic along with the GetExistingGitInstallation() function of which it was the sole caller. Co-authored-by: Johannes Schindelin <[email protected]>
When installing and uninstalling Git LFS, the Git LFS program configures its "clean" and "smudge" Git filters, and to do this it executes Git so as to change the Git global configuration (or, optionally, the system-wide, local user, or worktree-specific configuration). The Git program executed is the first one found using the PATH environment variable (and, on Windows, the PATHEXT environment variable). Therefore, when 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. For instance, on Linux the "secure_path" configuration value might be set in /etc/sudoers before running the command "sudo git lfs install --system". We can, however, attempt to assist the administrator on Windows where we provide a dedicated installer and also anticipate that Git will be installed under a common set of directories. For that reason we update our Inno Setup installer script so that if it detects that the Git program found with the relevant PATH and PATHEXT environment variables (either the user or system ones, depending on the user's role) is not within either of the "C:\Program Files" or "C:\Program Files (x86)" directories, then a warning is displayed and the user prompted to decide whether to continue. And for convenience, we now report a failure message if no Git program is found, which avoids subsequent errors during the installation or uninstallation steps for any user. Note, though, that if 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. Co-authored-by: Johannes Schindelin <[email protected]>
It would confuse users to see "Do you want to register Git LFS?" when uninstalling. Let's show the message "Do you want to deregister Git LFS?" in that case instead. Signed-off-by: Johannes Schindelin <[email protected]>
When Git cannot be found, we still proceed with installing Git LFS, we're just not registering it with Git via the system config. Let's clarify that. Signed-off-by: Johannes Schindelin <[email protected]>
When the Git executable on the PATH is in an unexpected location, we want to warn the user about that. But we only want to do that once, not four times. Let's cache the result once we have it, and ask only once. Signed-off-by: Johannes Schindelin <[email protected]>
When uninstalling Git LFS on Windows, as of commit cbd52be we now use the term "deregister" in part of the message we display when a Git exectuable is found in an unconventional location. Since we also use the term "registered" in another part of that message, we change it to switch to the term "deregistered" in the uninstallation case.
In commit a5b751f we added a GitFoundInPath() function which we check before executing a full installation; if it returns False, we do not install the Git LFS binary. However, we still install the uninstaller and add the installation path to the PATH and GIT_LFS_PATH environment variables, which leads to a partial and incomplete setup; also, the Inno Setup installer still displays a final dialog box with the message "Setup has finished installing Git LFS on your computer", which is confusing. However, if we call our GitFoundInPath() check function in the InitializeSetup() event function and return its value, then if it returns False the entire setup process is halted and no actual installation occurs at all. This leads to a cleaner and more intuitive experience for users. This also aligns the installation and uninstallation steps, since the uninstaller also checks GitFoundInPath() in its initialization event function and if the return value is False then it terminates immediately without making any changes.
bk2204
approved these changes
Apr 1, 2022
Merged
chrisd8088
added a commit
to chrisd8088/git-lfs
that referenced
this pull request
Mar 25, 2024
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() in the Windows version of the "os/exec" package, including the fact that in both implementations, the contents of the PATHEXT environment variable are converted to lower-case. Both the "os/exec" package version circa 2020, from which our code was derived, and the current "os/exec" implementation perform this conversion: https://github.com/golang/go/blob/7bb721b9384bdd196befeaed593b185f7f2a5589/src/os/exec/lp_windows.go#L64 https://github.com/golang/go/blob/5f5b20c4268c1a3aa6a3b132aeede6dc82adf344/src/os/exec/lp_windows.go#L119 as does our findPathExtensions() function: https://github.com/git-lfs/git-lfs/blob/add64b8f91827db7922c8a4f6574b019b7cedfb8/subprocess/path_windows.go#L79 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.
chrisd8088
added a commit
to chrisd8088/git-lfs
that referenced
this pull request
Mar 25, 2024
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.
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.
When installing and uninstalling Git LFS, the Git LFS program configures its
cleanandsmudgeGit filters, and to do this it executes Git so as to change the Git global configuration (or, optionally, the system-wide, local user, or worktree-specific configuration).The Git program executed is the first one found using the
PATHenvironment variable (and, on Windows, thePATHEXTenvironment variable). Therefore, when installing Git LFS as an administrator with elevated privileges, final responsibility lies with the administrator to ensure there are no compromised executables in their systemPATH. For instance, on Linux thesecure_pathconfiguration value might be set in/etc/sudoersbefore running the commandsudo git lfs install --system.We can, however, attempt to assist the administrator on Windows where we provide a dedicated installer and also anticipate that Git will be installed under a common set of directories.
For that reason we update our Inno Setup installer script so that if it detects that the Git program found with the relevant
PATHandPATHEXTenvironment variables (either the user or system ones, depending on the user's role) is not within either of theC:\Program FilesorC:\Program Files (x86)directories, then a warning is displayed and the user prompted to decide whether to continue.And for convenience, we now report a failure message if no Git program is found, which avoids subsequent errors during the installation or uninstallation steps for any user.
Note, though, that if a Windows administrator runs the
git-lfs.exe installcommand 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 runningsudo git-lfs installwithout confidence that the systemPATHand installed Git binary are already secure.Also, in PR #875 in 2016 the Windows installer was changed from NSIS to Inno Setup, and so logic was added in commit 1651e55 to silently try to run the old NSIS
git-lfs-uninstaller.exebinary if it existed in the same directory asgit.exe, as found using the normalPATHenvironment variables.As all users of Git LFS should have upgraded to a recent version installed with the Inno Setup installer, we can simply drop this additional logic along with the
GetExistingGitInstallation()function of which it was the sole caller./cc @dscho as co-author and Git for Windows maintainer