Merge branch 'security-arbitrary-protocol-redirection-15-5' into '15-5-stable'

Restrict arbitrary protocol redirection to only https or http URLs

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

Merged-by: Ahmad Tolba <atolba@gitlab.com>
Approved-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 3014936b..06c85eeb 100644
--- a/internal/auth/auth.go
+++ b/internal/auth/auth.go
@@ -39,10 +39,11 @@ const (
 	authorizeProxyTemplate = "%s?domain=%s&state=%s"
 	authSessionMaxAge      = 60 * 10 // 10 minutes
 
-	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 4236d695..77e45d8e 100644
--- a/internal/auth/auth_test.go
+++ b/internal/auth/auth_test.go
@@ -171,6 +171,21 @@ func TestTryAuthenticateWithDomainAndState(t *testing.T) {
 	require.Equal(t, "/public-gitlab.example.com/oauth/authorize?client_id=id&redirect_uri=http://pages.gitlab-example.com/auth&response_type=code&state=state&scope=scope", redirect.String())
 }
 
+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()