Information disclosure / Access control

PoC (exploit before fix, and behavior after fix): Prerequisites: - VictoriaMetrics deployed with retentionPeriod (e.g., 30 days) and futureRetention (e.g., 30 days). - -denyQueriesOutsideRetention enabled. - Access to vmselect/victoriametrics HTTP API (e.g., localhost:8428). 1) Reproduce behavior before the fix (potential vulnerability): - Issue a query_range against the API that requests data partially outside the configured retention window, for example querying from 60 days ago to now when retention is 30 days and futureRetention is also 30 days. - Command (example): curl -sS "http://localhost:8428/api/v1/query_range?query=up&start=$(date -u -d '60 days ago' +%s)&end=$(date -u +%s)&step=3600" | jq - Expected (pre-fix): The server could return data spanning outside the retention window, potentially exposing data beyond what should be accessible. Depending on the exact path exercised, some responses might bypass retention checks. 2) Reproduce behavior after the fix (should be blocked): - Use the same query as above. - Expected: HTTP 400 Bad Request with an error indicating the time range is outside the allowed -retentionPeriod/-futureRetention bounds. - Example response body (illustrative): { "error": "the given time range %s is outside the allowed -retentionPeriod=%s, -futureRetention=%s according to -denyQueriesOutsideRetention" } Notes: - The PoC demonstrates the intended protection: queries that span outside both retention and future retention boundaries are rejected with a 400 status, preventing potential information disclosure across retention boundaries. - Exact error payloads may vary depending on the server’s error formatting, but the HTTP status should be 400 for violations post-fix.

diff --git a/app/vmselect/netstorage/netstorage.go b/app/vmselect/netstorage/netstorage.go index 4a31c05a9dfc5..c89236bf4bf7a 100644 --- a/app/vmselect/netstorage/netstorage.go +++ b/app/vmselect/netstorage/netstorage.go @@ -990,9 +990,6 @@ func ExportBlocks(qt *querytracer.Tracer, sq *storage.SearchQuery, deadline sear return fmt.Errorf("timeout exceeded before starting data export: %s", deadline.String()) } tr := sq.GetTimeRange() - if err := vmstorage.CheckTimeRange(tr); err != nil { - return err - } tfss, err := setupTfss(qt, tr, sq.TagFilterss, sq.MaxMetrics, deadline) if err != nil { return err @@ -1098,9 +1095,6 @@ func SearchMetricNames(qt *querytracer.Tracer, sq *storage.SearchQuery, deadline // Setup search. tr := sq.GetTimeRange() - if err := vmstorage.CheckTimeRange(tr); err != nil { - return nil, err - } tfss, err := setupTfss(qt, tr, sq.TagFilterss, sq.MaxMetrics, deadline) if err != nil { return nil, err @@ -1127,9 +1121,6 @@ func ProcessSearchQuery(qt *querytracer.Tracer, sq *storage.SearchQuery, deadlin // Setup search. tr := sq.GetTimeRange() - if err := vmstorage.CheckTimeRange(tr); err != nil { - return nil, err - } tfss, err := setupTfss(qt, tr, sq.TagFilterss, sq.MaxMetrics, deadline) if err != nil { return nil, err diff --git a/app/vmstorage/main.go b/app/vmstorage/main.go index 8a0d12cc98813..3cd432b095c66 100644 --- a/app/vmstorage/main.go +++ b/app/vmstorage/main.go @@ -56,8 +56,8 @@ var ( logNewSeries = flag.Bool("logNewSeries", false, "Whether to log new series. This option is for debug purposes only. It can lead to performance issues "+ "when big number of new series are ingested into VictoriaMetrics") - denyQueriesOutsideRetention = flag.Bool("denyQueriesOutsideRetention", false, "Whether to deny queries outside the configured -retentionPeriod. "+ - "When set, then /api/v1/query_range would return '503 Service Unavailable' error for queries with 'from' value outside -retentionPeriod. "+ + denyQueriesOutsideRetention = flag.Bool("denyQueriesOutsideRetention", false, "Whether to deny queries outside the configured -retentionPeriod and -futureRetention. "+ + "When set, then /api/v1/query_range will return an error for queries with 'from' value outside -retentionPeriod or 'to' value beyond -futureRetention. "+ "This may be useful when multiple data sources with distinct retentions are hidden behind query-tee") maxHourlySeries = flag.Int64("storage.maxHourlySeries", 0, "The maximum number of unique series can be added to the storage during the last hour. "+ "Excess series are logged and dropped. This can be useful for limiting series cardinality. See https://docs.victoriametrics.com/victoriametrics/single-server-victoriametrics/#cardinality-limiter . "+ @@ -103,21 +103,6 @@ var ( "If set to 0 or a negative value, defaults to 1% of allowed memory.") ) -// CheckTimeRange returns true if the given tr is denied for querying. -func CheckTimeRange(tr storage.TimeRange) error { - if !*denyQueriesOutsideRetention { - return nil - } - minAllowedTimestamp := int64(fasttime.UnixTimestamp()*1000) - retentionPeriod.Milliseconds() - if tr.MinTimestamp > minAllowedTimestamp { - return nil - } - return &httpserver.ErrorWithStatusCode{ - Err: fmt.Errorf("the given time range %s is outside the allowed -retentionPeriod=%s according to -denyQueriesOutsideRetention", &tr, retentionPeriod), - StatusCode: http.StatusServiceUnavailable, - } -} - // Init initializes vmstorage. func Init(resetCacheIfNeeded func(mrs []storage.MetricRow)) { if err := encoding.CheckPrecisionBits(uint8(*precisionBits)); err != nil { @@ -151,14 +136,15 @@ func Init(resetCacheIfNeeded func(mrs []storage.MetricRow)) { startTime := time.Now() WG = syncwg.WaitGroup{} opts := storage.OpenOptions{ - Retention: retentionPeriod.Duration(), - FutureRetention: futureRetention.Duration(), - MaxHourlySeries: getMaxHourlySeries(), - MaxDailySeries: getMaxDailySeries(), - DisablePerDayIndex: *disablePerDayIndex, - TrackMetricNamesStats: *trackMetricNamesStats, - IDBPrefillStart: *idbPrefillStart, - LogNewSeries: *logNewSeries, + Retention: retentionPeriod.Duration(), + FutureRetention: futureRetention.Duration(), + DenyQueriesOutsideRetention: *denyQueriesOutsideRetention, + MaxHourlySeries: getMaxHourlySeries(), + MaxDailySeries: getMaxDailySeries(), + DisablePerDayIndex: *disablePerDayIndex, + TrackMetricNamesStats: *trackMetricNamesStats, + IDBPrefillStart: *idbPrefillStart, + LogNewSeries: *logNewSeries, } strg := storage.MustOpenStorage(*DataPath, opts) Storage = strg diff --git a/lib/storage/search_test.go b/lib/storage/search_test.go index c287db9be0e82..70a333b7f46bb 100644 --- a/lib/storage/search_test.go +++ b/lib/storage/search_test.go @@ -228,6 +228,7 @@ func testAssertSearchResult(st *Storage, tr TimeRange, tfs *TagFilters, want []M var s Search s.Init(nil, st, []*TagFilters{tfs}, tr, 1e5, noDeadline) + defer s.MustClose() var mbs []metricBlock for s.NextMetricBlock() { var b Block @@ -241,7 +242,6 @@ func testAssertSearchResult(st *Storage, tr TimeRange, tfs *TagFilters, want []M if err := s.Error(); err != nil { return fmt.Errorf("search error: %w", err) } - s.MustClose() var got []MetricRow var mn MetricName diff --git a/lib/storage/storage.go b/lib/storage/storage.go index 277e18c355f08..03837b098d02b 100644 --- a/lib/storage/storage.go +++ b/lib/storage/storage.go @@ -61,10 +61,11 @@ type Storage struct { // indexdb rotation. legacyNextRotationTimestamp atomic.Int64 - path string - cachePath string - retentionMsecs int64 - futureRetentionMsecs int64 + path string + cachePath string + retentionMsecs int64 + futureRetentionMsecs int64 + denyQueriesOutsideRetention bool // lock file for exclusive access to the storage on the given path. flockF *os.File @@ -161,14 +162,15 @@ type Storage struct { // OpenOptions optional args for MustOpenStorage type OpenOptions struct { - Retention time.Duration - FutureRetention time.Duration - MaxHourlySeries int - MaxDailySeries int - DisablePerDayIndex bool - TrackMetricNamesStats bool - IDBPrefillStart time.Duration - LogNewSeries bool + Retention time.Duration + FutureRetention time.Duration + DenyQueriesOutsideRetention bool + MaxHourlySeries int + MaxDailySeries int + DisablePerDayIndex bool + TrackMetricNamesStats bool + IDBPrefillStart time.Duration + LogNewSeries bool } // MustOpenStorage opens storage on the given path with the given retentionMsecs. @@ -190,12 +192,13 @@ func MustOpenStorage(path string, opts OpenOptions) *Storage { idbPrefillStart = time.Hour } s := &Storage{ - path: path, - cachePath: filepath.Join(path, cacheDirname), - retentionMsecs: retention.Milliseconds(), - futureRetentionMsecs: futureRetention.Milliseconds(), - stopCh: make(chan struct{}), - idbPrefillStartSeconds: idbPrefillStart.Milliseconds() / 1000, + path: path, + cachePath: filepath.Join(path, cacheDirname), + retentionMsecs: retention.Milliseconds(), + futureRetentionMsecs: futureRetention.Milliseconds(), + denyQueriesOutsideRetention: opts.DenyQueriesOutsideRetention, + stopCh: make(chan struct{}), + idbPrefillStartSeconds: idbPrefillStart.Milliseconds() / 1000, } s.logNewSeries.Store(opts.LogNewSeries) @@ -1225,6 +1228,25 @@ func searchAndMergeUniq(qt *querytracer.Tracer, s *Storage, tr TimeRange, search return res, nil } +// checkTimeRange returns an error if time range is outside the allowed +// -retentionPeriod or -futureRetention window when +// -denyQueriesOutsideRetention flag is set +func (s *Storage) checkTimeRange(tr TimeRange) error { + if !s.denyQueriesOutsideRetention { + return nil + } + + minTimestamp, maxTimestamp := s.tb.getMinMaxTimestamps() + if minTimestamp <= tr.MinTimestamp && tr.MaxTimestamp <= maxTimestamp { + return nil + } + + retention := time.Duration(s.retentionMsecs) * time.Millisecond + futureRetention := time.Duration(s.futureRetentionMsecs) * time.Millisecond + return fmt.Errorf("the given time range %s is outside the allowed -retentionPeriod=%s, -futureRetention=%s "+ + "according to -denyQueriesOutsideRetention", &tr, retention, futureRetention) +} + // SearchTSIDs searches the TSIDs that correspond to filters within the given // time range. // @@ -1236,6 +1258,10 @@ func (s *Storage) SearchTSIDs(qt *querytracer.Tracer, tfss []*TagFilters, tr Tim qt = qt.NewChild("search TSIDs: filters=%s, timeRange=%s, maxMetrics=%d", tfss, &tr, maxMetrics) defer qt.Done() + if err := s.checkTimeRange(tr); err != nil { + return nil, err + } + search := func(qt *querytracer.Tracer, idb *indexDB, tr TimeRange) ([]TSID, error) { return idb.SearchTSIDs(qt, tfss, tr, maxMetrics, deadline) } @@ -1271,6 +1297,9 @@ func (s *Storage) SearchTSIDs(qt *querytracer.Tracer, tfss []*TagFilters, tr Tim // MetricName.UnmarshalString(). func (s *Storage) SearchMetricNames(qt *querytracer.Tracer, tfss []*TagFilters, tr TimeRange, maxMetrics int, deadline uint64) ([]string, error) { qt = qt.NewChild("search metric names: filters=%s, timeRange=%s, maxMetrics: %d", tfss, &tr, maxMetrics) + if err := s.checkTimeRange(tr); err != nil { + return nil, err + } search := func(qt *querytracer.Tracer, idb *indexDB, tr TimeRange) ([]string, error) { return idb.SearchMetricNames(qt, tfss, tr, maxMetrics, deadline) } diff --git a/lib/storage/storage_synctest_test.go b/lib/storage/storage_synctest_test.go index 9a31b0d329b2b..a6cecf6c99c5b 100644 --- a/lib/storage/storage_synctest_test.go +++ b/lib/storage/storage_synctest_test.go @@ -6,6 +6,7 @@ import ( "fmt" "math/rand" "path/filepath" + "slices" "testing" "testing/synctest" "time" @@ -1144,3 +1145,268 @@ func TestStorage_partitionsOutsideRetentionAreRemoved(t *testing.T) { s.MustClose() }) } + +func TestStorage_denyQueriesOutsideRetention(t *testing.T) { + defer testRemoveAll(t) + + tfsAll := NewTagFilters() + if err := tfsAll.Add(nil, []byte(".*"), false, true); err != nil { + t.Fatalf("TagFilters.Add() failed unexpectedly: %v", err) + } + tfssAll := []*TagFilters{tfsAll} + + assertData := func(t *testing.T, s *Storage, tr TimeRange, wantData []MetricRow, wantErr bool) { + t.Helper() + err := testAssertSearchResult(s, tr, tfsAll, wantData) + gotErr := err != nil + if gotErr != wantErr { + t.Fatalf("Search: unmet error expectation for timeRange=%v: got %t, want %t (err: %v)", &tr, gotErr, wantErr, err) + } + } + assertMetricNames := func(t *testing.T, s *Storage, tr TimeRange, wantData []string, wantErr bool) { + t.Helper() + metricNames, err := s.SearchMetricNames(nil, tfssAll, tr, 1e9, noDeadline) + gotErr := err != nil + if gotErr != wantErr { + t.Fatalf("SearchMetricNames(): unmet error expectation for timeRange=%v: got %t, want %t (err: %v)", &tr, gotErr, wantErr, err) + } + var gotData []string + for _, name := range metricNames { + var mn MetricName + if err := mn.UnmarshalString(name); err != nil { + t.Fatalf("Could not unmarshal metric name %q: %v", name, err) + } + gotData = append(gotData, string(mn.MetricGroup)) + } + if diff := cmp.Diff(wantData, gotData); diff != "" { + t.Fatalf("unexpected metric names (-want, +got):\n%s", diff) + } + } + assertLabelNames := func(t *testing.T, s *Storage, tr TimeRange, wantData []string, wantErr bool) { + t.Helper() + gotData, err := s.SearchLabelNames(nil, nil, tr, 1e9, 1e9, noDeadline) + gotErr := err != nil + if gotErr != wantErr { + t.Fatalf("SearchLabelNames(): unmet error expectation for timeRange=%v: got %t, want %t (err: %v)", &tr, gotErr, wantErr, err) + } + slices.Sort(gotData) + slices.Sort(wantData) + if diff := cmp.Diff(wantData, gotData); diff != "" { + t.Fatalf("unexpected label names (-want, +got):\n%s", diff) + } + } + assertLabelValues := func(t *testing.T, s *Storage, tr TimeRange, wantData []string, wantErr bool) { + t.Helper() + gotData, err := s.SearchLabelValues(nil, "__name__", nil, tr, 1e9, 1e9, noDeadline) + gotErr := err != nil + if gotErr != wantErr { + t.Fatalf("SearchLabelValues(): unmet error expectation for timeRange=%v: got %t, want %t (err: %v)", &tr, gotErr, wantErr, err) + } + slices.Sort(gotData) + slices.Sort(wantData) + if diff := cmp.Diff(wantData, gotData); diff != "" { + t.Fatalf("unexpected label values (-want, +got):\n%s", diff) + } + } + assertTagValueSuffixes := func(t *testing.T, s *Storage, tr TimeRange, wantData []string, wantErr bool) { + t.Helper() + gotData, err := s.SearchTagValueSuffixes(nil, tr, "", "", '.', 1e9, noDeadline) + gotErr := err != nil + if gotErr != wantErr { + t.Fatalf("SearchTagValueSuffixes(): unmet error expectation for timeRange=%v: got %t, want %t (err: %v)", &tr, gotErr, wantErr, err) + } + slices.Sort(gotData) + + if diff := cmp.Diff(wantData, gotData); diff != "" { + t.Errorf("unexpected tag value suffixes (-want, +got):\n%s", diff) + } + } + + assertGraphitePaths := func(t *testing.T, s *Storage, tr TimeRange, wantData []string, wantErr bool) { + t.Helper() + gotData, err := s.SearchGraphitePaths(nil, tr, []byte("*"), 1e9, noDeadline) + gotErr := err != nil + if gotErr != wantErr { + t.Fatalf("SearchTagValueSuffixes(): unmet error expectation for timeRange=%v: got %t, want %t (err: %v)", &tr, gotErr, wantErr, err) + } + slices.Sort(gotData) + + if diff := cmp.Diff(wantData, gotData); diff != "" { + t.Errorf("unexpected graphite paths (-want, +got):\n%s", diff) + } + } + + synctest.Test(t, func(t *testing.T) { + // synctests start at 2000-01-01T00:00:00Z + now := time.Now().UTC() + retention := 30 * 24 * time.Hour + futureRetention := 30 * 24 * time.Hour + day := 24 * time.Hour + trMatches := TimeRange{ + MinTimestamp: now.Add(-retention).UnixMilli(), + MaxTimestamp: now.Add(futureRetention).UnixMilli(), + } + trContains := TimeRange{ + MinTimestamp: now.Add(-(retention + day)).UnixMilli(), + MaxTimestamp: now.Add(futureRetention + day).UnixMilli(), + } + trInside := TimeRange{ + MinTimestamp: now.Add(-(retention - day)).UnixMilli(), + MaxTimestamp: now.Add(futureRetention - day).UnixMilli(), + } + trOverlapsLeft := TimeRange{ + MinTimestamp: now.Add(-(retention + day)).UnixMilli(), + MaxTimestamp: now.Add(futureRetention - day).UnixMilli(), + } + trOverlapsRight := TimeRange{ + MinTimestamp: now.Add(-(retention - day)).UnixMilli(), + MaxTimestamp: now.Add(futureRetentio ... [truncated]