Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(session): Make session encryption more robust #47396

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

ChristophWurst
Copy link
Member

@ChristophWurst ChristophWurst commented Aug 21, 2024

Summary

Move encryption/decryption from a wrapper class to a PHP session handler.

TODO

  • Make it work for new installations
  • Make it work for upgrades
  • Encrypt/decrypt
  • Testing, testing, testing
  • Find a good approach for \OCP\ISession::getId

How to test - new session

  1. Clean your sessions (restart web server for local session file handler, clear Redis for redis, etc)
  2. Clear your cookies for the dev setup domain
  3. Open Nextcloud
  4. Log in with the browser network tools open

There will be a cookie named like the instanceid of your setup. The value is a combination of a shorter string, | and a longer string. The short string is the internal session ID. If you look into the local session file or Redis the session data will be encrypted binary data.

How to test - upgrade

  1. Check out master
  2. Clean your sessions (restart web server for local session file handler, clear Redis for redis, etc)
  3. Clear your cookies for the dev setup domain
  4. Open Nextcloud
  5. Log in

There will be a cookie named like the instanceid of your setup and a oc_sessionPassphrase cookie. The first cookie will have a relatively short string. f you look into the local session file or Redis the session data will not be encrypted. You should see one key encrypted_session_data with an encrypted value.

  1. Switch to this branch
  2. Reload the page with the browser network tools open

The cookies will stay the same. But the session file changes. You will no longer see the encrypted_session_data because all session data is encrypted.
Nextcloud continues to work like before.

Checklist

use function session_encode;
use function strlen;

class CryptoSessionHandler extends SessionHandler {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately writing unit tests is not an option for a session handler because PHP sessions have so many side effects. E.g. an actual session has to be opened to test read/write, but you can't close and re-open the session because opening a session sets a header, and setting headers is not possible once any kind of output was written 🫠

@ChristophWurst
Copy link
Member Author

Discovered

* TODO: Remove this in a future release with an approach such as
* https://github.com/owncloud/core/pull/17866
as I'm writing code comments for changed/new classes. Seems a similar approach was tried before in owncloud/core#17866, yet with a more complex impact. E.g. calling session_* functions from within the handler. My testing showed that this can lead to PHP session deadlocks, where PHP hangs until I kill the web server. My approach is very lean. It shouldn't run into the problems of the old PR.

if ($encryptedData === '') {
return '';
}
return $this->crypto->decrypt($encryptedData, $passphrase);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO: catch decryption error, log and continue with empty session data

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: HMAC does not match. Could not decrypt or decode encrypted session data
1 participant