search

apereo / phpCAS

Apereo PHP CAS Client
https://apereo.github.io/phpCAS/
Apache License 2.0
795 stars 396 forks source link

Regression of #248 - Support long tickets #348

Closed m0n1ker closed 4 years ago

m0n1ker commented 4 years ago

It looks like a recent commit to refactor session handling (76c55909f545780f516b983aa5b92739323cb5ff) has caused a regression of #248. #258 introduced a salted SHA-256 hash to ensure session IDs were not too long in newer versions of Shibboleth with additional information encoded in the tickets. The current version of master is causing a redirect loop when used with these longer Shibboleth tickets.

phy25 commented 4 years ago

Could you attach debug logs and confirm the previous version works while the latest one does not work?

m0n1ker commented 4 years ago

Sure, I can if it would help. In the meantime I verified that this change(https://github.com/m0n1ker/phpCAS/commit/2d6e1f4265945bfe5eb7a9f6b640e034f2b8cf94) resolves the issue. It looks like this specific instance may have just been accidentally reverted. Other places are still using the _sessionIdForTicket function. If this change looks appropriate I can open a PR.

m0n1ker commented 4 years ago

Here's a debug log showing the issue with the latest version of master (3762ab809d). I've replaced our internal service domains and my SAML attributes for privacy but it demonstrates the issue. cas.log

Looking at lines 182-186 you can see where it's calling the _renameSession function with a ticket that has a length of 403 characters. Each time the redirect occurs, the following two lines are printed in the Apache error log

PHP Warning: SessionHandler::read(): The session id is too long or contains illegal characters, valid characters are a-z, A-Z, 0-9 and '-,' in /vagrant/vendor/apereo/phpcas/source/CAS/Client.php on line 1230
PHP Warning: session_start(): Failed to read session data: user (path: /var/lib/php/session/mysite.example.com) in /vagrant/vendor/apereo/phpcas/source/CAS/Client.php on line 1230
m0n1ker commented 4 years ago

It's worth noting that the handleLogoutRequests function is still using this hashing function here: https://github.com/apereo/phpCAS/blob/3762ab809d43387d52ecea8af42c6a96eb9c3ae3/source/CAS/Client.php#L1972

I believe that means this issue is also impacting sign out requests in certain situations since the session is being created using the plain ticket value, but this code is attempting to destroy the session using the hashed ticket. I'm unable to test this since I cannot sign in initially with the longer ticket.

jfritschi commented 4 years ago

Very good analysis and thank you for the pull request.

It seems due to the long wait of PR #340 idea (from 2016) it was based on an older version. During the rework of the PR this must have slipped through...