From a2e1ddb56f4d153fff5cf5b834d6a1b252abb231 Mon Sep 17 00:00:00 2001 From: Garrett Delfosse Date: Wed, 27 May 2026 14:30:11 -0400 Subject: [PATCH] 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
Implementation details - `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.
> Generated by Coder Agents on behalf of @f0ssel --- .../provisionerdserver/provisionerdserver.go | 5 +- coderd/provisionerdserver/upload_file_test.go | 6 +- provisionerd/provisionerd.go | 5 +- provisionerd/runner/init.go | 7 +- provisionersdk/proto/dataupload.go | 28 ++++- provisionersdk/proto/dataupload_test.go | 106 +++++++++++++++++- provisionersdk/session.go | 28 +++-- 7 files changed, 161 insertions(+), 24 deletions(-) diff --git a/coderd/provisionerdserver/provisionerdserver.go b/coderd/provisionerdserver/provisionerdserver.go index 5a52ecc0a1..1c66333aef 100644 --- a/coderd/provisionerdserver/provisionerdserver.go +++ b/coderd/provisionerdserver/provisionerdserver.go @@ -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}, diff --git a/coderd/provisionerdserver/upload_file_test.go b/coderd/provisionerdserver/upload_file_test.go index d041bb9f98..f235095742 100644 --- a/coderd/provisionerdserver/upload_file_test.go +++ b/coderd/provisionerdserver/upload_file_test.go @@ -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() diff --git a/provisionerd/provisionerd.go b/provisionerd/provisionerd.go index 769bdb8446..2cbdb6eabd 100644 --- a/provisionerd/provisionerd.go +++ b/provisionerd/provisionerd.go @@ -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 { diff --git a/provisionerd/runner/init.go b/provisionerd/runner/init.go index 45c762b7fa..13a8c5066a 100644 --- a/provisionerd/runner/init.go +++ b/provisionerd/runner/init.go @@ -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, diff --git a/provisionersdk/proto/dataupload.go b/provisionersdk/proto/dataupload.go index e9b6d9ddfb..f8832d3616 100644 --- a/provisionersdk/proto/dataupload.go +++ b/provisionersdk/proto/dataupload.go @@ -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 } diff --git a/provisionersdk/proto/dataupload_test.go b/provisionersdk/proto/dataupload_test.go index 496a7956c9..d8876240b0 100644 --- a/provisionersdk/proto/dataupload_test.go +++ b/provisionersdk/proto/dataupload_test.go @@ -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) diff --git a/provisionersdk/session.go b/provisionersdk/session.go index 094fe38aba..543fdd3a51 100644 --- a/provisionersdk/session.go +++ b/provisionersdk/session.go @@ -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