Skip to content

Conversation

@gergelyfabian
Copy link
Contributor

No description provided.

@gergelyfabian gergelyfabian requested a review from a team as a code owner February 27, 2025 07:55
@chrisd8088
Copy link
Member

Hey, thanks for the PR and for jumping on this issue. Dependabot should offer PRs for these kinds of issues too, but I think you've pre-empted it! Fortunately, as with #5941 this is another case where the vulnerability doesn't affect the Git LFS client.

This new vulnerability, addressed by the update to the x/crypto Go module in v0.35.0 and reported as CVE-2025-22869, only pertains to the x/crypto/ssh package, which the Git LFS client does not use.

The Git LFS client never acts as an SSH server, so there is no immediate necessity for us to upgrade the module and release a new version of Git LFS.

While some of the Go packages we use indirectly import packages from the x/crypto module, none of them are the x/crypto/ssh package. We can confirm this is the case by running go list -json all | grep x/crypto/ssh, which returns no output.

Nevertheless, we might as well upgrade the module, so that when we do release v3.7.0 of the Git LFS client, security scanners will not generate false positive reports about the version of the x/crypto module we use.

For reference, the details of the vulnerability and its mitigation are described in GO-2025-3487 and also in the release announcements for version 0.35.0 of the x/crypto module.


As for when we'll merge this PR, I think we'll probably wait a few weeks, since it's not urgent and because we'll need to upgrade our CI and Docker builds to Go 1.24 first, and that in turn is likely to expose some other issues which we should address first (see #5968). I have a set of changes in progress to resolve those, but until the other member of our core team is available to review them, they'll have to wait.

@chrisd8088 chrisd8088 added the go Pull requests that update Go code label Feb 28, 2025
@chrisd8088 chrisd8088 merged commit b52f295 into git-lfs:main Mar 21, 2025
10 checks passed
@chrisd8088 chrisd8088 added the dependencies Pull requests that update a dependency file label Mar 21, 2025
chrisd8088 added a commit to chrisd8088/git-lfs that referenced this pull request Jun 19, 2025
When we build Debian and RPM Linux packages, we define the minimum
versions of Git and Go required by the Git LFS client.  However,
the minimum versions we specify are at present somewhat out of date.

Specifically, both the "control" file for our Debian packages and the
SPEC file for our RPM packages state that we require at least Git
version 1.8.2, and the former also specifies that we require at least
Go version 1.12.0.

In practice, though, since we introduced the "git lfs migrate" command
in PR git-lfs#2353, Git v2.0.0 has been the earliest version of Git we support,
as per commit 5aea841 of that PR.

We have also required at least Go v1.23.0 to build the Git LFS client
since commit 70e23fa of PR git-lfs#5997,
when we updated the minimum version of the x/crypto Go module specified
in our "go.mod" file and the "go mod tidy" command then also updated
the minimum required version of Go to 1.23.0.

Because we anticipate making a v3.7.0 release of the Git LFS client in
the near future, we now update the "control" file for our Debian
packages and the SPEC file for our RPM packages to indicate that
the Git LFS client requires at least Git v2.0.0 and Go v1.23.0.
chrisd8088 added a commit to chrisd8088/git-lfs that referenced this pull request Nov 3, 2025
In commit 8607cd4 of PR git-lfs#3780 we
revised the SmudgeToFile() method of the GitFilter structure in our
"lfs" package to construct an absolute path from its "filename"
parameter and use this path when creating or truncating the file
it creates and populates with the content of a Git LFS object.

We made this change with the goal of resolving the problem reported
in git-lfs#3772 regarding Windows systems where the maximum length of a
file path is often restricted to the MAX_PATH limit of 260 characters.
Beginning with certain versions of Windows 10, system administrators
may now change the maximum file path length to a significantly higher
value, but this change only takes effect for applications which
opt into the new setting.  For reference, see:

  https://learn.microsoft.com/en-us/windows/win32/fileio/maximum-file-path-limitation#enable-long-paths-in-windows-10-version-1607-and-later

Since the inclusion of Change List (CL) 32451 in Go version 1.8, many of
the functions and methods in the "os" package of the Go standard library
have supported the use of absolute paths longer than the MAX_PATH limit
on Windows by internally converting such paths into "root local" DOS
device paths before passing them to Windows API system calls.  These
types of DOS device paths begin with a "\\?\" prefix, and are the only
file path format which Windows guarantees will always be accepted even
if they exceed the MAX_PATH length limit:

  https://go.dev/cl/32451
  https://learn.microsoft.com/en-us/windows/win32/fileio/maximum-file-path-limitation
  https://learn.microsoft.com/en-us/windows/win32/fileio/naming-a-file#win32-file-namespaces

At the time of our change in PR git-lfs#3780, we were using Go v1.11.1 to build
the Git LFS client.  By changing our SmudgeToFile() method to construct
an absolute path with the Abs() function from the "path/filepath" package
of the Go standard library, we were able to take advantage of the fact
that when we passed the resultant path to the Create() function of the
"os" package, it would translate that path into one beginning with "\\?\"
and thus the final file path could exceed the MAX_PATH length limit.

Note that the Abs() function itself does not convert paths into the
"root local" DOS device format, because it is implemented as a wrapper
around a system call to the GetFullPathName() Windows API function.
That function merges the full path to the current working directory,
which may be a conventional DOS path beginning with a disk designator
like "C:\", to a relative path, and returns the result without converting
it into a DOS device path:

  https://github.com/golang/go/blob/go1.25.3/src/path/filepath/path.go#L162
  https://github.com/golang/go/blob/go1.25.3/src/path/filepath/path_windows.go#L62
  https://github.com/golang/go/blob/go1.25.3/src/syscall/exec_windows.go#L162
  https://learn.microsoft.com/en-ca/windows/win32/api/fileapi/nf-fileapi-getfullpathnamea

However, because our SmudgeToFile() method passed the absolute path
returned by the Abs() function to the Create() function of the "os"
package, our change in PR git-lfs#3780 was effective in allowing file paths
longer than the MAX_PATH limit to be created on Windows, at least in
some cases.  The Create() function, like others in the "os" package,
invokes a chain of internal functions which ultimately pass the input
path to the fixLongPath() function, and that function converts any
absolute path into one beginning with "\\?\":

  https://github.com/golang/go/blob/go1.11.1/src/os/file.go#L274
  https://github.com/golang/go/blob/go1.11.1/src/os/file.go#L284
  https://github.com/golang/go/blob/go1.11.1/src/os/file_windows.go#L158
  https://github.com/golang/go/blob/go1.11.1/src/os/file_windows.go#L97
  https://github.com/golang/go/blob/go1.11.1/src/os/path_windows.go#L131-L209

While the changes in PR git-lfs#3780 resolved some instances where a long
relative path caused errors on Windows when running "git lfs checkout"
or "git lfs pull" commands, they were likely insufficient to resolve
all such cases.

Prior to commit 0cffe93 of our "main"
development branch and commit 2902e9d
of our "release-3.7" branch, the SmudgeToFile() method continued to pass
relative paths to the MkdirAll() function in our "tools" package, which
in turn calls the MkdirAll() function in the "os" package of the standard
Go library.  If the relative path provided to the SmudgeToFile() method
was longer than the MAX_PATH limit even with the trailing file name
removed, and some of the directories named in that path did not exist,
then the MkdirAll() method would have failed, at least with versions of
Go prior to v1.23.0.

Fortunately, ever since commit 70e23fa
of PR git-lfs#5997, when we upgraded the version of Go we use to build the
Git LFS client to Go v1.23.0, this problem and any similar issues should
now be effectively eliminated.  Go v1.23.0 includes the revisions to its
internal fixLongPath() function from CLs (Change Lists) 570995 and 574695,
particularly commits golang/go@2e8d84f
and golang/go@7c89ad6, which ensure
that the function now correctly handles both UNC paths and, most
importantly for our purposes here, relative paths:

  https://go.dev/cl/570995
  https://go.dev/cl/574695
  https://github.com/golang/go/blob/go1.23.0/src/os/path_windows.go#L41-L152

These improvements in the Go standard library therefore apply to our
use of the Remove() and OpenFile() functions from the "os" package in
our SmudgeToFile() method.  Further, they also apply to our use of the
"os" package's Mkdir() function to create any missing directories between
the root of the current Git working tree and the file path provided to
the SmudgeToFile() method.  As noted above, we no longer call the
MkdirAll() function since commits 0cffe93
and 2902e9d.  Instead, the walk() method
of the new DirWalker structure in our "tools" package invokes the Mkdir()
function in the same package for each missing directory, and this
helper function then calls the "os" package's Mkdir() function.

As a consequence, we can simply remove the call to the Abs() function
from our SmudgeToFile() method and use the relative path provided in
its "filename" parameter without any adjustment.
chrisd8088 added a commit to chrisd8088/git-lfs that referenced this pull request Nov 11, 2025
In commit 8607cd4 of PR git-lfs#3780 we
revised the SmudgeToFile() method of the GitFilter structure in our
"lfs" package to construct an absolute path from its "filename"
parameter and use this path when creating or truncating the file
it creates and populates with the content of a Git LFS object.

We made this change with the goal of resolving the problem reported
in git-lfs#3772 regarding Windows systems where the maximum length of a
file path is often restricted to the MAX_PATH limit of 260 characters.
Beginning with certain versions of Windows 10, system administrators
may now change the maximum file path length to a significantly higher
value, but this change only takes effect for applications which
opt into the new setting.  For reference, see:

  https://learn.microsoft.com/en-us/windows/win32/fileio/maximum-file-path-limitation#enable-long-paths-in-windows-10-version-1607-and-later

Since the inclusion of Change List (CL) 32451 in Go version 1.8, many of
the functions and methods in the "os" package of the Go standard library
have supported the use of absolute paths longer than the MAX_PATH limit
on Windows by internally converting such paths into "root local" DOS
device paths before passing them to Windows API system calls.  These
types of DOS device paths begin with a "\\?\" prefix, and are the only
file path format which Windows guarantees will always be accepted even
if they exceed the MAX_PATH length limit:

  https://go.dev/cl/32451
  https://learn.microsoft.com/en-us/windows/win32/fileio/maximum-file-path-limitation
  https://learn.microsoft.com/en-us/windows/win32/fileio/naming-a-file#win32-file-namespaces

At the time of our change in PR git-lfs#3780, we were using Go v1.11.1 to build
the Git LFS client.  By changing our SmudgeToFile() method to construct
an absolute path with the Abs() function from the "path/filepath" package
of the Go standard library, we were able to take advantage of the fact
that when we passed the resultant path to the Create() function of the
"os" package, it would translate that path into one beginning with "\\?\"
and thus the final file path could exceed the MAX_PATH length limit.

Note that the Abs() function itself does not convert paths into the
"root local" DOS device format, because it is implemented as a wrapper
around a system call to the GetFullPathName() Windows API function.
That function merges the full path to the current working directory,
which may be a conventional DOS path beginning with a disk designator
like "C:\", to a relative path, and returns the result without converting
it into a DOS device path:

  https://github.com/golang/go/blob/go1.25.3/src/path/filepath/path.go#L162
  https://github.com/golang/go/blob/go1.25.3/src/path/filepath/path_windows.go#L62
  https://github.com/golang/go/blob/go1.25.3/src/syscall/exec_windows.go#L162
  https://learn.microsoft.com/en-ca/windows/win32/api/fileapi/nf-fileapi-getfullpathnamea

However, because our SmudgeToFile() method passed the absolute path
returned by the Abs() function to the Create() function of the "os"
package, our change in PR git-lfs#3780 was effective in allowing file paths
longer than the MAX_PATH limit to be created on Windows, at least in
some cases.  The Create() function, like others in the "os" package,
invokes a chain of internal functions which ultimately pass the input
path to the fixLongPath() function, and that function converts any
absolute path into one beginning with "\\?\":

  https://github.com/golang/go/blob/go1.11.1/src/os/file.go#L274
  https://github.com/golang/go/blob/go1.11.1/src/os/file.go#L284
  https://github.com/golang/go/blob/go1.11.1/src/os/file_windows.go#L158
  https://github.com/golang/go/blob/go1.11.1/src/os/file_windows.go#L97
  https://github.com/golang/go/blob/go1.11.1/src/os/path_windows.go#L131-L209

While the changes in PR git-lfs#3780 resolved some instances where a long
relative path caused errors on Windows when running "git lfs checkout"
or "git lfs pull" commands, they were likely insufficient to resolve
all such cases.

Prior to commit 0cffe93 of our "main"
development branch and commit 2902e9d
of our "release-3.7" branch, the SmudgeToFile() method continued to pass
relative paths to the MkdirAll() function in our "tools" package, which
in turn calls the MkdirAll() function in the "os" package of the standard
Go library.  If the relative path provided to the SmudgeToFile() method
was longer than the MAX_PATH limit even with the trailing file name
removed, and some of the directories named in that path did not exist,
then the MkdirAll() method would have failed, at least with versions of
Go prior to v1.23.0.

Fortunately, ever since commit 70e23fa
of PR git-lfs#5997, when we upgraded the version of Go we use to build the
Git LFS client to Go v1.23.0, this problem and any similar issues should
now be effectively eliminated.  Go v1.23.0 includes the revisions to its
internal fixLongPath() function from CLs (Change Lists) 570995 and 574695,
particularly commits golang/go@2e8d84f
and golang/go@7c89ad6, which ensure
that the function now correctly handles both UNC paths and, most
importantly for our purposes here, relative paths:

  https://go.dev/cl/570995
  https://go.dev/cl/574695
  https://github.com/golang/go/blob/go1.23.0/src/os/path_windows.go#L41-L152

These improvements in the Go standard library therefore apply to our
use of the Remove() and OpenFile() functions from the "os" package in
our SmudgeToFile() method.  Further, they also apply to our use of the
"os" package's Mkdir() function to create any missing directories between
the root of the current Git working tree and the file path provided to
the SmudgeToFile() method.  As noted above, we no longer call the
MkdirAll() function since commits 0cffe93
and 2902e9d.  Instead, the walk() method
of the new DirWalker structure in our "tools" package invokes the Mkdir()
function in the same package for each missing directory, and this
helper function then calls the "os" package's Mkdir() function.

As a consequence, we can simply remove the call to the Abs() function
from our SmudgeToFile() method and use the relative path provided in
its "filename" parameter without any adjustment.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

dependencies Pull requests that update a dependency file go Pull requests that update Go code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants