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
2021-10-20 16:52:15 +11:00 · 2021-10-20 16:52:15 +11:00 · 976038424b
parent 8b7720e3bf
commit 976038424b
3 changed files with 220 additions and 1 deletions
--- 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 {
--- a/pkg/session/authentication_test.go
+++ 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)
+	}
+}
--- 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))