Skip to content

Conversation

@chrisd8088
Copy link
Member

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.


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.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.

/cc @dscho as co-author and Git for Windows maintainer

chrisd8088 and others added 7 commits March 7, 2022 20:20
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.
@chrisd8088 chrisd8088 requested a review from a team as a code owner March 31, 2022 20:49
@chrisd8088 chrisd8088 changed the title add Inno Setup check of Git install paths and remove old setup checks add Inno Setup check of Git install paths and remove old uninstaller checks Mar 31, 2022
@chrisd8088 chrisd8088 merged commit a52712c into git-lfs:main Apr 1, 2022
@chrisd8088 chrisd8088 deleted the add-win-install-warning branch April 1, 2022 16:14
@chrisd8088 chrisd8088 mentioned this pull request May 24, 2022
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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants