Add a session wrapper to encrypt sessiondata

[LukasReschke]

This PR extends the default PHP \SessionHandler and adds the following changes to it:

1. When running on at least PHP 5.5.1 a custom create_sid function is offering some protection against a potential session collision on systems which are likely misconfigured or misrunning. This is achieved by writing each used session to the database.
2. The whole session data is encrypted using the original session id. This one is however never written to the local system since the extended handler only writes a hashed form of it.

I decided to extend the default \SessionHandler instead of writing a custom one since there are instances already using stuff such as the memcached extension and dealing with session locking on our own can be pretty risky.

This fixes https://github.com/owncloud/enterprise/issues/518 and also is related to https://github.com/owncloud/enterprise/issues/445. Replaces #17744.

Impact

• Sessions will always be stored encrypted. I'd advise against a configuration switch since this is something were we really need proper testing which can only be ensured if each user uses this code path.
• Since the session is encrypted custom integrations that operate on $_SESSION and are not including base.php will not work anymore. Note that this does not affect ownCloud apps and the core code.
• On each request a timestamp of the last usage will get updated within the new sessions table. This needs considerations how this behaves in huge environments. One should note that this is the same approach as in https://github.com/LukasReschke/session_login_tracker which was used at https://github.com/owncloud/enterprise/issues/445 and does not seem to have had a huge performance impact. @butonic may know more about DB stuff than me 🙊
• If it was not possible to generate an unique secure session ID the server will issue a fatal error
• Adds a new table to track the used encrypted sessions. Needs upgrade testing as well as testing on each supported database.

This requires real proper testing on all supported systems and PHP versions and can in no case be backported. Since it is pretty independent from core with only one small change in base.php I'd say we should merge this within master as soon as possible to get early feedback on this. In case of a problem we can anyways remove this again.

Test plan

Local storage

• session_destroy() cleans the session from the DB as well as from the local storage
• Garbage collector is properly cleaning old sessions from the DB as well as from the local storage
• Original session ID is not stored on local storage
• Data within the session is properly encrypted
• Setup still works
• Updating the instance still works (switch from installed master to this branch)
• Cookies are not sent multiple times when not required

Memcache

• session_destroy() cleans the session from the DB as well as from the memcache
• Garbage collector is properly cleaning old sessions from the DB as well as from the memcache
• Original session ID is not stored on memcache
• Data within the session is properly encrypted
• Setup still works
• Updating the instance still works (switch from installed master to this branch)
• Cookies are not sent multiple times when not required

PHP versions tested

• PHP 7
• PHP 5.6
• PHP 5.5
• PHP 5.4

Distributions tested

• Mac OS X
• CentOS 6.6
• CentOS 7
• Ubuntu 15.04
• Ubuntu 14.10
• Debian 7
• Debian 8

---

@butonic Please review and test. Especially the memcache part requires your expertise.
@DeepDiver1975 @MorrisJobke Please review.

[LukasReschke]

Example how a session is stored now:

ecd5dedc4e12cf2b2fa01d8c137eecca31858dbb140fb5bbd63f8a31695697ba12e260ec5f3818d1cdb87aad27b1579aa73fcb7a98c8cf370449766d62ac97a16b586d0a22335a74e1e3948637d276aca670623ec0072137b8da9f71c85299814cf2d864f44bff2e7cc209b224100c55c090d930f65fe6fcd8e020945434fd90955529435d26731dc0230826c4e2161aeddc589663139ac48e862336edb747b10a6f6d0b1cb1f2f1e4648b8cbd270cbf1fc8bea0e15691f33b7abaa65719dd3d428adf9b4d5e9222d10b77bcce660327293e1abafa7baa8493884e66158fcc8625c1e90c00c9de96f7f97a72086e7054|l5m1OV27HILPwSNf|9ad7b097c202488eac065823c78b26c4d41550dcc4abe1729a49d0

vs.

OC_Version_Timestamp|i:1437825857;OC_Version|a:4:{i:0;i:8;i:1;i:2;i:2;i:0;i:3;i:3;}OC_VersionString|s:13:"8.2 pre alpha";OC_Build|s:0:"";OC_Channel|s:3:"git";user_id|s:5:"admin";loginname|s:5:"admin";

I do operate on the whole session blob here instead of the single elements.

[LukasReschke]

So and now let's spam erm summon @karlitschek and @MTRichards as well.

[RobinMcCorkell]

@LukasReschke I take it this will be very useful for enhancing SMB with OC login, making it more secure?

[LukasReschke]

@LukasReschke I take it this will be very useful for enhancing SMB with OC login, making it more secure?

This is correct. An admin can still intercept the encryption key obviously but on storage the whole session is stored encrypted.

The key is the actual session ID which is not stored on the application server in plain-text, with this change it is only stored as a hashed form. Still allowing some form of memory-time-tradeoff attacks but if an admin wants to bruteforce a 192bit strong key he can also just intercept the key 🙊

[karlitschek]

I think this is a good approach. I think it is possible that this might break some custom 3rdparty libraries or apps but let's try this for now. Maybe we need to add a switch later if we discover incompatibilities.
The 5.5.1 dependency should also be fine.
👍 for now!

[LukasReschke]

@DeepDiver1975 I'm confused. I declared last_used to be of type timestamp but MySQL makes datetime out of it which leads to problems:

mysql> describe oc_sessions;
+-----------+-------------+------+-----+---------+--------+
| Field     | Type        | Null | Key | Default | Extra  |
+-----------+-------------+------+-----+---------+--------+
| hashed_id | varchar(128) | NO   | PRI | NULL   |        |
| last_used | datetime     | YES  |     | NULL   |        |
+-----------+-------------+------+-----+---------+--------+
2 rows in set (0.00 sec)

I don't really have any clue about our DB layer. What is the most pragmatic way to handle this cross-platform? 🙊

[LukasReschke]

Especially this leads on MySQL to:

Technical details
Remote Address: ::1
Request ID: LldTuDns8N8SA5mXWChO
Type: Doctrine\DBAL\Exception\DriverException
Code: 0
Message: An exception occurred while executing 'INSERT INTO `oc_sessions` (`hashed_id`, `last_used`) VALUES(?, ?)' with params ["a9ee6e069657b61932bb255fb21d478c0e00f72bfa2f1dd28c4eaaf07867e2790dd5ed8aa5f504ae3f15f9929e590c0d691e61532495091b0469e121a3d5e886", 1437843295]: SQLSTATE[22007]: Invalid datetime format: 1292 Incorrect datetime value: '1437843295' for column 'last_used' at row 1
File: /Users/lukasreschke/Documents/Programming/master/3rdparty/doctrine/dbal/lib/Doctrine/DBAL/Driver/AbstractMySQLDriver.php
Line: 115

[RobinMcCorkell]

@LukasReschke Use the int type for timestamps (I think)

[LukasReschke]

What a simple solution that I didn't even think of 🙊

[RobinMcCorkell]

You forgot to update the unit test after your last commit:

Parameter 1 for invocation OCP\Security\ISecureRandom::generate(64, 'abcdefghijklmnopqrstuvwxyzABC...456789') does not match expected value.
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'abcdefghijklmnopqrstuvwxyz0123456789'
+'abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789'

[DeepDiver1975]

@LukasReschke should we add a user column to allow session enum on user basis?

I'm thinking about the scenario of password-change where we want to invalidate all user sessions ....

(there was an issue once by @danimo ....)

[LukasReschke]

Good call. We can indeed implement this with this approach. – I will think about an actual implementation and merge it into this PR.

Todo:

1. Add user id to database
2. Read action needs to check if session is terminated, if yes: invalidate the session

[LukasReschke]

Ok. This is rather hard to do since we have a circular dependency, in order to read the user we need to read the session which is not going to work here. There are ways to work around this by moving this code to another place but this requires quite some more changes than the integration we have right now.

I'll think about it…

[scrutinizer-notifier]

A new inspection was created.

[ghost]

🚀 Test PASSed.🚀

[LukasReschke]

@butonic Had time to test this on a memcache setup?

[DeepDiver1975]

@rperezb qa help is appreciated - THX

[jvillafanez]

It seems that php 5.5.9 doesn't like it...

{"reqId":"Bnt6IEGs5WIYFFcVCytb","remoteAddr":"82.159.139.58","app":"PHP","message":"Allowed memory size of 536870912 bytes exhausted (tried to allocate 70062853586944 bytes) at \/opt\/owncloud\/lib\/private\/session\/cryptosessionhandler.php#109","level":3,"time":"2015-08-10T07:43:05+00:00"}

If you want to check: http://docker.oc.solidgear.es:51680/ the ssh port is 51682. Feel free to use it.

[DeepDiver1975]

tried to allocate 70062853586944 bytes

70 TB?

[jvillafanez]

I've also tried with a local VM with ubuntu 14.04 and php 5.5.9 with the same result and figures

{"reqId":"M\/9NKy7jM1AMCMzMKbuc","remoteAddr":"10.40.30.21","app":"PHP","message":"Allowed memory size of 536870912 bytes exhausted (tried to allocate 69924873568256 bytes) at \/home\/jvillafanez\/src\/owncloud\/owncloud4\/lib\/private\/session\/cryptosessionhandler.php#109","level":3,"time":"2015-08-10T09:09:11+00:00"}

Checked with mysql and postgresql DBs with the same result.

Except for the VM (which I'd need to have a look to check what it contains), the other systems are fresh installed.

[LukasReschke]

php 5.5.9

Gotta love distributions which only backport security patches… 🙊

I will work around this… later…

[LukasReschke]

Ok. We need then a completely custom session handler. This is a little bit "more difficult" but not impossible. Basically this allows us to completely handle the session stuff on our own but also makes us incompatible with the default adapters. (such as the memcached one)

@butonic Do you know what session storages are used at the moment at customers? Memcache and local ones? If so I will also write a memcached backend…

[DeepDiver1975]

Ok. We need then a completely custom session handler. This is a little bit "more difficult" but not impossible. Basically this allows us to completely handle the session stuff on our own but also makes us incompatible with the default adapters. (such as the memcached one)

hmm ... kind of critical with respect to existing installations out there.
How can we ensure a proper migration path?

[LukasReschke]

Sessions are ephemeral data, so there is no permanent data stored in there. What happens when an instance that previously used Memcache as session storage via the native PHP instance updates is that it will now use the local storage unless configured otherwise within ownCloud. This can lead to problems when the load-balancer is not based on some sticky component.

In this case we would "just" need to advise the administrator that the storage handler they defined needs manual migration. We can for example block the instance when we detect that memcache has been configured. (using http://php.net/manual/en/function.session-module-name.php)

The actual migration would then be to edit config.php and enter the data required to connect to the remote session storage.

With regard to the implementation others such as Symfony have already done it and we would "only" need to adjust it: http://symfony.com/doc/current/components/http_foundation/session_configuration.html#custom-save-handlers

• Advantages:
  • Gives us complete control over how we store session and administrators cannot misconfigure their instance by mistake (leading to collisions and all kind of shenanigan).
  • We do not need to do black magic that will break some clients with regard with a second cookie.
• Disadvantages:
  • Requires that administrators change their configuration if they use memcached for session storage
  • Requires really proper QA since we're now handling all the stuff on our own.

[LukasReschke]

What we have now: This is a wrapper executing encryption stuff. It then tries to invoke the parent session handler to store the data. This does not seem to work reliably with all PHP versions.

What it would be: Wrapper executing encryption stuff, it handles storage of the data on it's own. For that we need a list of what we want to support. Suggestion: Memcache and Local.

[LukasReschke]

… or we go back to #17744, which requires however two cookies and cannot provide other advantages such as a nearly perfect detection against collisions, misconfigured and misbehaving servers etc.

It's all a trade-off ;-)

[DeepDiver1975]

What we have now: This is a wrapper executing encryption stuff. It then tries to invoke the parent session handler to store the data. This does not seem to work reliably with all PHP versions.

as a result: wrapping is not the way to go ... we should discuss this in a bigger round

@karlitschek @MTRichards @cmonteroluque

Lets set this on hold for the time being - THX

[MTRichards]

Perhaps at the conference? Can't wait too long, we need to get something in for 8.2, but of course the right something.

[DeepDiver1975]

Perhaps at the conference? Can't wait too long, we need to get something in for 8.2, but of course the right something.

well - after the conf we have only one week left for development - kind of critical to change a core component that late in the game.

[LukasReschke]

as a result: wrapping is not the way to go ... we should discuss this in a bigger round

It is possible to use wrapping but we can't call the parent session handler (inheriting) in all versions which means we need to handle session storage on our own.
(which is what other frameworks such as Symfony do for a reason ;-))

[DeepDiver1975]

(which is what other frameworks such as Symfony do for a reason ;-))

in case we go with our own implementation of the php session storage we will rely on symfony for sure

[LukasReschke]

If we want something "not as secure" but also working thing: #17744

We should however really remove the conditional switch and also be aware of the other implications. It's not going to be worse than now, but also not much better :-)

[karlitschek]

conference is too late and should be reserved for community topics anyways.

[LukasReschke]

So 8.3 for this and I will reanimate Joas' PR for 8.2? - Thoughts?

While it does not offer the same level of protections we do not need to play around with PHP internals for now...

[karlitschek]

@LukasReschke sonds like the best option. @DeepDiver1975 what do yo think?

[LukasReschke]

#18482

(closed because too buggy)