Skip to content

Conversation

@chrisd8088
Copy link
Member

@chrisd8088 chrisd8088 commented Mar 25, 2024

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.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 our implementation converts the contents of the PATHEXT environment variable to lower-case in the same manner the older Go implementation. The current os/exec implementation 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 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 #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.

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.
@chrisd8088 chrisd8088 marked this pull request as ready for review March 25, 2024 22:30
@chrisd8088 chrisd8088 requested a review from a team as a code owner March 25, 2024 22:30
Copy link
Contributor

@dscho dscho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!

@chrisd8088 chrisd8088 merged commit c4dc541 into git-lfs:main Mar 26, 2024
@chrisd8088 chrisd8088 deleted the windows-install-lower-file-ext branch March 26, 2024 16:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants