From efcc4c4e657305aa7e784d8b327859a0396ab313 Mon Sep 17 00:00:00 2001 From: Alessandro Ros Date: Mon, 7 Aug 2023 17:16:33 +0200 Subject: [PATCH] fix crash in case of specially-crafted HTTP requests (#2166) (#2169) --- internal/core/api.go | 8 +-- internal/core/hls_http_server.go | 5 +- internal/core/metrics.go | 8 +-- internal/core/pprof.go | 4 +- internal/core/webrtc_http_server.go | 5 +- internal/httpserv/handler_exit_on_panic.go | 27 ++++++++ internal/httpserv/handler_filter_requests.go | 18 ++++++ internal/httpserv/handler_logger.go | 63 +++++++++++++++++++ internal/httpserv/handler_server_header.go | 15 +++++ internal/httpserv/middleware_logger.go | 55 ---------------- internal/httpserv/middleware_server_header.go | 11 ---- internal/httpserv/wrapped_server.go | 41 +++++------- internal/httpserv/wrapped_server_test.go | 45 +++++++++++++ 13 files changed, 200 insertions(+), 105 deletions(-) create mode 100644 internal/httpserv/handler_exit_on_panic.go create mode 100644 internal/httpserv/handler_filter_requests.go create mode 100644 internal/httpserv/handler_logger.go create mode 100644 internal/httpserv/handler_server_header.go delete mode 100644 internal/httpserv/middleware_logger.go delete mode 100644 internal/httpserv/middleware_server_header.go create mode 100644 internal/httpserv/wrapped_server_test.go diff --git a/internal/core/api.go b/internal/core/api.go index 15cace43..b83d9f9f 100644 --- a/internal/core/api.go +++ b/internal/core/api.go @@ -7,6 +7,7 @@ import ( "reflect" "strconv" "sync" + "time" "github.com/gin-gonic/gin" "github.com/google/uuid" @@ -237,9 +238,7 @@ func newAPI( router := gin.New() router.SetTrustedProxies(nil) - mwLog := httpserv.MiddlewareLogger(a) - router.NoRoute(mwLog, httpserv.MiddlewareServerHeader) - group := router.Group("/", mwLog, httpserv.MiddlewareServerHeader) + group := router.Group("/") group.GET("/v2/config/get", a.onConfigGet) group.POST("/v2/config/set", a.onConfigSet) @@ -301,10 +300,11 @@ func newAPI( a.httpServer, err = httpserv.NewWrappedServer( network, address, - readTimeout, + time.Duration(readTimeout), "", "", router, + a, ) if err != nil { return nil, err diff --git a/internal/core/hls_http_server.go b/internal/core/hls_http_server.go index 0bbbce5f..bcf42d2e 100644 --- a/internal/core/hls_http_server.go +++ b/internal/core/hls_http_server.go @@ -65,7 +65,7 @@ func newHLSHTTPServer( //nolint:dupl router := gin.New() router.SetTrustedProxies(trustedProxies.ToTrustedProxies()) - router.NoRoute(httpserv.MiddlewareLogger(s), httpserv.MiddlewareServerHeader, s.onRequest) + router.NoRoute(s.onRequest) network, address := restrictNetwork("tcp", address) @@ -73,10 +73,11 @@ func newHLSHTTPServer( //nolint:dupl s.inner, err = httpserv.NewWrappedServer( network, address, - readTimeout, + time.Duration(readTimeout), serverCert, serverKey, router, + s, ) if err != nil { return nil, err diff --git a/internal/core/metrics.go b/internal/core/metrics.go index 58f6b108..d127fc77 100644 --- a/internal/core/metrics.go +++ b/internal/core/metrics.go @@ -5,6 +5,7 @@ import ( "net/http" "strconv" "sync" + "time" "github.com/gin-gonic/gin" @@ -46,9 +47,7 @@ func newMetrics( router := gin.New() router.SetTrustedProxies(nil) - mwLog := httpserv.MiddlewareLogger(m) - router.NoRoute(mwLog) - router.GET("/metrics", mwLog, m.onMetrics) + router.GET("/metrics", m.onMetrics) network, address := restrictNetwork("tcp", address) @@ -56,10 +55,11 @@ func newMetrics( m.httpServer, err = httpserv.NewWrappedServer( network, address, - readTimeout, + time.Duration(readTimeout), "", "", router, + m, ) if err != nil { return nil, err diff --git a/internal/core/pprof.go b/internal/core/pprof.go index 28701a7a..9eb87656 100644 --- a/internal/core/pprof.go +++ b/internal/core/pprof.go @@ -2,6 +2,7 @@ package core import ( "net/http" + "time" // start pprof _ "net/http/pprof" @@ -36,10 +37,11 @@ func newPPROF( pp.httpServer, err = httpserv.NewWrappedServer( network, address, - readTimeout, + time.Duration(readTimeout), "", "", http.DefaultServeMux, + pp, ) if err != nil { return nil, err diff --git a/internal/core/webrtc_http_server.go b/internal/core/webrtc_http_server.go index ad331ecf..7007ff68 100644 --- a/internal/core/webrtc_http_server.go +++ b/internal/core/webrtc_http_server.go @@ -68,7 +68,7 @@ func newWebRTCHTTPServer( //nolint:dupl router := gin.New() router.SetTrustedProxies(trustedProxies.ToTrustedProxies()) - router.NoRoute(httpserv.MiddlewareLogger(s), httpserv.MiddlewareServerHeader, s.onRequest) + router.NoRoute(s.onRequest) network, address := restrictNetwork("tcp", address) @@ -76,10 +76,11 @@ func newWebRTCHTTPServer( //nolint:dupl s.inner, err = httpserv.NewWrappedServer( network, address, - readTimeout, + time.Duration(readTimeout), serverCert, serverKey, router, + s, ) if err != nil { return nil, err diff --git a/internal/httpserv/handler_exit_on_panic.go b/internal/httpserv/handler_exit_on_panic.go new file mode 100644 index 00000000..c3b0e51e --- /dev/null +++ b/internal/httpserv/handler_exit_on_panic.go @@ -0,0 +1,27 @@ +package httpserv + +import ( + "fmt" + "net/http" + "os" + "runtime" +) + +// exit when there's a panic inside the HTTP handler. +// https://github.com/golang/go/issues/16542 +type handlerExitOnPanic struct { + http.Handler +} + +func (h *handlerExitOnPanic) ServeHTTP(w http.ResponseWriter, r *http.Request) { + defer func() { + err := recover() + if err != nil { + buf := make([]byte, 1<<20) + n := runtime.Stack(buf, true) + fmt.Fprintf(os.Stderr, "panic: %v\n\n%s", err, buf[:n]) + os.Exit(1) + } + }() + h.Handler.ServeHTTP(w, r) +} diff --git a/internal/httpserv/handler_filter_requests.go b/internal/httpserv/handler_filter_requests.go new file mode 100644 index 00000000..682d711e --- /dev/null +++ b/internal/httpserv/handler_filter_requests.go @@ -0,0 +1,18 @@ +package httpserv + +import ( + "net/http" +) + +// reject requests with empty paths. +type handlerFilterRequests struct { + http.Handler +} + +func (h *handlerFilterRequests) ServeHTTP(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == "" || r.URL.Path[0] != '/' { + w.WriteHeader(http.StatusBadRequest) + return + } + h.Handler.ServeHTTP(w, r) +} diff --git a/internal/httpserv/handler_logger.go b/internal/httpserv/handler_logger.go new file mode 100644 index 00000000..9e463f9f --- /dev/null +++ b/internal/httpserv/handler_logger.go @@ -0,0 +1,63 @@ +package httpserv + +import ( + "bytes" + "fmt" + "net/http" + "net/http/httputil" + + "github.com/bluenviron/mediamtx/internal/logger" +) + +type loggerWriter struct { + w http.ResponseWriter + status int + buf bytes.Buffer +} + +func (w *loggerWriter) Header() http.Header { + return w.w.Header() +} + +func (w *loggerWriter) Write(b []byte) (int, error) { + if w.status == 0 { + w.status = http.StatusOK + } + w.buf.Write(b) + return w.w.Write(b) +} + +func (w *loggerWriter) WriteHeader(statusCode int) { + w.status = statusCode + w.w.WriteHeader(statusCode) +} + +func (w *loggerWriter) dump() string { + var buf bytes.Buffer + fmt.Fprintf(&buf, "%s %d %s\n", "HTTP/1.1", w.status, http.StatusText(w.status)) + w.w.Header().Write(&buf) + buf.Write([]byte("\n")) + if w.buf.Len() > 0 { + fmt.Fprintf(&buf, "(body of %d bytes)", w.buf.Len()) + } + return buf.String() +} + +// log requests and responses. +type handlerLogger struct { + http.Handler + log logger.Writer +} + +func (h *handlerLogger) ServeHTTP(w http.ResponseWriter, r *http.Request) { + h.log.Log(logger.Debug, "[conn %v] %s %s", r.RemoteAddr, r.Method, r.URL.Path) + + byts, _ := httputil.DumpRequest(r, true) + h.log.Log(logger.Debug, "[conn %v] [c->s] %s", r.RemoteAddr, string(byts)) + + logw := &loggerWriter{w: w} + + h.Handler.ServeHTTP(logw, r) + + h.log.Log(logger.Debug, "[conn %v] [s->c] %s", r.RemoteAddr, logw.dump()) +} diff --git a/internal/httpserv/handler_server_header.go b/internal/httpserv/handler_server_header.go new file mode 100644 index 00000000..86a2121e --- /dev/null +++ b/internal/httpserv/handler_server_header.go @@ -0,0 +1,15 @@ +package httpserv + +import ( + "net/http" +) + +// set the Server header. +type handlerServerHeader struct { + http.Handler +} + +func (h *handlerServerHeader) ServeHTTP(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Server", "mediamtx") + h.Handler.ServeHTTP(w, r) +} diff --git a/internal/httpserv/middleware_logger.go b/internal/httpserv/middleware_logger.go deleted file mode 100644 index f17f12e4..00000000 --- a/internal/httpserv/middleware_logger.go +++ /dev/null @@ -1,55 +0,0 @@ -package httpserv - -import ( - "bytes" - "fmt" - "net/http" - "net/http/httputil" - - "github.com/gin-gonic/gin" - - "github.com/bluenviron/mediamtx/internal/logger" -) - -type loggerWriter struct { - gin.ResponseWriter - buf bytes.Buffer -} - -func (w *loggerWriter) Write(b []byte) (int, error) { - w.buf.Write(b) - return w.ResponseWriter.Write(b) -} - -func (w *loggerWriter) WriteString(s string) (int, error) { - w.buf.WriteString(s) - return w.ResponseWriter.WriteString(s) -} - -func (w *loggerWriter) dump() string { - var buf bytes.Buffer - fmt.Fprintf(&buf, "%s %d %s\n", "HTTP/1.1", w.ResponseWriter.Status(), http.StatusText(w.ResponseWriter.Status())) - w.ResponseWriter.Header().Write(&buf) - buf.Write([]byte("\n")) - if w.buf.Len() > 0 { - fmt.Fprintf(&buf, "(body of %d bytes)", w.buf.Len()) - } - return buf.String() -} - -// MiddlewareLogger is a middleware that logs requests and responses. -func MiddlewareLogger(p logger.Writer) func(*gin.Context) { - return func(ctx *gin.Context) { - p.Log(logger.Debug, "[conn %v] %s %s", ctx.Request.RemoteAddr, ctx.Request.Method, ctx.Request.URL.Path) - - byts, _ := httputil.DumpRequest(ctx.Request, true) - p.Log(logger.Debug, "[conn %v] [c->s] %s", ctx.Request.RemoteAddr, string(byts)) - - logw := &loggerWriter{ResponseWriter: ctx.Writer} - ctx.Writer = logw - - ctx.Next() - - p.Log(logger.Debug, "[conn %v] [s->c] %s", ctx.Request.RemoteAddr, logw.dump()) - } -} diff --git a/internal/httpserv/middleware_server_header.go b/internal/httpserv/middleware_server_header.go deleted file mode 100644 index 680402fb..00000000 --- a/internal/httpserv/middleware_server_header.go +++ /dev/null @@ -1,11 +0,0 @@ -package httpserv - -import ( - "github.com/gin-gonic/gin" -) - -// MiddlewareServerHeader is a middleware that sets the Server header. -func MiddlewareServerHeader(ctx *gin.Context) { - ctx.Writer.Header().Set("Server", "mediamtx") - ctx.Next() -} diff --git a/internal/httpserv/wrapped_server.go b/internal/httpserv/wrapped_server.go index d6108e5c..0974c178 100644 --- a/internal/httpserv/wrapped_server.go +++ b/internal/httpserv/wrapped_server.go @@ -4,15 +4,12 @@ package httpserv import ( "context" "crypto/tls" - "fmt" "log" "net" "net/http" - "os" - "runtime" "time" - "github.com/bluenviron/mediamtx/internal/conf" + "github.com/bluenviron/mediamtx/internal/logger" ) type nilWriter struct{} @@ -21,29 +18,13 @@ func (nilWriter) Write(p []byte) (int, error) { return len(p), nil } -// exit when there's a panic inside the HTTP handler. -// https://github.com/golang/go/issues/16542 -type exitOnPanicHandler struct { - http.Handler -} - -func (h exitOnPanicHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { - defer func() { - err := recover() - if err != nil { - buf := make([]byte, 1<<20) - n := runtime.Stack(buf, true) - fmt.Fprintf(os.Stderr, "panic: %v\n\n%s", err, buf[:n]) - os.Exit(1) - } - }() - h.Handler.ServeHTTP(w, r) -} - // WrappedServer is a wrapper around http.Server that provides: // - net.Listener allocation and closure // - TLS allocation // - exit on panic +// - logging +// - server header +// - filtering of invalid requests type WrappedServer struct { ln net.Listener inner *http.Server @@ -53,10 +34,11 @@ type WrappedServer struct { func NewWrappedServer( network string, address string, - readTimeout conf.StringDuration, + readTimeout time.Duration, serverCert string, serverKey string, handler http.Handler, + parent logger.Writer, ) (*WrappedServer, error) { ln, err := net.Listen(network, address) if err != nil { @@ -76,12 +58,19 @@ func NewWrappedServer( } } + h := handler + h = &handlerFilterRequests{h} + h = &handlerFilterRequests{h} + h = &handlerServerHeader{h} + h = &handlerLogger{h, parent} + h = &handlerExitOnPanic{h} + s := &WrappedServer{ ln: ln, inner: &http.Server{ - Handler: exitOnPanicHandler{handler}, + Handler: h, TLSConfig: tlsConfig, - ReadHeaderTimeout: time.Duration(readTimeout), + ReadHeaderTimeout: readTimeout, ErrorLog: log.New(&nilWriter{}, "", 0), }, } diff --git a/internal/httpserv/wrapped_server_test.go b/internal/httpserv/wrapped_server_test.go new file mode 100644 index 00000000..02b04ff7 --- /dev/null +++ b/internal/httpserv/wrapped_server_test.go @@ -0,0 +1,45 @@ +package httpserv + +import ( + "io" + "net" + "testing" + "time" + + "github.com/stretchr/testify/require" + + "github.com/bluenviron/mediamtx/internal/logger" +) + +type testLogger struct{} + +func (testLogger) Log(_ logger.Level, _ string, _ ...interface{}) { + // fmt.Printf(format, args...) +} + +func TestFilterEmptyPath(t *testing.T) { + s, err := NewWrappedServer( + "tcp", + "localhost:4555", + 10*time.Second, + "", + "", + nil, + &testLogger{}) + require.NoError(t, err) + defer s.Close() + + conn, err := net.Dial("tcp", "localhost:4555") + require.NoError(t, err) + defer conn.Close() + + _, err = conn.Write([]byte("OPTIONS http://localhost HTTP/1.1\n" + + "Host: localhost:8889\n" + + "Accept-Encoding: gzip\n" + + "User-Agent: Go-http-client/1.1\n\n")) + require.NoError(t, err) + + buf := make([]byte, 20) + _, err = io.ReadFull(conn, buf) + require.NoError(t, err) +}