fix crash in case of specially-crafted HTTP requests (#2166) (#2169)

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)
+	group := router.Group("/")
-	router.NoRoute(mwLog, httpserv.MiddlewareServerHeader)
-	group := router.Group("/", mwLog, httpserv.MiddlewareServerHeader)
 
 	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

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

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.GET("/metrics", m.onMetrics)
-	router.NoRoute(mwLog)
-	router.GET("/metrics", mwLog, 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

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

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

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)
+}

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)
+}

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())
+}

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)
+}

internal/httpserv/middleware_logger.go

@@ -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())
-	}
-}

internal/httpserv/middleware_server_header.go

@@ -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()
-}

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),
 		},
 	}

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)
+}