serve s3: reject bogus multipart part sizes in the reorder buffer GHSA-2p48-j3qc-rx9f
The multipart reorder-buffer admission trusted the client-declared part length. A negative length was accepted, and `buffered + size` could overflow int64 for a huge declared length, wrapping the running total negative and admitting further parts past --multipart-streaming-buffer-limit. Reject a negative length and use the overflow-safe comparison `size <= bufferLimit - buffered` so an untrusted Content-Length can neither poison nor overflow the budget.
This commit is contained in:
@@ -235,14 +235,21 @@ func (b *s3Backend) UploadPart(ctx context.Context, bucketName, objectName strin
|
||||
// admitted so the sink can keep draining; so is a single part bigger than the
|
||||
// limit when the buffer is empty, to guarantee progress. Reserved bytes are
|
||||
// returned with release, or by the pump as the part is streamed.
|
||||
//
|
||||
// size is the client-declared part length and is not trusted: a negative value
|
||||
// is rejected, and the admission test is written so a huge value can't overflow
|
||||
// the running total and wrongly admit further parts past the limit.
|
||||
func (up *multipartUpload) waitForTurn(partNumber int, size int64) error {
|
||||
if size < 0 {
|
||||
return gofakes3.ErrInvalidArgument
|
||||
}
|
||||
up.mu.Lock()
|
||||
defer up.mu.Unlock()
|
||||
for {
|
||||
if up.closed {
|
||||
return gofakes3.ErrNoSuchUpload
|
||||
}
|
||||
if up.bufferLimit <= 0 || partNumber <= up.nextPart || up.buffered == 0 || up.buffered+size <= up.bufferLimit {
|
||||
if up.bufferLimit <= 0 || partNumber <= up.nextPart || up.buffered == 0 || size <= up.bufferLimit-up.buffered {
|
||||
up.buffered += size
|
||||
return nil
|
||||
}
|
||||
|
||||
@@ -8,6 +8,7 @@ import (
|
||||
"crypto/md5"
|
||||
"fmt"
|
||||
"io"
|
||||
"math"
|
||||
"net/url"
|
||||
"path"
|
||||
"sync"
|
||||
@@ -1022,3 +1023,38 @@ func TestUploadPartNoReserveBeforeBody(t *testing.T) {
|
||||
require.Less(t, reader.recorded, wantPages,
|
||||
"UploadPart preallocated pool pages from the declared Content-Length before any body bytes arrived")
|
||||
}
|
||||
|
||||
// TestWaitForTurnRejectsBogusSize checks that the reorder-buffer admission
|
||||
// rejects a negative client-declared part length and that a huge declared
|
||||
// length cannot overflow the running total so as to admit a further part past
|
||||
// the buffer limit.
|
||||
func TestWaitForTurnRejectsBogusSize(t *testing.T) {
|
||||
up := newMultipartUpload("bucket", "key", "bucket/key", "bucket/key", nil, 1<<20)
|
||||
|
||||
// A part length can never be negative.
|
||||
require.ErrorIs(t, up.waitForTurn(1, -1), gofakes3.ErrInvalidArgument)
|
||||
|
||||
// A huge out-of-order part is admitted once because the buffer is empty,
|
||||
// driving buffered near the top of the int64 range.
|
||||
require.NoError(t, up.waitForTurn(2, math.MaxInt64))
|
||||
|
||||
// A further out-of-order part must wait, not be wrongly admitted by an
|
||||
// overflow of buffered+size.
|
||||
admitted := make(chan struct{})
|
||||
go func() {
|
||||
_ = up.waitForTurn(3, math.MaxInt64)
|
||||
close(admitted)
|
||||
}()
|
||||
select {
|
||||
case <-admitted:
|
||||
t.Fatal("out-of-order part admitted past the buffer limit via overflow")
|
||||
case <-time.After(50 * time.Millisecond):
|
||||
}
|
||||
|
||||
// Wake the blocked goroutine so it doesn't leak.
|
||||
up.mu.Lock()
|
||||
up.closed = true
|
||||
up.cond.Broadcast()
|
||||
up.mu.Unlock()
|
||||
<-admitted
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user