BED-5301/5298: PenTest Result: Internal IP Address Disclosure AND Rate limit Bypass #1163
Conversation
…ementing our current rate limiting middleware logic
… I need to fix (maybe even add)
ALCooper12
added
the
work in progress
ALCooper12
removed
the
work in progress
| ipGetter := stdlib.WithKeyGetter( | ||
| func(r *http.Request) string { | ||
| if xff := r.Header.Get("X-Forwarded-For"); xff == "" { | ||
| slog.DebugContext(r.Context(), "No data found in X-Forwarded-For header") | ||
| return r.RemoteAddr | ||
| } else { | ||
| ips := strings.Split(xff, ",") | ||
| rightMostIP := strings.TrimSpace(ips[len(ips)-1]) | ||
|
|
||
| return rightMostIP | ||
| } | ||
| }, | ||
| ) | ||
|
|
||
| middleware := stdlib.NewMiddleware(limiter, ipGetter) | ||
|
|
||
| return middleware.Handler | ||
|
|
There was a problem hiding this comment.
I was trying to figure out why we couldn't use the reference X Forwarded For implementation from limiter but stumbled on the fact that you'd expect the leftmost to be the IP, but because we don't have multiple reverse proxies and the loadbalancer doesn't strip incoming X Forwarded For (in our case, almost always spoofed IPs), we can't rely on it. However, this puts us in a weird position because if someone is self hosting BloodHound, this will make it so anyone hosting with multiple reverse proxies/load balancers in front of them will end up detecting internal IPs for rate limiting.
I don't know if there's a one size fits all solution here, but we should probably consider the consequences before moving forward.
There was a problem hiding this comment.
We can't solely rely on the left-most IP because of spoofing but if we take the rightmost IP we're basically rate limiting all traffic that comes in via the ALB (not good).
In a production deployment we only expect there to be 1 proxy in the X-Forwarded-For chain. It's not a perfect solution but I think it's sufficient for rate limiting at the service level:
- If there is no
X-Forwarded-Forheader then limit byrequest.RemoteAddr - If there are exactly 2 IPs in the
X-Forwarded-Forchain then we can limit by the left-most IP since the only 2 IPs should be the real client ID and the ALB proxy IP. - If there are more than 2 IPs in the
X-Forwarded-Forchain then there are more proxies in the chain than we expect (likely spoofing) and we can limit on the second to last IP because it is the untrusted proxy right before it hits the ALB.
Mind you, this isn't perfect but anything more sophisticated than this requires additional configuration on the ALB and/or external services.
…orrected some logic
…pper function for better understanding and readability
| idxIP = 0 | ||
| } | ||
|
|
||
| return strings.TrimSpace(ips[idxIP]) |
There was a problem hiding this comment.
In the case of just the ALB in the chain this will limit on the ip of the ALB and not the client ip.
len(ips) == 2
cfg.TrustedProxies == 1
len(ips) - cfg.TrustedProxies == 1
ips[1] = "ALB IP"
| @@ -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"` | |||
There was a problem hiding this comment.
I don't think we need this configuration value and can assume only 1 reverse proxy for the majority of cases. Otherwise, we might consider defaulting to 1 instead because coordinating configuration changes for production deployments is painful.
Description
/api/v2/loginendpoint in order to address part of the requirements for BED-5300Motivation and Context
This PR addresses:
Why is this change required? What problem does it solve?
How Has This Been Tested?
Screenshots (optional):
UI
Terminal
Types of changes
Checklist: