Implement Server side encryption #160

brantje · 2017-01-02T19:07:12Z

This improves the general security of passman by encrypting the credentials again, but this time on the server side. It used AES-256 as cipher with CBC as method.

Key is stretched using the PBKDF2 algorithm.
Hides the Initialization Vector
Does HMAC verification of source data

Todo:

Add update script to convert passwords prior to RC4
Add <lib>openssl</lib> under <dependencies>

animalillo · 2017-01-03T01:45:00Z

I will be away for ~1 week, will review this when I'm back.

EdOverflow · 2017-01-09T17:25:18Z

lib/Service/EncryptService.php

+	/**
+	 * @var int $rounds The number of rounds to feed into PBKDF2 for key generation
+	 */
+	protected $rounds = 100;


It might be a good idea to bump this up to 10'000 rounds in production.

i think 10000 rounds might be too much for production, also remind this is just a feature that adds an extra layer of security in case somebody unauthorized got access to the database.
Remember this code will be executed on almost every api request on passman, we need to keep it fast!

Would you please experiment with different values to test performance? 100 appears to be far too low.

5000 seems nice in between.

Apache .htpasswd "APR1" and OpenSSL "passwd" use 1000 rounds of MD5 key stretching.
I think we are good to go with 1000 then?

source: https://en.wikipedia.org/wiki/Key_stretching

Ah ok, since openssl is 'proven' then i guess 1000 or 1500 seems enough.

1000 rounds with 50 credentials takes about 2 seconds to load.
1000 seems too high.

i've tested it with 1500 rounds and it takes ~2.5 seconds to load 50 credentials, most of that time spent only stretching the encryption keys, i think 100 rounds is more than enough, as we are using sha512 instead of md5 for password stretching.

If we are paranoid this could be set to 250 or so, but I don't think password stretching is even needed for this purpose, the encryption key is the one used by nextcloud to keep passwords safe, also user dependant data is used per credential in the encryption, increasing crack time and password length. Somebody would be more lucky hacking the server and extracting the master keys than bruteforcing the password salt.

If anything, the server secret could be appended to the passwordsalt as serverside encryption key, i think that wouldn't penalize the performance and would provide a lot harder to guess encryption key.

Example from a test config file:

'passwordsalt' => 'bscvTQyWylV0ep1czZLPw5zbbEe8IQ',
'secret' => 'YTdoJZJbwgI+X05q3KQXPgPoZc/k1yhSe8x6qDAB3vBHO2wd',

OK, as long as you have experimented with different values, I believe it is reasonable to leave it as is. The only thing you might want to add is a comment referring to this discussion.

animalillo · 2017-01-10T12:21:34Z

lib/Db/CredentialRevision.php

@@ -49,7 +49,7 @@ class CredentialRevision extends Entity implements \JsonSerializable {
 	protected $credentialId;
 	protected $userId;
 	protected $created;
-	protected $credentialData;
+	public $credentialData;


Why public and not using it's getter and setter?

animalillo

@brantje check out my comments on this and let's see if it can be improved! :D
It's mostly OK tho.

animalillo · 2017-01-10T13:30:15Z

lib/Service/CredentialService.php

 			}
-			throw new DoesNotExistException("Did expect one result but found none when executing");


why was this exception removed?

animalillo · 2017-01-10T13:56:03Z

lib/Service/ShareService.php

 	) {
 		$this->sharingACL = $sharingACL;
 		$this->shareRequest = $shareRequest;
 		$this->credential = $credentials;
 		$this->revisions = $revisions;
+		$this->encryptService = $encryptService;
+		$this->server_key = \OC::$server->getConfig()->getSystemValue('passwordsalt', '');


I don't think this is needed here

animalillo · 2017-01-10T18:37:59Z

lib/Service/EncryptService.php

+	public $encrypted_credential_fields = array(
+		'description', 'username', 'password', 'files', 'custom_fields', 'otp', 'email', 'tags', 'url'
+	);
+


the fields before this comment should be constant i think, shouldn't be changed on runtime.

animalillo · 2017-01-10T18:44:56Z

lib/Service/EncryptService.php

+	 */
+	public function decrypt($data_hex, $key) {
+
+		if (!function_exists('hex2bin')) {


Unify polyfills in a single file and avoid them inside classes so we get more code consistency and avoid possible errors and repeating code.

Ah good point. We'll need to create a Polyfills class that we can import.
But that's kina out of the scope for this PR imo.

animalillo · 2017-01-10T19:21:35Z

lib/Service/EncryptService.php

+	 * @returns string The encrypted data
+	 */
+	public function encrypt($data, $key) {
+		$salt = openssl_random_pseudo_bytes(128);


for improved security check if random_bytes is defined and use it instead of openssl_random_pseudo_bytes because it uses system random number generator instead of pseudo random numbers

if (function_exist('random_bytes')){
$key = random_bytes(128);
} else{
$key = openssl_random_pseudo_bytes(128);
}

animalillo · 2017-01-10T19:23:34Z

lib/Service/EncryptService.php

+	 * @param $data
+	 * @return bool|string
+	 */
+	protected function unpad($data) {


Never used and not needed, delete?

It's used on line 161 on EncryptService.php

animalillo · 2017-01-10T19:43:11Z

lib/Service/EncryptService.php

+	 * @throws \Exception
+	 */
+	private function handleCredential($credential, $op) {
+		$service_function = ($op === 'encrypt') ? 'encrypt' : 'decrypt';


instead of an if i would better create a constant static like OP_ENCRYPT = "encrypt"; and OP_DECRYPT = "decrypt"; and use them instead of text, would lead to less errors while maintaining the code, but this is just an opinion. this is perfectly fine, the if is still not needed imho.

animalillo · 2017-01-10T19:43:31Z

lib/Service/EncryptService.php

+	 * @throws \Exception
+	 */
+	private function handleFile($file, $op) {
+		$service_function = ($op === 'encrypt') ? 'encrypt' : 'decrypt';


same as in handleCredential

animalillo · 2017-01-10T20:08:42Z

lib/Service/EncryptService.php

+		$userKey = '';
+		$userSuppliedKey = '';
+		if ($file instanceof File) {
+			$userSuppliedKey = $file->getSize();


maybe use the file label or name instead of the size as usersuppliedkey and use size . mimetype on userkey

animalillo · 2017-01-10T20:16:49Z

lib/Service/EncryptService.php

+	/**
+	 * @var int $rounds The number of rounds to feed into PBKDF2 for key generation
+	 */
+	protected $rounds = 100;


i think 10000 rounds might be too much for production, also remind this is just a feature that adds an extra layer of security in case somebody unauthorized got access to the database.
Remember this code will be executed on almost every api request on passman, we need to keep it fast!

animalillo · 2017-01-10T20:29:10Z

Also would be neat to have a unit test on this EncryptService, it's a very critical point in the app!

animalillo · 2017-01-11T19:19:21Z

Great job there @brantje !

brantje force-pushed the SSE branch 4 times, most recently from 93f13b8 to dc6d25c Compare January 2, 2017 21:03

brantje added the Needs Review label Jan 2, 2017

brantje requested a review from animalillo January 2, 2017 21:04

brantje added this to the V2 milestone Jan 2, 2017

brantje force-pushed the SSE branch from 69aadec to 070b876 Compare January 2, 2017 21:15

brantje force-pushed the SSE branch 14 times, most recently from a075032 to 612bed2 Compare January 9, 2017 10:47

EdOverflow reviewed Jan 9, 2017

View reviewed changes

animalillo reviewed Jan 10, 2017

View reviewed changes

animalillo requested changes Jan 10, 2017

View reviewed changes

brantje added 3 commits January 11, 2017 18:09

Create encryption class

e5c8c5d

Implement encryption class

734496e

Add migration script

997071e

brantje added 2 commits January 11, 2017 18:09

Bump app version, add openssl as dependency

b05137b

Fixes

3f4afba

brantje force-pushed the SSE branch 4 times, most recently from 934f211 to c9e614d Compare January 11, 2017 18:24

passman-bot approved these changes Jan 11, 2017

View reviewed changes

brantje force-pushed the SSE branch from c9e614d to 94c9b70 Compare January 11, 2017 18:41

Adjust reviewed code

5f76616

brantje force-pushed the SSE branch from 94c9b70 to 5f76616 Compare January 11, 2017 19:07

animalillo approved these changes Jan 11, 2017

View reviewed changes

animalillo merged commit 5f76616 into master Jan 11, 2017

brantje deleted the SSE branch January 29, 2017 14:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement Server side encryption #160

Implement Server side encryption #160

brantje commented Jan 2, 2017 •

edited

animalillo commented Jan 3, 2017

EdOverflow Jan 9, 2017

animalillo Jan 10, 2017

EdOverflow Jan 11, 2017

brantje Jan 11, 2017

animalillo Jan 11, 2017

brantje Jan 11, 2017

brantje Jan 11, 2017

animalillo Jan 11, 2017

EdOverflow Jan 14, 2017

animalillo Jan 10, 2017

animalillo left a comment

animalillo Jan 10, 2017

animalillo Jan 10, 2017

animalillo Jan 10, 2017

animalillo Jan 10, 2017

brantje Jan 11, 2017

animalillo Jan 10, 2017

animalillo Jan 10, 2017

animalillo Jan 10, 2017

brantje Jan 11, 2017

animalillo Jan 10, 2017

animalillo Jan 10, 2017

animalillo Jan 10, 2017

animalillo Jan 10, 2017

animalillo commented Jan 10, 2017

animalillo commented Jan 11, 2017

		}
		throw new DoesNotExistException("Did expect one result but found none when executing");

Implement Server side encryption #160

Implement Server side encryption #160

Conversation

brantje commented Jan 2, 2017 • edited

animalillo commented Jan 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

animalillo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

animalillo commented Jan 10, 2017

animalillo commented Jan 11, 2017

brantje commented Jan 2, 2017 •

edited