mirror of
https://github.com/coder/coder.git
synced 2026-06-02 20:48:20 +00:00
fix: validate FileSize in NewDataBuilder to prevent OOM DoS (#25710)
`NewDataBuilder` allocated `make([]byte, 0, req.FileSize)` using the client-supplied `int64` with no upper-bound check. The DRPC 4 MiB wire cap limits message size but not the integer value, so a crafted message with `FileSize = 1<<40` forces a 1 TiB allocation, triggering an unrecoverable `runtime.throw` that kills the entire `coderd` process. Add a `MaxFileSize` constant (100 MiB, matching `HTTPFileMaxBytes` in `coderd/files.go`) and reject negative or oversized `FileSize`, plus negative or excessive `Chunks`, before the allocation. `BytesToDataUpload` also returns an error for oversized data to preserve the encode/decode round-trip contract. Fix a pre-existing reversed subtraction in the `Add()` overflow error message. Closes https://linear.app/codercom/issue/PLAT-231 <details> <summary>Implementation details</summary> - `provisionersdk/proto/dataupload.go`: New exported `MaxFileSize` constant; validation in `NewDataBuilder` and `BytesToDataUpload`. Fixed reversed subtraction in `Add()` error. - `provisionersdk/proto/dataupload_test.go`: New `TestNewDataBuilderValidation` with 7 subtests. - Updated all 5 callers of `BytesToDataUpload` for new error return. - Audited all `make([]byte, ...)` in provisioner paths; no other client-supplied sizes. </details> > Generated by Coder Agents on behalf of @f0ssel
This commit is contained in:
@@ -1588,7 +1588,10 @@ func (s *server) DownloadFile(request *proto.FileRequest, stream proto.DRPCProvi
|
||||
return fail(xerrors.Errorf("unsupported file upload type: %s", request.UploadType))
|
||||
}
|
||||
|
||||
upload, chunks := sdkproto.BytesToDataUpload(sdkproto.DataUploadType_UPLOAD_TYPE_MODULE_FILES, file.Data)
|
||||
upload, chunks, err := sdkproto.BytesToDataUpload(sdkproto.DataUploadType_UPLOAD_TYPE_MODULE_FILES, file.Data)
|
||||
if err != nil {
|
||||
return fail(xerrors.Errorf("prepare file upload: %w", err))
|
||||
}
|
||||
|
||||
err = stream.Send(&sdkproto.FileUpload{
|
||||
Type: &sdkproto.FileUpload_DataUpload{DataUpload: upload},
|
||||
|
||||
@@ -48,7 +48,8 @@ func TestUploadFileLargeModuleFiles(t *testing.T) {
|
||||
require.NoError(t, err)
|
||||
|
||||
// Convert to upload format
|
||||
upload, chunks := sdkproto.BytesToDataUpload(sdkproto.DataUploadType_UPLOAD_TYPE_MODULE_FILES, moduleData)
|
||||
upload, chunks, err := sdkproto.BytesToDataUpload(sdkproto.DataUploadType_UPLOAD_TYPE_MODULE_FILES, moduleData)
|
||||
require.NoError(t, err)
|
||||
|
||||
stream := newMockUploadStream(upload, chunks...)
|
||||
|
||||
@@ -93,7 +94,8 @@ func TestUploadFileErrorScenarios(t *testing.T) {
|
||||
_, err := crand.Read(moduleData)
|
||||
require.NoError(t, err)
|
||||
|
||||
upload, chunks := sdkproto.BytesToDataUpload(sdkproto.DataUploadType_UPLOAD_TYPE_MODULE_FILES, moduleData)
|
||||
upload, chunks, err := sdkproto.BytesToDataUpload(sdkproto.DataUploadType_UPLOAD_TYPE_MODULE_FILES, moduleData)
|
||||
require.NoError(t, err)
|
||||
|
||||
t.Run("chunk_before_upload", func(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
@@ -533,7 +533,10 @@ func (p *Server) UploadModuleFiles(ctx context.Context, moduleFiles []byte) erro
|
||||
}
|
||||
defer stream.Close()
|
||||
|
||||
dataUp, chunks := sdkproto.BytesToDataUpload(sdkproto.DataUploadType_UPLOAD_TYPE_MODULE_FILES, moduleFiles)
|
||||
dataUp, chunks, err := sdkproto.BytesToDataUpload(sdkproto.DataUploadType_UPLOAD_TYPE_MODULE_FILES, moduleFiles)
|
||||
if err != nil {
|
||||
return nil, xerrors.Errorf("prepare module files upload: %w", err)
|
||||
}
|
||||
|
||||
err = stream.Send(&sdkproto.FileUpload{Type: &sdkproto.FileUpload_DataUpload{DataUpload: dataUp}})
|
||||
if err != nil {
|
||||
|
||||
@@ -19,14 +19,17 @@ func (r *Runner) init(ctx context.Context, omitModules bool, templateArchive []b
|
||||
// If `moduleTar` is populated, `init` will send it over in multiple parts. This
|
||||
// It must be called before the initial request to populate the correct hash if
|
||||
// there is data to send. This is safe to call on nil or empty slices.
|
||||
data, chunks := sdkproto.BytesToDataUpload(sdkproto.DataUploadType_UPLOAD_TYPE_MODULE_FILES, moduleTar)
|
||||
data, chunks, err := sdkproto.BytesToDataUpload(sdkproto.DataUploadType_UPLOAD_TYPE_MODULE_FILES, moduleTar)
|
||||
if err != nil {
|
||||
return nil, r.failedJobf("prepare module files upload: %v", err)
|
||||
}
|
||||
|
||||
hash := []byte{}
|
||||
if len(moduleTar) > 0 {
|
||||
hash = data.DataHash
|
||||
}
|
||||
|
||||
err := r.session.Send(&sdkproto.Request{Type: &sdkproto.Request_Init{Init: &sdkproto.InitRequest{
|
||||
err = r.session.Send(&sdkproto.Request{Type: &sdkproto.Request_Init{Init: &sdkproto.InitRequest{
|
||||
TemplateSourceArchive: templateArchive,
|
||||
OmitModuleFiles: omitModules,
|
||||
InitialModuleTarHash: hash,
|
||||
|
||||
Generated
+24
-4
@@ -9,7 +9,8 @@ import (
|
||||
)
|
||||
|
||||
const (
|
||||
ChunkSize = 2 << 20 // 2 MiB
|
||||
ChunkSize = 2 << 20 // 2 MiB
|
||||
MaxFileSize = 10 * (10 << 20) // 100 MiB, matches coderd HTTPFileMaxBytes
|
||||
)
|
||||
|
||||
type DataBuilder struct {
|
||||
@@ -29,6 +30,21 @@ func NewDataBuilder(req *DataUpload) (*DataBuilder, error) {
|
||||
return nil, xerrors.Errorf("data hash must be 32 bytes, got %d bytes", len(req.DataHash))
|
||||
}
|
||||
|
||||
if req.FileSize < 0 {
|
||||
return nil, xerrors.Errorf("file size must not be negative, got %d", req.FileSize)
|
||||
}
|
||||
if req.FileSize > MaxFileSize {
|
||||
return nil, xerrors.Errorf("file size %d exceeds maximum allowed %d", req.FileSize, MaxFileSize)
|
||||
}
|
||||
if req.Chunks < 0 {
|
||||
return nil, xerrors.Errorf("chunk count must not be negative, got %d", req.Chunks)
|
||||
}
|
||||
//nolint:gosec // FileSize is validated to be <= MaxFileSize, well within int32 range
|
||||
maxChunks := int32((req.FileSize + ChunkSize - 1) / ChunkSize)
|
||||
if req.Chunks > maxChunks {
|
||||
return nil, xerrors.Errorf("chunk count %d exceeds maximum %d for file size %d", req.Chunks, maxChunks, req.FileSize)
|
||||
}
|
||||
|
||||
return &DataBuilder{
|
||||
Type: req.UploadType,
|
||||
Hash: req.DataHash,
|
||||
@@ -60,7 +76,7 @@ func (b *DataBuilder) Add(chunk *ChunkPiece) (bool, error) {
|
||||
expectedSize := len(b.data) + len(chunk.Data)
|
||||
if expectedSize > int(b.Size) {
|
||||
return b.done(), xerrors.Errorf("data exceeds expected size, data is now %d bytes, %d bytes over the limit of %d",
|
||||
expectedSize, b.Size-int64(expectedSize), b.Size)
|
||||
expectedSize, int64(expectedSize)-b.Size, b.Size)
|
||||
}
|
||||
|
||||
b.data = append(b.data, chunk.Data...)
|
||||
@@ -103,7 +119,11 @@ func (b *DataBuilder) done() bool {
|
||||
return b.chunkIndex >= b.ChunkCount
|
||||
}
|
||||
|
||||
func BytesToDataUpload(dataType DataUploadType, data []byte) (*DataUpload, []*ChunkPiece) {
|
||||
func BytesToDataUpload(dataType DataUploadType, data []byte) (*DataUpload, []*ChunkPiece, error) {
|
||||
if int64(len(data)) > MaxFileSize {
|
||||
return nil, nil, xerrors.Errorf("data size %d exceeds maximum allowed %d", len(data), MaxFileSize)
|
||||
}
|
||||
|
||||
fullHash := sha256.Sum256(data)
|
||||
//nolint:gosec // not going over int32
|
||||
size := int32(len(data))
|
||||
@@ -135,5 +155,5 @@ func BytesToDataUpload(dataType DataUploadType, data []byte) (*DataUpload, []*Ch
|
||||
chunks = append(chunks, chunk)
|
||||
}
|
||||
|
||||
return req, chunks
|
||||
return req, chunks, nil
|
||||
}
|
||||
|
||||
Generated
+104
-2
@@ -2,6 +2,7 @@ package proto_test
|
||||
|
||||
import (
|
||||
crand "crypto/rand"
|
||||
"crypto/sha256"
|
||||
"math/rand"
|
||||
"testing"
|
||||
|
||||
@@ -10,6 +11,101 @@ import (
|
||||
"github.com/coder/coder/v2/provisionersdk/proto"
|
||||
)
|
||||
|
||||
func TestNewDataBuilderValidation(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
validHash := sha256.Sum256([]byte{})
|
||||
|
||||
t.Run("ExactMaxFileSize", func(t *testing.T) {
|
||||
t.Parallel()
|
||||
builder, err := proto.NewDataBuilder(&proto.DataUpload{
|
||||
DataHash: validHash[:],
|
||||
FileSize: proto.MaxFileSize,
|
||||
Chunks: int32((proto.MaxFileSize + proto.ChunkSize - 1) / proto.ChunkSize),
|
||||
UploadType: proto.DataUploadType_UPLOAD_TYPE_MODULE_FILES,
|
||||
})
|
||||
require.NoError(t, err)
|
||||
require.NotNil(t, builder)
|
||||
})
|
||||
|
||||
t.Run("OversizedFileSize", func(t *testing.T) {
|
||||
t.Parallel()
|
||||
_, err := proto.NewDataBuilder(&proto.DataUpload{
|
||||
DataHash: validHash[:],
|
||||
FileSize: proto.MaxFileSize + 1,
|
||||
Chunks: 1,
|
||||
UploadType: proto.DataUploadType_UPLOAD_TYPE_MODULE_FILES,
|
||||
})
|
||||
require.ErrorContains(t, err, "exceeds maximum allowed")
|
||||
})
|
||||
|
||||
t.Run("NegativeFileSize", func(t *testing.T) {
|
||||
t.Parallel()
|
||||
_, err := proto.NewDataBuilder(&proto.DataUpload{
|
||||
DataHash: validHash[:],
|
||||
FileSize: -1,
|
||||
Chunks: 1,
|
||||
UploadType: proto.DataUploadType_UPLOAD_TYPE_MODULE_FILES,
|
||||
})
|
||||
require.ErrorContains(t, err, "must not be negative")
|
||||
})
|
||||
|
||||
t.Run("NegativeChunks", func(t *testing.T) {
|
||||
t.Parallel()
|
||||
_, err := proto.NewDataBuilder(&proto.DataUpload{
|
||||
DataHash: validHash[:],
|
||||
FileSize: 100,
|
||||
Chunks: -1,
|
||||
UploadType: proto.DataUploadType_UPLOAD_TYPE_MODULE_FILES,
|
||||
})
|
||||
require.ErrorContains(t, err, "chunk count must not be negative")
|
||||
})
|
||||
|
||||
t.Run("ExcessiveChunkCount", func(t *testing.T) {
|
||||
t.Parallel()
|
||||
_, err := proto.NewDataBuilder(&proto.DataUpload{
|
||||
DataHash: validHash[:],
|
||||
FileSize: 100,
|
||||
Chunks: 1000,
|
||||
UploadType: proto.DataUploadType_UPLOAD_TYPE_MODULE_FILES,
|
||||
})
|
||||
require.ErrorContains(t, err, "chunk count 1000 exceeds maximum")
|
||||
})
|
||||
|
||||
t.Run("ZeroFileSize", func(t *testing.T) {
|
||||
t.Parallel()
|
||||
builder, err := proto.NewDataBuilder(&proto.DataUpload{
|
||||
DataHash: validHash[:],
|
||||
FileSize: 0,
|
||||
Chunks: 0,
|
||||
UploadType: proto.DataUploadType_UPLOAD_TYPE_MODULE_FILES,
|
||||
})
|
||||
require.NoError(t, err)
|
||||
require.True(t, builder.IsDone(), "zero-chunk upload should be immediately done")
|
||||
})
|
||||
|
||||
t.Run("ValidRoundTrip", func(t *testing.T) {
|
||||
t.Parallel()
|
||||
data := make([]byte, 256)
|
||||
_, _ = crand.Read(data)
|
||||
|
||||
first, chunks, err := proto.BytesToDataUpload(proto.DataUploadType_UPLOAD_TYPE_MODULE_FILES, data)
|
||||
require.NoError(t, err)
|
||||
|
||||
builder, err := proto.NewDataBuilder(first)
|
||||
require.NoError(t, err)
|
||||
|
||||
for _, chunk := range chunks {
|
||||
_, err = builder.Add(chunk)
|
||||
require.NoError(t, err)
|
||||
}
|
||||
|
||||
got, err := builder.Complete()
|
||||
require.NoError(t, err)
|
||||
require.Equal(t, data, got)
|
||||
})
|
||||
}
|
||||
|
||||
// Fuzz must be run manually with the `-fuzz` flag to generate random test cases.
|
||||
// By default, it only runs the added seed corpus cases.
|
||||
// go test -fuzz=FuzzBytesToDataUpload
|
||||
@@ -25,7 +121,11 @@ func FuzzBytesToDataUpload(f *testing.F) {
|
||||
}
|
||||
|
||||
f.Fuzz(func(t *testing.T, data []byte) {
|
||||
first, chunks := proto.BytesToDataUpload(proto.DataUploadType_UPLOAD_TYPE_MODULE_FILES, data)
|
||||
first, chunks, err := proto.BytesToDataUpload(proto.DataUploadType_UPLOAD_TYPE_MODULE_FILES, data)
|
||||
if err != nil {
|
||||
// Data exceeds MaxFileSize, which is expected for large fuzz inputs.
|
||||
return
|
||||
}
|
||||
|
||||
builder, err := proto.NewDataBuilder(first)
|
||||
require.NoError(t, err)
|
||||
@@ -62,7 +162,9 @@ func TestBytesToDataUpload(t *testing.T) {
|
||||
_, err := crand.Read(data)
|
||||
require.NoError(t, err)
|
||||
|
||||
first, chunks := proto.BytesToDataUpload(proto.DataUploadType_UPLOAD_TYPE_MODULE_FILES, data)
|
||||
first, chunks, err := proto.BytesToDataUpload(proto.DataUploadType_UPLOAD_TYPE_MODULE_FILES, data)
|
||||
require.NoError(t, err)
|
||||
|
||||
builder, err := proto.NewDataBuilder(first)
|
||||
require.NoError(t, err)
|
||||
|
||||
|
||||
+16
-12
@@ -246,24 +246,28 @@ func (s *Session) handleInitRequest(init *proto.InitRequest, requests <-chan *pr
|
||||
s.Logger.Info(s.Context(), "plan response too large, sending modules as stream",
|
||||
slog.F("size_bytes", len(complete.ModuleFiles)),
|
||||
)
|
||||
dataUp, chunks := proto.BytesToDataUpload(proto.DataUploadType_UPLOAD_TYPE_MODULE_FILES, complete.ModuleFiles)
|
||||
|
||||
complete.ModuleFiles = nil // sent over the stream
|
||||
complete.ModuleFilesHash = dataUp.DataHash
|
||||
|
||||
err := s.stream.Send(&proto.Response{Type: &proto.Response_DataUpload{DataUpload: dataUp}})
|
||||
dataUp, chunks, err := proto.BytesToDataUpload(proto.DataUploadType_UPLOAD_TYPE_MODULE_FILES, complete.ModuleFiles)
|
||||
if err != nil {
|
||||
complete.Error = fmt.Sprintf("send data upload: %s", err.Error())
|
||||
complete.Error = fmt.Sprintf("prepare module files upload: %s", err.Error())
|
||||
} else {
|
||||
for i, chunk := range chunks {
|
||||
err := s.stream.Send(&proto.Response{Type: &proto.Response_ChunkPiece{ChunkPiece: chunk}})
|
||||
if err != nil {
|
||||
complete.Error = fmt.Sprintf("send data piece upload %d/%d: %s", i, dataUp.Chunks, err.Error())
|
||||
break
|
||||
complete.ModuleFiles = nil // sent over the stream
|
||||
complete.ModuleFilesHash = dataUp.DataHash
|
||||
|
||||
err := s.stream.Send(&proto.Response{Type: &proto.Response_DataUpload{DataUpload: dataUp}})
|
||||
if err != nil {
|
||||
complete.Error = fmt.Sprintf("send data upload: %s", err.Error())
|
||||
} else {
|
||||
for i, chunk := range chunks {
|
||||
err := s.stream.Send(&proto.Response{Type: &proto.Response_ChunkPiece{ChunkPiece: chunk}})
|
||||
if err != nil {
|
||||
complete.Error = fmt.Sprintf("send data piece upload %d/%d: %s", i, dataUp.Chunks, err.Error())
|
||||
break
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
s.initialized = true
|
||||
|
||||
return complete, nil
|
||||
|
||||
Reference in New Issue
Block a user