Merge branch 'security-arbitrary-protocol-redirection' into 'master'

Restrict arbitrary protocol redirection to only https or http URLs

See merge request https://gitlab.com/gitlab-org/security/gitlab-pages/-/merge_requests/49

Merged-by: Mayra Cabrera <mcabrera@gitlab.com>
Approved-by: Rohit Shambhuni <rshambhuni@gitlab.com>
Approved-by: Vladimir Shushlin <vshushlin@gitlab.com>
Approved-by: Mayra Cabrera <mcabrera@gitlab.com>
Reviewed-by: Jaime Martinez <jmartinez@gitlab.com>
Co-authored-by: ngala <ngala@gitlab.com>

2 files changed, 30 insertions, 4 deletions

diff --git a/internal/auth/auth.go b/internal/auth/auth.go
index 05e6c23c..8df98178 100644
--- a/internal/auth/auth.go
+++ b/internal/auth/auth.go
@@ -38,10 +38,11 @@ const (
 	callbackPath           = "/auth"
 	authorizeProxyTemplate = "%s?domain=%s&state=%s"
 
-	failAuthErrMsg         = "failed to authenticate request"
-	fetchAccessTokenErrMsg = "fetching access token failed"
-	queryParameterErrMsg   = "failed to parse domain query parameter"
-	saveSessionErrMsg      = "failed to save the session"
+	failAuthErrMsg             = "failed to authenticate request"
+	fetchAccessTokenErrMsg     = "fetching access token failed"
+	queryParameterErrMsg       = "failed to parse domain query parameter"
+	saveSessionErrMsg          = "failed to save the session"
+	domainQueryParameterErrMsg = "domain query parameter only supports http/https protocol"
 )
 
 var (
@@ -197,6 +198,7 @@ func (a *Auth) domainAllowed(ctx context.Context, name string, domains source.So
 	return (domain != nil && err == nil)
 }
 
+// nolint: gocyclo // TODO refactor this function https://gitlab.com/gitlab-org/gitlab-pages/-/issues/813
 func (a *Auth) handleProxyingAuth(session *hostSession, w http.ResponseWriter, r *http.Request, domains source.Source) bool {
 	// handle auth callback e.g. https://gitlab.io/auth?domain=domain&state=state
 	if shouldProxyAuthToGitlab(r) {
@@ -211,6 +213,15 @@ func (a *Auth) handleProxyingAuth(session *hostSession, w http.ResponseWriter, r
 			httperrors.Serve500(w)
 			return true
 		}
+
+		// domain query param can only contain https or http URLs.
+		if proxyurl.Scheme != "http" && proxyurl.Scheme != "https" {
+			logRequest(r).WithField("domain_query", domain).Warn(domainQueryParameterErrMsg)
+
+			httperrors.Serve401(w)
+			return true
+		}
+
 		host, _, err := net.SplitHostPort(proxyurl.Host)
 		if err != nil {
 			host = proxyurl.Host
diff --git a/internal/auth/auth_test.go b/internal/auth/auth_test.go
index e16c8f0b..9226f847 100644
--- a/internal/auth/auth_test.go
+++ b/internal/auth/auth_test.go
@@ -194,6 +194,21 @@ func TestCheckAuthenticationWhenStateIsAlreadySet(t *testing.T) {
 	require.Equal(t, "given_state", session.Values["state"], "did not reuse the pre-set state")
 }
 
+func TestTryAuthenticateWithNonHttpDomainAndState(t *testing.T) {
+	auth := createTestAuth(t, "", "")
+
+	result := httptest.NewRecorder()
+
+	r, err := http.NewRequest("Get", "https://example.com/auth?domain=mailto://example.com?body=TESTBODY&state=state", nil)
+	require.NoError(t, err)
+
+	mockCtrl := gomock.NewController(t)
+
+	mockSource := mock.NewMockSource(mockCtrl)
+	require.True(t, auth.TryAuthenticate(result, r, mockSource))
+	require.Equal(t, http.StatusUnauthorized, result.Code)
+}
+
 func testTryAuthenticateWithCodeAndState(t *testing.T, https bool) {
 	t.Helper()