From 715605b0e5529cfa3ebe9e07602b6908aba3ebad 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 | 16 +++ server.go | 76 +++++++++++--- test/malformed_method_test.go | 177 ++++++++++++++++++++++++++++++++ 3 files changed, 253 insertions(+), 16 deletions(-) create mode 100644 test/malformed_method_test.go diff --git a/internal/envconfig/envconfig.go b/internal/envconfig/envconfig.go index 73931a94..e5ed9632 100644 --- a/internal/envconfig/envconfig.go +++ b/internal/envconfig/envconfig.go @@ -28,6 +28,7 @@ const ( prefix = "GRPC_GO_" retryStr = prefix + "RETRY" txtErrIgnoreStr = prefix + "IGNORE_TXT_ERRORS" + strictPathChk = prefix + "EXPERIMENTAL_DISABLE_STRICT_PATH_CHECKING" ) var ( @@ -35,4 +36,19 @@ var ( Retry = strings.EqualFold(os.Getenv(retryStr), "on") // TXTErrIgnore is set if TXT errors should be ignored ("GRPC_GO_IGNORE_TXT_ERRORS" is not "false"). TXTErrIgnore = !strings.EqualFold(os.Getenv(txtErrIgnoreStr), "false") + // 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 = !strings.EqualFold(os.Getenv(strictPathChk), "false") ) diff --git a/server.go b/server.go index c2c7cae6..ac4cfaa9 100644 --- a/server.go +++ b/server.go @@ -43,6 +43,7 @@ import ( "google.golang.org/grpc/internal/binarylog" "google.golang.org/grpc/internal/channelz" "google.golang.org/grpc/internal/grpcrand" + "google.golang.org/grpc/internal/envconfig" "google.golang.org/grpc/internal/grpcsync" "google.golang.org/grpc/internal/transport" "google.golang.org/grpc/keepalive" @@ -116,6 +117,7 @@ type Server struct { czData *channelzData serverWorkerChannels []chan *serverWorkerData + strictPathCheckingLogEmitted atomic.Bool } type serverOptions struct { @@ -164,6 +166,8 @@ type ServerOption interface { // This API is EXPERIMENTAL. type EmptyServerOption struct{} +type any = interface{} + func (EmptyServerOption) apply(*serverOptions) {} // funcServerOption wraps a function that modifies serverOptions into an @@ -1461,28 +1465,68 @@ func (s *Server) processStreamingRPC(t transport.ServerTransport, stream *transp return err } +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(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(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(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.ResourceExhausted, errDesc)); err != nil { - if trInfo != nil { - trInfo.tr.LazyLog(&fmtStringer{"%v", []interface{}{err}}, true) - trInfo.tr.SetError() - } - channelz.Warningf(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] diff --git a/test/malformed_method_test.go b/test/malformed_method_test.go new file mode 100644 index 00000000..00e391a2 --- /dev/null +++ b/test/malformed_method_test.go @@ -0,0 +1,177 @@ +/* + * + * Copyright 2026 gRPC authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + */ + +package test + +import ( + "bytes" + "context" + "net" + "testing" + + "golang.org/x/net/http2" + "golang.org/x/net/http2/hpack" + "google.golang.org/grpc/internal/envconfig" + "google.golang.org/grpc/internal/stubserver" + "google.golang.org/grpc/internal/testutils" + + testpb "google.golang.org/grpc/interop/grpc_testing" +) + +// TestMalformedMethodPath tests that the server responds with Unimplemented +// when the method path is malformed. This verifies that the server does not +// route requests with a malformed method path to the application handler. +func (s) TestMalformedMethodPath(t *testing.T) { + tests := []struct { + name string + path string + envVar bool + wantStatus string // string representation of codes.Code + }{ + { + name: "missing_leading_slash_disableStrictPathChecking_false", + path: "grpc.testing.TestService/UnaryCall", + wantStatus: "12", // Unimplemented + }, + { + name: "empty_path_disableStrictPathChecking_false", + path: "", + wantStatus: "12", // Unimplemented + }, + { + name: "just_slash_disableStrictPathChecking_false", + path: "/", + wantStatus: "12", // Unimplemented + }, + { + name: "missing_leading_slash_disableStrictPathChecking_true", + path: "grpc.testing.TestService/UnaryCall", + envVar: true, + wantStatus: "0", // OK + }, + { + name: "empty_path_disableStrictPathChecking_true", + path: "", + envVar: true, + wantStatus: "12", // Unimplemented + }, + { + name: "just_slash_disableStrictPathChecking_true", + path: "/", + envVar: true, + wantStatus: "12", // Unimplemented + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout) + defer cancel() + + testutils.SetEnvConfig(t, &envconfig.DisableStrictPathChecking, tc.envVar) + + ss := &stubserver.StubServer{ + UnaryCallF: func(context.Context, *testpb.SimpleRequest) (*testpb.SimpleResponse, error) { + return &testpb.SimpleResponse{Payload: &testpb.Payload{Body: []byte("pwned")}}, nil + }, + } + if err := ss.Start(nil); err != nil { + t.Fatalf("Error starting endpoint server: %v", err) + } + defer ss.Stop() + + // Open a raw TCP connection to the server and speak HTTP/2 directly. + tcpConn, err := net.Dial("tcp", ss.Address) + if err != nil { + t.Fatalf("Failed to dial tcp: %v", err) + } + defer tcpConn.Close() + + // Write the HTTP/2 connection preface and the initial settings frame. + if _, err := tcpConn.Write([]byte("PRI * HTTP/2.0\r\n\r\nSM\r\n\r\n")); err != nil { + t.Fatalf("Failed to write preface: %v", err) + } + framer := http2.NewFramer(tcpConn, tcpConn) + if err := framer.WriteSettings(); err != nil { + t.Fatalf("Failed to write settings: %v", err) + } + + // Encode and write the HEADERS frame. + var headerBuf bytes.Buffer + enc := hpack.NewEncoder(&headerBuf) + writeHeader := func(name, value string) { + enc.WriteField(hpack.HeaderField{Name: name, Value: value}) + } + writeHeader(":method", "POST") + writeHeader(":scheme", "http") + writeHeader(":authority", ss.Address) + writeHeader(":path", tc.path) + writeHeader("content-type", "application/grpc") + writeHeader("te", "trailers") + if err := framer.WriteHeaders(http2.HeadersFrameParam{ + StreamID: 1, + BlockFragment: headerBuf.Bytes(), + EndStream: false, + EndHeaders: true, + }); err != nil { + t.Fatalf("Failed to write headers: %v", err) + } + + // Send a small gRPC-encoded data frame (0 length). + if err := framer.WriteData(1, true, []byte{0, 0, 0, 0, 0}); err != nil { + t.Fatalf("Failed to write data: %v", err) + } + + // Read responses and look for grpc-status. + gotStatus := "" + dec := hpack.NewDecoder(4096, func(f hpack.HeaderField) { + if f.Name == "grpc-status" { + gotStatus = f.Value + } + }) + done := make(chan struct{}) + go func() { + defer close(done) + for { + frame, err := framer.ReadFrame() + if err != nil { + return + } + if headers, ok := frame.(*http2.HeadersFrame); ok { + if _, err := dec.Write(headers.HeaderBlockFragment()); err != nil { + return + } + if headers.StreamEnded() { + return + } + } + } + }() + + select { + case <-done: + case <-ctx.Done(): + t.Fatalf("Timed out waiting for response") + } + + if gotStatus != tc.wantStatus { + t.Errorf("Got grpc-status %v, want %v", gotStatus, tc.wantStatus) + } + }) + } +} -- 2.53.0