fix: validate "state" for OpenID (#39642) (#39744)

routers/web/user/setting/security/openid.go

@@ -5,11 +5,14 @@ package security
 
 import (
 	"errors"
+	"fmt"
 	"net/http"
+	"time"
 
 	audit_model "gitea.dev/models/audit"
 	user_model "gitea.dev/models/user"
 	"gitea.dev/modules/auth/openid"
+	"gitea.dev/modules/base"
 	"gitea.dev/modules/log"
 	"gitea.dev/modules/setting"
 	"gitea.dev/modules/util"
@@ -19,6 +22,8 @@ import (
 	"gitea.dev/services/forms"
 )
 
+const openIDAssociationCodeMinutes = 10
+
 // OpenIDPost response for change user's openid
 func OpenIDPost(ctx *context.Context) {
 	if user_model.IsFeatureDisabledWithLoginType(ctx.Doer, setting.UserFeatureManageCredentials) {
@@ -70,7 +75,8 @@ func OpenIDPost(ctx *context.Context) {
 		}
 	}
 
-	redirectTo := setting.AppURL + "user/settings/security"
+	state := base.CreateTimeLimitCode(fmt.Sprintf("openid_association:%d", ctx.Doer.ID), openIDAssociationCodeMinutes, time.Now(), nil)
+	redirectTo := setting.AppURL + "user/settings/security?state=" + state
 	url, err := openid.RedirectURL(id, redirectTo, setting.AppURL)
 	if err != nil {
 		loadSecurityData(ctx)
@@ -82,6 +88,11 @@ func OpenIDPost(ctx *context.Context) {
 }
 
 func settingsOpenIDVerify(ctx *context.Context) {
+	if !base.VerifyTimeLimitCode(time.Now(), fmt.Sprintf("openid_association:%d", ctx.Doer.ID), openIDAssociationCodeMinutes, ctx.FormString("state")) {
+		ctx.HTTPError(http.StatusForbidden)
+		return
+	}
+
 	log.Trace("Incoming call to: " + ctx.Req.URL.String())
 
 	fullURL := setting.AppURL + ctx.Req.URL.String()[1:]

routers/web/user/setting/security/openid_test.go

@@ -4,15 +4,103 @@
 package security
 
 import (
+	"fmt"
 	"net/http"
+	"net/http/httptest"
+	"net/url"
 	"testing"
+	"time"
 
 	"gitea.dev/models/unittest"
+	user_model "gitea.dev/models/user"
+	"gitea.dev/modules/base"
+	"gitea.dev/modules/setting"
+	"gitea.dev/modules/test"
+	"gitea.dev/modules/web"
 	"gitea.dev/services/contexttest"
+	"gitea.dev/services/forms"
 
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 )
 
+func TestOpenIDAssociation(t *testing.T) {
+	unittest.PrepareTestEnv(t)
+	defer test.MockVariableValue(&setting.AppURL, "https://gitea.example/")()
+	defer test.MockVariableValue(&setting.AppSubURL, "")()
+	defer test.MockVariableValue(&setting.Security.AllowedHostList, "127.0.0.1:*")()
+
+	provider := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		if r.Method == http.MethodPost {
+			assert.Equal(t, "check_authentication", r.FormValue("openid.mode"))
+			w.Header().Set("Content-Type", "text/plain")
+			fmt.Fprint(w, "ns:http://specs.openid.net/auth/2.0\nis_valid:true\n")
+			return
+		}
+		w.Header().Set("Content-Type", "text/html")
+		fmt.Fprintf(w, `<html><head><link rel="openid2.provider" href="http://%s/provider"></head></html>`, r.Host)
+	}))
+	defer provider.Close()
+	identity := provider.URL + "/identity"
+	ctx, resp := contexttest.MockContext(t, "POST /user/settings/security/openid")
+	contexttest.LoadUser(t, ctx, 2)
+	web.SetForm(ctx, &forms.AddOpenIDForm{Openid: identity})
+	OpenIDPost(ctx)
+	require.Equal(t, http.StatusSeeOther, resp.Code)
+	redirect, err := url.Parse(resp.Header().Get("Location"))
+	require.NoError(t, err)
+	callback, err := url.Parse(redirect.Query().Get("openid.return_to"))
+	require.NoError(t, err)
+	state := callback.Query().Get("state")
+	require.NotEmpty(t, state)
+
+	params := url.Values{
+		"openid.ns":             {"http://specs.openid.net/auth/2.0"},
+		"openid.mode":           {"id_res"},
+		"openid.op_endpoint":    {provider.URL + "/provider"},
+		"openid.claimed_id":     {identity},
+		"openid.identity":       {identity},
+		"openid.return_to":      {callback.String()},
+		"openid.response_nonce": {time.Now().UTC().Format("2006-01-02T15:04:05Z") + "association"},
+		"openid.assoc_handle":   {"test-association"},
+		"openid.signed":         {"op_endpoint,claimed_id,identity,return_to,response_nonce,assoc_handle"},
+		"openid.sig":            {"test-signature"},
+	}
+	for _, tc := range []struct {
+		name  string
+		uid   int64
+		state string
+	}{
+		{"unsolicited", 2, ""},
+		{"wrong state", 2, "wrong"},
+		{"other user", 1, state},
+		{"expired", 2, base.CreateTimeLimitCode("openid_association:2", openIDAssociationCodeMinutes, time.Now().Add(-time.Hour), nil)},
+		{"other purpose", 2, base.CreateTimeLimitCode("reset_password:2", openIDAssociationCodeMinutes, time.Now(), nil)},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			params.Set("state", tc.state)
+			ctx, resp := contexttest.MockContext(t, "GET /user/settings/security?"+params.Encode())
+			contexttest.LoadUser(t, ctx, tc.uid)
+			Security(ctx)
+			assert.Equal(t, http.StatusForbidden, resp.Code)
+			unittest.AssertNotExistsBean(t, &user_model.UserOpenID{URI: identity})
+		})
+	}
+
+	params.Set("state", state)
+	ctx, resp = contexttest.MockContext(t, "GET /user/settings/security?"+params.Encode())
+	contexttest.LoadUser(t, ctx, 2)
+	Security(ctx)
+	require.Equal(t, http.StatusSeeOther, resp.Code)
+	unittest.AssertExistsAndLoadBean(t, &user_model.UserOpenID{UID: 2, URI: identity})
+
+	ctx, resp = contexttest.MockContext(t, "GET /user/settings/security?"+params.Encode())
+	contexttest.LoadUser(t, ctx, 2)
+	Security(ctx)
+	assert.Equal(t, http.StatusOK, resp.Code)
+	assert.Equal(t, "Nonce already used", ctx.Flash.ErrorMsg)
+}
+
 func TestDeleteOpenIDReturnsNotFoundForOtherUsersAddress(t *testing.T) {
 	unittest.PrepareTestEnv(t)
 	ctx, _ := contexttest.MockContext(t, "POST /user/settings/security?id=1")