diff --git a/route/access_rules.go b/route/access_rules.go index c572d25a0..4b73108f7 100644 --- a/route/access_rules.go +++ b/route/access_rules.go @@ -36,14 +36,22 @@ func (t *Target) AccessDeniedHTTP(r *http.Request) bool { // check xff source if present if xff := r.Header.Get("X-Forwarded-For"); xff != "" { - // only use left-most element (client) - xff = strings.TrimSpace(strings.SplitN(xff, ",", 2)[0]) - // only continue if xff differs from host - if xff != host { - if ip = net.ParseIP(xff); ip == nil { - log.Printf("[WARN] failed to parse xff address %s", xff) + // Trusting XFF headers sent from clients is dangerous and generally + // bad practice. Therefore, we cannot assume which if any of the elements + // is the actual client address. To try and avoid the chance of spoofed + // headers and/or loose upstream proxies we validate all elements in the header. + // Specifically AWS does not strip XFF from anonymous internet sources: + // https://docs.aws.amazon.com/elasticloadbalancing/latest/classic/x-forwarded-headers.html#x-forwarded-for + // See lengthy github discussion for more background: https://github.com/fabiolb/fabio/pull/449 + for _, xip := range strings.Split(xff, ",") { + xip = strings.TrimSpace(xip) + if xip == host { + continue + } + if ip = net.ParseIP(xip); ip == nil { + log.Printf("[WARN] failed to parse xff address %s", xip) + continue } - // check xff source and return if denied if t.denyByIP(ip) { return true } diff --git a/route/access_rules_test.go b/route/access_rules_test.go index 34547b1c6..080201d11 100644 --- a/route/access_rules_test.go +++ b/route/access_rules_test.go @@ -165,11 +165,11 @@ func TestAccessRules_AccessDeniedHTTP(t *testing.T) { denied bool }{ { - desc: "denied xff and allowed remote addr", + desc: "single denied xff and allowed remote addr", target: &Target{ Opts: map[string]string{"allow": "ip:10.0.0.0/8,ip:192.168.0.0/24"}, }, - xff: "1.1.1.2, 10.11.12.13, 10.11.12.14", + xff: "10.11.12.13, 1.1.1.2, 10.11.12.14", remote: "10.11.12.1:65500", denied: true, }, @@ -183,13 +183,13 @@ func TestAccessRules_AccessDeniedHTTP(t *testing.T) { denied: true, }, { - desc: "allowed xff and allowed remote addr", + desc: "single allowed xff and allowed remote addr", target: &Target{ Opts: map[string]string{"allow": "ip:10.0.0.0/8,ip:192.168.0.0/24"}, }, xff: "10.11.12.13, 1.2.3.4", remote: "192.168.0.12:65500", - denied: false, + denied: true, }, { desc: "denied xff and denied remote addr", @@ -200,6 +200,15 @@ func TestAccessRules_AccessDeniedHTTP(t *testing.T) { remote: "200.17.18.20:65500", denied: true, }, + { + desc: "all allowed xff and allowed remote addr", + target: &Target{ + Opts: map[string]string{"allow": "ip:10.0.0.0/8,ip:192.168.0.0/24"}, + }, + xff: "10.11.12.13, 10.110.120.130", + remote: "192.168.0.12:65500", + denied: false, + }, } for i, tt := range tests {