security: malformed Cookie metadata can panic the control plane
Nobody has claimed this yet.
- Dominant language
- Go
- Stars
- 1.4k
- Forks
- 364
- Avg merge
- 1d 4h
- Merged PRs (30d)
- 84
Description
Problem
extractCookie in pkg/rpc/rpcauth/auth.go parses the incoming gRPC cookie metadata by splitting on ; and then, for each segment, splitting on = with strings.SplitN(..., "=", 2). It then unconditionally indexes subs[1]:
cs := strings.Split(rawCookie[0], ";")
cookie := make(map[string]string, len(cs))
for _, c := range cs {
subs := strings.SplitN(strings.TrimSpace(c), "=", 2)
cookie[subs[0]] = subs[1]
}
If any ;-separated segment does not contain an = (for example a trailing ;, a bare cookie name, or two consecutive ;;), strings.SplitN returns a slice of length 1, and subs[1] panics with index out of range [1] with length 1.
Impact
This is reachable before authentication succeeds. extractCookie is called as the very first step of JWTUnaryServerInterceptor (pkg/rpc/rpcauth/interceptor.go:189), i.e. before the JWT itself is verified:
func JWTUnaryServerInterceptor(verifier jwt.Verifier, authorizer RBACAuthorizer, logger *zap.Logger) grpc.UnaryServerInterceptor {
return func(ctx context.Context, req interface{}, info *grpc.UnaryServerInfo, handler grpc.UnaryHandler) (interface{}, error) {
cookie, err := extractCookie(ctx)
...
This interceptor is installed on the WebAPI gRPC server via rpc.WithJWTAuthUnaryInterceptor in cmd/controlplane/server.go:309. So any unauthenticated caller that can reach the WebAPI gRPC port and send a request with a malformed cookie metadata value (no valid session/JWT required) can trigger the panic on any WebAPI RPC method.
Each gRPC request is served in its own goroutine spawned directly by grpc-go (go f() in (*Server).serveStreams, see google.golang.org/grpc@v1.79.3/server.go), with no recover() anywhere in that call path. I confirmed there is no panic-recovery interceptor registered anywhere in pkg/rpc/ or pkg/app/server/ (grep -r "recover()" and searches for grpc_recovery/recovery interceptors both come back empty). An unrecovered panic in any goroutine terminates the entire Go process.
The WebAPI, PipedAPI, and APIService gRPC servers all run inside the same controlplane OS process, started from the same errgroup in cmd/controlplane/server.go (server.run). So a single malformed, unauthenticated request to the WebAPI port crashes the whole control-plane process, not just the WebAPI listener — taking down piped↔control-plane communication (PipedAPI) and the external API (APIService) as well until the process is restarted.
Technical Details
- Vulnerable code:
extractCookie, pkg/rpc/rpcauth/auth.go:127-143, specifically the indexing at line 140 (cookie[subs[0]] = subs[1]). - Call path: unauthenticated gRPC request →
JWTUnaryServerInterceptor(pkg/rpc/rpcauth/interceptor.go:189) →extractCookie(auth.go:136-141) → panic onsubs[1]. - No bounds check exists on
len(subs)before the indexing. - No panic-recovery interceptor is registered on the WebAPI server (or any other gRPC server) in this codebase, and grpc-go itself does not recover panics from handler/interceptor goroutines.
- WebAPI, PipedAPI, and APIService all run in the same OS process (
cmd/controlplane), so the crash is process-wide, not limited to the WebAPI listener.
Regression
This is a regression introduced by #6933 ("Update extract cookie logic to allow cookie contains equal sign"). Before that PR, the code was:
subs := strings.Split(strings.TrimSpace(c), "=")
if len(subs) != 2 {
return nil, status.Error(codes.Unauthenticated, "cookie is malformed")
}
cookie[subs[0]] = subs[1]
#6933 changed strings.Split to strings.SplitN(..., "=", 2) (to correctly support cookie values that themselves contain =, e.g. base64 padding) but removed the len(subs) != 2 bounds check entirely, so the fix for one bug reintroduced a panic for a different malformed-input case. There is currently no test case covering a cookie segment without = (pkg/rpc/rpcauth/auth_test.go's TestExtractCookie only covers well-formed key=value segments and the "value contains =" case added by #6933).
Reproduction
Locally reproduced with a temporary test against extractCookie (not committed):
ctx := metadata.NewIncomingContext(context.Background(), metadata.MD{
"cookie": []string{"token=xxx;"},
})
_, _ = extractCookie(ctx)
Result:
panic: runtime error: index out of range [1] with length 1
github.com/pipe-cd/pipecd/pkg/rpc/rpcauth.extractCookie(...)
pkg/rpc/rpcauth/auth.go:140
The same panic is triggered by any segment lacking =, e.g. Cookie: justatoken or Cookie: token=xxx;;.
Equivalently, over the wire, sending any WebAPI RPC with gRPC metadata cookie: token=xxx; (no valid JWT required, since the panic happens before verification) is expected to crash the control-plane process.
Related Issues
#6612 requests a generic panic-recovery interceptor for the server ("Server should not crash when panic occurs") and is already being worked on. It would provide useful defense-in-depth against panics like this one, but it does not identify or document this specific extractCookie trigger, and adding generic recovery does not address the underlying missing bounds check that lets unauthenticated input reach an out-of-range index in the first place. This issue is about fixing that specific input-validation regression; #6612 is a separate, complementary hardening effort.
Suggested Fix
Restore bounds validation before indexing the result of SplitN in extractCookie, e.g. verify len(subs) == 2 before accessing subs[1], and reject/skip malformed segments instead of indexing them unconditionally. This should be done without regressing the fix from #6933 (i.e. cookie values containing = should still be supported).
Separately, landing #6612's generic panic-recovery interceptor would add useful defense-in-depth for this class of bug, but should not be treated as a substitute for fixing this specific unauthenticated-input-handling issue.
Priority
This repository doesn't have a P0 label; the closest equivalent is priority/P1 ("Top priority"). I believe this qualifies:
- Triggerable by an unauthenticated request (no valid credentials/JWT needed — the panic occurs before verification).
- Requires only a single, trivially crafted gRPC metadata value.
- Confirmed to crash the entire
controlplaneprocess (WebAPI, PipedAPI, and APIService all run in one process with no panic recovery anywhere in the call path), not just the affected request. - It's a regression in currently-released code (#6933), not a theoretical/unreached code path.
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start in pkg/rpc/rpcauth/auth.go at extractCookie and read TestExtractCookie in pkg/rpc/rpcauth/auth_test.go. Run the existing auth tests, then cover cookie segments without '=' while preserving values containing '='. Done means malformed metadata returns an authentication error without panicking and the existing valid-cookie cases still pass.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- go
- Domain
- authentication, backend, security
- Issue type
- Bug
- Difficulty
- 2/5
- Estimated time
- 1-3 hours
- Activity status
- Active
- Clarity
- Clearly specified
- Newbie friendliness
- 85/100