From 94f42bd6b26e6f21efc221a4f182f8e5cc89442e Mon Sep 17 00:00:00 2001 From: kolaente Date: Thu, 9 Apr 2026 17:15:17 +0200 Subject: [PATCH] fix(files): derive file size from reader at creation boundary Authoritative size now comes from the reader instead of the caller's claim in CreateWithMimeAndSession. The migration import path accepts attacker-controlled metadata (GHSA-qh78-rvg3-cv54), so trusting realsize for the limit check allowed oversized uploads to be accepted and stored. measureReaderSize leaves the reader seeked to 0 so the measured value matches the bytes storage backends will actually write. --- pkg/files/files.go | 51 ++++++++++++++++++++++++-- pkg/files/files_test.go | 57 ++++++++++++++++++++++++++++-- pkg/models/task_attachment_test.go | 5 +-- 3 files changed, 105 insertions(+), 8 deletions(-) diff --git a/pkg/files/files.go b/pkg/files/files.go index 3c702c233..a103a5237 100644 --- a/pkg/files/files.go +++ b/pkg/files/files.go @@ -20,6 +20,7 @@ import ( "errors" "fmt" "io" + "math" "os" "strconv" "time" @@ -114,15 +115,59 @@ func CreateWithMime(f io.ReadSeeker, realname string, realsize uint64, a web.Aut return file, s.Commit() } +// measureReaderSize returns the byte length of r and leaves r seeked to +// 0, so the size we check matches what storage backends will write +// (they also Seek(0, io.SeekStart) before reading). +func measureReaderSize(r io.ReadSeeker) (uint64, error) { + if _, err := r.Seek(0, io.SeekStart); err != nil { + return 0, err + } + end, err := r.Seek(0, io.SeekEnd) + if err != nil { + return 0, err + } + if _, err := r.Seek(0, io.SeekStart); err != nil { + return 0, err + } + if end < 0 { + return 0, fmt.Errorf("reader end %d is negative", end) + } + return uint64(end), nil +} + func CreateWithMimeAndSession(s *xorm.Session, f io.ReadSeeker, realname string, realsize uint64, a web.Auth, mime string, checkFileSizeLimit bool) (file *File, err error) { - if realsize > config.GetMaxFileSizeInMBytes()*uint64(datasize.MB) && checkFileSizeLimit { - return nil, ErrFileIsTooLarge{Size: realsize} + // Authoritative size comes from the reader, not the caller: the + // migration import path accepts attacker-controlled metadata + // (GHSA-qh78-rvg3-cv54) and several other callers pass stale values. + measured, mErr := measureReaderSize(f) + if mErr != nil { + return nil, fmt.Errorf("failed to measure file size: %w", mErr) + } + + if checkFileSizeLimit { + // Overflow-safe: exabyte-range configs would wrap uint64. + maxMB := config.GetMaxFileSizeInMBytes() + var maxBytes uint64 + if maxMB > math.MaxUint64/uint64(datasize.MB) { + maxBytes = math.MaxUint64 + } else { + maxBytes = maxMB * uint64(datasize.MB) + } + if measured > maxBytes { + return nil, ErrFileIsTooLarge{Size: measured} + } + } + + // Surface buggy callers that lie about size. + if realsize != 0 && realsize != measured { + log.Debugf("files.Create: caller-supplied size %d does not match measured size %d for %q", + realsize, measured, realname) } // We first insert the file into the db to get it's ID file = &File{ Name: realname, - Size: realsize, + Size: measured, CreatedByID: a.GetID(), Mime: mime, } diff --git a/pkg/files/files_test.go b/pkg/files/files_test.go index d8dc9494c..50a633b89 100644 --- a/pkg/files/files_test.go +++ b/pkg/files/files_test.go @@ -24,6 +24,7 @@ import ( "os" "testing" + "code.vikunja.io/api/pkg/config" "code.vikunja.io/api/pkg/db" "github.com/stretchr/testify/assert" @@ -42,7 +43,8 @@ func TestCreate(t *testing.T) { t.Run("Normal", func(t *testing.T) { initFixtures(t) ta := &testauth{id: 1} - createdFile, err := Create(bytes.NewReader([]byte("testfile")), "testfile", 100, ta) + content := []byte("testfile") + createdFile, err := Create(bytes.NewReader(content), "testfile", uint64(len(content)), ta) require.NoError(t, err) // Check the file was created correctly @@ -51,18 +53,67 @@ func TestCreate(t *testing.T) { require.NoError(t, err) assert.Equal(t, int64(1), file.CreatedByID) assert.Equal(t, "testfile", file.Name) - assert.Equal(t, uint64(100), file.Size) + assert.Equal(t, uint64(len(content)), file.Size) }) t.Run("Too Large", func(t *testing.T) { initFixtures(t) + // initFixtures resets FilesMaxSize, so override afterwards. + oldMax := config.FilesMaxSize.GetString() + require.NoError(t, config.SetMaxFileSizeMBytesFromString("1MB")) + t.Cleanup(func() { _ = config.SetMaxFileSizeMBytesFromString(oldMax) }) + ta := &testauth{id: 1} - _, err := Create(bytes.NewReader([]byte("testfile")), "testfile", 99999999999, ta) + _, err := Create(bytes.NewReader(make([]byte, 2*1024*1024)), "big.bin", 0, ta) require.Error(t, err) assert.True(t, IsErrFileIsTooLarge(err)) }) } +func TestCreate_HardenedAgainstLyingCaller(t *testing.T) { + // Regression: GHSA-qh78-rvg3-cv54. Caller's claimed size must not + // influence the limit check or the stored size. + + // setLimit must run AFTER initFixtures since initFixtures resets it. + setLimit := func(t *testing.T, size string) { + t.Helper() + oldMax := config.FilesMaxSize.GetString() + require.NoError(t, config.SetMaxFileSizeMBytesFromString(size)) + t.Cleanup(func() { + _ = config.SetMaxFileSizeMBytesFromString(oldMax) + }) + } + + ta := &testauth{id: 1} + + t.Run("content over limit, claimed size under limit", func(t *testing.T) { + initFixtures(t) + setLimit(t, "1MB") + payload := make([]byte, 2*1024*1024) + _, err := Create(bytes.NewReader(payload), "sneaky.bin", 1, ta) + require.Error(t, err) + assert.True(t, IsErrFileIsTooLarge(err)) + }) + + t.Run("content under limit, claimed size over limit", func(t *testing.T) { + initFixtures(t) + setLimit(t, "1MB") + payload := []byte("hi") + f, err := Create(bytes.NewReader(payload), "honest.txt", 99999999999, ta) + require.NoError(t, err) + assert.Equal(t, uint64(len(payload)), f.Size) + }) + + t.Run("content exactly at limit", func(t *testing.T) { + initFixtures(t) + setLimit(t, "1MB") + payload := make([]byte, 1*1024*1024) + f, err := Create(bytes.NewReader(payload), "edge.bin", 0, ta) + require.NoError(t, err) + assert.Equal(t, uint64(1*1024*1024), f.Size) + }) +} + func TestCreateDetectsMimeType(t *testing.T) { initFixtures(t) ta := &testauth{id: 1} diff --git a/pkg/models/task_attachment_test.go b/pkg/models/task_attachment_test.go index a2e06a365..8aa7b7090 100644 --- a/pkg/models/task_attachment_test.go +++ b/pkg/models/task_attachment_test.go @@ -104,7 +104,8 @@ func TestTaskAttachment_NewAttachment(t *testing.T) { } testuser := &user.User{ID: 1} - err := ta.NewAttachment(s, bytes.NewReader([]byte("testingstuff")), "testfile", 100, testuser) + content := []byte("testingstuff") + err := ta.NewAttachment(s, bytes.NewReader(content), "testfile", uint64(len(content)), testuser) require.NoError(t, err) assert.NotEqual(t, 0, ta.FileID) _, err = files.FileStat(ta.File) @@ -122,7 +123,7 @@ func TestTaskAttachment_NewAttachment(t *testing.T) { require.NoError(t, err) assert.Equal(t, testuser.ID, ta.File.CreatedByID) assert.Equal(t, "testfile", ta.File.Name) - assert.Equal(t, uint64(100), ta.File.Size) + assert.Equal(t, uint64(len(content)), ta.File.Size) assert.NotEmpty(t, ta.File.Mime, "mime type should be detected and stored") // Extra test for max size test