Authorization bypass

PoC (demonstrates potential pre-fix behavior vs post-fix): Scenario: An API protected by extra_label/extra_filters policy is accessible via vmselect/vmauth. The policy should be driven by URL query parameters, not POST form values. 1) Before the fix (potential bypass): - Attacker crafts a request that supplies conflicting values in the URL query and the POST form for extra_label and extra_filters[]. - Example (conceptual): curl -X POST "http://victoriametrics.example:8429/select/?extra_label=env=prod&extra_label=tenant=prod&extra_filters[]={tenant=prod}" \ -d 'extra_label=env=dev&extra_label=tenant=dev&extra_filters[]={tenant="dev"}' \ -H 'Content-Type: application/x-www-form-urlencoded' - If the server historically combined both sources without a deterministic precedence, the resulting tag filters could include the unintended dev values, potentially widening access or bypassing policy enforcement. 2) After the fix (deterministic precedence): - The server uses only the URL query parameters for extra_label and extra_filters[] when both sources are present. - The same request would now effectively apply extra_label=env=prod&extra_label=tenant=prod and extra_filters[]={tenant=prod}, ignoring the conflicting form values. Expected observable difference: - Pre-fix: Tag filters could reflect values from the POST body (e.g., env=dev) alongside/overriding URL params, enabling potential policy bypass in some race/order conditions. - Post-fix: Tag filters strictly reflect URL query params, ensuring policy enforced by proxies/vmauth cannot be overridden by crafted form data. Notes: - This PoC is conceptual because exact endpoints/response formats depend on the VictoriaMetrics deployment (vmselect/vmauth). The curl example demonstrates the input vectors that could trigger the behavior and how to observe the difference in resulting tag filters when the server applies the updated precedence rule.

diff --git a/app/vmselect/searchutil/searchutil.go b/app/vmselect/searchutil/searchutil.go index 97a1122ce068b..fd69c6c2f440b 100644 --- a/app/vmselect/searchutil/searchutil.go +++ b/app/vmselect/searchutil/searchutil.go @@ -132,9 +132,20 @@ func (d *Deadline) String() string { // // {env="prod",team="devops",t1="v1",t2="v2"} // {env=~"dev|staging",team!="devops",t1="v1",t2="v2"} +// +// Query args from URL path have precedence over post form args. func GetExtraTagFilters(r *http.Request) ([][]storage.TagFilter, error) { var tagFilters []storage.TagFilter - for _, match := range r.Form["extra_label"] { + urlQueryValues := r.URL.Query() + getRequestParam := func(key string) []string { + // query request param must always take precedence over form values + // in order to simplify security enforcement policy for extra_label and extra_filters + if uv, ok := urlQueryValues[key]; ok { + return uv + } + return r.Form[key] + } + for _, match := range getRequestParam("extra_label") { tmp := strings.SplitN(match, "=", 2) if len(tmp) != 2 { return nil, fmt.Errorf("`extra_label` query arg must have the format `name=value`; got %q", match) @@ -148,8 +159,8 @@ func GetExtraTagFilters(r *http.Request) ([][]storage.TagFilter, error) { Value: []byte(tmp[1]), }) } - extraFilters := append([]string{}, r.Form["extra_filters"]...) - extraFilters = append(extraFilters, r.Form["extra_filters[]"]...) + extraFilters := append([]string{}, getRequestParam("extra_filters")...) + extraFilters = append(extraFilters, getRequestParam("extra_filters[]")...) if len(extraFilters) == 0 { if len(tagFilters) == 0 { return nil, nil diff --git a/app/vmselect/searchutil/searchutil_test.go b/app/vmselect/searchutil/searchutil_test.go index 640e557d9c199..00f061a7a4ca9 100644 --- a/app/vmselect/searchutil/searchutil_test.go +++ b/app/vmselect/searchutil/searchutil_test.go @@ -20,6 +20,7 @@ func TestGetExtraTagFilters(t *testing.T) { } return &http.Request{ Form: q, + URL: &url.URL{RawQuery: q.Encode()}, } } f := func(t *testing.T, r *http.Request, want []string, wantErr bool) { @@ -79,6 +80,24 @@ func TestGetExtraTagFilters(t *testing.T) { nil, false, ) + + formValues, err := url.ParseQuery(`extra_label=env=prod&extra_label=job=vmsingle&extra_label=tenant=prod&extra_filters[]={foo="bar"}&extra_filters[]={tenant="prod"}`) + if err != nil { + t.Fatalf("BUG: cannot parse query: %s", err) + } + urlValues, err := url.ParseQuery(`extra_label=job=vmagent&extra_label=env=dev&extra_filters[]={tenant="dev"}`) + if err != nil { + t.Fatalf("BUG: cannot parse query: %s", err) + } + httpReqWithBothFormAndURLParams := &http.Request{ + Form: formValues, + URL: &url.URL{ + RawQuery: urlValues.Encode(), + }, + } + f(t, httpReqWithBothFormAndURLParams, + []string{`{tenant="dev",job="vmagent",env="dev"}`}, + false) } func TestParseMetricSelectorSuccess(t *testing.T) {