Crash/DoS due to unhandled panic in gRPC handlers

Post-fix proof-of-concept (demonstrates the safe behavior with the recovery interceptors installed): // Minimal demonstration using in-process gRPC channel with panic-recovery interceptors package main import ( "context" "fmt" grpchan "github.com/fullstorydev/grpchan/inprocgrpc" "github.com/grafana/grafana/pkg/services/grpcserver/interceptors" "github.com/grafana/grafana/pkg/storage/unified/resourcepb" ) type panickingResourceStore struct { resourcepb.UnimplementedResourceStoreServer } func (panickingResourceStore) Read(context.Context, *resourcepb.ReadRequest) (*resourcepb.ReadResponse, error) { panic("boom-unary") } func (panickingResourceStore) Watch(*resourcepb.WatchRequest, resourcepb.ResourceStore_WatchServer) error { panic("boom-stream") } func main() { channel := &grpchan.Channel{} desc := grpchan.InterceptServer( &resourcepb.ResourceStore_ServiceDesc, interceptors.UnaryPanicRecoveryInterceptor(), interceptors.StreamPanicRecoveryInterceptor(), ) channel.RegisterService(desc, panickingResourceStore{}) client := resourcepb.NewResourceStoreClient(channel) // Unary call should return an INTERNAL error, not crash the process if _, err := client.Read(context.Background(), &resourcepb.ReadRequest{}); err != nil { fmt.Println("Unary Read error:", err) } // Streaming call would return an error on Recv() as the server panics in the stream handler stream, err := client.Watch(context.Background(), &resourcepb.WatchRequest{}) if err != nil { fmt.Println("Stream open error:", err) return } if _, err = stream.Recv(); err != nil { fmt.Println("Stream Recv error:", err) } } Pre-fix imaginary PoC (for comparison, not recommended to run): // Remove or omit the recovery interceptors and run a similar setup. A panic inside handlers would // crash the handler goroutine and could bring down the server process depending on the panic scope. // This demonstrates the vulnerability path that the fix mitigates. package main import ( "context" "fmt" grpchan "github.com/fullstorydev/grpchan/inprocgrpc" "github.com/grafana/grafana/pkg/storage/unified/resourcepb" ) type panickingResourceStore struct { resourcepb.UnimplementedResourceStoreServer } func (panickingResourceStore) Read(context.Context, *resourcepb.ReadRequest) (*resourcepb.ReadResponse, error) { panic("boom-unary") } func (panickingResourceStore) Watch(*resourcepb.WatchRequest, resourcepb.ResourceStore_WatchServer) error { panic("boom-stream") } func main() { channel := &grpchan.Channel{} desc := &resourcepb.ResourceStore_ServiceDesc channel.RegisterService(desc, panickingResourceStore{}) // No recovery interceptors installed client := resourcepb.NewResourceStoreClient(channel) // This would crash the process or cause a severe failure in a real server _, _ = client.Read(context.Background(), &resourcepb.ReadRequest{}) }

diff --git a/pkg/services/grpcserver/interceptors/recovery.go b/pkg/services/grpcserver/interceptors/recovery.go new file mode 100644 index 0000000000000..0bda427a4bd2b --- /dev/null +++ b/pkg/services/grpcserver/interceptors/recovery.go @@ -0,0 +1,42 @@ +package interceptors + +import ( + "context" + "fmt" + "runtime/debug" + + "github.com/grpc-ecosystem/go-grpc-middleware/v2/interceptors/recovery" + "google.golang.org/grpc" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" + + "github.com/grafana/grafana/pkg/infra/log" +) + +var recoveryLogger = log.New("grpc-server-recovery") + +// recoveryHandler logs the recovered panic with a stack trace and returns a +// generic Internal status. The panic value is not included in the error sent +// to the client to avoid leaking internal details. +func recoveryHandler(ctx context.Context, p any) error { + recoveryLogger.FromContext(ctx).Error( + "recovered from panic in gRPC handler", + "panic", fmt.Sprintf("%v", p), + "stack", string(debug.Stack()), + ) + return status.Errorf(codes.Internal, "internal server error") +} + +// UnaryPanicRecoveryInterceptor returns a unary server interceptor that +// recovers from panics in gRPC handlers. Without it, an unrecovered panic in +// any handler goroutine crashes the entire process (gRPC has no built-in +// handler-level panic recovery). +func UnaryPanicRecoveryInterceptor() grpc.UnaryServerInterceptor { + return recovery.UnaryServerInterceptor(recovery.WithRecoveryHandlerContext(recoveryHandler)) +} + +// StreamPanicRecoveryInterceptor is the streaming counterpart of +// UnaryPanicRecoveryInterceptor. +func StreamPanicRecoveryInterceptor() grpc.StreamServerInterceptor { + return recovery.StreamServerInterceptor(recovery.WithRecoveryHandlerContext(recoveryHandler)) +} diff --git a/pkg/services/grpcserver/service.go b/pkg/services/grpcserver/service.go index 6f90466ce1ea8..1edb5bc9c036c 100644 --- a/pkg/services/grpcserver/service.go +++ b/pkg/services/grpcserver/service.go @@ -76,11 +76,16 @@ func provideService(cfg *setting.Cfg, authenticator interceptors.Authenticator, } } + // Recovery is listed first so it is outermost and catches panics in every + // subsequent interceptor and in the handlers themselves. Without it an + // unrecovered panic crashes the whole process. unaryInterceptors := []grpc.UnaryServerInterceptor{ + interceptors.UnaryPanicRecoveryInterceptor(), interceptors.LoggingUnaryInterceptor(s.logger, s.cfg.EnableLogging), // needs to be registered after tracing interceptor to get trace id middleware.UnaryServerInstrumentInterceptor(grpcRequestDuration), } streamInterceptors := []grpc.StreamServerInterceptor{ + interceptors.StreamPanicRecoveryInterceptor(), interceptors.TracingStreamInterceptor(tracer), interceptors.LoggingStreamInterceptor(s.logger, s.cfg.EnableLogging), middleware.StreamServerInstrumentInterceptor(grpcRequestDuration), diff --git a/pkg/storage/unified/resource/client.go b/pkg/storage/unified/resource/client.go index 8df5ac000d5a7..70845456f196b 100644 --- a/pkg/storage/unified/resource/client.go +++ b/pkg/storage/unified/resource/client.go @@ -14,6 +14,7 @@ import ( "github.com/fullstorydev/grpchan/inprocgrpc" "github.com/go-jose/go-jose/v4" "github.com/go-jose/go-jose/v4/jwt" + grpc_middleware "github.com/grpc-ecosystem/go-grpc-middleware" grpc_retry "github.com/grpc-ecosystem/go-grpc-middleware/retry" grpcAuth "github.com/grpc-ecosystem/go-grpc-middleware/v2/interceptors/auth" "go.opentelemetry.io/otel" @@ -29,6 +30,7 @@ import ( "github.com/grafana/grafana/pkg/infra/log" authnGrpcUtils "github.com/grafana/grafana/pkg/services/authn/grpcutils" "github.com/grafana/grafana/pkg/services/featuremgmt" + "github.com/grafana/grafana/pkg/services/grpcserver/interceptors" "github.com/grafana/grafana/pkg/setting" grpcUtils "github.com/grafana/grafana/pkg/storage/unified/resource/grpc" "github.com/grafana/grafana/pkg/storage/unified/resourcepb" @@ -120,15 +122,24 @@ func NewLocalResourceClient(srv ResourceServer) ResourceClient { &resourcepb.Diagnostics_ServiceDesc, &resourcepb.Quotas_ServiceDesc, } { - wrapped := desc if metricsInt != nil && desc == &resourcepb.ResourceStore_ServiceDesc { - wrapped = grpchan.InterceptServer(wrapped, metricsInt, nil) + desc = grpchan.InterceptServer(desc, metricsInt, nil) } + + // Recovery is listed first so it is outermost and catches panics in auth and the handler. + // The shared grpcserver wires this same interceptor for the remote path; the in-proc + // channel here is its own server, so it needs its own wrap. channel.RegisterService( grpchan.InterceptServer( - wrapped, - grpcAuth.UnaryServerInterceptor(grpcAuthInt), - grpcAuth.StreamServerInterceptor(grpcAuthInt), + desc, + grpc_middleware.ChainUnaryServer( + interceptors.UnaryPanicRecoveryInterceptor(), + grpcAuth.UnaryServerInterceptor(grpcAuthInt), + ), + grpc_middleware.ChainStreamServer( + interceptors.StreamPanicRecoveryInterceptor(), + grpcAuth.StreamServerInterceptor(grpcAuthInt), + ), ), srv, ) diff --git a/pkg/storage/unified/resource/grpc_interceptor_test.go b/pkg/storage/unified/resource/grpc_interceptor_test.go new file mode 100644 index 0000000000000..30939be82b99e --- /dev/null +++ b/pkg/storage/unified/resource/grpc_interceptor_test.go @@ -0,0 +1,59 @@ +package resource + +import ( + "context" + "testing" + + "github.com/fullstorydev/grpchan" + "github.com/fullstorydev/grpchan/inprocgrpc" + "github.com/stretchr/testify/require" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" + + "github.com/grafana/grafana/pkg/services/grpcserver/interceptors" + "github.com/grafana/grafana/pkg/storage/unified/resourcepb" +) + +type panickingResourceStore struct { + resourcepb.UnimplementedResourceStoreServer +} + +func (panickingResourceStore) Read(context.Context, *resourcepb.ReadRequest) (*resourcepb.ReadResponse, error) { + panic("boom-unary") +} + +func (panickingResourceStore) Watch(*resourcepb.WatchRequest, resourcepb.ResourceStore_WatchServer) error { + panic("boom-stream") +} + +func newRecoveryTestClient(t *testing.T) resourcepb.ResourceStoreClient { + t.Helper() + channel := &inprocgrpc.Channel{} + desc := grpchan.InterceptServer( + &resourcepb.ResourceStore_ServiceDesc, + interceptors.UnaryPanicRecoveryInterceptor(), + interceptors.StreamPanicRecoveryInterceptor(), + ) + channel.RegisterService(desc, panickingResourceStore{}) + return resourcepb.NewResourceStoreClient(channel) +} + +func TestPanicRecoveryInterceptor_Unary(t *testing.T) { + client := newRecoveryTestClient(t) + + _, err := client.Read(t.Context(), &resourcepb.ReadRequest{}) + + require.Error(t, err) + require.Equal(t, codes.Internal, status.Code(err), "panic should surface as codes.Internal, got %v", err) +} + +func TestPanicRecoveryInterceptor_Stream(t *testing.T) { + client := newRecoveryTestClient(t) + + stream, err := client.Watch(t.Context(), &resourcepb.WatchRequest{}) + require.NoError(t, err, "opening the stream should succeed; the panic surfaces on Recv") + + _, err = stream.Recv() + require.Error(t, err) + require.Equal(t, codes.Internal, status.Code(err), "panic in stream handler should surface as codes.Internal, got %v", err) +}