From 450c0560bc832760e58ac9b1dee21bec1ae1d4e7 Mon Sep 17 00:00:00 2001 From: "Marco A." Date: Mon, 28 Sep 2026 06:10:31 +0200 Subject: [PATCH] fix(idp): reject SAML assertions from a different IdP than the intent The SAML ACS handler verified the response signature with the IdP from the request path, but resolved the user under the IdP of the login intent without comparing the two. Anyone controlling the signing key of one SAML IdP on an instance could complete a login intent of another SAML IdP and obtain a session as any user linked to it. Reject the request when the path IdP does not match the intent's IdP, in both the ACS and the SLO handler. Co-authored-by: Claude Opus 4.8 (cherry picked from commit b834f7b1b2303a6442edda565759047d61257502) --- internal/api/idp/idp.go | 26 +++++- internal/api/idp/idp_test.go | 66 +++++++++++++ internal/api/idp/integration_test/idp_test.go | 92 +++++++++++++++++++ 3 files changed, 183 insertions(+), 1 deletion(-) diff --git a/internal/api/idp/idp.go b/internal/api/idp/idp.go index 3de0a7701e..ff44d32aab 100644 --- a/internal/api/idp/idp.go +++ b/internal/api/idp/idp.go @@ -286,6 +286,14 @@ func (h *Handler) handleACS(w http.ResponseWriter, r *http.Request) { return } + // The provider/certificate used to verify the response is selected from the request path (data.IDPID), + // but the user is resolved under the intent's IDP. Make sure they are the same IDP, otherwise an + // assertion signed by one SAML IDP could be accepted for an intent targeting a different one. + if err := checkSAMLIntentIDP(data.IDPID, intent.IDPID); err != nil { + http.Error(w, err.Error(), http.StatusBadRequest) + return + } + session, err := saml2.NewSession(samlProvider, intent.RequestID, r) if err != nil { http.Error(w, err.Error(), http.StatusBadRequest) @@ -461,7 +469,12 @@ func (h *Handler) handleSLO(w http.ResponseWriter, r *http.Request) { return } - // For the moment we just make sure the callback matches the IDP it was started on / intended for. + // Make sure the callback matches the IDP the logout was started on / intended for, so a logout + // response cannot be consumed through a different SAML IDP's path. + if err := checkSAMLIntentIDP(data.IDPID, logoutState.IDPID); err != nil { + http.Error(w, err.Error(), http.StatusBadRequest) + return + } provider, err := h.getProvider(ctx, data.IDPID) if err != nil { @@ -573,6 +586,17 @@ func (h *Handler) fetchIDPUserFromCode(ctx context.Context, identityProvider idp return user, session, nil } +// checkSAMLIntentIDP ensures the IdP named in the request path (which selects the certificate used to +// verify the assertion/response) is the same IdP the intent or federated logout was started for. +// Without this check an assertion signed by one SAML IdP is accepted for an intent targeting a +// different one (IdP confusion). +func checkSAMLIntentIDP(requestIDPID, intentIDPID string) error { + if requestIDPID != intentIDPID { + return zerrors.ThrowInvalidArgument(nil, "SAML-9aj20fb1xl", "Errors.Intent.IDPInvalid") + } + return nil +} + func (h *Handler) checkExternalUser(ctx context.Context, idpID, externalUserID string) (userID string, err error) { idQuery, err := query.NewIDPUserLinkIDPIDSearchQuery(idpID) if err != nil { diff --git a/internal/api/idp/idp_test.go b/internal/api/idp/idp_test.go index 2f64f598a9..88b3074ba9 100644 --- a/internal/api/idp/idp_test.go +++ b/internal/api/idp/idp_test.go @@ -172,6 +172,72 @@ func Test_redirectToFailureURLErr(t *testing.T) { } } +func Test_checkSAMLIntentIDP(t *testing.T) { + type args struct { + requestIDPID string + intentIDPID string + } + type res struct { + err bool + } + tests := []struct { + name string + args args + res res + }{ + { + "match", + args{ + requestIDPID: "idp-a", + intentIDPID: "idp-a", + }, + res{ + err: false, + }, + }, + { + "mismatch", + args{ + requestIDPID: "idp-a", + intentIDPID: "idp-b", + }, + res{ + err: true, + }, + }, + { + "both empty match", + args{ + requestIDPID: "", + intentIDPID: "", + }, + res{ + err: false, + }, + }, + { + "one empty mismatch", + args{ + requestIDPID: "idp-a", + intentIDPID: "", + }, + res{ + err: true, + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := checkSAMLIntentIDP(tt.args.requestIDPID, tt.args.intentIDPID) + if tt.res.err { + assert.Error(t, err) + } else { + assert.NoError(t, err) + } + }) + } +} + func Test_parseCallbackRequest(t *testing.T) { type args struct { url string diff --git a/internal/api/idp/integration_test/idp_test.go b/internal/api/idp/integration_test/idp_test.go index be93c7678f..11506786e7 100644 --- a/internal/api/idp/integration_test/idp_test.go +++ b/internal/api/idp/integration_test/idp_test.go @@ -458,6 +458,64 @@ func TestServer_SAMLACS(t *testing.T) { } } +// TestServer_SAMLACSIntentIDPMismatch reproduces the SAML IdP-confusion issue: an assertion validly +// signed by and targeted at IdP-A must not be accepted for a login intent that was started for a +// different IdP-B. Before the fix the ACS handler verified the assertion against the IdP named in the +// request path but resolved the user under the intent's IdP, so the forged response below succeeded +// and returned a session as the victim. It must now be rejected. +func TestServer_SAMLACSIntentIDPMismatch(t *testing.T) { + victim := Instance.CreateHumanUser(CTX) + externalUserID := "victim-external-id" + + // IdP-B is the victim's IdP (the intent target); IdP-A is the attacker's IdP (whose signing key the + // attacker controls). Both are backed by the same instance SAML key here, which is irrelevant to the + // attack — it turns on Destination/Audience/InResponseTo, not on a distinct key. + idpB := Instance.AddSAMLRedirectProvider(CTX, "") + idpA := Instance.AddSAMLRedirectProvider(CTX, "") + Instance.CreateUserIDPlink(CTX, victim.UserId, externalUserID, idpB, externalUserID) + + baseURL := http_util.BuildOrigin(Instance.Host(), Instance.Config.Secure) + idp, err := getIDP(baseURL, []string{idpA, idpB}, externalUserID, externalUserID+"-2") + require.NoError(t, err) + + const ( + successURL = "https://example.com/success" + failureURL = "https://example.com/failure" + ) + + // Start a genuine intent for IdP-B and keep its RelayState; this is the intent the attacker confuses. + intentB, err := Client.StartIdentityProviderIntent(CTX, &user.StartIdentityProviderIntentRequest{ + IdpId: idpB, + Content: &user.StartIdentityProviderIntentRequest_Urls{Urls: &user.RedirectURLs{SuccessUrl: successURL, FailureUrl: failureURL}}, + }) + require.NoError(t, err) + authURLB, err := url.Parse(intentB.GetAuthUrl()) + require.NoError(t, err) + relayStateB := authURLB.Query().Get("RelayState") + reqB := &http.Request{Method: http.MethodGet, URL: authURLB} + + // Start an intent for IdP-A only to obtain a valid AuthnRequest targeting IdP-A's SP. + intentA, err := Client.StartIdentityProviderIntent(CTX, &user.StartIdentityProviderIntentRequest{ + IdpId: idpA, + Content: &user.StartIdentityProviderIntentRequest_Urls{Urls: &user.RedirectURLs{SuccessUrl: successURL, FailureUrl: failureURL}}, + }) + require.NoError(t, err) + authURLA, err := url.Parse(intentA.GetAuthUrl()) + require.NoError(t, err) + reqA := &http.Request{Method: http.MethodGet, URL: authURLA} + + // Forge a response validly signed for IdP-A (Destination/Audience = IdP-A's SP), but whose + // InResponseTo is copied from the IdP-B intent so it satisfies that intent's RequestID check. + response := createConfusedResponse(t, idp, reqA, reqB, externalUserID, string(saml.PersistentNameIDFormat)) + + // Post it to IdP-A's ACS with the IdP-B intent's RelayState: the URL names A, the RelayState names B. + callbackURL := baseURL + "/idps/" + idpA + "/saml/acs" + _, err = integration.CheckPost(callbackURL, httpPostFormRequest(relayStateB, response)) + // Must be rejected. Before the fix this returned a 302 to the success URL carrying the victim's + // session, so CheckPost would return no error; now the mismatch yields a 400 (no Location). + require.Error(t, err) +} + func getIDP(zitadelBaseURL string, idpIDs []string, user1, user2 string) (*saml.IdentityProvider, error) { baseURL, err := url.Parse("http://localhost:8000") if err != nil { @@ -548,6 +606,40 @@ func createResponse(t *testing.T, idp *saml.IdentityProvider, req *http.Request, return base64.StdEncoding.EncodeToString(responseBuf) } +// createConfusedResponse builds a SAML response that is validly signed for and targeted at the SP of +// spReq's IdP, but whose InResponseTo is taken from idReq (a different IdP's intent). This mirrors the +// IdP-confusion attack: the assertion verifies against one IdP while answering another IdP's intent. +func createConfusedResponse(t *testing.T, idp *saml.IdentityProvider, spReq, idReq *http.Request, nameID, nameIDFormat string) string { + sp, err := saml.NewIdpAuthnRequest(idp, spReq) + require.NoError(t, err) + require.NoError(t, sp.Validate()) + + id, err := saml.NewIdpAuthnRequest(idp, idReq) + require.NoError(t, err) + require.NoError(t, id.Validate()) + + // Keep spReq's SP (Destination/Recipient/Audience), but answer idReq's request ID so the response + // passes the RequestID (InResponseTo) check of the confused intent. + sp.Request.ID = id.Request.ID + + err = idp.AssertionMaker.MakeAssertion(sp, &saml.Session{ + CreateTime: time.Now().UTC(), + NameID: nameID, + NameIDFormat: nameIDFormat, + UserName: nameID, + }) + require.NoError(t, err) + require.NoError(t, sp.MakeResponse()) + + doc := etree.NewDocument() + doc.SetRoot(sp.ResponseEl) + responseBuf, err := doc.WriteToBytes() + require.NoError(t, err) + responseBuf = append([]byte(""), responseBuf...) + + return base64.StdEncoding.EncodeToString(responseBuf) +} + func httpGETRequest(t *testing.T, callbackURL string, relayState, response, sig, sigAlg string) *http.Request { req, err := http.NewRequest("GET", callbackURL, nil) require.NoError(t, err)