Browse Source

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>
Joe Chen 20 hours ago
parent
commit
1cba9bc81b
3 changed files with 112 additions and 26 deletions
  1. 35 4
      internal/db/repo_editor.go
  2. 14 0
      internal/osutil/osutil.go
  3. 63 22
      internal/osutil/osutil_test.go

+ 35 - 4
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)
 		}

+ 14 - 0
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")

+ 63 - 22
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))
+		})
+	}
+}