From cd6b6519c1c12b67ae2abce7815d3724f4fa5ba0 Mon Sep 17 00:00:00 2001 From: Aaron Hurt Date: Mon, 26 Feb 2018 17:41:44 -0600 Subject: [PATCH 1/3] handle indeterminate length proxy chains - fixes #449 --- route/access_rules.go | 29 +++++++++++++++++++---------- route/access_rules_test.go | 17 +++++++++++++---- 2 files changed, 32 insertions(+), 14 deletions(-) diff --git a/route/access_rules.go b/route/access_rules.go index c572d25a0..2336bf215 100644 --- a/route/access_rules.go +++ b/route/access_rules.go @@ -36,16 +36,25 @@ 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) - } - // check xff source and return if denied - if t.denyByIP(ip) { - return true + // 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, ",") { + // ensure we only get the ip string + xip = strings.TrimSpace(xip) + // only continue if xip differs from host + if xip != host { + if ip = net.ParseIP(xip); ip == nil { + log.Printf("[WARN] failed to parse xff address %s", xip) + } + // 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 { From 3bfe186a952b4d428c6561d72e92fe7d78398f7a Mon Sep 17 00:00:00 2001 From: Aaron Hurt Date: Tue, 27 Feb 2018 15:31:11 -0600 Subject: [PATCH 2/3] flatten conditionals --- route/access_rules.go | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/route/access_rules.go b/route/access_rules.go index 2336bf215..ba2002d28 100644 --- a/route/access_rules.go +++ b/route/access_rules.go @@ -47,14 +47,15 @@ func (t *Target) AccessDeniedHTTP(r *http.Request) bool { // ensure we only get the ip string xip = strings.TrimSpace(xip) // only continue if xip differs from host - if xip != host { - if ip = net.ParseIP(xip); ip == nil { - log.Printf("[WARN] failed to parse xff address %s", xip) - } - // check xff source and return if denied - if t.denyByIP(ip) { - return true - } + if xip == host { + continue + } + if ip = net.ParseIP(xip); ip == nil { + log.Printf("[WARN] failed to parse xff address %s", xip) + } + // check xff source and return if denied + if t.denyByIP(ip) { + return true } } } From a1d7b54d4b5dfb79ebaf379a25c51ab388444e8c Mon Sep 17 00:00:00 2001 From: Aaron Hurt Date: Tue, 27 Feb 2018 16:45:48 -0600 Subject: [PATCH 3/3] clarify resulting action on failed parse and remove some self-explanatory comments --- route/access_rules.go | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/route/access_rules.go b/route/access_rules.go index ba2002d28..4b73108f7 100644 --- a/route/access_rules.go +++ b/route/access_rules.go @@ -44,16 +44,14 @@ func (t *Target) AccessDeniedHTTP(r *http.Request) bool { // 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, ",") { - // ensure we only get the ip string xip = strings.TrimSpace(xip) - // only continue if xip differs from host 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 }