From b2e0aa70889e6bccfdd915e32d3f4baf5b52d70f Mon Sep 17 00:00:00 2001 From: Ibrahim Serdar Acikgoz Date: Thu, 4 May 2023 09:29:21 +0300 Subject: [PATCH] server/utils: verify trusted ip headers are valid ip addresses (#23140) Co-authored-by: Ibrahim Serdar Acikgoz --- server/channels/utils/utils.go | 9 +- server/channels/utils/utils_test.go | 215 ++++++++++++++++------------ 2 files changed, 127 insertions(+), 97 deletions(-) diff --git a/server/channels/utils/utils.go b/server/channels/utils/utils.go index bba3d52cac5..cc98e5dadbd 100644 --- a/server/channels/utils/utils.go +++ b/server/channels/utils/utils.go @@ -107,16 +107,15 @@ func GetIPAddress(r *http.Request, trustedProxyIPHeader []string) string { } } - if address != "" { + if address != "" && net.ParseIP(address) != nil { return address } + } - if address == "" { - address, _, _ = net.SplitHostPort(r.RemoteAddr) - } + host, _, _ := net.SplitHostPort(r.RemoteAddr) - return address + return host } func GetHostnameFromSiteURL(siteURL string) string { diff --git a/server/channels/utils/utils_test.go b/server/channels/utils/utils_test.go index 6f275dd23a9..0c9784f8d6c 100644 --- a/server/channels/utils/utils_test.go +++ b/server/channels/utils/utils_test.go @@ -50,119 +50,150 @@ func TestStringSliceDiff(t *testing.T) { } func TestGetIPAddress(t *testing.T) { - // Test with a single IP in the X-Forwarded-For - httpRequest1 := http.Request{ - Header: http.Header{ - "X-Forwarded-For": []string{"10.0.0.1"}, - "X-Real-Ip": []string{"10.1.0.1"}, - }, - RemoteAddr: "10.2.0.1:12345", - } + t.Run("Single IP in the X-Forwarded-For", func(t *testing.T) { + httpRequest1 := http.Request{ + Header: http.Header{ + "X-Forwarded-For": []string{"10.0.0.1"}, + "X-Real-Ip": []string{"10.1.0.1"}, + }, + RemoteAddr: "10.2.0.1:12345", + } - assert.Equal(t, "10.0.0.1", GetIPAddress(&httpRequest1, []string{"X-Forwarded-For"})) + assert.Equal(t, "10.0.0.1", GetIPAddress(&httpRequest1, []string{"X-Forwarded-For"})) + }) - // Test with multiple IPs in the X-Forwarded-For - httpRequest2 := http.Request{ - Header: http.Header{ - "X-Forwarded-For": []string{"10.0.0.1, 10.0.0.2, 10.0.0.3"}, - "X-Real-Ip": []string{"10.1.0.1"}, - }, - RemoteAddr: "10.2.0.1:12345", - } + t.Run("Multiple IPs in the X-Forwarded-For", func(t *testing.T) { + httpRequest2 := http.Request{ + Header: http.Header{ + "X-Forwarded-For": []string{"10.0.0.1, 10.0.0.2, 10.0.0.3"}, + "X-Real-Ip": []string{"10.1.0.1"}, + }, + RemoteAddr: "10.2.0.1:12345", + } - assert.Equal(t, "10.0.0.1", GetIPAddress(&httpRequest2, []string{"X-Forwarded-For"})) + assert.Equal(t, "10.0.0.1", GetIPAddress(&httpRequest2, []string{"X-Forwarded-For"})) + }) - // Test with an empty X-Forwarded-For - httpRequest3 := http.Request{ - Header: http.Header{ - "X-Forwarded-For": []string{""}, - "X-Real-Ip": []string{"10.1.0.1"}, - }, - RemoteAddr: "10.2.0.1:12345", - } + t.Run("Empty X-Forwarded-For", func(t *testing.T) { + httpRequest3 := http.Request{ + Header: http.Header{ + "X-Forwarded-For": []string{""}, + "X-Real-Ip": []string{"10.1.0.1"}, + }, + RemoteAddr: "10.2.0.1:12345", + } - assert.Equal(t, "10.1.0.1", GetIPAddress(&httpRequest3, []string{"X-Forwarded-For", "X-Real-Ip"})) + assert.Equal(t, "10.1.0.1", GetIPAddress(&httpRequest3, []string{"X-Forwarded-For", "X-Real-Ip"})) + }) - // Test without an X-Forwarded-For - httpRequest4 := http.Request{ - Header: http.Header{ - "X-Real-Ip": []string{"10.1.0.1"}, - }, - RemoteAddr: "10.2.0.1:12345", - } + t.Run("Without an X-Forwarded-For", func(t *testing.T) { + httpRequest4 := http.Request{ + Header: http.Header{ + "X-Real-Ip": []string{"10.1.0.1"}, + }, + RemoteAddr: "10.2.0.1:12345", + } - assert.Equal(t, "10.1.0.1", GetIPAddress(&httpRequest4, []string{"X-Forwarded-For", "X-Real-Ip"})) + assert.Equal(t, "10.1.0.1", GetIPAddress(&httpRequest4, []string{"X-Forwarded-For", "X-Real-Ip"})) + }) - // Test without any headers - httpRequest5 := http.Request{ - RemoteAddr: "10.2.0.1:12345", - } + t.Run("Without any headers", func(t *testing.T) { + httpRequest5 := http.Request{ + RemoteAddr: "10.2.0.1:12345", + } - assert.Equal(t, "10.2.0.1", GetIPAddress(&httpRequest5, []string{"X-Forwarded-For", "X-Real-Ip"})) + assert.Equal(t, "10.2.0.1", GetIPAddress(&httpRequest5, []string{"X-Forwarded-For", "X-Real-Ip"})) + }) - // Test with both headers, but both untrusted - httpRequest6 := http.Request{ - Header: http.Header{ - "X-Forwarded-For": []string{"10.3.0.1"}, - "X-Real-Ip": []string{"10.1.0.1"}, - }, - RemoteAddr: "10.2.0.1:12345", - } + t.Run("Two headers, but both untrusted", func(t *testing.T) { + httpRequest6 := http.Request{ + Header: http.Header{ + "X-Forwarded-For": []string{"10.3.0.1"}, + "X-Real-Ip": []string{"10.1.0.1"}, + }, + RemoteAddr: "10.2.0.1:12345", + } - assert.Equal(t, "10.2.0.1", GetIPAddress(&httpRequest6, nil)) + assert.Equal(t, "10.2.0.1", GetIPAddress(&httpRequest6, nil)) + }) - // Test with both headers, but only X-Real-Ip trusted - httpRequest7 := http.Request{ - Header: http.Header{ - "X-Forwarded-For": []string{"10.3.0.1"}, - "X-Real-Ip": []string{"10.1.0.1"}, - }, - RemoteAddr: "10.2.0.1:12345", - } + t.Run("Two headers, but only X-Real-Ip trusted", func(t *testing.T) { + httpRequest7 := http.Request{ + Header: http.Header{ + "X-Forwarded-For": []string{"10.3.0.1"}, + "X-Real-Ip": []string{"10.1.0.1"}, + }, + RemoteAddr: "10.2.0.1:12345", + } - assert.Equal(t, "10.1.0.1", GetIPAddress(&httpRequest7, []string{"X-Real-Ip"})) + assert.Equal(t, "10.1.0.1", GetIPAddress(&httpRequest7, []string{"X-Real-Ip"})) + }) - // Test with X-Forwarded-For, comma separated, untrusted - httpRequest8 := http.Request{ - Header: http.Header{ - "X-Forwarded-For": []string{"10.3.0.1, 10.1.0.1"}, - }, - RemoteAddr: "10.2.0.1:12345", - } + t.Run("X-Forwarded-For, comma separated, untrusted", func(t *testing.T) { + httpRequest8 := http.Request{ + Header: http.Header{ + "X-Forwarded-For": []string{"10.3.0.1, 10.1.0.1"}, + }, + RemoteAddr: "10.2.0.1:12345", + } - assert.Equal(t, "10.2.0.1", GetIPAddress(&httpRequest8, nil)) + assert.Equal(t, "10.2.0.1", GetIPAddress(&httpRequest8, nil)) + }) - // Test with X-Forwarded-For, comma separated, untrusted - httpRequest9 := http.Request{ - Header: http.Header{ - "X-Forwarded-For": []string{"10.3.0.1, 10.1.0.1"}, - }, - RemoteAddr: "10.2.0.1:12345", - } + t.Run("X-Forwarded-For, comma separated, untrusted", func(t *testing.T) { + httpRequest9 := http.Request{ + Header: http.Header{ + "X-Forwarded-For": []string{"10.3.0.1, 10.1.0.1"}, + }, + RemoteAddr: "10.2.0.1:12345", + } - assert.Equal(t, "10.3.0.1", GetIPAddress(&httpRequest9, []string{"X-Forwarded-For"})) + assert.Equal(t, "10.3.0.1", GetIPAddress(&httpRequest9, []string{"X-Forwarded-For"})) + }) - // Test with both headers, both allowed, first one in trusted used - httpRequest10 := http.Request{ - Header: http.Header{ - "X-Forwarded-For": []string{"10.3.0.1"}, - "X-Real-Ip": []string{"10.1.0.1"}, - }, - RemoteAddr: "10.2.0.1:12345", - } + t.Run("Two headers, both allowed, first one in trusted used", func(t *testing.T) { + httpRequest10 := http.Request{ + Header: http.Header{ + "X-Forwarded-For": []string{"10.3.0.1"}, + "X-Real-Ip": []string{"10.1.0.1"}, + }, + RemoteAddr: "10.2.0.1:12345", + } - assert.Equal(t, "10.1.0.1", GetIPAddress(&httpRequest10, []string{"X-Real-Ip", "X-Forwarded-For"})) + assert.Equal(t, "10.1.0.1", GetIPAddress(&httpRequest10, []string{"X-Real-Ip", "X-Forwarded-For"})) + }) - // Test with multiple IPs in the X-Forwarded-For with no spaces - httpRequest11 := http.Request{ - Header: http.Header{ - "X-Forwarded-For": []string{"10.0.0.1,10.0.0.2,10.0.0.3"}, - "X-Real-Ip": []string{"10.1.0.1"}, - }, - RemoteAddr: "10.2.0.1:12345", - } + t.Run("Multiple IPs in the X-Forwarded-For with no spaces", func(t *testing.T) { + httpRequest11 := http.Request{ + Header: http.Header{ + "X-Forwarded-For": []string{"10.0.0.1,10.0.0.2,10.0.0.3"}, + "X-Real-Ip": []string{"10.1.0.1"}, + }, + RemoteAddr: "10.2.0.1:12345", + } - assert.Equal(t, "10.0.0.1", GetIPAddress(&httpRequest11, []string{"X-Forwarded-For"})) + assert.Equal(t, "10.0.0.1", GetIPAddress(&httpRequest11, []string{"X-Forwarded-For"})) + }) + + t.Run("Make sure that the parsed value from headers only accept IP Addresses", func(t *testing.T) { + httpRequest12 := http.Request{ + Header: http.Header{ + "X-Forwarded-For": []string{"127.0.0.1"}, + }, + RemoteAddr: "10.2.0.1:12345", + } + + assert.Equal(t, "127.0.0.1", GetIPAddress(&httpRequest12, []string{"X-Forwarded-For"})) + + httpRequest13 := http.Request{ + Header: http.Header{ + "X-Forwarded-For": []string{"localhost"}, + }, + RemoteAddr: "10.2.0.1:12345", + } + + assert.Equal(t, "10.2.0.1", GetIPAddress(&httpRequest13, []string{"X-Forwarded-For"})) + }) } func TestRemoveStringFromSlice(t *testing.T) {