diff --git a/CHANGELOG.md b/CHANGELOG.md index cebf526fc..fda79c75f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -91,6 +91,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm - Close schema files opened by `ParseFile` in `go.opentelemetry.io/otel/schema/v1.0` and `go.opentelemetry.io/otel/schema/v1.1`. ([GHSA-995v-fvrw-c78m](https://github.com/open-telemetry/opentelemetry-go/security/advisories/GHSA-995v-fvrw-c78m)) - Enforce the 8192-byte baggage size limit during extraction/parsing, changing behavior when the limit is exceeded in `go.opentelemetry.io/otel/baggage` and `go.opentelemetry.io/otel/propagation`. (#8222) - Fix `go.opentelemetry.io/otel/semconv/v1.41.0` to include `Attr*` helper methods for required attributes on observable instruments. (#8361) +- Limit baggage extraction error reporting in `go.opentelemetry.io/otel/propagation` to prevent malformed or oversized baggage headers from flooding logs. ([GHSA-5wrp-cwcj-q835](https://github.com/open-telemetry/opentelemetry-go/security/advisories/GHSA-5wrp-cwcj-q835)) diff --git a/propagation/baggage.go b/propagation/baggage.go index afa5f4541..d81b709a2 100644 --- a/propagation/baggage.go +++ b/propagation/baggage.go @@ -7,6 +7,7 @@ import ( "context" "errors" "fmt" + "sync" "go.opentelemetry.io/otel/baggage" "go.opentelemetry.io/otel/internal/errorhandler" @@ -23,6 +24,10 @@ const ( maxBytesPerBaggageString = 8192 ) +// handleExtractErrOnce limits error reporting for attacker-controlled baggage headers +// to one process-wide emission, preventing repeated extraction from flooding logs. +var handleExtractErrOnce sync.Once + // Baggage is a propagator that supports the W3C Baggage format. // // This propagates user-defined baggage associated with a trace. The complete @@ -62,7 +67,9 @@ func extractSingleBaggage(parent context.Context, carrier TextMapCarrier) contex bag, err := baggage.Parse(bStr) if err != nil { - errorhandler.GetErrorHandler().Handle(err) + handleExtractErrOnce.Do(func() { + errorhandler.GetErrorHandler().Handle(err) + }) } if bag.Len() == 0 { return parent @@ -91,11 +98,13 @@ func extractMultiBaggage(parent context.Context, carrier ValuesGetter) context.C // individually. Mirror the single-header behavior of // reporting the error and returning the parent context // with no baggage attached. - errorhandler.GetErrorHandler().Handle(fmt.Errorf( - "baggage: aggregate header size %d exceeds %d byte limit", - totalBytes, - maxBytesPerBaggageString, - )) + handleExtractErrOnce.Do(func() { + errorhandler.GetErrorHandler().Handle(fmt.Errorf( + "baggage: aggregate header size %d exceeds %d byte limit", + totalBytes, + maxBytesPerBaggageString, + )) + }) return parent } @@ -124,7 +133,9 @@ func extractMultiBaggage(parent context.Context, carrier ValuesGetter) context.C truncateErr = errors.Join(truncateErr, err) } if truncateErr != nil { - errorhandler.GetErrorHandler().Handle(truncateErr) + handleExtractErrOnce.Do(func() { + errorhandler.GetErrorHandler().Handle(truncateErr) + }) } if b.Len() == 0 { diff --git a/propagation/baggage_test.go b/propagation/baggage_test.go index 0cc724676..8deee6800 100644 --- a/propagation/baggage_test.go +++ b/propagation/baggage_test.go @@ -539,10 +539,29 @@ func TestExtractOversizedSingleBaggageHeader(t *testing.T) { } type errHandler struct { - err error + err error + count int } -func (e *errHandler) Handle(err error) { e.err = err } +func (e *errHandler) Handle(err error) { + e.err = err + e.count++ +} + +func setBaggageErrHandler(t *testing.T) *errHandler { + t.Helper() + + propagation.ResetHandleExtractErrOnce() + originalErrorHandler := otel.GetErrorHandler() + eh := &errHandler{} + otel.SetErrorHandler(eh) + t.Cleanup(func() { + otel.SetErrorHandler(originalErrorHandler) + propagation.ResetHandleExtractErrOnce() + }) + + return eh +} func TestExtractManyBaggageHeader(t *testing.T) { tests := []struct { @@ -619,10 +638,7 @@ func TestExtractManyBaggageHeader(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - originalErrorHandler := otel.GetErrorHandler() - eh := &errHandler{} - otel.SetErrorHandler(eh) - t.Cleanup(func() { otel.SetErrorHandler(originalErrorHandler) }) + eh := setBaggageErrHandler(t) prop := propagation.Baggage{} req, _ := http.NewRequestWithContext(t.Context(), http.MethodGet, "http://example.com", http.NoBody) @@ -634,9 +650,66 @@ func TestExtractManyBaggageHeader(t *testing.T) { got := baggage.FromContext(ctx) assert.Equal(t, tt.want().Baggage(t), got) - assert.Error(t, eh.err) - for _, s := range tt.wantErrStr { - assert.Contains(t, eh.err.Error(), s) + if assert.Error(t, eh.err) { + for _, s := range tt.wantErrStr { + assert.Contains(t, eh.err.Error(), s) + } + } + }) + } +} + +func TestExtractInvalidBaggageReportsErrorOnce(t *testing.T) { + tests := []struct { + name string + carrier func(t *testing.T) propagation.TextMapCarrier + wantErr string + }{ + { + name: "single header parse error", + carrier: func(t *testing.T) propagation.TextMapCarrier { + t.Helper() + return propagation.MapCarrier{"baggage": "invalid"} + }, + wantErr: "invalid baggage list-member", + }, + { + name: "multiple header parse errors", + carrier: func(t *testing.T) propagation.TextMapCarrier { + t.Helper() + req, _ := http.NewRequestWithContext(t.Context(), http.MethodGet, "http://example.com", http.NoBody) + req.Header.Add("baggage", "invalid") + return propagation.HeaderCarrier(req.Header) + }, + wantErr: "invalid baggage list-member", + }, + { + name: "aggregate header size error", + carrier: func(t *testing.T) propagation.TextMapCarrier { + t.Helper() + req, _ := http.NewRequestWithContext(t.Context(), http.MethodGet, "http://example.com", http.NoBody) + req.Header.Add("baggage", "k="+strings.Repeat("v", maxBytesPerBaggageString/2-2)) + req.Header.Add("baggage", "y="+strings.Repeat("v", maxBytesPerBaggageString/2-2)) + return propagation.HeaderCarrier(req.Header) + }, + wantErr: "exceeds 8192 byte limit", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + eh := setBaggageErrHandler(t) + prop := propagation.Baggage{} + + for range 10 { + ctx := prop.Extract(t.Context(), tt.carrier(t)) + got := baggage.FromContext(ctx) + assert.Equal(t, 0, got.Len(), "invalid header should result in empty baggage") + } + + assert.Equal(t, 1, eh.count, "invalid baggage extraction should report only once") + if assert.Error(t, eh.err) { + assert.Contains(t, eh.err.Error(), tt.wantErr) } }) } diff --git a/propagation/export_test.go b/propagation/export_test.go new file mode 100644 index 000000000..45b43eddd --- /dev/null +++ b/propagation/export_test.go @@ -0,0 +1,11 @@ +// Copyright The OpenTelemetry Authors +// SPDX-License-Identifier: Apache-2.0 + +package propagation + +import "sync" + +// ResetHandleExtractErrOnce resets handleExtractErrOnce for tests. +func ResetHandleExtractErrOnce() { + handleExtractErrOnce = sync.Once{} +}