diff --git a/CHANGELOG.md b/CHANGELOG.md index c2352d344..ad8f98a7c 100644 --- a/CHANGELOG.md +++ b/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) diff --git a/conf/app.ini b/conf/app.ini index f9b6f2b4a..d73885a10 100644 --- a/conf/app.ini +++ b/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. diff --git a/internal/conf/conf.go b/internal/conf/conf.go index bea7a69f0..8c51c7b6c 100644 --- a/internal/conf/conf.go +++ b/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 { diff --git a/internal/conf/static.go b/internal/conf/static.go index 777cf4e83..2bc34bd72 100644 --- a/internal/conf/static.go +++ b/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 diff --git a/internal/database/repo.go b/internal/database/repo.go index 2cfb8c9b5..b5384160b 100644 --- a/internal/database/repo.go +++ b/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. diff --git a/internal/lfsutil/storage.go b/internal/lfsutil/storage.go index 36c4d7ac6..b53522f10 100644 --- a/internal/lfsutil/storage.go +++ b/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 } diff --git a/internal/lfsutil/storage_test.go b/internal/lfsutil/storage_test.go index 4dccdf235..479e0d414 100644 --- a/internal/lfsutil/storage_test.go +++ b/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) diff --git a/internal/route/lfs/basic.go b/internal/route/lfs/basic.go index ba2f9c416..70920ab07 100644 --- a/internal/route/lfs/basic.go +++ b/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 diff --git a/internal/route/lfs/route.go b/internal/route/lfs/route.go index f7766149c..cf32ab369 100644 --- a/internal/route/lfs/route.go +++ b/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()).