Check the validity of a file #4562
Conversation
There was a problem hiding this comment.
To fill in some context for others here, we received a security report that the security fix in 2.13.3 was incomplete, because if Git was not installed, invoking git-lfs would result in a git.bat or git.exe in the current directory being executed. Through our discussion, I found out that this is because on your system, the PATH variable ends in a semicolon, resulting in us using the current directory there, since an empty component representing the current directory is the standard semantics for PATH.
I will point out that we don't use the standard Go LookPath function because on Windows it prefers to run code out of the current directory. That's a security vulnerability in Go, although the Go team refuses to recognize it as one, and as a result we need to work around it. Unlike on Windows, where different shells result in different path searching behavior, Unix has simple, well-defined semantics here that have been well known and well understood for over 40 years.
One of the things that's known is that putting the current directory in PATH is a bad idea. This leads to a large number of security vulnerabilities, and it's why Go's LookPath function is poorly designed and the developers should not have written it in that way, since they should have reasonably known, due to extensive Unix experience, that that behavior is insecure.
However, for users who do want to have . in their PATH, it's recommended to put it at the end, since a malicious program can only be executed if a legitimate program is absent. In this case, that's what your system has configured. Note that Git LFS won't work at all without Git; it requires Git to do even the most basic functionality and it's completely useless without it. As a result, the fact that your system is misconfigured in this way, even if that's the default behavior for Windows, should not be a problem to the average user. Note that Windows has other defaults where it reads out of the current directory (e.g., in CreateProcessW), and those are also insecure (and at the time they were written, that should have been known to Microsoft due to the long history of Unix), so it isn't surprising to me that Windows has some insecure behavior in some cases.
Also, it should be noted that this problem doesn't occur in Git Bash or Git CMD, since those tools intentionally strip . and empty components out of the PATH, and as a result, if a user is using Git LFS in the normal way through one of these shells, then this should not end up being a problem, either.
More specifically to this patch, it lacks tests, and we'd need those in order to accept it. However, I think the thing you'll find in this case is that it's hard to test, since this functionality intrinsically makes Git LFS interactive, even in cases where it needs to be non-interactive, such as when it's invoked in a CI tool or from under Git. Especially in the case of git lfs filter-process, standard input is not always connected to the terminal, and if this code is ever triggered, it will break that tool.
As a result, I'm marking this as Changes Requested, because I don't think this approach is going to work as it stands, nor do I think that the problem it's trying to solve is actually a security vulnerability or a problem in normal usage.
A possibility I would be willing to entertain (and this isn't a commitment to accept such a patch) is to not read empty components as the current directory on Windows (and on Windows only), and just to skip them. This would improve security and users who had a practical problem with this could use WSL, where the semantics are clearer and better defined, or simply write . explicitly.
|
I see. Now I filter the current directory on Windows and pass all of the tests. |
| @@ -52,6 +53,10 @@ func LookPath(file string) (string, error) { | |||
| path := os.Getenv("PATH") | |||
| for _, dir := range filepath.SplitList(path) { | |||
| if dir == "" { | |||
| // Avoid using current directory on Windows | |||
| if runtime.GOOS == "windows" && file == "git" { | |||
There was a problem hiding this comment.
I don't think we want to restrict this to just git; we'd instead want to make it more generally applicable, so different binaries don't have different behavior.
And I think we'd also want some tests for this behavior, probably in t/t-path.sh.
|
I'm going to close this since a similar fix has been applied in #4603. |
Before executing a file, the program would use LookPath function.
However, some versions of Windows(Windows Server 2019, Windows10 1709, ...) have a semicolon in the PATH environment variable at last in default, which means that current directory is also in environment.
The program executing git, and if a malware git.bat/git.exe in the current directory while git isn't installed, the program will executing it, so I add the following code to make sure that users know they are going to execute a suspicious file.