From fa320b224aa060b17bcfb8951fbfa35969a3b172 Mon Sep 17 00:00:00 2001 From: Easwar Swaminathan Date: Tue, 17 Mar 2026 16:35:32 -0700 Subject: [PATCH] grpc: enforce strict path checking for incoming requests on the server (#8985) RELEASE NOTES: * server: fix an authorization bypass where malformed :path headers (missing the leading slash) could bypass path-based restricted "deny" rules in interceptors like `grpc/authz`. Any request with a non-canonical path is now immediately rejected with an `Unimplemented` error. --- internal/envconfig/envconfig.go | 15 +++++++ server.go | 77 ++++++++++++++++++++++++++------- 2 files changed, 76 insertions(+), 16 deletions(-) diff --git a/internal/envconfig/envconfig.go b/internal/envconfig/envconfig.go index 77c2c0b8..ea4991bc 100644 --- a/internal/envconfig/envconfig.go +++ b/internal/envconfig/envconfig.go @@ -43,6 +43,21 @@ var ( // ALTSMaxConcurrentHandshakes is the maximum number of concurrent ALTS // handshakes that can be performed. ALTSMaxConcurrentHandshakes = uint64FromEnv("GRPC_ALTS_MAX_CONCURRENT_HANDSHAKES", 100, 1, 100) + // DisableStrictPathChecking indicates whether strict path checking is + // disabled. This feature can be disabled by setting the environment + // variable GRPC_GO_EXPERIMENTAL_DISABLE_STRICT_PATH_CHECKING to "true". + // + // When strict path checking is enabled, gRPC will reject requests with + // paths that do not conform to the gRPC over HTTP/2 specification found at + // https://github.com/grpc/grpc/blob/master/doc/PROTOCOL-HTTP2.md. + // + // When disabled, gRPC will allow paths that do not contain a leading slash. + // Enabling strict path checking is recommended for security reasons, as it + // prevents potential path traversal vulnerabilities. + // + // A future release will remove this environment variable, enabling strict + // path checking behavior unconditionally. + DisableStrictPathChecking = boolFromEnv("GRPC_GO_EXPERIMENTAL_DISABLE_STRICT_PATH_CHECKING", false) ) func boolFromEnv(envVar string, def bool) bool { diff --git a/server.go b/server.go index b44c7e86..eeaa34be 100644 --- a/server.go +++ b/server.go @@ -43,6 +43,7 @@ import ( "google.golang.org/grpc/internal" "google.golang.org/grpc/internal/binarylog" "google.golang.org/grpc/internal/channelz" + "google.golang.org/grpc/internal/envconfig" "google.golang.org/grpc/internal/grpcsync" "google.golang.org/grpc/internal/grpcutil" "google.golang.org/grpc/internal/transport" @@ -140,6 +141,8 @@ type Server struct { czData *channelzData serverWorkerChannel chan func() + + strictPathCheckingLogEmitted atomic.Bool } type serverOptions struct { @@ -187,6 +190,8 @@ type ServerOption interface { apply(*serverOptions) } +type any = interface{} + // EmptyServerOption does not alter the server configuration. It can be embedded // in another structure to build custom server options. // @@ -1702,28 +1707,68 @@ func (s *Server) processStreamingRPC(t transport.ServerTransport, stream *transp return t.WriteStatus(ss.s, statusOK) } +func (s *Server) handleMalformedMethodName(t transport.ServerTransport, stream *transport.Stream, ti *traceInfo) { + if ti != nil { + ti.tr.LazyLog(&fmtStringer{"Malformed method name %q", []any{stream.Method()}}, true) + ti.tr.SetError() + } + errDesc := fmt.Sprintf("malformed method name: %q", stream.Method()) + if err := t.WriteStatus(stream, status.New(codes.Unimplemented, errDesc)); err != nil { + if ti != nil { + ti.tr.LazyLog(&fmtStringer{"%v", []any{err}}, true) + ti.tr.SetError() + } + channelz.Warningf(logger, s.channelzID, "grpc: Server.handleStream failed to write status: %v", err) + } + if ti != nil { + ti.tr.Finish() + } +} + func (s *Server) handleStream(t transport.ServerTransport, stream *transport.Stream, trInfo *traceInfo) { + ctx := stream.Context() + ctx = NewContextWithServerTransportStream(ctx, stream) + var ti *traceInfo + if EnableTracing { + tr := trace.New("grpc.Recv."+methodFamily(stream.Method()), stream.Method()) + ctx = trace.NewContext(ctx, tr) + ti = &traceInfo{ + tr: tr, + firstLine: firstLine{ + client: false, + remoteAddr: t.RemoteAddr(), + }, + } + if dl, ok := ctx.Deadline(); ok { + ti.firstLine.deadline = time.Until(dl) + } + } + sm := stream.Method() - if sm != "" && sm[0] == '/' { + if sm == "" { + s.handleMalformedMethodName(t, stream, ti) + return + } + if sm[0] != '/' { + // TODO(easwars): Add a link to the CVE in the below log messages once + // published. + if envconfig.DisableStrictPathChecking { + if old := s.strictPathCheckingLogEmitted.Swap(true); !old { + channelz.Warningf(logger, s.channelzID, "grpc: Server.handleStream received malformed method name %q. Allowing it because the environment variable GRPC_GO_EXPERIMENTAL_DISABLE_STRICT_PATH_CHECKING is set to true, but this option will be removed in a future release.", sm) + } + } else { + if old := s.strictPathCheckingLogEmitted.Swap(true); !old { + channelz.Warningf(logger, s.channelzID, "grpc: Server.handleStream rejected malformed method name %q. To temporarily allow such requests, set the environment variable GRPC_GO_EXPERIMENTAL_DISABLE_STRICT_PATH_CHECKING to true. Note that this is not recommended as it may allow requests to bypass security policies.", sm) + } + s.handleMalformedMethodName(t, stream, ti) + return + } + } else { sm = sm[1:] } pos := strings.LastIndex(sm, "/") if pos == -1 { - if trInfo != nil { - trInfo.tr.LazyLog(&fmtStringer{"Malformed method name %q", []interface{}{sm}}, true) - trInfo.tr.SetError() - } - errDesc := fmt.Sprintf("malformed method name: %q", stream.Method()) - if err := t.WriteStatus(stream, status.New(codes.Unimplemented, errDesc)); err != nil { - if trInfo != nil { - trInfo.tr.LazyLog(&fmtStringer{"%v", []interface{}{err}}, true) - trInfo.tr.SetError() - } - channelz.Warningf(logger, s.channelzID, "grpc: Server.handleStream failed to write status: %v", err) - } - if trInfo != nil { - trInfo.tr.Finish() - } + s.handleMalformedMethodName(t, stream, ti) return } service := sm[:pos] -- 2.54.0