From 1cba9bc81b6febf2c2bef3784665c22ada312bbf Mon Sep 17 00:00:00 2001 From: Joe Chen Date: Sun, 8 Jun 2025 18:28:28 -0400 Subject: [PATCH] web_editor: prohibit CRUD to symbolic files (#7981) Fixes [GHSA-wj44-9vcg-wjq7](https://github.com/gogs/gogs/security/advisories/GHSA-wj44-9vcg-wjq7) --------- Co-authored-by: deepsource-autofix[bot] <62050782+deepsource-autofix[bot]@users.noreply.github.com> --- internal/db/repo_editor.go | 39 ++++++++++++++-- internal/osutil/osutil.go | 14 ++++++ internal/osutil/osutil_test.go | 85 +++++++++++++++++++++++++--------- 3 files changed, 112 insertions(+), 26 deletions(-) diff --git a/internal/db/repo_editor.go b/internal/db/repo_editor.go index a39a31b15..750da9cb8 100644 --- a/internal/db/repo_editor.go +++ b/internal/db/repo_editor.go @@ -164,7 +164,12 @@ func (repo *Repository) UpdateRepoFile(doer *User, opts UpdateRepoFileOptions) ( // If it's meant to be a new file, make sure it doesn't exist. if opts.IsNewFile { - if com.IsExist(filePath) { + // 🚨 SECURITY: Prevent updating files in surprising place, check if the file is + // a symlink. + if osutil.IsSymlink(filePath) { + return fmt.Errorf("cannot update symbolic link: %s", opts.NewTreeName) + } + if osutil.IsExist(filePath) { return ErrRepoFileAlreadyExist{filePath} } } @@ -172,6 +177,12 @@ func (repo *Repository) UpdateRepoFile(doer *User, opts UpdateRepoFileOptions) ( // Ignore move step if it's a new file under a directory. // Otherwise, move the file when name changed. if osutil.IsFile(oldFilePath) && opts.OldTreeName != opts.NewTreeName { + // 🚨 SECURITY: Prevent updating files in surprising place, check if the file is + // a symlink. + if osutil.IsSymlink(oldFilePath) { + return fmt.Errorf("cannot move symbolic link: %s", opts.OldTreeName) + } + if err = git.Move(localPath, opts.OldTreeName, opts.NewTreeName); err != nil { return fmt.Errorf("git mv %q %q: %v", opts.OldTreeName, opts.NewTreeName, err) } @@ -236,10 +247,15 @@ func (repo *Repository) GetDiffPreview(branch, treePath, content string) (diff * localPath := repo.LocalCopyPath() filePath := path.Join(localPath, treePath) + + // 🚨 SECURITY: Prevent updating files in surprising place, check if the target is + // a symlink. + if osutil.IsSymlink(filePath) { + return nil, fmt.Errorf("cannot get diff preview for symbolic link: %s", treePath) + } if err = os.MkdirAll(filepath.Dir(filePath), os.ModePerm); err != nil { return nil, err - } - if err = os.WriteFile(filePath, []byte(content), 0600); err != nil { + } else if err = os.WriteFile(filePath, []byte(content), 0600); err != nil { return nil, fmt.Errorf("write file: %v", err) } @@ -310,7 +326,15 @@ func (repo *Repository) DeleteRepoFile(doer *User, opts DeleteRepoFileOptions) ( } localPath := repo.LocalCopyPath() - if err = os.Remove(path.Join(localPath, opts.TreePath)); err != nil { + filePath := path.Join(localPath, opts.TreePath) + + // 🚨 SECURITY: Prevent updating files in surprising place, check if the file is + // a symlink. + if osutil.IsSymlink(filePath) { + return fmt.Errorf("cannot delete symbolic link: %s", opts.TreePath) + } + + if err = os.Remove(filePath); err != nil { return fmt.Errorf("remove file %q: %v", opts.TreePath, err) } @@ -561,6 +585,13 @@ func (repo *Repository) UploadRepoFiles(doer *User, opts UploadRepoFileOptions) } targetPath := path.Join(dirPath, upload.Name) + + // 🚨 SECURITY: Prevent updating files in surprising place, check if the target + // is a symlink. + if osutil.IsSymlink(targetPath) { + return fmt.Errorf("cannot overwrite symbolic link: %s", upload.Name) + } + if err = com.Copy(tmpPath, targetPath); err != nil { return fmt.Errorf("copy: %v", err) } diff --git a/internal/osutil/osutil.go b/internal/osutil/osutil.go index b5c824557..98135e3a8 100644 --- a/internal/osutil/osutil.go +++ b/internal/osutil/osutil.go @@ -34,6 +34,20 @@ func IsExist(path string) bool { return err == nil || os.IsExist(err) } +// IsSymlink returns true if given path is a symbolic link. +func IsSymlink(path string) bool { + if !IsExist(path) { + return false + } + + fileInfo, err := os.Lstat(path) + if err != nil { + return false + } + + return fileInfo.Mode()&os.ModeSymlink != 0 +} + // CurrentUsername returns the username of the current user. func CurrentUsername() string { username := os.Getenv("USER") diff --git a/internal/osutil/osutil_test.go b/internal/osutil/osutil_test.go index 65ead6189..747f52a66 100644 --- a/internal/osutil/osutil_test.go +++ b/internal/osutil/osutil_test.go @@ -9,50 +9,51 @@ import ( "testing" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestIsFile(t *testing.T) { tests := []struct { - path string - expVal bool + path string + want bool }{ { - path: "osutil.go", - expVal: true, + path: "osutil.go", + want: true, }, { - path: "../osutil", - expVal: false, + path: "../osutil", + want: false, }, { - path: "not_found", - expVal: false, + path: "not_found", + want: false, }, } for _, test := range tests { t.Run("", func(t *testing.T) { - assert.Equal(t, test.expVal, IsFile(test.path)) + assert.Equal(t, test.want, IsFile(test.path)) }) } } func TestIsDir(t *testing.T) { tests := []struct { - path string - expVal bool + path string + want bool }{ { - path: "osutil.go", - expVal: false, + path: "osutil.go", + want: false, }, { - path: "../osutil", - expVal: true, + path: "../osutil", + want: true, }, { - path: "not_found", - expVal: false, + path: "not_found", + want: false, }, } for _, test := range tests { t.Run("", func(t *testing.T) { - assert.Equal(t, test.expVal, IsDir(test.path)) + assert.Equal(t, test.want, IsDir(test.path)) }) } } @@ -82,13 +83,53 @@ func TestIsExist(t *testing.T) { func TestCurrentUsername(t *testing.T) { if oldUser, ok := os.LookupEnv("USER"); ok { - defer func() { _ = os.Setenv("USER", oldUser) }() + defer func() { t.Setenv("USER", oldUser) }() } else { defer func() { _ = os.Unsetenv("USER") }() } - if err := os.Setenv("USER", "__TESTING::USERNAME"); err != nil { - t.Skip("Could not set the USER environment variable:", err) - } + t.Setenv("USER", "__TESTING::USERNAME") assert.Equal(t, "__TESTING::USERNAME", CurrentUsername()) } + +func TestIsSymlink(t *testing.T) { + // Create a temporary file + tempFile, err := os.CreateTemp("", "symlink-test-*") + require.NoError(t, err, "create temporary file") + tempFilePath := tempFile.Name() + _ = tempFile.Close() + defer func() { _ = os.Remove(tempFilePath) }() + + // Create a temporary symlink + tempSymlinkPath := tempFilePath + "-symlink" + err = os.Symlink(tempFilePath, tempSymlinkPath) + require.NoError(t, err, "create temporary symlink") + defer func() { _ = os.Remove(tempSymlinkPath) }() + + tests := []struct { + name string + path string + want bool + }{ + { + name: "non-existent path", + path: "not_found", + want: false, + }, + { + name: "regular file", + path: tempFilePath, + want: false, + }, + { + name: "symlink", + path: tempSymlinkPath, + want: true, + }, + } + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + assert.Equal(t, test.want, IsSymlink(test.path)) + }) + } +}