Cap the X-Forwarded-For hop walk at 64 entries (closes #124)
All checks were successful
check / check (push) Successful in 7s
All checks were successful
check / check (push) Successful in 7s
The walk now keeps only the rightmost 64 hops, so an attacker-supplied chain cannot burn unbounded CPU in the rate-limit key function. Running off the end of the truncated slice falls back to the peer address, the same fail-closed direction the rest of the function takes. Also corrects the unparseable-RemoteAddr comment, which overclaimed about Unix-socket peers.
This commit was merged in pull request #129.
This commit is contained in:
@@ -32,6 +32,13 @@ const (
|
|||||||
// receiver rate limit. The configured limit is expressed in
|
// receiver rate limit. The configured limit is expressed in
|
||||||
// requests per minute.
|
// requests per minute.
|
||||||
receiverRateInterval = 1 * time.Minute
|
receiverRateInterval = 1 * time.Minute
|
||||||
|
|
||||||
|
// maxForwardedHops bounds how many X-Forwarded-For entries the
|
||||||
|
// chain walk examines. Real chains are one to three hops, but a
|
||||||
|
// client can pad the header up to MaxHeaderBytes, so without a
|
||||||
|
// bound every request pays a walk proportional to whatever the
|
||||||
|
// client sent.
|
||||||
|
maxForwardedHops = 64
|
||||||
)
|
)
|
||||||
|
|
||||||
// normalizeAddr strips the IPv4-in-IPv6 wrapper and any zone from
|
// normalizeAddr strips the IPv4-in-IPv6 wrapper and any zone from
|
||||||
@@ -70,12 +77,19 @@ func (m *Middleware) isTrustedProxy(addr netip.Addr) bool {
|
|||||||
// a trusted proxy is the client. A hop that cannot be read as a bare
|
// a trusted proxy is the client. A hop that cannot be read as a bare
|
||||||
// address ends the walk: past it the chain is not the shape assumed
|
// address ends the walk: past it the chain is not the shape assumed
|
||||||
// here, so the caller falls back to the peer address.
|
// here, so the caller falls back to the peer address.
|
||||||
|
//
|
||||||
|
// Only the last maxForwardedHops entries are examined. A longer chain
|
||||||
|
// is padding, and running out of hops falls back to the peer address
|
||||||
|
// the same way an unreadable hop does.
|
||||||
func (m *Middleware) forwardedClientAddr(
|
func (m *Middleware) forwardedClientAddr(
|
||||||
r *http.Request,
|
r *http.Request,
|
||||||
) (netip.Addr, bool) {
|
) (netip.Addr, bool) {
|
||||||
hops := strings.Split(
|
hops := strings.Split(
|
||||||
strings.Join(r.Header.Values("X-Forwarded-For"), ","), ",",
|
strings.Join(r.Header.Values("X-Forwarded-For"), ","), ",",
|
||||||
)
|
)
|
||||||
|
if len(hops) > maxForwardedHops {
|
||||||
|
hops = hops[len(hops)-maxForwardedHops:]
|
||||||
|
}
|
||||||
|
|
||||||
for _, hop := range slices.Backward(hops) {
|
for _, hop := range slices.Backward(hops) {
|
||||||
hop = strings.TrimSpace(hop)
|
hop = strings.TrimSpace(hop)
|
||||||
@@ -113,8 +127,10 @@ func (m *Middleware) clientKey(r *http.Request) string {
|
|||||||
peer, err := netip.ParseAddr(ipFromHostPort(r.RemoteAddr))
|
peer, err := netip.ParseAddr(ipFromHostPort(r.RemoteAddr))
|
||||||
if err != nil {
|
if err != nil {
|
||||||
// Not an address we can reason about; key on the raw
|
// Not an address we can reason about; key on the raw
|
||||||
// value rather than collapsing such peers into one
|
// value, the most specific identity left. On a
|
||||||
// shared bucket.
|
// Unix-socket listener every peer carries the same
|
||||||
|
// RemoteAddr and so shares one bucket, which is the
|
||||||
|
// fail-closed direction.
|
||||||
return r.RemoteAddr
|
return r.RemoteAddr
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -8,7 +8,9 @@ import (
|
|||||||
"net/http/httptest"
|
"net/http/httptest"
|
||||||
"net/netip"
|
"net/netip"
|
||||||
"os"
|
"os"
|
||||||
|
"strings"
|
||||||
"testing"
|
"testing"
|
||||||
|
"time"
|
||||||
|
|
||||||
"github.com/stretchr/testify/assert"
|
"github.com/stretchr/testify/assert"
|
||||||
"sneak.berlin/go/webhooker/internal/config"
|
"sneak.berlin/go/webhooker/internal/config"
|
||||||
@@ -568,6 +570,45 @@ func TestRateLimitKey_ChainWalkSkipsClientPrepended(t *testing.T) {
|
|||||||
)
|
)
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// TestRateLimitKey_LongChainCapsWalkAndFallsBackToPeer covers the
|
||||||
|
// hop-walk cap. A client behind the trusted proxy can pad
|
||||||
|
// X-Forwarded-For with tens of thousands of trusted-looking hops,
|
||||||
|
// which costs a walk proportional to the padding and, once the walk
|
||||||
|
// runs off the left end of the chain, reaches the entry the client
|
||||||
|
// put there. Capping the walk stops both: the key falls back to the
|
||||||
|
// peer address, so rotating the head of the chain mints no bucket,
|
||||||
|
// and the run does not scale with the chain length.
|
||||||
|
func TestRateLimitKey_LongChainCapsWalkAndFallsBackToPeer(
|
||||||
|
t *testing.T,
|
||||||
|
) {
|
||||||
|
t.Parallel()
|
||||||
|
|
||||||
|
// 50k hops is roughly 0.9 MB, within the default
|
||||||
|
// MaxHeaderBytes.
|
||||||
|
const hops = 50000
|
||||||
|
|
||||||
|
padding := strings.Repeat(", 10.0.0.2", hops-1)
|
||||||
|
|
||||||
|
start := time.Now()
|
||||||
|
|
||||||
|
assertSharedBucket(
|
||||||
|
t, trustedProxies("10.0.0.0/8"), "10.0.0.1:44444",
|
||||||
|
func(i int) map[string]string {
|
||||||
|
return map[string]string{
|
||||||
|
headerXFF: fmt.Sprintf("9.9.9.%d%s", i+1, padding),
|
||||||
|
}
|
||||||
|
},
|
||||||
|
"a padded X-Forwarded-For chain must fall back to the "+
|
||||||
|
"peer address, not reach the client-controlled entry "+
|
||||||
|
"at the head of the chain",
|
||||||
|
)
|
||||||
|
|
||||||
|
assert.Less(
|
||||||
|
t, time.Since(start), 2*time.Second,
|
||||||
|
"the capped walk must not scale with the chain length",
|
||||||
|
)
|
||||||
|
}
|
||||||
|
|
||||||
// TestReceiverRateLimit_IgnoresForwardedFromUntrustedPeer proves
|
// TestReceiverRateLimit_IgnoresForwardedFromUntrustedPeer proves
|
||||||
// the receiver limiter uses the same gated key function as the
|
// the receiver limiter uses the same gated key function as the
|
||||||
// POST limiters.
|
// POST limiters.
|
||||||
|
|||||||
Reference in New Issue
Block a user