From 976038424b94a8e3f489cf4ed23ee5efe118054e Mon Sep 17 00:00:00 2001 From: WithoutPants <53250216+WithoutPants@users.noreply.github.com> Date: Wed, 20 Oct 2021 16:52:15 +1100 Subject: [PATCH] Fix parsing ipv6 address with scope id (#1841) * Fix parsing ipv6 address with scope id Also allows connections from link local unicast address * Add unit tests --- pkg/session/authentication.go | 11 +- pkg/session/authentication_test.go | 209 ++++++++++++++++++ .../components/Changelog/versions/v0110.md | 1 + 3 files changed, 220 insertions(+), 1 deletion(-) create mode 100644 pkg/session/authentication_test.go diff --git a/pkg/session/authentication.go b/pkg/session/authentication.go index b1b155123..ff617c774 100644 --- a/pkg/session/authentication.go +++ b/pkg/session/authentication.go @@ -29,7 +29,16 @@ func CheckAllowPublicWithoutAuth(c *config.Instance, r *http.Request) error { return fmt.Errorf("error parsing remote host (%s): %w", r.RemoteAddr, err) } + // presence of scope ID in IPv6 addresses prevents parsing. Remove if present + scopeIDIndex := strings.Index(requestIPString, "%") + if scopeIDIndex != -1 { + requestIPString = requestIPString[0:scopeIDIndex] + } + requestIP := net.ParseIP(requestIPString) + if requestIP == nil { + return fmt.Errorf("unable to parse remote host (%s)", requestIPString) + } if r.Header.Get("X-FORWARDED-FOR") != "" { // Request was proxied @@ -92,7 +101,7 @@ func CheckExternalAccessTripwire(c *config.Instance) *ExternalAccessError { func isLocalIP(requestIP net.IP) bool { _, cgNatAddrSpace, _ := net.ParseCIDR("100.64.0.0/10") - return requestIP.IsPrivate() || requestIP.IsLoopback() || cgNatAddrSpace.Contains(requestIP) + return requestIP.IsPrivate() || requestIP.IsLoopback() || requestIP.IsLinkLocalUnicast() || cgNatAddrSpace.Contains(requestIP) } func isIPTrustedProxy(ip net.IP, trustedProxies []string) bool { diff --git a/pkg/session/authentication_test.go b/pkg/session/authentication_test.go new file mode 100644 index 000000000..1cf967c8f --- /dev/null +++ b/pkg/session/authentication_test.go @@ -0,0 +1,209 @@ +package session + +import ( + "errors" + "net/http" + "testing" + + "github.com/stashapp/stash/pkg/manager/config" +) + +func TestCheckAllowPublicWithoutAuth(t *testing.T) { + c := config.GetInstance() + _ = c.SetInitialMemoryConfig() + + doTest := func(caseIndex int, r *http.Request, expectedErr interface{}) { + t.Helper() + err := CheckAllowPublicWithoutAuth(c, r) + + if expectedErr == nil && err == nil { + return + } + + if expectedErr == nil { + t.Errorf("[%d]: unexpected error: %v", caseIndex, err) + return + } + + if !errors.As(err, expectedErr) { + t.Errorf("[%d]: expected %T, got %v (%T)", caseIndex, expectedErr, err, err) + return + } + } + + { + // direct connection tests + testCases := []struct { + address string + err error + }{ + {"192.168.1.1:8080", nil}, + {"192.168.1.1:8080", nil}, + {"100.64.0.1:8080", nil}, + {"127.0.0.1:8080", nil}, + {"[::1]:8080", nil}, + {"[fe80::c081:1c1a:ae39:d3cd%Ethernet 5]:9999", nil}, + {"193.168.1.1:8080", &ExternalAccessError{}}, + {"[2002:9fc4:ed97:e472:5170:5766:520c:c901]:9999", &ExternalAccessError{}}, + } + + // try with no X-FORWARDED-FOR and valid one + xFwdVals := []string{"", "192.168.1.1"} + + for i, xFwdVal := range xFwdVals { + header := make(http.Header) + header.Set("X-FORWARDED-FOR", xFwdVal) + + for ii, tc := range testCases { + r := &http.Request{ + RemoteAddr: tc.address, + Header: header, + } + + doTest((i*len(testCases) + ii), r, tc.err) + } + } + } + + { + // X-FORWARDED-FOR without trusted proxy + testCases := []struct { + proxyChain string + err error + }{ + {"192.168.1.1, 192.168.1.2, 100.64.0.1, 127.0.0.1", nil}, + {"192.168.1.1, 193.168.1.1", &ExternalAccessError{}}, + {"193.168.1.1, 192.168.1.1", &ExternalAccessError{}}, + } + + const remoteAddr = "192.168.1.1:8080" + + header := make(http.Header) + + for i, tc := range testCases { + header.Set("X-FORWARDED-FOR", tc.proxyChain) + r := &http.Request{ + RemoteAddr: remoteAddr, + Header: header, + } + + doTest(i, r, tc.err) + } + } + + { + // X-FORWARDED-FOR with trusted proxy + var trustedProxies = []string{"8.8.8.8", "4.4.4.4"} + c.Set(config.TrustedProxies, trustedProxies) + + testCases := []struct { + address string + proxyChain string + err error + }{ + {"192.168.1.1:8080", "192.168.1.1, 192.168.1.2, 100.64.0.1, 127.0.0.1", &UntrustedProxyError{}}, + {"8.8.8.8:8080", "192.168.1.2, 127.0.0.1", &UntrustedProxyError{}}, + {"8.8.8.8:8080", "193.168.1.1, 4.4.4.4", &ExternalAccessError{}}, + {"8.8.8.8:8080", "4.4.4.4", &ExternalAccessError{}}, + {"8.8.8.8:8080", "192.168.1.1, 4.4.4.4a", &UntrustedProxyError{}}, + {"8.8.8.8:8080", "192.168.1.1a, 4.4.4.4", &ExternalAccessError{}}, + {"8.8.8.8:8080", "192.168.1.1, 4.4.4.4", nil}, + {"8.8.8.8:8080", "192.168.1.1", nil}, + } + + header := make(http.Header) + + for i, tc := range testCases { + header.Set("X-FORWARDED-FOR", tc.proxyChain) + r := &http.Request{ + RemoteAddr: tc.address, + Header: header, + } + + doTest(i, r, tc.err) + } + } + + { + // test invalid request IPs + invalidIPs := []string{"192.168.1.a:9999", "192.168.1.1"} + + for _, remoteAddr := range invalidIPs { + r := &http.Request{ + RemoteAddr: remoteAddr, + } + + err := CheckAllowPublicWithoutAuth(c, r) + if errors.As(err, &UntrustedProxyError{}) || errors.As(err, &ExternalAccessError{}) { + t.Errorf("[%s]: unexpected error: %v", remoteAddr, err) + continue + } + + if err == nil { + t.Errorf("[%s]: expected error", remoteAddr) + continue + } + } + } + + { + // test overrides + r := &http.Request{ + RemoteAddr: "193.168.1.1:8080", + } + + c.Set(config.Username, "admin") + c.Set(config.Password, "admin") + + if err := CheckAllowPublicWithoutAuth(c, r); err != nil { + t.Errorf("unexpected error: %v", err) + } + + c.Set(config.Username, "") + c.Set(config.Password, "") + + // HACK - this key isn't publically exposed + c.Set("dangerous_allow_public_without_auth", true) + + if err := CheckAllowPublicWithoutAuth(c, r); err != nil { + t.Errorf("unexpected error: %v", err) + } + } +} + +func TestCheckExternalAccessTripwire(t *testing.T) { + c := config.GetInstance() + _ = c.SetInitialMemoryConfig() + + c.Set(config.SecurityTripwireAccessedFromPublicInternet, "4.4.4.4") + + // always return nil if authentication configured or dangerous key set + c.Set(config.Username, "admin") + c.Set(config.Password, "admin") + + if err := CheckExternalAccessTripwire(c); err != nil { + t.Errorf("unexpected error %v", err) + } + + c.Set(config.Username, "") + c.Set(config.Password, "") + + // HACK - this key isn't publically exposed + c.Set("dangerous_allow_public_without_auth", true) + + if err := CheckExternalAccessTripwire(c); err != nil { + t.Errorf("unexpected error %v", err) + } + + c.Set("dangerous_allow_public_without_auth", false) + + if err := CheckExternalAccessTripwire(c); err == nil { + t.Errorf("expected error %v", ExternalAccessError("4.4.4.4")) + } + + c.Set(config.SecurityTripwireAccessedFromPublicInternet, "") + + if err := CheckExternalAccessTripwire(c); err != nil { + t.Errorf("unexpected error %v", err) + } +} diff --git a/ui/v2.5/src/components/Changelog/versions/v0110.md b/ui/v2.5/src/components/Changelog/versions/v0110.md index c5ca7342e..f4f1bd188 100644 --- a/ui/v2.5/src/components/Changelog/versions/v0110.md +++ b/ui/v2.5/src/components/Changelog/versions/v0110.md @@ -9,6 +9,7 @@ * Optimised scanning process. ([#1816](https://github.com/stashapp/stash/pull/1816)) ### 🐛 Bug fixes +* Fix accessing Stash via IPv6 link local address causing security tripwire to be activated. ([#1841](https://github.com/stashapp/stash/pull/1841)) * Fix Twitter value defaulting to freeones in built-in Freeones scraper. ([#1853](https://github.com/stashapp/stash/pull/1853)) * Fix colour codes not outputting correctly when logging to file on Windows. ([#1846](https://github.com/stashapp/stash/pull/1846)) * Sort directory listings using case sensitive collation. ([#1823](https://github.com/stashapp/stash/pull/1823))