Check user matches logout request before reporting logout success

[cjbarth]

@srd90 Has reported a bug with SLO in node-saml, passport-saml, here. This adjusts passport-saml to call node-saml correctly to get a success of fail response to send to the IdP. A logout is attempted no matter what, assuming that the SAML from the IdP is meant for us. However, a logout is only reported as successful if the user that is currently logged in matches the one that is specified in the logout request from the IdP.

[cjbarth]

@mikkopiu, @markstos I am @-mentioning you because of your involvement with the above-mentioned SLO issue.

[cjbarth]

This is currently marked as a draft because it depends on the corresponding PR in node-saml landing first and being released to NPM, at least as a beta, so this code can point to it.

[srd90]

Based on discussions at #419 and in this particular PR and implementation of espoon-voltti/evaka#738 it should be clear that there aren't any silver bullets which could be used to fix passport-saml so that it would return at least proper status code at LogoutResponse let alone to fix it so that IdP initiated LogoutRequest would just work "out of the box" from client programs point of view.

One solution would be to disable SLO feature by default and let developers to explicitly enable it if and when they understand the very specific conditions when node-saml/passport-saml's SLO current implementation has any chance to work proberly (i.e. if IdP and SP are at same TLD and TLD is not at public suffix list(*) etc.).

SLO feature flag would

1. increase awareness of passport-saml's SLO flaw. This problem is not going to fade away. It is easy to ignore this SLO flaw as long as one doesn't have to make explicit decisions whether to use it (current SLO implementation) or not regardless of known issues
2. introduce incentive to create and share solution for it i.e. force client applications to take charge of using flawed SLO implementation or to create fix to it (and share that fix with rest of the community if they don't want to keep maintaining it privately)

Another solution to increase awareness and create incentive to come up with solution of/to SLO weakness would be to create e.g Github Security Advisory (https://docs.github.com/en/code-security/security-advisories/creating-a-security-advisory) and request CVE number for it. This way issue would become more visible (e.g. npm might/could raise a warning about it) and once again client programs using pasport-saml would have to evaluate and take proper actions to handle that security warning.

(*)
https://wiki.mozilla.org/Public_Suffix_List
https://publicsuffix.org/
https://publicsuffix.org/list/
( links contained valid information/were referenced 3th of July 2021 )

@cjbarth @markstos

btw. that not so long ago fixed "missing cert configuration option allows authentication bypass" issue would be also a good candidate for GSA

[cjbarth]

I feel like since nameID is required in the SAML profile, that would be how one would have to look up a user in a database, or whatever other method is used to store sessions. In any case, if we make this change to enforce a deep equal of the user object, and that doesn't happen, SLO is effectively disabled (the default becomes fail instead of success). If a deep equal works, then I think we can be confident that we are logging out the correct user.

I'll see about creating a CVE.

[srd90]

@cjbarth you commented:

I feel like since nameID is required in the SAML profile, that would be how one would have to look up a user in a database, or whatever other method is used to store sessions.

About nameids. There are different nameid formats and especially transient. See https://stackoverflow.com/questions/11693297/what-are-the-different-nameid-format-used-for

About transient and Shibboleth:

…
Strictly speaking, SAML assertions don't have to contain a name identifier. The subject may be implicitly identified as the bearer of the token or anybody able to demonstrate possession of a key. In SSO use cases, one reason for including an identifier is to enable the relying party to refer to the subject later, such as in a query, or a logout request. So-called "transient" identifiers that are generated uniquely for each assertion are often used to support those use cases and are a common pattern in Shibboleth deployments.
…
Shibboleth deployments traditionally have focused on the use of Attributes to describe subjects, and default to the use of transient name identifiers (or omitting them). Commercial SAML deployments less commonly make use of Attributes and tend to use loosely or improperly specified name identifiers.
…

source: https://wiki.shibboleth.net/confluence/display/CONCEPT/NameIdentifiers

So there are quite a few (Shibboleth) IdPs which communicate identity of the user with some attribute (i.e. IdP and SP are aware which attribute that is) instead of nameid.

So there are quite a few verify function implementations out there which expect to find some other attribute than value of NameID from the profile provided by passport-saml in order to lookup user information from some datastore (see passport samls own example of verify function which uses email attribute to fetch user data). If application defined verify functions use case is changed so that it is suddenly (from application pov) used also during logout phase and with profile without neccessary information the result would quite probably be some error condition instead of clean ”logout success/failure response”.

@cjbarth you wrote:

If a deep equal works, then I think we can be confident that we are logging out the correct user.

Unless session is reintroduced into session store due combination of some resave funtionality: https://github.com/expressjs/session/tree/0048bcac451ad867299d404aca94c79cc8bc751d#resave
and concurrent request processing see ”BTW.” from this comment: #619 (comment)

What I am trying to say is that passport-saml cannot be confident unless it is involved in authorization loop of each request or unless it delegate whole processing of logout to application.

[cjbarth]

What I am trying to say is that passport-saml cannot be confident unless it is involved in authorization loop of each request or unless it delegate whole processing of logout to application.

I think I've come to the same understanding you have. The only method that is truly in-scope for passport-saml is to delegate this to the application. See #619 (comment). It would be a braking change for sure, but at least it would call attention to the fact that a developer actually needs to think about their SLO flows, whereas right now it appears automatic to the developer.

[cjbarth]

@markstos @srd90 I think this is ready for its final review now that the node-saml piece is completed.

[markstos]

The proposal here to verify the user doing logout is sound, but it appears that the PR could use a rebase or perhaps even a fresh start in a new branch.

The current PR adds over 1,000 lines and removes over 12,000. Surely that kind of churn isn't required to verify a user during logout.

[cjbarth]

Acknowledged @markstos . I've split off some of the changes here that aren't strictly related to logout validation. That should help thin out this PR a little once that lands and I rebase this one.

#658

[markstos]

Thanks, @cjbarth. It's looking better.