lfs: verify content hash and prevent object overwrite (#8166)

Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-authored-by: deepsource-autofix[bot] <62050782+deepsource-autofix[bot]@users.noreply.github.com>

CHANGELOG.md

@@ -4,6 +4,10 @@ All notable changes to Gogs are documented in this file.
 
 ## 0.15.0+dev (`main`)
 
+### Fixed
+
+- _Security:_ Cross-repository LFS object overwrite via missing content hash verification. [#8166](https://github.com/gogs/gogs/pull/8166) - [GHSA-gmf8-978x-2fg2](https://github.com/gogs/gogs/security/advisories/GHSA-gmf8-978x-2fg2)
+
 ### Removed
 
 - The `gogs cert` subcommand. [#8153](https://github.com/gogs/gogs/pull/8153)

conf/app.ini

@@ -279,6 +279,8 @@ ACCESS_CONTROL_ALLOW_ORIGIN =
 STORAGE = local
 ; The root path to store LFS objects on local file system.
 OBJECTS_PATH = data/lfs-objects
+; The path to temporarily store LFS objects during upload verification.
+OBJECTS_TEMP_PATH = data/tmp/lfs-objects
 
 [attachment]
 ; Whether to enabled upload attachments in general.

internal/conf/conf.go

@@ -346,6 +346,7 @@ func Init(customConf string) error {
 		return errors.Wrap(err, "mapping [lfs] section")
 	}
 	LFS.ObjectsPath = ensureAbs(LFS.ObjectsPath)
+	LFS.ObjectsTempPath = ensureAbs(LFS.ObjectsTempPath)
 
 	handleDeprecated()
 	if !HookMode {

internal/conf/static.go

@@ -361,8 +361,9 @@ type DatabaseOpts struct {
 var Database DatabaseOpts
 
 type LFSOpts struct {
-	Storage     string
-	ObjectsPath string
+	Storage         string
+	ObjectsPath     string
+	ObjectsTempPath string
 }
 
 // LFS settings

internal/database/repo.go

@@ -162,6 +162,7 @@ func NewRepoContext() {
 	}
 
 	RemoveAllWithNotice("Clean up repository temporary data", filepath.Join(conf.Server.AppDataPath, "tmp"))
+	RemoveAllWithNotice("Clean up LFS temporary data", conf.LFS.ObjectsTempPath)
 }
 
 // Repository contains information of a repository.

internal/lfsutil/storage.go

@@ -1,6 +1,8 @@
 package lfsutil
 
 import (
+	"crypto/sha256"
+	"encoding/hex"
 	"io"
 	"os"
 	"path/filepath"
@@ -10,7 +12,10 @@ import (
 	"gogs.io/gogs/internal/osutil"
 )
 
-var ErrObjectNotExist = errors.New("Object does not exist")
+var (
+	ErrObjectNotExist = errors.New("object does not exist")
+	ErrOIDMismatch    = errors.New("content hash does not match OID")
+)
 
 // Storager is an storage backend for uploading and downloading LFS objects.
 type Storager interface {
@@ -39,6 +44,8 @@ var _ Storager = (*LocalStorage)(nil)
 type LocalStorage struct {
 	// The root path for storing LFS objects.
 	Root string
+	// The path for storing temporary files during upload verification.
+	TempDir string
 }
 
 func (*LocalStorage) Storage() Storage {
@@ -58,29 +65,50 @@ func (s *LocalStorage) Upload(oid OID, rc io.ReadCloser) (int64, error) {
 		return 0, ErrInvalidOID
 	}
 
-	var err error
 	fpath := s.storagePath(oid)
-	defer func() {
-		rc.Close()
+	dir := filepath.Dir(fpath)
 
-		if err != nil {
-			_ = os.Remove(fpath)
-		}
-	}()
+	defer rc.Close()
 
-	err = os.MkdirAll(filepath.Dir(fpath), os.ModePerm)
-	if err != nil {
+	if err := os.MkdirAll(dir, os.ModePerm); err != nil {
 		return 0, errors.Wrap(err, "create directories")
 	}
-	w, err := os.Create(fpath)
-	if err != nil {
-		return 0, errors.Wrap(err, "create file")
-	}
-	defer w.Close()
 
-	written, err := io.Copy(w, rc)
+	// If the object file already exists, skip the upload and return the
+	// existing file's size.
+	if fi, err := os.Stat(fpath); err == nil {
+		_, _ = io.Copy(io.Discard, rc)
+		return fi.Size(), nil
+	}
+
+	// Write to a temp file and verify the content hash before publishing.
+	// This ensures the final path always contains a complete, hash-verified
+	// file, even when concurrent uploads of the same OID race.
+	if err := os.MkdirAll(s.TempDir, os.ModePerm); err != nil {
+		return 0, errors.Wrap(err, "create temp directory")
+	}
+	tmp, err := os.CreateTemp(s.TempDir, "upload-*")
 	if err != nil {
-		return 0, errors.Wrap(err, "copy file")
+		return 0, errors.Wrap(err, "create temp file")
+	}
+	tmpPath := tmp.Name()
+	defer os.Remove(tmpPath)
+
+	hash := sha256.New()
+	written, err := io.Copy(tmp, io.TeeReader(rc, hash))
+	if closeErr := tmp.Close(); err == nil && closeErr != nil {
+		err = closeErr
+	}
+	if err != nil {
+		return 0, errors.Wrap(err, "write object file")
+	}
+
+	if computed := hex.EncodeToString(hash.Sum(nil)); computed != string(oid) {
+		return 0, ErrOIDMismatch
+	}
+
+	if err := os.Rename(tmpPath, fpath); err != nil && !os.IsExist(err) {
+		return 0, errors.Wrap(err, "publish object file")
 	}
 	return written, nil
 }

internal/lfsutil/storage_test.go

@@ -10,6 +10,9 @@ import (
 	"testing"
 
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+
+	"gogs.io/gogs/internal/osutil"
 )
 
 func TestLocalStorage_storagePath(t *testing.T) {
@@ -46,50 +49,54 @@ func TestLocalStorage_storagePath(t *testing.T) {
 }
 
 func TestLocalStorage_Upload(t *testing.T) {
+	base := t.TempDir()
 	s := &LocalStorage{
-		Root: filepath.Join(os.TempDir(), "lfs-objects"),
+		Root:    filepath.Join(base, "lfs-objects"),
+		TempDir: filepath.Join(base, "tmp", "lfs"),
 	}
-	t.Cleanup(func() {
-		_ = os.RemoveAll(s.Root)
+
+	const helloWorldOID = OID("c0535e4be2b79ffd93291305436bf889314e4a3faec05ecffcbb7df31ad9e51a") // "Hello world!"
+
+	t.Run("invalid OID", func(t *testing.T) {
+		written, err := s.Upload("bad_oid", io.NopCloser(strings.NewReader("")))
+		assert.Equal(t, int64(0), written)
+		assert.Equal(t, ErrInvalidOID, err)
 	})
 
-	tests := []struct {
-		name       string
-		oid        OID
-		content    string
-		expWritten int64
-		expErr     error
-	}{
-		{
-			name:   "invalid oid",
-			oid:    "bad_oid",
-			expErr: ErrInvalidOID,
-		},
+	t.Run("valid OID", func(t *testing.T) {
+		written, err := s.Upload(helloWorldOID, io.NopCloser(strings.NewReader("Hello world!")))
+		require.NoError(t, err)
+		assert.Equal(t, int64(12), written)
+	})
 
-		{
-			name:       "valid oid",
-			oid:        "ef797c8118f02dfb649607dd5d3f8c7623048c9c063d532cc95c5ed7a898a64f",
-			content:    "Hello world!",
-			expWritten: 12,
-		},
-	}
-	for _, test := range tests {
-		t.Run(test.name, func(t *testing.T) {
-			written, err := s.Upload(test.oid, io.NopCloser(strings.NewReader(test.content)))
-			assert.Equal(t, test.expWritten, written)
-			assert.Equal(t, test.expErr, err)
-		})
-	}
+	t.Run("valid OID but wrong content", func(t *testing.T) {
+		oid := OID("ef797c8118f02dfb649607dd5d3f8c7623048c9c063d532cc95c5ed7a898a64f")
+		written, err := s.Upload(oid, io.NopCloser(strings.NewReader("Hello world!")))
+		assert.Equal(t, int64(0), written)
+		assert.Equal(t, ErrOIDMismatch, err)
+
+		// File should have been cleaned up.
+		assert.False(t, osutil.IsFile(s.storagePath(oid)))
+	})
+
+	t.Run("duplicate upload returns existing size", func(t *testing.T) {
+		written, err := s.Upload(helloWorldOID, io.NopCloser(strings.NewReader("should be ignored")))
+		require.NoError(t, err)
+		assert.Equal(t, int64(12), written)
+
+		// Verify original content is preserved.
+		var buf bytes.Buffer
+		err = s.Download(helloWorldOID, &buf)
+		require.NoError(t, err)
+		assert.Equal(t, "Hello world!", buf.String())
+	})
 }
 
 func TestLocalStorage_Download(t *testing.T) {
 	oid := OID("ef797c8118f02dfb649607dd5d3f8c7623048c9c063d532cc95c5ed7a898a64f")
 	s := &LocalStorage{
-		Root: filepath.Join(os.TempDir(), "lfs-objects"),
+		Root: filepath.Join(t.TempDir(), "lfs-objects"),
 	}
-	t.Cleanup(func() {
-		_ = os.RemoveAll(s.Root)
-	})
 
 	fpath := s.storagePath(oid)
 	err := os.MkdirAll(filepath.Dir(fpath), os.ModePerm)

internal/route/lfs/basic.go

@@ -91,7 +91,7 @@ func (h *basicHandler) serveUpload(c *macaron.Context, repo *database.Repository
 	s := h.DefaultStorager()
 	written, err := s.Upload(oid, c.Req.Request.Body)
 	if err != nil {
-		if err == lfsutil.ErrInvalidOID {
+		if err == lfsutil.ErrInvalidOID || err == lfsutil.ErrOIDMismatch {
 			responseJSON(c.Resp, http.StatusBadRequest, responseError{
 				Message: err.Error(),
 			})
@@ -105,8 +105,8 @@ func (h *basicHandler) serveUpload(c *macaron.Context, repo *database.Repository
 	err = h.store.CreateLFSObject(c.Req.Context(), repo.ID, oid, written, s.Storage())
 	if err != nil {
 		// NOTE: It is OK to leave the file when the whole operation failed
-		// with a DB error, a retry on client side can safely overwrite the
-		// same file as OID is seen as unique to every file.
+		// with a DB error, a retry on client side will skip the upload as
+		// the file already exists on disk.
 		internalServerError(c.Resp)
 		log.Error("Failed to create object [repo_id: %d, oid: %s]: %v", repo.ID, oid, err)
 		return

internal/route/lfs/route.go

@@ -30,7 +30,7 @@ func RegisterRoutes(r *macaron.Router) {
 				store:          store,
 				defaultStorage: lfsutil.Storage(conf.LFS.Storage),
 				storagers: map[lfsutil.Storage]lfsutil.Storager{
-					lfsutil.StorageLocal: &lfsutil.LocalStorage{Root: conf.LFS.ObjectsPath},
+					lfsutil.StorageLocal: &lfsutil.LocalStorage{Root: conf.LFS.ObjectsPath, TempDir: conf.LFS.ObjectsTempPath},
 				},
 			}
 			r.Combo("/:oid", verifyOID()).