[MM-69889] Improve handling of RelayState in SAML flow (#37837)

* [MM-69889] Improve handling of RelayState in SAML flow

RelayState was base64-decoded and trusted without any integrity check,
letting its contents be tampered with client-side. Sign relayProps with
an HMAC key (generated once, cached, stored like AsymmetricSigningKey)
before handing it to the IdP, and verify the signature before trusting
any of its fields on the way back.

* Add short expiry to signed RelayState

Bound the signed RelayState's validity to 5 minutes to restrict the
window in which a captured, unmodified RelayState could be replayed.

* [MM-69889] Use maps.Copy in SignSamlRelayState

---------

Co-authored-by: Mattermost Build <build@mattermost.com>
This commit is contained in:
Bill Gardner
2026-08-14 09:36:17 +02:00
committed by GitHub
co-authored by Mattermost Build
parent 663ad3dae9
commit 22eaa8b03b
9 changed files with 452 additions and 19 deletions
+6 -1
View File
@@ -42,7 +42,8 @@ type Channels struct {
filestore filestore.FileBackend
exportFilestore filestore.FileBackend
postActionCookieSecret []byte
postActionCookieSecret []byte
samlRelayStateSigningKey []byte
pluginCommandsLock sync.RWMutex
pluginCommands []*PluginCommand
@@ -313,6 +314,10 @@ func (ch *Channels) Start() error {
return errors.Wrapf(err, "unable to ensure PostAction cookie secret")
}
if err := ch.ensureSamlRelayStateSigningKey(); err != nil {
return errors.Wrapf(err, "unable to ensure SAML RelayState signing key")
}
return nil
}
+68
View File
@@ -133,6 +133,66 @@ func (ch *Channels) ensurePostActionCookieSecret() error {
return nil
}
// ensureSamlRelayStateSigningKey ensures that the key for signing SAML RelayState exists
// and future calls to SamlRelayStateSigningKey will always return a valid key, same on all
// servers in the cluster
func (ch *Channels) ensureSamlRelayStateSigningKey() error {
if ch.samlRelayStateSigningKey != nil {
return nil
}
var secret *model.SystemSamlRelayStateSigningKey
value, err := ch.srv.Store().System().GetByName(model.SystemSamlRelayStateSigningKeyKey)
if err == nil {
if err := json.Unmarshal([]byte(value.Value), &secret); err != nil {
return err
}
}
// If we don't already have a key, try to generate one.
if secret == nil {
newSecret := &model.SystemSamlRelayStateSigningKey{
Secret: make([]byte, 32),
}
_, err := rand.Reader.Read(newSecret.Secret)
if err != nil {
return err
}
system := &model.System{
Name: model.SystemSamlRelayStateSigningKeyKey,
}
v, err := json.Marshal(newSecret)
if err != nil {
return err
}
system.Value = string(v)
// If we were able to save the key, use it, otherwise log the error.
if err = ch.srv.Store().System().Save(system); err != nil {
mlog.Warn("Failed to save SamlRelayStateSigningKey", mlog.Err(err))
} else {
secret = newSecret
}
}
// If we weren't able to save a new key above, another server must have beat us to it. Get the
// key from the database, and if that fails, error out.
if secret == nil {
value, err := ch.srv.Store().System().GetByName(model.SystemSamlRelayStateSigningKeyKey)
if err != nil {
return err
}
if err := json.Unmarshal([]byte(value.Value), &secret); err != nil {
return err
}
}
ch.samlRelayStateSigningKey = secret.Secret
return nil
}
func (s *Server) ensureInstallationDate() error {
_, appErr := s.platform.GetSystemInstallDate()
if appErr == nil {
@@ -188,6 +248,14 @@ func (a *App) PostActionCookieSecret() []byte {
return a.ch.PostActionCookieSecret()
}
func (ch *Channels) SamlRelayStateSigningKey() []byte {
return ch.samlRelayStateSigningKey
}
func (a *App) SamlRelayStateSigningKey() []byte {
return a.ch.SamlRelayStateSigningKey()
}
func (a *App) GetCookieDomain() string {
if *a.Config().ServiceSettings.AllowCookiesForSubdomains {
if siteURL, err := url.Parse(*a.Config().ServiceSettings.SiteURL); err == nil {
+1
View File
@@ -31,6 +31,7 @@ func GetMockStoreForSetupFunctions() *mocks.Store {
systemStore.On("GetByName", "ContentExtractionConfigMigrationComplete").Return(&model.System{Name: "ContentExtractionConfigMigrationComplete", Value: "true"}, nil)
systemStore.On("GetByName", "AsymmetricSigningKey").Return(nil, model.NewAppError("FakeError", "app.system.get_by_name.app_error", nil, "", http.StatusInternalServerError))
systemStore.On("GetByName", "PostActionCookieSecret").Return(nil, model.NewAppError("FakeError", "app.system.get_by_name.app_error", nil, "", http.StatusInternalServerError))
systemStore.On("GetByName", "SamlRelayStateSigningKey").Return(nil, model.NewAppError("FakeError", "app.system.get_by_name.app_error", nil, "", http.StatusInternalServerError))
systemStore.On("GetByName", "InstallationDate").Return(&model.System{Name: "InstallationDate", Value: strconv.FormatInt(model.GetMillis(), 10)}, nil)
systemStore.On("GetByName", "FirstServerRunTimestamp").Return(&model.System{Name: "FirstServerRunTimestamp", Value: "10"}, nil)
systemStore.On("GetByName", "AdvancedPermissionsMigrationComplete").Return(&model.System{Name: "AdvancedPermissionsMigrationComplete", Value: "true"}, nil)
+13 -6
View File
@@ -4,7 +4,6 @@
package web
import (
b64 "encoding/base64"
"html"
"net/http"
"strconv"
@@ -18,6 +17,11 @@ import (
const maxSAMLResponseSize = 2 * 1024 * 1024 // 2MB
// maxRelayStatePropsSize is a sanity bound on the signed relayProps payload size. It isn't tied
// to any storage constraint (RelayState is no longer persisted) - it just keeps a maliciously
// large redirect_to from bloating the RelayState round-tripped through the IdP indefinitely.
const maxRelayStatePropsSize = 4096
func (w *Web) InitSaml() {
w.MainRouter.Handle("/login/sso/saml", w.APIHandler(loginWithSaml)).Methods(http.MethodGet)
w.MainRouter.Handle("/login/sso/saml", w.APIHandlerTrustRequester(completeSaml)).Methods(http.MethodPost)
@@ -86,7 +90,12 @@ func loginWithSaml(c *Context, w http.ResponseWriter, r *http.Request) {
relayProps[model.UserAuthServiceIsMobile] = strconv.FormatBool(isMobile)
if len(relayProps) > 0 {
relayState = b64.StdEncoding.EncodeToString([]byte(model.MapToJSON(relayProps)))
if size := len(model.MapToJSON(relayProps)); size > maxRelayStatePropsSize {
c.Err = model.NewAppError("loginWithSaml", "api.user.saml.relay_state_too_long.app_error", nil, "", http.StatusBadRequest)
return
}
relayState = model.SignSamlRelayState(c.App.SamlRelayStateSigningKey(), relayProps)
}
data, err := samlInterface.BuildRequest(c.AppContext, relayState)
@@ -112,14 +121,12 @@ func completeSaml(c *Context, w http.ResponseWriter, r *http.Request) {
relayProps := make(map[string]string)
if relayState != "" {
stateStr := ""
b, err := b64.StdEncoding.DecodeString(relayState)
props, err := model.VerifySamlRelayState(c.App.SamlRelayStateSigningKey(), relayState)
if err != nil {
c.Err = model.NewAppError("completeSaml", "api.user.authorize_oauth_user.invalid_state.app_error", nil, "", http.StatusFound).Wrap(err)
return
}
stateStr = string(b)
relayProps = model.MapFromJSON(strings.NewReader(stateStr))
relayProps = props
}
auditRec := c.MakeAuditRecord(model.AuditEventCompleteSaml, model.AuditStatusFail)
+131 -12
View File
@@ -5,13 +5,21 @@ package web
import (
"encoding/base64"
"net/http"
"net/http/httptest"
"net/url"
"strings"
"testing"
saml2 "github.com/mattermost/gosaml2"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/mock"
"github.com/stretchr/testify/require"
"github.com/mattermost/mattermost/server/public/model"
"github.com/mattermost/mattermost/server/v8/channels/app"
"github.com/mattermost/mattermost/server/v8/einterfaces"
"github.com/mattermost/mattermost/server/v8/einterfaces/mocks"
)
// TestSamlCallbackIncludesSrvParameter verifies that mobile SAML callbacks
@@ -67,22 +75,133 @@ func TestSamlCallbackIncludesSrvParameter(t *testing.T) {
})
}
// TestCompleteSamlRelayState tests that relay state is properly handled
func TestCompleteSamlRelayState(t *testing.T) {
t.Run("should decode relay state correctly", func(t *testing.T) {
relayProps := map[string]string{
"action": model.OAuthActionMobile,
"redirect_to": "mmauth://callback",
// registerFakeSamlInterface installs a mocked SamlInterface before the server is created,
// mirroring the pattern in channels/app/enterprise_test.go. Must be called before Setup(t).
func registerFakeSamlInterface(t *testing.T) *mocks.SamlInterface {
t.Helper()
fakeSaml := &mocks.SamlInterface{}
fakeSaml.On("ConfigureSP", mock.Anything).Return(nil)
app.RegisterSamlInterface(func(a *app.App) einterfaces.SamlInterface {
return fakeSaml
})
t.Cleanup(func() { app.RegisterSamlInterface(nil) })
return fakeSaml
}
func postCompleteSaml(t *testing.T, th *TestHelper, samlResponse, relayState string) *httptest.ResponseRecorder {
t.Helper()
form := url.Values{}
form.Set("SAMLResponse", samlResponse)
form.Set("RelayState", relayState)
req := httptest.NewRequest(http.MethodPost, "/login/sso/saml", strings.NewReader(form.Encode()))
req.Header.Set("Content-Type", "application/x-www-form-urlencoded")
res := httptest.NewRecorder()
th.Web.MainRouter.ServeHTTP(res, req)
return res
}
// TestCompleteSamlRelayStateRejectsForgedRelayState verifies that completeSaml rejects the
// pre-fix RelayState format (plain base64(JSON), no signature - the shape of the MM-69889
// forged team_id/invite_id attack) as well as arbitrary tampered/malformed strings, since
// RelayState must now carry a valid HMAC signature produced by the server.
func TestCompleteSamlRelayStateRejectsForgedRelayState(t *testing.T) {
fakeSaml := registerFakeSamlInterface(t)
th := Setup(t)
t.Run("legacy base64-encoded relayProps blob is rejected", func(t *testing.T) {
forgedRelayProps := map[string]string{
"action": model.OAuthActionSignup,
"team_id": "forged-team-id",
}
forgedRelayState := base64.StdEncoding.EncodeToString([]byte(model.MapToJSON(forgedRelayProps)))
relayState := base64.StdEncoding.EncodeToString([]byte(model.MapToJSON(relayProps)))
res := postCompleteSaml(t, th, "dummy-encoded-xml", forgedRelayState)
// Decode and verify
decoded, err := base64.StdEncoding.DecodeString(relayState)
assert.Equal(t, http.StatusFound, res.Code)
fakeSaml.AssertNotCalled(t, "DoLogin", mock.Anything, mock.Anything, mock.Anything)
})
t.Run("arbitrary tampered string is rejected", func(t *testing.T) {
res := postCompleteSaml(t, th, "dummy-encoded-xml", "not-a-real-relay-state")
assert.Equal(t, http.StatusFound, res.Code)
fakeSaml.AssertNotCalled(t, "DoLogin", mock.Anything, mock.Anything, mock.Anything)
})
t.Run("validly-formatted but tampered payload is rejected", func(t *testing.T) {
signed := model.SignSamlRelayState(th.App.SamlRelayStateSigningKey(), map[string]string{
"action": model.OAuthActionSignup,
"team_id": "legit-team-id",
})
payloadPart, sigPart, ok := strings.Cut(signed, ".")
require.True(t, ok)
payload, err := base64.RawURLEncoding.DecodeString(payloadPart)
require.NoError(t, err)
tampered := strings.Replace(string(payload), "legit-team-id", "forged-team-id", 1)
require.NotEqual(t, string(payload), tampered)
decodedProps := model.MapFromJSON(strings.NewReader(string(decoded)))
assert.Equal(t, model.OAuthActionMobile, decodedProps["action"])
assert.Equal(t, "mmauth://callback", decodedProps["redirect_to"])
forgedRelayState := base64.RawURLEncoding.EncodeToString([]byte(tampered)) + "." + sigPart
res := postCompleteSaml(t, th, "dummy-encoded-xml", forgedRelayState)
assert.Equal(t, http.StatusFound, res.Code)
fakeSaml.AssertNotCalled(t, "DoLogin", mock.Anything, mock.Anything, mock.Anything)
})
}
// TestCompleteSamlRelayStateSignedRoundTrip verifies the fixed flow end-to-end: loginWithSaml
// signs relayProps into an opaque RelayState, and completeSaml verifies the signature and
// recovers the original relayProps.
func TestCompleteSamlRelayStateSignedRoundTrip(t *testing.T) {
fakeSaml := registerFakeSamlInterface(t)
th := Setup(t).InitBasic(t)
var capturedRelayState string
fakeSaml.On("BuildRequest", mock.Anything, mock.AnythingOfType("string")).
Run(func(args mock.Arguments) { capturedRelayState = args.String(1) }).
Return(&model.SamlAuthRequest{URL: "https://idp.example.com/sso"}, nil)
req := httptest.NewRequest(http.MethodGet, "/login/sso/saml?action=login&id=inviteABC", nil)
res := httptest.NewRecorder()
th.Web.MainRouter.ServeHTTP(res, req)
require.Equal(t, http.StatusFound, res.Code)
// RelayState on the wire must be payload.signature - exactly one separator, both halves
// valid base64url.
require.NotEmpty(t, capturedRelayState)
payloadPart, sigPart, ok := strings.Cut(capturedRelayState, ".")
require.True(t, ok)
assert.False(t, strings.Contains(sigPart, "."))
_, err := base64.RawURLEncoding.DecodeString(payloadPart)
assert.NoError(t, err)
_, err = base64.RawURLEncoding.DecodeString(sigPart)
assert.NoError(t, err)
var capturedRelayProps map[string]string
fakeSaml.On("DoLogin", mock.Anything, mock.Anything, mock.AnythingOfType("map[string]string")).
Run(func(args mock.Arguments) { capturedRelayProps = args.Get(2).(map[string]string) }).
Return(th.BasicUser, (*saml2.AssertionInfo)(nil), nil)
res = postCompleteSaml(t, th, "dummy-encoded-xml", capturedRelayState)
assert.Equal(t, http.StatusFound, res.Code)
assert.Equal(t, model.OAuthActionLogin, capturedRelayProps["action"])
assert.Equal(t, "inviteABC", capturedRelayProps["invite_id"])
// The internal expiry field is bookkeeping only and must not leak into relayProps.
_, hasExp := capturedRelayProps["exp"]
assert.False(t, hasExp)
// A signed RelayState is not single-use: replaying it within its expiry window succeeds
// again. Restricting the replay window to a few minutes (rather than eliminating replay
// entirely) is an intentional, low-cost defense-in-depth measure requested by the security
// team, on top of the fact that no RelayState field is trusted without independent
// revalidation at the point it's consumed (invite validity, redirect_to scheme/host, etc.).
res = postCompleteSaml(t, th, "dummy-encoded-xml", capturedRelayState)
assert.Equal(t, http.StatusFound, res.Code)
fakeSaml.AssertNumberOfCalls(t, "DoLogin", 2)
}
+4
View File
@@ -5142,6 +5142,10 @@
"id": "api.user.saml.not_available.app_error",
"translation": "SAML 2.0 is not configured or supported on this server."
},
{
"id": "api.user.saml.relay_state_too_long.app_error",
"translation": "SAML relay state is too long."
},
{
"id": "api.user.send_cloud_welcome_email.error",
"translation": "Failed to send cloud welcome email"
+78
View File
@@ -0,0 +1,78 @@
// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved.
// See LICENSE.txt for license information.
package model
import (
"bytes"
"crypto/hmac"
"crypto/sha256"
"encoding/base64"
"errors"
"maps"
"strconv"
"strings"
)
// SamlRelayStateExpiryTime bounds how long a signed RelayState is valid for. It only needs to
// survive a single SAML login round-trip, so this is intentionally short.
const SamlRelayStateExpiryTime = 1000 * 60 * 5 // 5 minutes
const samlRelayStateExpKey = "exp"
// SignSamlRelayState signs relayProps with key and returns an opaque, tamper-evident RelayState
// string safe to round-trip through the IdP. relayProps itself is not modified.
func SignSamlRelayState(key []byte, relayProps map[string]string) string {
signed := make(map[string]string, len(relayProps)+1)
maps.Copy(signed, relayProps)
signed[samlRelayStateExpKey] = strconv.FormatInt(GetMillis()+SamlRelayStateExpiryTime, 10)
payload := []byte(MapToJSON(signed))
mac := hmac.New(sha256.New, key)
mac.Write(payload)
return base64.RawURLEncoding.EncodeToString(payload) + "." + base64.RawURLEncoding.EncodeToString(mac.Sum(nil))
}
// VerifySamlRelayState verifies a RelayState produced by SignSamlRelayState. It returns the
// original relayProps (with the expiry field stripped) if, and only if, the signature is valid
// and the embedded expiry has not passed.
func VerifySamlRelayState(key []byte, relayState string) (map[string]string, error) {
payloadPart, sigPart, ok := strings.Cut(relayState, ".")
if !ok {
return nil, errors.New("malformed relay state")
}
payload, err := base64.RawURLEncoding.DecodeString(payloadPart)
if err != nil {
return nil, errors.New("malformed relay state payload")
}
sig, err := base64.RawURLEncoding.DecodeString(sigPart)
if err != nil {
return nil, errors.New("malformed relay state signature")
}
mac := hmac.New(sha256.New, key)
mac.Write(payload)
if !hmac.Equal(sig, mac.Sum(nil)) {
return nil, errors.New("invalid relay state signature")
}
relayProps := MapFromJSON(bytes.NewReader(payload))
expStr, ok := relayProps[samlRelayStateExpKey]
if !ok {
return nil, errors.New("relay state missing expiry")
}
exp, err := strconv.ParseInt(expStr, 10, 64)
if err != nil {
return nil, errors.New("relay state has invalid expiry")
}
if GetMillis() > exp {
return nil, errors.New("relay state expired")
}
delete(relayProps, samlRelayStateExpKey)
return relayProps, nil
}
@@ -0,0 +1,146 @@
// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved.
// See LICENSE.txt for license information.
package model
import (
"crypto/hmac"
"crypto/rand"
"crypto/sha256"
"encoding/base64"
"strconv"
"strings"
"testing"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)
func testSamlRelayStateKey(t *testing.T) []byte {
t.Helper()
key := make([]byte, 32)
_, err := rand.Read(key)
require.NoError(t, err)
return key
}
func TestSignAndVerifySamlRelayStateRoundTrip(t *testing.T) {
key := testSamlRelayStateKey(t)
relayProps := map[string]string{
"action": OAuthActionSignup,
"invite_id": "some-invite-id",
}
relayState := SignSamlRelayState(key, relayProps)
require.NotEmpty(t, relayState)
got, err := VerifySamlRelayState(key, relayState)
require.NoError(t, err)
assert.Equal(t, OAuthActionSignup, got["action"])
assert.Equal(t, "some-invite-id", got["invite_id"])
// The expiry field is internal bookkeeping and must not leak into the caller's relayProps.
_, hasExp := got["exp"]
assert.False(t, hasExp)
}
func TestSignSamlRelayStateDoesNotMutateInput(t *testing.T) {
key := testSamlRelayStateKey(t)
relayProps := map[string]string{"action": OAuthActionLogin}
SignSamlRelayState(key, relayProps)
assert.Equal(t, map[string]string{"action": OAuthActionLogin}, relayProps)
}
func TestVerifySamlRelayStateRejectsTamperedPayload(t *testing.T) {
key := testSamlRelayStateKey(t)
relayState := SignSamlRelayState(key, map[string]string{"action": OAuthActionSignup, "team_id": "legit-team"})
payloadPart, sigPart, ok := strings.Cut(relayState, ".")
require.True(t, ok)
payload, err := base64.RawURLEncoding.DecodeString(payloadPart)
require.NoError(t, err)
tampered := strings.Replace(string(payload), "legit-team", "forged-team", 1)
require.NotEqual(t, string(payload), tampered, "test payload must actually contain the substring being tampered")
forgedRelayState := base64.RawURLEncoding.EncodeToString([]byte(tampered)) + "." + sigPart
_, err = VerifySamlRelayState(key, forgedRelayState)
assert.Error(t, err)
}
func TestVerifySamlRelayStateRejectsTamperedSignature(t *testing.T) {
key := testSamlRelayStateKey(t)
relayState := SignSamlRelayState(key, map[string]string{"action": OAuthActionSignup})
payloadPart, sigPart, ok := strings.Cut(relayState, ".")
require.True(t, ok)
sig, err := base64.RawURLEncoding.DecodeString(sigPart)
require.NoError(t, err)
sig[0] ^= 0xFF
forgedRelayState := payloadPart + "." + base64.RawURLEncoding.EncodeToString(sig)
_, err = VerifySamlRelayState(key, forgedRelayState)
assert.Error(t, err)
}
func TestVerifySamlRelayStateRejectsWrongKey(t *testing.T) {
signingKey := testSamlRelayStateKey(t)
verifyingKey := testSamlRelayStateKey(t)
relayState := SignSamlRelayState(signingKey, map[string]string{"action": OAuthActionSignup})
_, err := VerifySamlRelayState(verifyingKey, relayState)
assert.Error(t, err)
}
func TestVerifySamlRelayStateRejectsMalformedInput(t *testing.T) {
key := testSamlRelayStateKey(t)
testCases := []string{
"",
"not-a-real-relay-state",
"missing-separator-entirely",
"!!!invalid-base64!!!." + base64.RawURLEncoding.EncodeToString([]byte("sig")),
base64.RawURLEncoding.EncodeToString([]byte("{}")) + ".!!!invalid-base64!!!",
}
for _, tc := range testCases {
_, err := VerifySamlRelayState(key, tc)
assert.Error(t, err, "expected rejection for input: %q", tc)
}
}
func TestVerifySamlRelayStateRejectsExpired(t *testing.T) {
key := testSamlRelayStateKey(t)
// Hand-construct a relay state with a backdated expiry, using the same wire format as
// SignSamlRelayState, since that function always embeds a fresh (non-expired) expiry.
expiredProps := map[string]string{
"action": OAuthActionSignup,
"exp": strconv.FormatInt(GetMillis()-1000, 10),
}
payload := []byte(MapToJSON(expiredProps))
mac := hmac.New(sha256.New, key)
mac.Write(payload)
relayState := base64.RawURLEncoding.EncodeToString(payload) + "." + base64.RawURLEncoding.EncodeToString(mac.Sum(nil))
_, err := VerifySamlRelayState(key, relayState)
assert.Error(t, err)
}
func TestVerifySamlRelayStateRejectsLegacyBase64Blob(t *testing.T) {
key := testSamlRelayStateKey(t)
// The pre-fix RelayState format: plain base64(JSON), no signature at all. Confirms the
// old wire format (and thus the MM-69889 forged team_id/invite_id attack shape) is rejected.
legacy := base64.StdEncoding.EncodeToString([]byte(MapToJSON(map[string]string{
"action": OAuthActionSignup,
"team_id": "forged-team-id",
})))
_, err := VerifySamlRelayState(key, legacy)
assert.Error(t, err)
}
+5
View File
@@ -15,6 +15,7 @@ const (
SystemLastComplianceTime = "LastComplianceTime"
SystemAsymmetricSigningKeyKey = "AsymmetricSigningKey"
SystemPostActionCookieSecretKey = "PostActionCookieSecret"
SystemSamlRelayStateSigningKeyKey = "SamlRelayStateSigningKey"
SystemInstallationDateKey = "InstallationDate"
SystemOrganizationName = "OrganizationName"
SystemFirstAdminRole = "FirstAdminRole"
@@ -62,6 +63,10 @@ type SystemPostActionCookieSecret struct {
Secret []byte `json:"key,omitempty"`
}
type SystemSamlRelayStateSigningKey struct {
Secret []byte `json:"key,omitempty"`
}
type SystemAsymmetricSigningKey struct {
ECDSAKey *SystemECDSAKey `json:"ecdsa_key,omitempty"`
}