BED-5301/5298: PenTest Result: Internal IP Address Disclosure AND Rate limit Bypass

cmd/api/src/api/middleware/rate_limit.go

@@ -17,75 +17,88 @@
 package middleware
 
 import (
+	"fmt"
+	"log/slog"
+	"net"
 	"net/http"
+	"strings"
 	"time"
 
-	"github.com/specterops/bloodhound/headers"
-
-	"github.com/didip/tollbooth/v6"
-	"github.com/didip/tollbooth/v6/limiter"
 	"github.com/gorilla/mux"
-	"github.com/specterops/bloodhound/src/api"
+	"github.com/specterops/bloodhound/src/config"
+	"github.com/ulule/limiter/v3"
+	"github.com/ulule/limiter/v3/drivers/middleware/stdlib"
+	"github.com/ulule/limiter/v3/drivers/store/memory"
 )
 
 // DefaultRateLimit is the default number of allowed requests per second
 const DefaultRateLimit = 55
 
-// RateLimitHandler returns a http.Handler that limits the rate of requests
-// for a given handler
-//
-// Usage:
-//
-//	limiter := tollbooth.NewLimiter(1, nil).
-//		SetMethods([]string{"GET", "POST"}).
-//		...configure tollbooth Limiter ...
-//
-//	router.Handle("/teapot", RateLimitHandler(limiter, handler))
-func RateLimitHandler(limiter *limiter.Limiter, handler http.Handler) http.Handler {
-	return http.HandlerFunc(func(response http.ResponseWriter, request *http.Request) {
-		if err := tollbooth.LimitByRequest(limiter, response, request); err != nil {
-			// In case SetOnLimitReached was called
-			limiter.ExecOnLimitReached(response, request)
-
-			api.WriteErrorResponse(request.Context(), &api.ErrorWrapper{
-				HTTPStatus: err.StatusCode,
-				Timestamp:  time.Now(),
-				RequestID:  request.Header.Get(headers.RequestID.String()),
-				Errors: []api.ErrorDetails{
-					{
-						Context: "middleware",
-						Message: err.Error(),
-					},
-				},
-			}, response)
-		} else {
-			handler.ServeHTTP(response, request)
-		}
-	})
-}
-
 // RateLimitMiddleware is a convenience function for creating rate limiting middleware
 //
-// Usage:
-//
-//	limiter := tollbooth.NewLimiter(1, nil).
-//		SetMethods([]string{"GET", "POST"}).
-//		...configure tollbooth Limiter ...
-//
 //	router.Use(RateLimitMiddleware(limiter))
-func RateLimitMiddleware(limiter *limiter.Limiter) mux.MiddlewareFunc {
+func RateLimitMiddleware(cfg config.Configuration, limiter *limiter.Limiter) mux.MiddlewareFunc {
+	ipGetter := stdlib.WithKeyGetter(
+		func(r *http.Request) string {
+			var remoteIP string
+
+			if host, _, err := net.SplitHostPort(r.RemoteAddr); err != nil {
+				slog.WarnContext(r.Context(), fmt.Sprintf("Error parsing remoteAddress '%s': %s", r.RemoteAddr, err))
+				remoteIP = r.RemoteAddr
+			} else {
+				remoteIP = host
+			}
+
+			if cfg.TrustedProxies == 0 {
+				return remoteIP
+			} else if xff := r.Header.Get("X-Forwarded-For"); xff == "" {
+				return remoteIP
+			} else {
+				ips := strings.Split(xff, ",")
+
+				idxIP := len(ips) - cfg.TrustedProxies
+				if idxIP < 0 {
+					slog.WarnContext(r.Context(), "Not enough IPs in X-Forwarded-For, defaulting to first IP")
+					idxIP = 0
+				}
+
+				return strings.TrimSpace(ips[idxIP])
+			}
+		},
+	)
+
+	middleware := stdlib.NewMiddleware(limiter, ipGetter)
+
 	return func(next http.Handler) http.Handler {
-		return RateLimitHandler(limiter, next)
+		return middleware.Handler(RemoveRateLimitHeadersMiddleware(next))
 	}
 }
 
+// RemoveRateLimitHeadersMiddleware removes rate limit headers that we do not want appearing in our responses
+func RemoveRateLimitHeadersMiddleware(next http.Handler) http.Handler {
+	return http.HandlerFunc(func(response http.ResponseWriter, request *http.Request) {
+		response.Header().Del("X-RateLimit-Limit")
+		response.Header().Del("X-RateLimit-Remaining")
+		response.Header().Del("X-RateLimit-Reset")
+		next.ServeHTTP(response, request)
+	})
+}
+
 // DefaultRateLimitMiddleware is a convenience function for creating the default rate limiting middleware
 // for a router/route
 //
 // Usage:
 //
 //	router.Use(DefaultRateLimitMiddleware())
-func DefaultRateLimitMiddleware() mux.MiddlewareFunc {
-	limiter := tollbooth.NewLimiter(DefaultRateLimit, nil)
-	return RateLimitMiddleware(limiter)
+func DefaultRateLimitMiddleware(cfg config.Configuration) mux.MiddlewareFunc {
+	rate := limiter.Rate{
+		Period: 1 * time.Second,
+		Limit:  DefaultRateLimit,
+	}
+
+	store := memory.NewStore()
+
+	instance := limiter.New(store, rate)
+
+	return RateLimitMiddleware(cfg, instance)
 }

cmd/api/src/api/middleware/rate_limit_test.go

@@ -20,72 +20,33 @@ import (
 	"net/http"
 	"net/http/httptest"
 	"testing"
+	"time"
 
-	"github.com/didip/tollbooth/v6"
 	"github.com/gorilla/mux"
 	"github.com/specterops/bloodhound/src/api/middleware"
+	"github.com/specterops/bloodhound/src/config"
+	"github.com/stretchr/testify/require"
+	"github.com/ulule/limiter/v3"
+	"github.com/ulule/limiter/v3/drivers/store/memory"
 )
 
-func TestRateLimitHandler(t *testing.T) {
-
-	// Limit to 1 req/s
-	limiter := tollbooth.NewLimiter(1, nil)
-
-	count_429 := 0
-	limiter.SetOnLimitReached(func(response http.ResponseWriter, request *http.Request) {
-		count_429++
-	})
-
-	if req, err := http.NewRequest("GET", "/teapot", nil); err != nil {
-		t.Fatal(err)
-	} else {
-		req.Header.Set("X-Real-IP", "8.8.8.8")
-
-		handler := middleware.RateLimitHandler(limiter, &CountingHandler{})
-		router := mux.NewRouter()
-		router.Handle("/teapot", handler)
-
-		rr := httptest.NewRecorder()
-		router.ServeHTTP(rr, req)
-
-		// Expect a 200
-		if status := rr.Code; status != http.StatusOK {
-			t.Errorf("handler returned wrong status code: got %v want %v", status, http.StatusOK)
-		}
-
-		ch := make(chan int)
-		go func() {
-			rr := httptest.NewRecorder()
-			router.ServeHTTP(rr, req)
-
-			// Expect a 429
-			if status := rr.Code; status != http.StatusTooManyRequests {
-				t.Errorf("handler returned wrong status code: got %v want %v", status, http.StatusTooManyRequests)
-			}
-
-			if count_429 == 0 {
-				t.Error("OnLimitReached callback function should have been called")
-			}
-
-			close(ch)
-		}()
-		<-ch
+func TestRateLimitMiddleware(t *testing.T) {
+	allowedReqsPerSecond := 5
+	rate := limiter.Rate{
+		Period: 1 * time.Second,
+		Limit:  int64(allowedReqsPerSecond),
 	}
-}
 
-func TestRateLimitMiddleware(t *testing.T) {
+	store := memory.NewStore()
 
-	allowedReqsPerSecond := 5
-	limiter := tollbooth.NewLimiter(float64(allowedReqsPerSecond), nil)
+	instance := limiter.New(store, rate)
 
-	count_429 := 0
-	limiter.SetOnLimitReached(func(w http.ResponseWriter, r *http.Request) {
-		count_429++
-	})
+	cfg, err := config.NewDefaultConfiguration()
+	require.Nil(t, err)
 
 	testHandler := &CountingHandler{}
 	router := mux.NewRouter()
-	router.Use(middleware.RateLimitMiddleware(limiter))
+	router.Use(middleware.RateLimitMiddleware(cfg, instance))
 	router.Handle("/teapot", testHandler)
 
 	if req, err := http.NewRequest("GET", "/teapot", nil); err != nil {
@@ -103,17 +64,16 @@ func TestRateLimitMiddleware(t *testing.T) {
 	if testHandler.Count != allowedReqsPerSecond {
 		t.Errorf("invalid HTTP 200 count: got %v want %v", testHandler.Count, 5)
 	}
-
-	if count_429 != 1 {
-		t.Errorf("invalid HTTP 429 count: got %v want %v", count_429, 1)
-	}
 }
 
 func TestDefaultRateLimitMiddleware(t *testing.T) {
 	testHandler := &CountingHandler{}
 
+	cfg, err := config.NewDefaultConfiguration()
+	require.Nil(t, err)
+
 	router := mux.NewRouter()
-	router.Use(middleware.DefaultRateLimitMiddleware())
+	router.Use(middleware.DefaultRateLimitMiddleware(cfg))
 	router.Handle("/teapot", testHandler)
 
 	if req, err := http.NewRequest("GET", "/teapot", nil); err != nil {

cmd/api/src/api/registration/registration.go

@@ -19,6 +19,7 @@ package registration
 import (
 	"net/http"
 
+	"github.com/gorilla/mux"
 	"github.com/specterops/bloodhound/cache"
 	"github.com/specterops/bloodhound/dawgs/graph"
 	"github.com/specterops/bloodhound/src/api"
@@ -60,7 +61,9 @@ func RegisterFossRoutes(
 	authenticator api.Authenticator,
 	authorizer auth.Authorizer,
 ) {
-	router.With(middleware.DefaultRateLimitMiddleware,
+	router.With(func() mux.MiddlewareFunc {
+		return middleware.DefaultRateLimitMiddleware(cfg)
+	},
 		// Health Endpoint
 		routerInst.GET("/health", func(response http.ResponseWriter, _ *http.Request) {
 			response.WriteHeader(http.StatusOK)

cmd/api/src/api/registration/v2.go

@@ -19,7 +19,9 @@ package registration
 import (
 	"fmt"
 	"net/http"
+	"time"
 
+	"github.com/gorilla/mux"
 	"github.com/specterops/bloodhound/openapi"
 	"github.com/specterops/bloodhound/params"
 	"github.com/specterops/bloodhound/src/api"
@@ -29,6 +31,8 @@ import (
 	authapi "github.com/specterops/bloodhound/src/api/v2/auth"
 	"github.com/specterops/bloodhound/src/auth"
 	"github.com/specterops/bloodhound/src/model/appcfg"
+	"github.com/ulule/limiter/v3"
+	"github.com/ulule/limiter/v3/drivers/store/memory"
 )
 
 func registerV2Auth(resources v2.Resources, routerInst *router.Router, permissions auth.PermissionSet) {
@@ -37,9 +41,27 @@ func registerV2Auth(resources v2.Resources, routerInst *router.Router, permissio
 		managementResource = authapi.NewManagementResource(resources.Config, resources.DB, resources.Authorizer, resources.Authenticator)
 	)
 
-	routerInst.POST("/api/v2/login", loginResource.Login).Use(middleware.DefaultRateLimitMiddleware(), middleware.LoginTimer())
+	router.With(func() mux.MiddlewareFunc {
+		rate := limiter.Rate{
+			Period: 1 * time.Second,
+			Limit:  1,
+		}
 
-	router.With(middleware.DefaultRateLimitMiddleware,
+		store := memory.NewStore()
+
+		instance := limiter.New(store, rate)
+
+		return middleware.RateLimitMiddleware(resources.Config, instance)
+	},
+		// Login resource
+		routerInst.POST("/api/v2/login", func(response http.ResponseWriter, request *http.Request) {
+			middleware.LoginTimer()(http.HandlerFunc(loginResource.Login)).ServeHTTP(response, request)
+		}),
+	)
+
+	router.With(func() mux.MiddlewareFunc {
+		return middleware.DefaultRateLimitMiddleware(resources.Config)
+	},
 		// Login resources
 		routerInst.GET("/api/v2/self", managementResource.GetSelf),
 		routerInst.POST("/api/v2/logout", loginResource.Logout),
@@ -118,7 +140,9 @@ func NewV2API(resources v2.Resources, routerInst *router.Router) {
 	routerInst.POST(fmt.Sprintf("/api/v2/file-upload/{%s}", v2.FileUploadJobIdPathParameterName), resources.ProcessFileUpload).RequirePermissions(permissions.GraphDBIngest)
 	routerInst.POST(fmt.Sprintf("/api/v2/file-upload/{%s}/end", v2.FileUploadJobIdPathParameterName), resources.EndFileUploadJob).RequirePermissions(permissions.GraphDBIngest)
 
-	router.With(middleware.DefaultRateLimitMiddleware,
+	router.With(func() mux.MiddlewareFunc {
+		return middleware.DefaultRateLimitMiddleware(resources.Config)
+	},
 		// Version API
 		routerInst.GET("/api/version", v2.GetVersion).RequireAuth(),
 

cmd/api/src/config/config.go

@@ -166,6 +166,7 @@ type Configuration struct {
 	FedRAMPEULAText              string                    `json:"fedramp_eula_text"` // Enterprise only
 	EnableTextLogger             bool                      `json:"enable_text_logger"`
 	RecreateDefaultAdmin         bool                      `json:"recreate_default_admin"`
+	TrustedProxies               int                       `json:"trusted_proxies"`
 }
 
 func (s Configuration) AuthSessionTTL() time.Duration {

cmd/api/src/config/default.go

@@ -83,6 +83,7 @@ func NewDefaultConfiguration() (Configuration, error) {
 				LastName:      "User",
 				ExpireNow:     true,
 			},
+			TrustedProxies: 0,
 		}, nil
 	}
 }

cmd/api/src/go.mod

@@ -23,7 +23,6 @@ require (
 	github.com/channelmeter/iso8601duration v0.0.0-20150204201828-8da3af7a2a61
 	github.com/coreos/go-oidc/v3 v3.11.0
 	github.com/crewjam/saml v0.4.14
-	github.com/didip/tollbooth/v6 v6.1.2
 	github.com/go-chi/chi/v5 v5.0.8
 	github.com/gobeam/stringy v0.0.6
 	github.com/gofrs/uuid v4.4.0+incompatible
@@ -41,6 +40,7 @@ require (
 	github.com/russellhaering/goxmldsig v1.4.0
 	github.com/stretchr/testify v1.9.0
 	github.com/teambition/rrule-go v1.8.2
+	github.com/ulule/limiter/v3 v3.11.2
 	github.com/unrolled/secure v1.13.0
 	go.uber.org/mock v0.2.0
 	golang.org/x/oauth2 v0.23.0
@@ -57,7 +57,6 @@ require (
 	github.com/davecgh/go-spew v1.1.1 // indirect
 	github.com/felixge/httpsnoop v1.0.3 // indirect
 	github.com/go-jose/go-jose/v4 v4.0.2 // indirect
-	github.com/go-pkgz/expirable-cache v1.0.0 // indirect
 	github.com/golang/protobuf v1.5.3 // indirect
 	github.com/google/go-cmp v0.6.0 // indirect
 	github.com/jackc/pgpassfile v1.0.0 // indirect
@@ -79,7 +78,6 @@ require (
 	golang.org/x/sync v0.10.0 // indirect
 	golang.org/x/sys v0.28.0 // indirect
 	golang.org/x/text v0.21.0 // indirect
-	golang.org/x/time v0.3.0 // indirect
 	google.golang.org/protobuf v1.34.1 // indirect
 	gopkg.in/yaml.v3 v3.0.1 // indirect
 )