Authorization bypass / Privilege escalation (RBAC enforcement on no-op update)

Proof-of-concept (executable steps): Prereqs: - Grafana deployment with RBAC enabled - A resource under a specific group (e.g., playlist.grafana.app) in a namespace (default) - Two users: an admin (can update the resource) and a viewer (read-only/update-denied for that resource) - A resource created by admin with a known ResourceVersion RV_latest 1) Prepare a no-op payload identical to the latest stored value (payload_noop) and note RV_latest. 2) As the viewer, attempt a no-op update using the identical payload: - Key: { Group: "playlist.grafana.app", Resource: "<resource>", Namespace: "default", Name: "<name>" } - Value: payload_noop (identical to current) - ResourceVersion: RV_latest - This uses the same Update API as normal updates. 3) Expected behavior after the patch: - Server responds with a Forbidden error (HTTP 403) indicating the viewer is not allowed to update this resource (no no-op bypass). - If allowed, the response would be an error.Code 403 with a message like "not allowed to update resource"; otherwise, the ResourceVersion should remain RV_latest and no data change should occur. 4) For comparison (pre-fix behavior, illustrative): - Prior to this patch, a no-op update request by a viewer could bypass RBAC checks and the call would complete without enforcing permissions (no effective write, no RBAC enforcement). The exact response could vary, but the absence of a proper RBAC check for no-op paths opened privilege-escalation opportunities. Optional snippet (Go-like pseudo-code for a test client): package main // Assume gRPC client setup and authentication has been done func testNoOpUpdateRBAC(viewerClient resourcepb.ResourceServiceClient, key *resourcepb.ResourceKey, payload []byte, rv uint64) { req := &resourcepb.UpdateRequest{Key: key, Value: payload, ResourceVersion: rv} resp, err := viewerClient.Update(context.Background(), req) if err != nil { // Network/RPC error panic(err) } if resp.Error != nil { // Expected: Forbidden due to RBAC on no-op update println("No-op update rejected as expected: ", resp.Error.Message) } else { // Unexpected: no-op update allowed or not enforcing RBAC println("Unexpected success: no RBAC enforcement on no-op update. RV:", resp.ResourceVersion) } }

diff --git a/pkg/storage/unified/apistore/store.go b/pkg/storage/unified/apistore/store.go index 9b967cba6a3cb..6ff2f56481b30 100644 --- a/pkg/storage/unified/apistore/store.go +++ b/pkg/storage/unified/apistore/store.go @@ -6,7 +6,6 @@ package apistore import ( - "bytes" "context" "errors" "fmt" @@ -610,11 +609,10 @@ func (s *Storage) GuaranteedUpdate( ctx, span := tracer.Start(ctx, "apistore.Storage.GuaranteedUpdate") defer span.End() var ( - res storage.ResponseMeta - updatedObj runtime.Object - existingObj runtime.Object - existingBytes []byte - err error + res storage.ResponseMeta + updatedObj runtime.Object + existingObj runtime.Object + err error ) req := &resourcepb.UpdateRequest{} req.Key, err = s.getKey(key) @@ -673,7 +671,6 @@ func (s *Storage) GuaranteedUpdate( return s.Create(ctx, key, updatedObj, destination, 0) } - existingBytes = readResponse.Value existingObj, err = s.convertToObject(ctx, readResponse.Value, s.newFunc()) if err != nil { return err @@ -706,34 +703,29 @@ func (s *Storage) GuaranteedUpdate( return s.handleManagedResourceRouting(ctx, err, resourcepb.WatchEvent_MODIFIED, key, updatedObj, destination) } - // Only update (for real) if the bytes have changed - var rv uint64 req.Value = v.raw.Bytes() - if !bytes.Equal(req.Value, existingBytes) { - req.ResourceVersion = readResponse.ResourceVersion - updateResponse, err := s.store.Update(ctx, req) - if err != nil { - err = resource.GetError(resource.AsErrorResult(err)) - } else if updateResponse.Error != nil { - if attempt < MaxUpdateAttempts && updateResponse.Error.Code == http.StatusConflict { - continue // try the read again - } - err = resource.GetError(updateResponse.Error) - } - - // Cleanup secure values - if err = v.finish(ctx, err, s.opts.SecureValues); err != nil { - return err + req.ResourceVersion = readResponse.ResourceVersion + updateResponse, err := s.store.Update(ctx, req) // Also does RBAC check + if err != nil { + err = resource.GetError(resource.AsErrorResult(err)) + } else if updateResponse.Error != nil { + if attempt < MaxUpdateAttempts && updateResponse.Error.Code == http.StatusConflict { + continue // try the read again } + err = resource.GetError(updateResponse.Error) + } - rv = uint64(updateResponse.ResourceVersion) + // Cleanup secure values + if err = v.finish(ctx, err, s.opts.SecureValues); err != nil { + return err } if _, err := s.convertToObject(ctx, req.Value, destination); err != nil { return err } - if rv > 0 { + if updateResponse.ResourceVersion > 0 { + rv := uint64(updateResponse.ResourceVersion) if err := s.versioner.UpdateObject(destination, rv); err != nil { return err } diff --git a/pkg/storage/unified/resource/server.go b/pkg/storage/unified/resource/server.go index 8af4deb123006..9dd401e1cd807 100644 --- a/pkg/storage/unified/resource/server.go +++ b/pkg/storage/unified/resource/server.go @@ -1,6 +1,7 @@ package resource import ( + "bytes" "context" "encoding/json" "errors" @@ -1151,6 +1152,32 @@ func (s *server) update(ctx context.Context, user claims.AuthInfo, req *resource return rsp, nil } + // Skip write events when there is no change to the payload, but still enforce RBAC. + if bytes.Equal(req.Value, latest.Value) { + key := req.Key + a, err := s.access.Check(ctx, user, claims.CheckRequest{ + Verb: utils.VerbUpdate, + Group: key.Group, + Resource: key.Resource, + Namespace: key.Namespace, + Name: key.Name, + }, latest.Folder) + if err != nil { + return &resourcepb.UpdateResponse{Error: AsErrorResult(err)}, nil + } + if !a.Allowed { + return &resourcepb.UpdateResponse{ + Error: &resourcepb.ErrorResult{ + Message: "not allowed to update resource", + Code: http.StatusForbidden, + }, + }, nil + } + + rsp.ResourceVersion = latest.ResourceVersion // No change, return the current RV + return rsp, nil + } + event, e := s.newEvent(ctx, user, req.Key, req.ResourceVersion, req.Value, latest.Value) if e != nil { rsp.Error = e diff --git a/pkg/storage/unified/resource/server_test.go b/pkg/storage/unified/resource/server_test.go index d71af5b41a23b..6e604d2b3980f 100644 --- a/pkg/storage/unified/resource/server_test.go +++ b/pkg/storage/unified/resource/server_test.go @@ -480,22 +480,73 @@ func TestSimpleServer(t *testing.T) { }) require.NoError(t, err) - // Update should return a conflict error the second time - + // First update with different bytes advances the resource version. + rawV2 := []byte(strings.Replace(string(raw), `"title": "hello"`, `"title": "world"`, 1)) _, err = server.Update(ctx, &resourcepb.UpdateRequest{ Key: key, - Value: raw, + Value: rawV2, ResourceVersion: created.ResourceVersion}) require.NoError(t, err) + // Second update with stale RV and different bytes should return a conflict. + rawV3 := []byte(strings.Replace(string(raw), `"title": "hello"`, `"title": "again"`, 1)) rsp, err := server.Update(ctx, &resourcepb.UpdateRequest{ Key: key, - Value: raw, + Value: rawV3, ResourceVersion: created.ResourceVersion}) require.NoError(t, err) require.Equal(t, int32(http.StatusConflict), rsp.Error.Code) require.Contains(t, rsp.Error.Message, "requested RV does not match current RV") }) + + t.Run("playlist update with identical bytes does not increment RV", func(t *testing.T) { + raw := []byte(`{ + "apiVersion": "playlist.grafana.app/v0alpha1", + "kind": "Playlist", + "metadata": { + "name": "noop-rv-check", + "namespace": "default", + "uid": "noop-uid" + }, + "spec": { + "title": "hello", + "interval": "5m", + "items": [ + { + "type": "dashboard_by_uid", + "value": "vmie2cmWz" + } + ] + } + }`) + + key := &resourcepb.ResourceKey{ + Group: "playlist.grafana.app", + Resource: "rrrr", + Namespace: "default", + Name: "noop-rv-check", + } + + created, err := server.Create(ctx, &resourcepb.CreateRequest{ + Value: raw, + Key: key, + }) + require.NoError(t, err) + require.Nil(t, created.Error) + + rsp, err := server.Update(ctx, &resourcepb.UpdateRequest{ + Key: key, + Value: raw, + ResourceVersion: created.ResourceVersion}) + require.NoError(t, err) + require.Nil(t, rsp.Error) + require.Equal(t, created.ResourceVersion, rsp.ResourceVersion, "RV should not change when bytes are identical") + + // The stored resource version should also be unchanged. + read := server.backend.ReadResource(ctx, &resourcepb.ReadRequest{Key: key}) + require.Nil(t, read.Error) + require.Equal(t, created.ResourceVersion, read.ResourceVersion) + }) } func TestRunInQueue(t *testing.T) { diff --git a/pkg/tests/apis/folder/folders_test.go b/pkg/tests/apis/folder/folders_test.go index fd279dd340d1d..978a2f9956f3b 100644 --- a/pkg/tests/apis/folder/folders_test.go +++ b/pkg/tests/apis/folder/folders_test.go @@ -326,13 +326,17 @@ func doFolderTests(t *testing.T, helper *apis.K8sTestHelper) *apis.K8sTestHelper require.JSONEq(t, expectedResult, client.SanitizeJSON(found)) }) - t.Run("Do CRUD (just CR+List for now) via k8s (and check that legacy api still works)", func(t *testing.T) { + t.Run("Do CRUD via k8s (and check that legacy api still works)", func(t *testing.T) { client := helper.GetResourceClient(apis.ResourceClientArgs{ - // #TODO: figure out permissions topic User: helper.Org1.Admin, GVR: gvr, }) + clientViewer := helper.GetResourceClient(apis.ResourceClientArgs{ + User: helper.Org1.Viewer, + GVR: gvr, + }) + // Create the folder "test" first, err := client.Resource.Create(context.Background(), helper.LoadYAMLOrJSONFile("testdata/folder-test-create.yaml"), @@ -350,6 +354,16 @@ func doFolderTests(t *testing.T, helper *apis.K8sTestHelper) *apis.K8sTestHelper ) require.NoError(t, err) uids = append(uids, out.GetName()) + + // Update with same body will keep the same RV + again, err := client.Resource.Update(context.Background(), out, metav1.UpdateOptions{}) + require.NoError(t, err) + require.Equal(t, out.GetResourceVersion(), again.GetResourceVersion()) + + // Viewer should not be able to edit the same folder + _, err = clientViewer.Resource.Update(context.Background(), out, metav1.UpdateOptions{}) + require.Error(t, err) + require.True(t, apierrors.IsForbidden(err)) } slices.Sort(uids) // make list compare stable