Ignore submodules whose name or path in .gitmodules leads outside the repository (#6187)

If `.gitmodules` contains a submodule name like `../../../victim`,
removing that submodule in lazygit deletes the directory `victim` next
to the repository, along with everything in it. With enough `../`
components the path becomes `/`, and lazygit then deletes all files that
the user has permission to delete. An attacker only needs to commit such
a `.gitmodules` file to a repository. A user of that repository then
only has to remove the submodule in lazygit and confirm the prompt.

This happens because lazygit parses `.gitmodules` itself and uses the
names and paths in it without checking them. It deletes a submodule's
git dir with `os.RemoveAll`, joining `.git/modules` with the name, and
`filepath.Join` resolves the `..` components. Paths have a similar
problem. Lazygit runs some commands in the submodule's directory, so
with `path = ../other_repo` (or a symlink that points there), nuking the
working tree stashes the changes and untracked files of that other
repository. Git itself ignores such names (it prints "ignoring
suspicious submodule name"), and it only uses a path from `.gitmodules`
if its index has a submodule at that path.

This PR:
- Ignores submodules whose name fails git's own check in
`check_submodule_name()`: an empty name, or `..` as a component, with
both `/` and `\` counting as separators on every platform.
- Ignores submodules whose path isn't a directory inside the repository,
or whose path goes through a symlink.
- Refuses to remove a submodule whose git dir isn't below
`.git/modules`, before running any git command. This also catches the
name `.`, which git accepts. Removing a submodule with that name used to
delete the git dirs of all submodules.
- Anchors the regexes that parse `.gitmodules` at the start of a line.
This is unrelated to the security problem. A url containing `path=` was
taken for the submodule's path, and a commented-out section showed up as
a submodule.

Ignored entries no longer appear in the submodules list. Git doesn't
treat them as submodules either.

Fixes #6185.

pkg/commands/git_commands/submodule.go

@@ -2,6 +2,8 @@ package git_commands
 
 import (
 	"bufio"
+	"fmt"
+	"io"
 	"os"
 	"path/filepath"
 	"regexp"
@@ -46,7 +48,36 @@ func (self *SubmoduleCommands) GetConfigs(parentModule *models.SubmoduleConfig)
 	}
 	defer file.Close()
 
-	scanner := bufio.NewScanner(file)
+	parsedConfigs, err := parseGitmodules(file, parentModule)
+	if err != nil {
+		return nil, err
+	}
+
+	configs := []*models.SubmoduleConfig{}
+	for _, config := range parsedConfigs {
+		if isSuspiciousSubmoduleName(config.Name) {
+			self.Log.Warnf("Ignoring suspicious submodule name: %s", config.Name)
+			continue
+		}
+
+		if isSuspiciousSubmodulePath(dir, config.Path) {
+			self.Log.Warnf("Ignoring suspicious submodule path: %s", config.Path)
+			continue
+		}
+
+		configs = append(configs, config)
+
+		nestedConfigs, err := self.GetConfigs(config)
+		if err == nil {
+			configs = append(configs, nestedConfigs...)
+		}
+	}
+
+	return configs, nil
+}
+
+func parseGitmodules(reader io.Reader, parentModule *models.SubmoduleConfig) ([]*models.SubmoduleConfig, error) {
+	scanner := bufio.NewScanner(reader)
 	scanner.Split(bufio.ScanLines)
 
 	firstMatch := func(str string, regex string) (string, bool) {
@@ -60,27 +91,22 @@ func (self *SubmoduleCommands) GetConfigs(parentModule *models.SubmoduleConfig)
 	}
 
 	configs := []*models.SubmoduleConfig{}
-	lastConfigIdx := -1
 	for scanner.Scan() {
 		line := scanner.Text()
 
-		if name, ok := firstMatch(line, `\[submodule "(.*)"\]`); ok {
+		if name, ok := firstMatch(line, `^\s*\[submodule "(.*)"\]`); ok {
 			configs = append(configs, &models.SubmoduleConfig{
 				Name: name, ParentModule: parentModule,
 			})
-			lastConfigIdx = len(configs) - 1
 			continue
 		}
 
-		if lastConfigIdx != -1 {
-			if path, ok := firstMatch(line, `\s*path\s*=\s*(.*)\s*`); ok {
-				configs[lastConfigIdx].Path = path
-				nestedConfigs, err := self.GetConfigs(configs[lastConfigIdx])
-				if err == nil {
-					configs = append(configs, nestedConfigs...)
-				}
-			} else if url, ok := firstMatch(line, `\s*url\s*=\s*(.*)\s*`); ok {
-				configs[lastConfigIdx].Url = url
+		if len(configs) > 0 {
+			config := configs[len(configs)-1]
+			if path, ok := firstMatch(line, `^\s*path\s*=\s*(.*)\s*`); ok {
+				config.Path = path
+			} else if url, ok := firstMatch(line, `^\s*url\s*=\s*(.*)\s*`); ok {
+				config.Url = url
 			}
 		}
 	}
@@ -92,6 +118,52 @@ func (self *SubmoduleCommands) GetConfigs(parentModule *models.SubmoduleConfig)
 	return configs, nil
 }
 
+// isSuspiciousSubmoduleName applies the same rule as git's
+// check_submodule_name(); git ignores a submodule whose name fails it. The
+// git dir of a submodule is .git/modules/<name>, so a ".." component in the
+// name could put it outside the repository. Like git, we treat both slashes
+// and backslashes as separators on every platform.
+func isSuspiciousSubmoduleName(name string) bool {
+	if name == "" {
+		return true
+	}
+
+	components := strings.FieldsFunc(name, func(r rune) bool {
+		return r == '/' || r == '\\'
+	})
+	return lo.Contains(components, "..")
+}
+
+// isSuspiciousSubmodulePath reports whether a submodule path from the
+// .gitmodules file in parentDir might lead somewhere other than a directory
+// below parentDir. We run commands in the submodule's directory and read the
+// .gitmodules file in it, so the path must not lead anywhere else.
+//
+// Git never checks out a file through a symlink, so a path that goes through
+// a symlink can't be a submodule in the first place. We reject such paths
+// even if the symlink points to a directory below parentDir.
+func isSuspiciousSubmodulePath(parentDir string, path string) bool {
+	if !filepath.IsLocal(path) || filepath.Clean(path) == "." {
+		return true
+	}
+
+	dir := parentDir
+	for _, component := range strings.Split(filepath.Clean(path), string(filepath.Separator)) {
+		dir = filepath.Join(dir, component)
+		fileInfo, err := os.Lstat(dir)
+		if os.IsNotExist(err) {
+			// Nothing below this point exists (e.g. because the submodule
+			// isn't checked out), so it can't lead anywhere else either
+			return false
+		}
+		if err != nil || !fileInfo.IsDir() {
+			return true
+		}
+	}
+
+	return false
+}
+
 // AnyHaveStageableChanges reports whether any of the given submodule paths has
 // a checked-out commit that differs from the one recorded in the
 // superproject's index, i.e. a change that `git add <path>` would actually
@@ -237,6 +309,18 @@ func (self *SubmoduleCommands) runInParentModule(submodule *models.SubmoduleConf
 func (self *SubmoduleCommands) Delete(submodule *models.SubmoduleConfig) error {
 	// based on https://gist.github.com/myusuf3/7f645819ded92bda6677
 
+	// Git keeps the git dirs of submodules separately for each worktree
+	gitDirPath := submodule.GitDirPath(self.repoPaths.WorktreeGitDirPath())
+
+	// The name of the submodule comes from .gitmodules, so it is under the
+	// control of whoever committed that file. Make sure that we never delete
+	// anything other than a directory below the "modules" directory of the
+	// worktree's git dir.
+	modulesDirPath := filepath.Join(self.repoPaths.WorktreeGitDirPath(), "modules")
+	if relPath, err := filepath.Rel(modulesDirPath, gitDirPath); err != nil || relPath == "." || !filepath.IsLocal(relPath) {
+		return fmt.Errorf("refusing to delete %s because it is not inside %s", gitDirPath, modulesDirPath)
+	}
+
 	if err := self.runInParentModule(submodule, self.cmd.New(
 		NewGitCmd("submodule").
 			Arg("deinit", "--force", "--", submodule.Path).ToArgv(),
@@ -269,8 +353,7 @@ func (self *SubmoduleCommands) Delete(submodule *models.SubmoduleConfig) error {
 		self.Log.Error(err)
 	}
 
-	// Git keeps the git dirs of submodules separately for each worktree
-	return os.RemoveAll(submodule.GitDirPath(self.repoPaths.WorktreeGitDirPath()))
+	return os.RemoveAll(gitDirPath)
 }
 
 func (self *SubmoduleCommands) Add(name string, path string, url string) error {

pkg/commands/git_commands/submodule_test.go

@@ -1,16 +1,210 @@
 package git_commands
 
 import (
+	"os"
+	"path/filepath"
 	"strings"
 	"testing"
 
 	"github.com/go-errors/errors"
+	"github.com/jesseduffield/lazygit/pkg/commands/models"
 	"github.com/jesseduffield/lazygit/pkg/commands/oscommands"
 	"github.com/jesseduffield/lazygit/pkg/env"
 	"github.com/samber/lo"
 	"github.com/stretchr/testify/assert"
 )
 
+func TestParseGitmodules(t *testing.T) {
+	scenarios := []struct {
+		testName string
+		lines    []string
+		expected []*models.SubmoduleConfig
+	}{
+		{
+			testName: "two submodules",
+			lines: []string{
+				`[submodule "one"]`,
+				"\tpath = path/to/one",
+				"\turl = https://example.com/one.git",
+				`[submodule "two"]`,
+				"\tpath = two",
+				"\turl = ../two",
+			},
+			expected: []*models.SubmoduleConfig{
+				{Name: "one", Path: "path/to/one", Url: "https://example.com/one.git"},
+				{Name: "two", Path: "two", Url: "../two"},
+			},
+		},
+		{
+			testName: "url that contains path=",
+			lines: []string{
+				`[submodule "sub"]`,
+				"\tpath = sub",
+				"\turl = https://example.com/repo?path=other",
+			},
+			expected: []*models.SubmoduleConfig{
+				{Name: "sub", Path: "sub", Url: "https://example.com/repo?path=other"},
+			},
+		},
+		{
+			testName: "commented-out section",
+			lines: []string{
+				`[submodule "sub"]`,
+				"\tpath = sub",
+				"\turl = ../sub",
+				`#[submodule "old"]`,
+				"#\tpath = old",
+				"#\turl = ../old",
+			},
+			expected: []*models.SubmoduleConfig{
+				{Name: "sub", Path: "sub", Url: "../sub"},
+			},
+		},
+	}
+
+	for _, s := range scenarios {
+		t.Run(s.testName, func(t *testing.T) {
+			configs, err := parseGitmodules(strings.NewReader(strings.Join(s.lines, "\n")), nil)
+			assert.NoError(t, err)
+			assert.Equal(t, s.expected, configs)
+		})
+	}
+}
+
+func TestIsSuspiciousSubmoduleName(t *testing.T) {
+	scenarios := []struct {
+		name       string
+		suspicious bool
+	}{
+		{name: "sub", suspicious: false},
+		{name: "path/to/sub", suspicious: false},
+		{name: "path\\to\\sub", suspicious: false},
+		{name: ".", suspicious: false},
+		{name: "...", suspicious: false},
+		{name: "..sub", suspicious: false},
+		{name: "sub..", suspicious: false},
+		{name: "a..b/c", suspicious: false},
+		{name: "", suspicious: true},
+		{name: "..", suspicious: true},
+		{name: "../victim", suspicious: true},
+		{name: "../../../victim", suspicious: true},
+		{name: "sub/..", suspicious: true},
+		{name: "sub/../../victim", suspicious: true},
+		{name: "..\\victim", suspicious: true},
+		{name: "sub\\..\\..\\victim", suspicious: true},
+		{name: "sub/..\\..\\victim", suspicious: true},
+	}
+
+	for _, s := range scenarios {
+		t.Run(s.name, func(t *testing.T) {
+			assert.Equal(t, s.suspicious, isSuspiciousSubmoduleName(s.name))
+		})
+	}
+}
+
+func TestIsSuspiciousSubmodulePath(t *testing.T) {
+	dir := t.TempDir()
+	parentDir := filepath.Join(dir, "repo")
+	assert.NoError(t, os.MkdirAll(filepath.Join(parentDir, "sub"), 0o755))
+	assert.NoError(t, os.MkdirAll(filepath.Join(parentDir, "path", "to", "sub"), 0o755))
+	assert.NoError(t, os.MkdirAll(filepath.Join(dir, "other"), 0o755))
+	assert.NoError(t, os.WriteFile(filepath.Join(parentDir, "file"), []byte("content"), 0o644))
+
+	scenarios := []struct {
+		path       string
+		suspicious bool
+	}{
+		{path: "sub", suspicious: false},
+		{path: "sub/", suspicious: false},
+		{path: "path/to/sub", suspicious: false},
+		{path: "not_checked_out", suspicious: false},
+		{path: "path/not_checked_out/sub", suspicious: false},
+		{path: "", suspicious: true},
+		{path: ".", suspicious: true},
+		{path: "./", suspicious: true},
+		{path: "sub/..", suspicious: true},
+		{path: "..", suspicious: true},
+		{path: "../other", suspicious: true},
+		{path: "sub/../../other", suspicious: true},
+		{path: filepath.Join(dir, "other"), suspicious: true},
+		{path: "file", suspicious: true},
+		{path: "file/sub", suspicious: true},
+	}
+
+	for _, s := range scenarios {
+		t.Run(s.path, func(t *testing.T) {
+			assert.Equal(t, s.suspicious, isSuspiciousSubmodulePath(parentDir, s.path))
+		})
+	}
+}
+
+func TestIsSuspiciousSubmodulePathWithSymlinks(t *testing.T) {
+	dir := t.TempDir()
+	parentDir := filepath.Join(dir, "repo")
+	assert.NoError(t, os.MkdirAll(filepath.Join(parentDir, "sub"), 0o755))
+	assert.NoError(t, os.MkdirAll(filepath.Join(dir, "other", "sub"), 0o755))
+	if err := os.Symlink(filepath.Join("..", "other"), filepath.Join(parentDir, "link_outside")); err != nil {
+		t.Skipf("Can't create symlinks: %v", err)
+	}
+	assert.NoError(t, os.Symlink("sub", filepath.Join(parentDir, "link_inside")))
+
+	scenarios := []struct {
+		path       string
+		suspicious bool
+	}{
+		{path: "sub", suspicious: false},
+		{path: "link_outside", suspicious: true},
+		{path: "link_outside/sub", suspicious: true},
+		{path: "link_inside", suspicious: true},
+	}
+
+	for _, s := range scenarios {
+		t.Run(s.path, func(t *testing.T) {
+			assert.Equal(t, s.suspicious, isSuspiciousSubmodulePath(parentDir, s.path))
+		})
+	}
+}
+
+func TestSubmoduleDeleteOutsideModulesDir(t *testing.T) {
+	scenarios := []struct {
+		testName string
+		name     string
+		// relative to the directory that contains the repo
+		victimPath string
+	}{
+		{
+			testName:   "modules dir itself",
+			name:       ".",
+			victimPath: "repo/.git/modules/other_submodule",
+		},
+		{
+			testName:   "outside the repo",
+			name:       "../../../victim",
+			victimPath: "victim",
+		},
+	}
+
+	for _, s := range scenarios {
+		t.Run(s.testName, func(t *testing.T) {
+			dir := t.TempDir()
+			victimFile := filepath.Join(dir, s.victimPath, "file")
+			assert.NoError(t, os.MkdirAll(filepath.Dir(victimFile), 0o755))
+			assert.NoError(t, os.WriteFile(victimFile, []byte("content"), 0o644))
+
+			runner := oscommands.NewFakeRunner(t)
+			instance := buildSubmoduleCommands(commonDeps{
+				runner:    runner,
+				repoPaths: MockRepoPaths(filepath.Join(dir, "repo")),
+			})
+
+			err := instance.Delete(&models.SubmoduleConfig{Name: s.name, Path: "sub"})
+			assert.Error(t, err)
+			assert.FileExists(t, victimFile)
+			runner.CheckForMissingCalls()
+		})
+	}
+}
+
 func TestSubmoduleGetConflictCommits(t *testing.T) {
 	type scenario struct {
 		testName       string

pkg/integration/tests/submodule/ignore_suspicious_name.go

@@ -0,0 +1,36 @@
+package submodule
+
+import (
+	"github.com/jesseduffield/lazygit/pkg/config"
+	. "github.com/jesseduffield/lazygit/pkg/integration/components"
+)
+
+var IgnoreSuspiciousName = NewIntegrationTest(NewIntegrationTestArgs{
+	Description:  "Ignore a submodule whose name in .gitmodules would put its git dir outside the repository",
+	ExtraCmdArgs: []string{},
+	Skip:         false,
+	SetupConfig:  func(config *config.AppConfig) {},
+	SetupRepo: func(shell *Shell) {
+		shell.CreateFile("../victim/file", "content")
+
+		shell.EmptyCommit("first commit")
+		shell.CloneIntoSubmodule("my_submodule_name", "my_submodule_path")
+		shell.GitAddAll()
+		shell.Commit("add submodule")
+
+		// The git dir of a submodule is .git/modules/<name>, so this name
+		// points at the victim directory next to the repository
+		shell.UpdateFileAndAdd(".gitmodules", `[submodule "../../../victim"]
+	path = my_submodule_path
+	url = ../my_submodule_name
+`)
+		shell.Commit("rename submodule")
+	},
+	Run: func(t *TestDriver, keys config.KeybindingConfig) {
+		t.Views().Submodules().Focus().
+			IsEmpty()
+
+		t.FileSystem().
+			PathPresent("../victim/file")
+	},
+})

pkg/integration/tests/submodule/ignore_suspicious_path.go

@@ -0,0 +1,65 @@
+package submodule
+
+import (
+	"github.com/jesseduffield/lazygit/pkg/config"
+	. "github.com/jesseduffield/lazygit/pkg/integration/components"
+)
+
+var IgnoreSuspiciousPath = NewIntegrationTest(NewIntegrationTestArgs{
+	Description:  "Ignore a submodule whose path in .gitmodules leads outside the repository",
+	ExtraCmdArgs: []string{},
+	Skip:         false,
+	SetupConfig:  func(config *config.AppConfig) {},
+	SetupRepo: func(shell *Shell) {
+		// Two unrelated repos next to ours, each with an uncommitted change
+		for _, otherRepo := range []string{"other_repo", "linked_repo"} {
+			shell.Chdir("..")
+			shell.CreateDir(otherRepo)
+			shell.Chdir(otherRepo)
+			shell.Init()
+			shell.CreateFileAndAdd("file", "original")
+			shell.Commit("initial commit")
+			shell.UpdateFile("file", "modified")
+			shell.Chdir("../repo")
+		}
+
+		shell.EmptyCommit("first commit")
+		shell.RunCommand([]string{"ln", "-s", "../linked_repo", "link"})
+		shell.GitAdd("link")
+		shell.CreateFileAndAdd(".gitmodules", `[submodule "other"]
+	path = ../other_repo
+	url = ../other_repo
+[submodule "linked"]
+	path = link
+	url = ../linked_repo
+`)
+		shell.Commit("add submodules")
+		shell.CreateFile("untracked", "content")
+	},
+	Run: func(t *TestDriver, keys config.KeybindingConfig) {
+		t.Views().Submodules().
+			IsEmpty()
+
+		t.Views().Files().Focus().
+			Lines(
+				Contains("untracked"),
+			).
+			Press(keys.Files.ViewResetOptions).
+			Tap(func() {
+				t.ExpectPopup().Menu().
+					Title(Equals("")).
+					Select(Contains("Nuke working tree")).
+					Confirm()
+
+				t.ExpectPopup().Confirmation().
+					Title(Equals("Nuke working tree")).
+					Content(AnyString()).
+					Confirm()
+			}).
+			IsEmpty()
+
+		t.FileSystem().
+			FileContent("../other_repo/file", Equals("modified")).
+			FileContent("../linked_repo/file", Equals("modified"))
+	},
+})

pkg/integration/tests/test_list.go

@@ -578,6 +578,8 @@ var tests = []*components.IntegrationTest{
 	submodule.Enter,
 	submodule.EnterFromDotfileBareRepo,
 	submodule.EnterNested,
+	submodule.IgnoreSuspiciousName,
+	submodule.IgnoreSuspiciousPath,
 	submodule.Remove,
 	submodule.RemoveInLinkedWorktree,
 	submodule.RemoveNested,