make sure limit can not be bypassed

services/attachment/attachment.go

@@ -24,7 +24,7 @@ func NewAttachment(ctx context.Context, attach *repo_model.Attachment, file io.R
 		return nil, fmt.Errorf("attachment %s should belong to a repository", attach.Name)
 	}
 
-	err := db.WithTx(ctx, func(ctx context.Context) error {
+	if err := db.WithTx(ctx, func(ctx context.Context) error {
 		attach.UUID = uuid.New().String()
 		size, err := storage.Attachments.Save(attach.RelativePath(), file, size)
 		if err != nil {
@@ -33,24 +33,34 @@ func NewAttachment(ctx context.Context, attach *repo_model.Attachment, file io.R
 		attach.Size = size
 
 		return db.Insert(ctx, attach)
-	})
+	}); err != nil {
+		return nil, err
+	}
 
-	return attach, err
+	return attach, nil
 }
 
 type ErrAttachmentSizeExceed struct {
 	MaxSize int64
 	Size    int64
 }
 
-func (e ErrAttachmentSizeExceed) Error() string {
+func (e *ErrAttachmentSizeExceed) Error() string {
+	if e.Size == 0 {
+		return fmt.Sprintf("attachment size exceeds limit %d", e.MaxSize)
+	}
 	return fmt.Sprintf("attachment size %d exceeds limit %d", e.Size, e.MaxSize)
 }
 
-func (e ErrAttachmentSizeExceed) Unwrap() error {
+func (e *ErrAttachmentSizeExceed) Unwrap() error {
 	return util.ErrContentTooLarge
 }
 
+func (e *ErrAttachmentSizeExceed) Is(target error) bool {
+	_, ok := target.(*ErrAttachmentSizeExceed)
+	return ok
+}
+
 // UploadAttachment upload new attachment into storage and update database
 func UploadAttachment(ctx context.Context, file io.Reader, allowedTypes string, maxFileSize, fileSize int64, attach *repo_model.Attachment) (*repo_model.Attachment, error) {
 	buf := make([]byte, 1024)
@@ -61,11 +71,20 @@ func UploadAttachment(ctx context.Context, file io.Reader, allowedTypes string,
 		return nil, err
 	}
 
-	if maxFileSize >= 0 && fileSize > maxFileSize {
-		return nil, ErrAttachmentSizeExceed{MaxSize: maxFileSize, Size: fileSize}
+	reader := io.MultiReader(bytes.NewReader(buf), file)
+
+	// enforce file size limit
+	if maxFileSize >= 0 {
+		if fileSize > maxFileSize {
+			return nil, &ErrAttachmentSizeExceed{MaxSize: maxFileSize, Size: fileSize}
+		}
+		// limit reader to max file size with additional 1k more,
+		// to allow side-cases where encoding tells us its exactly maxFileSize but the actual created file is bit more,
+		// while still make sure the limit is enforced
+		reader = attachmentLimitedReader(reader, maxFileSize+1024)
 	}
 
-	return NewAttachment(ctx, attach, io.MultiReader(bytes.NewReader(buf), file), fileSize)
+	return NewAttachment(ctx, attach, reader, fileSize)
 }
 
 // UpdateAttachment updates an attachment, verifying that its name is among the allowed types.

services/attachment/attachment_test.go

@@ -15,31 +15,77 @@ import (
 	_ "code.gitea.io/gitea/models/actions"
 
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 )
 
 func TestMain(m *testing.M) {
 	unittest.MainTest(m)
 }
 
-func TestUploadAttachment(t *testing.T) {
+func TestNewAttachment(t *testing.T) {
 	assert.NoError(t, unittest.PrepareTestDatabase())
-
 	user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1})
 
 	fPath := "./attachment_test.go"
 	f, err := os.Open(fPath)
-	assert.NoError(t, err)
+	require.NoError(t, err)
 	defer f.Close()
+	fs, err := f.Stat()
+	require.NoError(t, err)
 
 	attach, err := NewAttachment(t.Context(), &repo_model.Attachment{
 		RepoID:     1,
 		UploaderID: user.ID,
 		Name:       filepath.Base(fPath),
-	}, f, -1)
+	}, f, fs.Size())
 	assert.NoError(t, err)
+	assert.Equal(t, fs.Size(), attach.Size)
 
 	attachment, err := repo_model.GetAttachmentByUUID(t.Context(), attach.UUID)
 	assert.NoError(t, err)
 	assert.Equal(t, user.ID, attachment.UploaderID)
 	assert.Equal(t, int64(0), attachment.DownloadCount)
 }
+
+func TestUploadAttachment(t *testing.T) {
+	assert.NoError(t, unittest.PrepareTestDatabase())
+	user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1})
+
+	fPath := "./attachment_test.go"
+	f, err := os.Open(fPath)
+	require.NoError(t, err)
+	defer f.Close()
+	fs, err := f.Stat()
+	require.NoError(t, err)
+
+	t.Run("size to big", func(t *testing.T) {
+		attach, err := UploadAttachment(t.Context(), f, "", 10, fs.Size(), &repo_model.Attachment{
+			RepoID:     1,
+			UploaderID: user.ID,
+			Name:       filepath.Base(fPath),
+		})
+		assert.ErrorIs(t, err, &ErrAttachmentSizeExceed{})
+		assert.Nil(t, attach)
+	})
+
+	t.Run("size was lied about", func(t *testing.T) {
+		attach, err := UploadAttachment(t.Context(), f, "", 10, 10, &repo_model.Attachment{
+			RepoID:     1,
+			UploaderID: user.ID,
+			Name:       filepath.Base(fPath),
+		})
+		assert.ErrorIs(t, err, &ErrAttachmentSizeExceed{})
+		assert.Nil(t, attach)
+	})
+
+	t.Run("size was correct", func(t *testing.T) {
+		attach, err := UploadAttachment(t.Context(), f, "", fs.Size(), fs.Size(), &repo_model.Attachment{
+			RepoID:     1,
+			UploaderID: user.ID,
+			Name:       filepath.Base(fPath),
+		})
+		assert.NoError(t, err)
+		require.NotNil(t, attach)
+		assert.Equal(t, user.ID, attach.UploaderID)
+	})
+}

services/attachment/reader.go

@@ -0,0 +1,32 @@
+// Copyright 2021 The Gitea Authors. All rights reserved.
+// SPDX-License-Identifier: MIT
+
+package attachment
+
+import "io"
+
+// attachmentLimitedReader returns a Reader that reads from r
+// but errors with ErrAttachmentSizeExceed after n bytes.
+// The underlying implementation is a *attachmentReader.
+func attachmentLimitedReader(r io.Reader, n int64) io.Reader { return &attachmentReader{r, n} }
+
+// A attachmentReader reads from R but limits the amount of
+// data returned to just N bytes. Each call to Read
+// updates N to reflect the new amount remaining.
+// Read returns EOF when N <= 0 or when the underlying R returns EOF.
+type attachmentReader struct {
+	R io.Reader // underlying reader
+	N int64     // max bytes remaining
+}
+
+func (l *attachmentReader) Read(p []byte) (n int, err error) {
+	if l.N <= 0 {
+		return 0, &ErrAttachmentSizeExceed{MaxSize: l.N}
+	}
+	if int64(len(p)) > l.N {
+		p = p[0:l.N]
+	}
+	n, err = l.R.Read(p)
+	l.N -= int64(n)
+	return
+}