From 8f9cc7bcbac1e56cdbd22e40c910a85c89e4275f Mon Sep 17 00:00:00 2001 From: Monis Khan Date: Sat, 7 Oct 2023 21:50:37 -0400 Subject: [PATCH] Prevent rapid reset http2 DOS on API server This change fully addresses CVE-2023-44487 and CVE-2023-39325 for the API server when combined with https://github.com/golang/net/commit/b225e7ca6dde1ef5a5ae5ce922861bda011cfabd The changes to util/runtime are required because otherwise a large number of requests can get blocked on the time.Sleep calls. For unauthenticated clients (either via 401 or the anonymous user), we simply no longer allow such clients to hold open http2 connections. They can use http2, but with the performance of http1 (or possibly slightly worse). For all other clients, we detect if the request ended via a timeout before the context's deadline. This likely means that the client reset the http2 stream early. We close the connection in this case as well. To mitigate issues related to clients creating more streams than the configured max, we rely on the golang.org/x/net fix noted above. The Kube API server now uses a max stream of 100 instead of 250 (this matches the Go http2 client default). This lowers the abuse limit from 1000 to 400. Signed-off-by: Monis Khan --- .../apimachinery/pkg/util/runtime/runtime.go | 15 +++++---- .../pkg/endpoints/filters/authentication.go | 28 ++++++++++++++++ .../apiserver/pkg/server/filters/goaway.go | 2 +- .../apiserver/pkg/server/filters/timeout.go | 32 +++++++++++++++++-- .../apiserver/pkg/server/secure_serving.go | 5 ++- 5 files changed, 71 insertions(+), 11 deletions(-) diff --git a/staging/src/k8s.io/apimachinery/pkg/util/runtime/runtime.go b/staging/src/k8s.io/apimachinery/pkg/util/runtime/runtime.go index 9f834fa538df5..d686732bf412e 100644 --- a/staging/src/k8s.io/apimachinery/pkg/util/runtime/runtime.go +++ b/staging/src/k8s.io/apimachinery/pkg/util/runtime/runtime.go @@ -126,14 +126,17 @@ type rudimentaryErrorBackoff struct { // OnError will block if it is called more often than the embedded period time. // This will prevent overly tight hot error loops. func (r *rudimentaryErrorBackoff) OnError(error) { + now := time.Now() // start the timer before acquiring the lock r.lastErrorTimeLock.Lock() - defer r.lastErrorTimeLock.Unlock() - d := time.Since(r.lastErrorTime) - if d < r.minPeriod { - // If the time moves backwards for any reason, do nothing - time.Sleep(r.minPeriod - d) - } + d := now.Sub(r.lastErrorTime) r.lastErrorTime = time.Now() + r.lastErrorTimeLock.Unlock() + + // Do not sleep with the lock held because that causes all callers of HandleError to block. + // We only want the current goroutine to block. + // A negative or zero duration causes time.Sleep to return immediately. + // If the time moves backwards for any reason, do nothing. + time.Sleep(r.minPeriod - d) } // GetCaller returns the caller of the function that calls it. diff --git a/staging/src/k8s.io/apiserver/pkg/endpoints/filters/authentication.go b/staging/src/k8s.io/apiserver/pkg/endpoints/filters/authentication.go index d6741bf3a3aa3..803a5abc1da72 100644 --- a/staging/src/k8s.io/apiserver/pkg/endpoints/filters/authentication.go +++ b/staging/src/k8s.io/apiserver/pkg/endpoints/filters/authentication.go @@ -29,6 +29,7 @@ import ( "k8s.io/apiserver/pkg/authentication/authenticator" "k8s.io/apiserver/pkg/authentication/authenticatorfactory" "k8s.io/apiserver/pkg/authentication/request/headerrequest" + "k8s.io/apiserver/pkg/authentication/user" "k8s.io/apiserver/pkg/endpoints/handlers/responsewriters" genericapirequest "k8s.io/apiserver/pkg/endpoints/request" "k8s.io/klog/v2" @@ -101,6 +102,14 @@ func withAuthentication(handler http.Handler, auth authenticator.Request, failed ) } + // http2 is an expensive protocol that is prone to abuse, + // see CVE-2023-44487 and CVE-2023-39325 for an example. + // Do not allow unauthenticated clients to keep these + // connections open (i.e. basically degrade them to http1). + if req.ProtoMajor == 2 && isAnonymousUser(resp.User) { + w.Header().Set("Connection", "close") + } + req = req.WithContext(genericapirequest.WithUser(req.Context(), resp.User)) handler.ServeHTTP(w, req) }) @@ -108,6 +117,13 @@ func withAuthentication(handler http.Handler, auth authenticator.Request, failed func Unauthorized(s runtime.NegotiatedSerializer) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { + // http2 is an expensive protocol that is prone to abuse, + // see CVE-2023-44487 and CVE-2023-39325 for an example. + // Do not allow unauthenticated clients to keep these + // connections open (i.e. basically degrade them to http1). + if req.ProtoMajor == 2 { + w.Header().Set("Connection", "close") + } ctx := req.Context() requestInfo, found := genericapirequest.RequestInfoFrom(ctx) if !found { @@ -127,3 +143,15 @@ func audiencesAreAcceptable(apiAuds, responseAudiences authenticator.Audiences) return len(apiAuds.Intersect(responseAudiences)) > 0 } + +func isAnonymousUser(u user.Info) bool { + if u.GetName() == user.Anonymous { + return true + } + for _, group := range u.GetGroups() { + if group == user.AllUnauthenticated { + return true + } + } + return false +} diff --git a/staging/src/k8s.io/apiserver/pkg/server/filters/goaway.go b/staging/src/k8s.io/apiserver/pkg/server/filters/goaway.go index 065bd22bbc90d..a792edf856822 100644 --- a/staging/src/k8s.io/apiserver/pkg/server/filters/goaway.go +++ b/staging/src/k8s.io/apiserver/pkg/server/filters/goaway.go @@ -64,7 +64,7 @@ type goaway struct { // ServeHTTP implement HTTP handler func (h *goaway) ServeHTTP(w http.ResponseWriter, r *http.Request) { - if r.Proto == "HTTP/2.0" && h.decider.Goaway(r) { + if r.ProtoMajor == 2 && h.decider.Goaway(r) { // Send a GOAWAY and tear down the TCP connection when idle. w.Header().Set("Connection", "close") } diff --git a/staging/src/k8s.io/apiserver/pkg/server/filters/timeout.go b/staging/src/k8s.io/apiserver/pkg/server/filters/timeout.go index dff4a01d1978a..104e8f4702ec4 100644 --- a/staging/src/k8s.io/apiserver/pkg/server/filters/timeout.go +++ b/staging/src/k8s.io/apiserver/pkg/server/filters/timeout.go @@ -143,13 +143,13 @@ func (t *timeoutHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { }() defer postTimeoutFn() - tw.timeout(err) + tw.timeout(r, err) } } type timeoutWriter interface { http.ResponseWriter - timeout(*apierrors.StatusError) + timeout(*http.Request, *apierrors.StatusError) } func newTimeoutWriter(w http.ResponseWriter) (timeoutWriter, http.ResponseWriter) { @@ -242,7 +242,7 @@ func copyHeaders(dst, src http.Header) { } } -func (tw *baseTimeoutWriter) timeout(err *apierrors.StatusError) { +func (tw *baseTimeoutWriter) timeout(r *http.Request, err *apierrors.StatusError) { tw.mu.Lock() defer tw.mu.Unlock() @@ -252,6 +252,14 @@ func (tw *baseTimeoutWriter) timeout(err *apierrors.StatusError) { // We can safely timeout the HTTP request by sending by a timeout // handler if !tw.wroteHeader && !tw.hijacked { + // http2 is an expensive protocol that is prone to abuse, + // see CVE-2023-44487 and CVE-2023-39325 for an example. + // Do not allow clients to reset these connections + // prematurely as that can trivially OOM the api server + // (i.e. basically degrade them to http1). + if isLikelyEarlyHTTP2Reset(r) { + tw.w.Header().Set("Connection", "close") + } tw.w.WriteHeader(http.StatusGatewayTimeout) enc := json.NewEncoder(tw.w) enc.Encode(&err.ErrStatus) @@ -274,6 +282,24 @@ func (tw *baseTimeoutWriter) timeout(err *apierrors.StatusError) { } } +// isLikelyEarlyHTTP2Reset returns true if an http2 stream was reset before the request deadline. +// Note that this does not prevent a client from trying to create more streams than the configured +// max, but https://github.com/golang/net/commit/b225e7ca6dde1ef5a5ae5ce922861bda011cfabd protects +// us from abuse via that vector. +func isLikelyEarlyHTTP2Reset(r *http.Request) bool { + if r.ProtoMajor != 2 { + return false + } + + deadline, ok := r.Context().Deadline() + if !ok { + return true // this context had no deadline but was canceled meaning the client likely reset it early + } + + // this context was canceled before its deadline meaning the client likely reset it early + return time.Now().Before(deadline) +} + func (tw *baseTimeoutWriter) CloseNotify() <-chan bool { tw.mu.Lock() defer tw.mu.Unlock() diff --git a/staging/src/k8s.io/apiserver/pkg/server/secure_serving.go b/staging/src/k8s.io/apiserver/pkg/server/secure_serving.go index 64bcc87ebf173..0a4fdc6932eda 100644 --- a/staging/src/k8s.io/apiserver/pkg/server/secure_serving.go +++ b/staging/src/k8s.io/apiserver/pkg/server/secure_serving.go @@ -189,7 +189,10 @@ func (s *SecureServingInfo) Serve(handler http.Handler, shutdownTimeout time.Dur if s.HTTP2MaxStreamsPerConnection > 0 { http2Options.MaxConcurrentStreams = uint32(s.HTTP2MaxStreamsPerConnection) } else { - http2Options.MaxConcurrentStreams = 250 + // match http2.initialMaxConcurrentStreams used by clients + // this makes it so that a malicious client can only open 400 streams before we forcibly close the connection + // https://github.com/golang/net/commit/b225e7ca6dde1ef5a5ae5ce922861bda011cfabd + http2Options.MaxConcurrentStreams = 100 } // increase the connection buffer size from the 1MB default to handle the specified number of concurrent streams