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.

Sign up for GitHub

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

Jump to bottom

Add a session wrapper to encrypt the data before storing it on disk #17744

Closed
wants to merge 1 commit into from
Closed

Add a session wrapper to encrypt the data before storing it on disk #17744

wants to merge 1 commit into from

Conversation

nickvergessen
Copy link
Contributor

@nickvergessen nickvergessen added this to the 8.2-current milestone Jul 20, 2015
nickvergessen
nickvergessen reviewed Jul 20, 2015
View reviewed changes
lib/private/server.php

/**
* @return \OC\Session\CryptoWrapper
**/
Copy link
Contributor Author

Choose a reason for hiding this comment

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

doc block failed

@LukasReschke
Copy link
Member

I have voiced my concerns at https://github.com/owncloud/enterprise/issues/518, please document properly in a PHPDoc that this is only obfuscation.

LukasReschke
LukasReschke reviewed Jul 20, 2015
View reviewed changes
lib/private/session/cryptowrapper.php
// TODO circular dependency
// $request = \OC::$server->getRequest();
// $this->passphrase = $request->getCookie('oc_sessionPassphrase');
$this->passphrase = $_COOKIE['oc_sessionPassphrase'];
Copy link
Member

Choose a reason for hiding this comment

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

This will make everything explode if a client does not resend the cookies - are we all REALLY aware of this?

This WILL break stuff.

Copy link
Member

Choose a reason for hiding this comment

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

Also does this check here for an empty cookie value? - In that case it will use the instance's secret which may lead to unexpected behaviour. We should hard-fail there rather.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the cookie does not exist, we will start a new one.

This WILL break stuff.

Executable code please, so I can test it.

@LukasReschke
Copy link
Member

@MTRichards @cmonteroluque I have voiced my concerns mainly ignored at https://github.com/owncloud/enterprise/issues/518 – if we add this we need to add a huge documentation in the code and as well in the documentation that this does not provide any further security against malicious administrators. – We shall not add crypto snakeoil without marking it as such, otherwise the world will laugh about us again 🙊

@MTRichards
Copy link
Contributor

I am fine with documenting this appropriately, no issues there at all.

LukasReschke
LukasReschke reviewed Jul 20, 2015
View reviewed changes
lib/private/session/cryptowrapper.php
*/
public function wrapSession(ISession $session) {
if (!($session instanceof Crypto) && $this->config->getSystemValue('encryptSessionData', false)) {
return new Crypto($session, $this->crypto, $this->passphrase);
Copy link
Member

Choose a reason for hiding this comment

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

Can we use a full namespace here or rename the class? "Crypto" is somewhat ambiguous as it is used in other contexts as well.

@scrutinizer-notifier
Copy link

A new inspection was created.

nickvergessen
nickvergessen reviewed Jul 20, 2015
View reviewed changes
lib/private/session/cryptowrapper.php
// TODO circular dependency
// $request = \OC::$server->getRequest();
// $this->passphrase = $request->getCookie(self::COOKIE_NAME);
$this->passphrase = $_COOKIE[self::COOKIE_NAME];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

copied to keep the discussion from @LukasReschke

This will make everything explode if a client does not resend the cookies - are we all REALLY aware of this?
This WILL break stuff.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Executable samples would be nice, so I can test this.

@ghost
Copy link

ghost commented Jul 20, 2015

🚀 Test PASSed.🚀
chuck

@LukasReschke
Copy link
Member

I'm actually wondering if it would be possible to have this happen on base of a cookie that is already handled properly: The PHP session cookie.

Something like:

  1. When user authenticates PHP assigns a session with a random session id to the user
  2. On the disk store the session values encrypted using a derived key from the session id all data should be encrypted using the original session id value (sent by the user). Such as PBKDF2. – Please do a benchmark for a somewhat sensible and fast solution per default. To somehow at least mitigate the risk of a time-memory trade-off it would make sense to add the username as salt. Far from perfect, yes, but better than nothing.
  3. When accessing ownCloud the user sends the random session id to the server which then ownCloud derives the stored session from and uses the sent session id as key.

@nickvergessen What do you think? – From my PoV this should be possible using a custom session handler.

@LukasReschke
Copy link
Member

Something like the following - note: simplified!

  1. User logs in, gets assigned LAHYqDaIzH
  2. ownCloud stores this session as hash_pbkdf2('sha256', 'LAHYqDaIzH', $salt, 20, 20); ($salt is subject to discussion - may be empty as well): a4bc6fa3c81b0b12b2df
  3. The data in the session is encrypted with the original session id: LAHYqDaIzH
  4. When the user with LAHYqDaIzH accesses oC, it maps this back to a4bc6fa3c81b0b12b2df and decrypts the data with LAHYqDaIzH as key.

This way we have the same level of security (though debatable) with a more stable approach and less bug potential. - Note that above algorithms are just an example :)

@karlitschek
Copy link
Contributor

I agree with @LukasReschke This might be the better and simpler solution

@LukasReschke
Copy link
Member

As discussed with @nickvergessen I will hijack this branch and try to implement my suggested approach.

@LukasReschke LukasReschke self-assigned this Jul 24, 2015
@nickvergessen
Copy link
Contributor Author

Feel free to take over, since I will be away the next weeks

@LukasReschke
Copy link
Member

But hijacking sounds so much cooler than taking over 😢

@LukasReschke
Copy link
Member

As discussed with @butonic I will also create a custom callback for create_sid with regards to users that have their system configured wrongly leading to a potential reuse of sessions and thus a data mixup.

@LukasReschke LukasReschke mentioned this pull request Jul 25, 2015
Closed
25 tasks
@LukasReschke
Copy link
Member

Superseded by #17866

@LukasReschke LukasReschke deleted the encrypt-session-data branch July 25, 2015 14:40
@LukasReschke LukasReschke restored the encrypt-session-data branch August 21, 2015 15:57
@LukasReschke LukasReschke mentioned this pull request Aug 21, 2015
Merged
@lock lock bot locked as resolved and limited conversation to collaborators Aug 10, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees

@LukasReschke LukasReschke

Labels
2 - Developing enhancement security technical debt
Projects
None yet
Milestone
8.2
Development

Successfully merging this pull request may close these issues.
6 participants
@nickvergessen @LukasReschke @MTRichards @scrutinizer-notifier @karlitschek @DeepDiver1975