Closed srd90 closed 1 year ago
Thanks for the detailed report.
@srd90 You have a clear understanding of the issue. Are you willing to submit a patch for it?
Original issue description talked about combination where end user has explicitly blocked third party cookies and certain IdP setups (iframe based SLO propagation) in cross-domain IdP initiated SLO situation.
IMHO upcoming change where browser vendors start to apply samesite lax cookie policy by default (https://www.chromestatus.com/feature/5088147346030592) breaks also those cross-domain SLO scenarios (when passport-saml is involved in the loop) which use http redirect hops from one SP to another. passport-saml
keeps reporting "Success" as it has done past few years but unlike before invocation of passport's logout()
function in LogoutRequest handling implementation has no effect at all. This change of passport-saml
's behaviour due sudden lack of session cookie in SLO request would most probably go unnoticed until someone tries to access protected resources after "successfull" SLO.
For additional information about samesite see for example
IMHO in a scenario described above redirect hops from one site to another those redirects would not be treated as "a top level navigation" which would/could mean that cookies are not attached to requests. I haven't actually tested this but it seems quite obvious.
Regarding your question about patch.
It must contain at least functionality which prevents passport-saml
to report Success if there is even small chance that session is not logged out and it must not "step out of the passport modules' boundaries/sandbox" (so that it could be used as-is without breaking changes or additional requirements of new middlewares to handle requests etc.).
I don't have skills to provide such a patch and I'm not using or planning to use passport-saml
.
Patch could look something like code visible in a diff at below but I can't see how that would ever work in clustered environment with all sort of session stores and possibly concurrent http requests from same browser instance etc.
This diff contains few lines of pseudo-code and must not be used as-is in production:
diff --git a/lib/passport-saml/saml.js b/lib/passport-saml/saml.js
index e7e9283..e68fb97 100644
--- a/lib/passport-saml/saml.js
+++ b/lib/passport-saml/saml.js
@@ -287,7 +287,8 @@ SAML.prototype.generateLogoutResponse = function (req, logoutRequest) {
},
'samlp:Status': {
'samlp:StatusCode': {
- '@Value': 'urn:oasis:names:tc:SAML:2.0:status:Success'
+ //
+ '@Value': req.samlLogoutResponseStatusCode
}
}
}
diff --git a/lib/passport-saml/strategy.js b/lib/passport-saml/strategy.js
index 4d57599..56c9881 100644
--- a/lib/passport-saml/strategy.js
+++ b/lib/passport-saml/strategy.js
@@ -43,9 +43,46 @@ Strategy.prototype.authenticate = function (req, options) {
}
if (loggedOut) {
+ let reqUsersNameIdAndSessionIndexMatchesLogoutRequestInfo = false;
+ // check that we have authenticated session
+ if (req.isAuthenticated()) {
+ // check that LogoutRequest by IdP contains necessary information
+ // to check whether req.user matches with information in logout request
+ if (profile && profile.nameID && profile.sessionIndex) {
+ // check that req.user (which is alias for req.session.passport.user
+ // and content was deserialized by some storage by function provided
+ // by surrounding application) contains same nameID and sessionIndex
+ // as logoutrequest sent by IdP
+ // NOTE: assumes that deserialize function has placed nameId and sessionIndex
+ // into following positions at deserialized user
+ if (req.user.nameID && req.user.sessionIndex) {
+ // check that nameID and sessionIndex matches
+ if (req.user.nameID === profile.nameID && req.user.sessionIndex === profile.sessionIndex) {
+ reqUsersNameIdAndSessionIndexMatchesLogoutRequestInfo = true;
+ }
+ }
+ }
+ }
+ // AFAIK it is possible that same browser instance has triggered two requests to
+ // application which are handled concurrently at different nodejs instances
+ // behind LB. Based on documentation express session resaves automatically
+ // even if there are not any changes to session (and automatically if changes
+ // were made) so its possible that concurrent request handling has a session
+ // which contains user data and that session is rewritten to session store
+ // after logoutrequest handling is ended which means that logout has not happened
+ // because next request is able to deserialize user object again.
req.logout();
if (profile) {
req.samlLogoutRequest = profile;
+ req.samlLogoutResponseStatusCode = "....by default some error code....";
+
+ if (reqUsersNameIdAndSessionIndexMatchesLogoutRequestInfo === true) {
+ //
+ // assumes that req.logout() managed to perform logout successfully
+ // (see comments before req.logout() line).
+ // ugly way to pass information for saml.generateLogoutResponse(...)
+ req.samlLogoutResponseStatusCode = "....success resultcode....";
+ }
return self._saml.getLogoutResponseUrl(req, options, redirectIfSuccess);
}
return self.pass();
If patch is defined as "solution for IdP initiated logout which just works"...
Idea 1:
Lets store mapping of nameId+sessionIndex --> sessionID
(i.e. let's create secondary index) during authentication process and delete express session somehow from the session store during logout process by looking up express sessionID with nameID and sessionIndex and perform some sessionStore.deleteBySessionId
operation.
This would not work
Idea 2:
Instead of storing mapping from nameid+sessionindex to express sessionID one could store nameId+sessionIndex --> samlLoginValidNotOnAfterTimestamp
to some "saml session cache".
Existence of this mapping could be checked during every http invocation (after deserialize function has provided data to req.user ).
If query with req.user.nameID + req.user.sessionIndex does not return anything session must have been terminated and request would have to be modified so that from that point on it is treated as unauthenticated (for example with invocation of req.logout() which is alias for passport's logout function just removes user property and deletes property from session ... see implementation of that function).
If query returned a timestamp one could check if notOnOrAfter provided by IdP for this SP session has been exceeded and modify request so that it is treated as unauthenticated (and mapping should be removed).
When IdP initiated LogoutRequest is received existence of nameId+sessionIndex mapping is checked and if it exists it is removed from "saml session cache", so that subsequent lookup operations do not return anything. If passport-saml
is 100% sure that user cannot access application as authenticated used anymore it could respond with Success.
When SP is logout initiator it must clear aforementioned mapping (along with other cleanup stuff made during logout process) from "saml session cache" before it redirect browser to IdP to initiate SLO.
Problems:
passport-saml
could not be sure whether user can access application as authenticated user or not even if nameId+sessionIndex mapping is removed from "saml session cache"Difference to "idea 1" is that passport-saml
controls insert and removal of "saml session cache" content.
Ideas described above are NOT tested in any way.
Links to external sites (from this comment) were valid Sun, 9 Feb 2020
I affirm that other SAML projects are also discussing the impact of Chrome's M80 release and "SameSite=None" cookies on SLO. Here's a description of the issue on the Shibboleth wiki:
IMHO wiki entry you referenced says: ShibbolethSP's SLO is unaffected of samesite change because it doesn't use/require cookies during SLO processing. It uses information from logout request (nameid) to invalidate login session managed by ShibbolethSP's own sessionmanagement.
If appliction which sits behind ShibbolethSP rely only on ShibbolethSP's session management and shibd is used in ”active mode” then SLO over redirect and POST binding keeps working from protected application point of view.
If application which sits behind ShibbolethSP has application specific session meaning that if application uses own session management in addition to ShibbolethSP's session mgmt (and if shibd is not used in ”active mode”) it is up to application how it is able to terminate user session upon receiving notification of session termination from ShibbolethSP (which received logout request from IdP via redirect or POST binding). https://wiki.shibboleth.net/confluence/display/SHIB2/SLOWebappAdaptation https://wiki.shibboleth.net/confluence/display/SHIB2/NativeSPNotify
This issue is not about ShibbolethSP.
IMO passport-saml SP has locally exploitable vulnerability in SLO handling. It seems to has had it years and recent samesite change has increased ”impact surface” from browser installations which has been configured to block 3rd party cookies to mainstream browser installations which block cross domain cookies by default (case: iframe based SLO orchestration at e.g. Shibboleth IdPv3 side). To make things worst SLO propagation could be working from end user point of as it has been working earlier. Visual feedback could be unaffected. IdP's SLO orchestration page could be showing that passport-saml site (among other sites that participted to same SSO) has terminated session but under the hood sessions are not actually terminated anymore by passport-saml.
IdP trusts that if SAML SP instance provides Success in logout response that session is actually closed at SP side. Passport-saml answers always Success with and without web application's session cookies available during logout request handling. Because passport/express-session's session management expects cookies and passport-saml doesn't have any extra session manamement stuff there is no way passport-saml is reporting correctly under every circumstances.
I suggest adding vulnerability label to this issue.
The approach that we seem to be taking with the code the way it is relies on the correct implementation of Express and Passport: https://www.passportjs.org/docs/logout/ and https://www.passportjs.org/docs/configure/ ("sessions" section). You might also reference https://stackoverflow.com/questions/22052258/what-does-passport-session-middleware-do.
The problem that is being exposed is that passport-saml
is built upon Passport, which is built upon Express. So, if either Express or Passport can't do their thing correctly, then passport-saml
will report the wrong results. There is no feedback from Express or Passport as to whether or not they did their thing correctly. The presumption is that if there is an active user as controlled by an Expression session identifier, and a logout request is received, it is that user that will be logged out. If Express didn't correctly load the session for the user, only then will we have this issue.
Having said all that, it is completely correct to say that we are calling req.logout()
before we determine that the logout request that we received actually matches the currently logged in user, and that should be fixed. We probably want to still log out the current user, but we want to reply accurately if we were able to verify that the user for whom the logout was requested was actually logged out. The tricky part is figuring out what information we can 100% rely upon to actually do this checking as not all implementations use serialize
and deserialize
functions. I have some ideas on how to do this, but I'd like to get passport-saml
and node-saml
split before taking it on as it will require changes to both halves of the system.
@srd90 , I see you describe the changes that we've made as "half fixes" for this problem. Based on my explanation of how Express and Passport are involved here, I'd like to hear what you have to say about potentially getting the other "half" fixed, though I'm not sure there is a way as that appears to me to be outside the scope of node-saml
and passport-saml
.
For the record (for those who might read this issue comment in the future) "half fix" refer to these (see also discussions from collapsed code review comments):
tl;dr; those fixes aim to NOT return "Success" by default unless client code of this library provide function which gives "permission" to report "Success" (i.e. those fixes aim to delegate responsibility of result code of SLO to client libraries).
I described those as "half fixes" because @node-saml/passport-saml
and @node-saml/node-saml
documentation has been and is stating that these libraries support IdP initiated SLO scenarios over various bindings. IMHO that part of the README.md
files can also be read as "this library implements IdP initiated SLO support (out of the box)".
@cjbarth About "other half": I cannot see any way to implement "fully functional out of the box" support due to
[1]
express-session
(possible race conditions due to session update) combined with concurrent request handlings [1]
passport
's lack of support for "cookieless" scenarios etc. i.e. solving this would require coordinated effort with passport
project [1]
[2]
Maybe "other half" could be handled by modifying @node-saml/passport-saml
and @node-saml/node-saml
README.md
files' SLO related section so that it states something like:
Fully functional IdP initiated SLO support is not provided out of the box. You have to inspect your use cases / implementation / deployment scenarios (location of IdP in respect to SP) and consider things / cases listed e.g. at issue(s) #221 and #419. Library provides you a mechanism to veto "Success" result but it does not provide hooks/interfaces to implement support for IdP initiated SLO which would work under all circumstances. You have to do it yourself.
or something like that. If client SW implementors are eager to implement fully functional IdP initiated SLO they can inspect currently available comments and implementations (e.g. those that are discoverable via linked PRs / commits of other repositories).
[1]
https://github.com/node-saml/passport-saml/issues/419#issuecomment-583877544 (*)
[2]
https://github.com/node-saml/passport-saml/issues/419#issuecomment-863560464
(*)
For the record / FYI for those who might read this comment / issue in the future: this issue and initial comments were created against passport-saml
version ac7939fc1f74c3a350cee99d68268de7391db41e. Since then codebase has been moved to node-saml organization, converted to Typescript, splitted to two parts etc. etc.
It's possible that
passport-saml
responds to IdP initiated logout request that sessions are closed even though sessions are still alive. This can happen at least in following situation:Possible result from end use point of view: IdP reports that session related to
passport-saml
site is successfully closed even though it is not touched during logout process in anyway.This problem has potential to hit a lot of end users when chrome starts to block third party cookies by default.
Following code is sending LogoutRequests to passport-saml like
console.logs
written by example code are available at the end of the issue.Output of
reference case (with appliaction's session mgmt cookie available during LogoutRequest handling)
Output of
IdP initiated LogoutRequest must work without additional information from e.g. cookies