logout_callback ignores the signature-verification error, so an unsigned LogoutRequest tears down a session

[shreemaan-abhishek]

Summary

logout_callback acts on a LogoutRequest/LogoutResponse whose signature failed verification, because it inspects only whether a document was parsed and never the verification error. An unsigned but schema-valid LogoutRequest therefore destroys the caller's session.

Where

lua/resty/saml.lua, logout_callback (current main, 7d88f4e):

• The POST and GET branches call parse_post / parse_redirect, then guard with if not doc then ... 400 (around lines 861, 867, 874). They never check the returned err.
• The binding layer returns the parsed document together with the verification error on a verify failure: binding_post_parse / binding_redirect_parse in src/lua_saml.c push (doc, error_string) when saml_binding_post_verify reports SAML_INVALID_SIGNATURE / SAML_UNSIGNED_IDENTITY.
• For contrast, login_callback does the right thing: if err then ... 400 (line 693). Only the logout path drops the error.

After the missing check, the request branch calls sess:destroy() (around line 910) and emits a signed LogoutResponse; issuer / NameID / SessionIndex mismatches are only WARN-logged, and LogoutRequest/@NotOnOrAfter is not read.

Reproducer

1. Authenticate normally so the browser holds an SP saml_session cookie.
2. POST a base64-encoded, unsigned, schema-valid LogoutRequest (any Issuer, any NameID) to logout_callback_uri, carrying that cookie.
3. Observed: the callback returns a redirect and the session is destroyed. Expected: the message is refused because its signature does not verify.

Verified end-to-end against a real gateway: the unsigned POST tears the session down; the same message over the GET/redirect binding is rejected only incidentally (no SigAlg), not by a verification check.

Impact

Forced logout / logout CSRF (availability, session denial). Not an authentication bypass. A request naming a different principal still tears down the current session.

Note

Pre-existing (present in v0.2.5). Distinct from #36, which is closed and concerned a LogoutRequest that smuggles a signed assertion inside Extensions and reads the wrong NameID; this issue is the separate case where the verification error itself is discarded. Related to #34.

Suggested fix

In logout_callback, check err from the binding (as login_callback does) and refuse the message when verification failed, before any sess:destroy() or response emission.