From bbfb3bb44915004a70702030aa1d2f9336a60938 Mon Sep 17 00:00:00 2001 From: drebs Date: Wed, 3 Jun 2015 15:56:40 -0300 Subject: [bug] remove mac from secrets file This is how a secret was stored in the secrets json file: * each secret is symmetrically encrypted amd MACed with keys derived from the user's passphrase. * the encrypted secrets dictionary is then MACed with another key derived * from the user's passphrase. * each key is derived using scrypt and a unique random salt. There are disadvantages to this approach: * repeating scrypt many times is a waste of time. * an attacker could crack whichever has weaker parameters, if they get out of sync. * if an attacker can modify the secret in a way it is good to decrypt the database, then she can also modify the MAC. The solution for this is: * completelly eliminate the MAC from the storage secrets file. * attempt to decrypt the database with whatever is got from the decryption of the secret. If that is wrong, report an error. Closes #6980. --- client/src/leap/soledad/client/secrets.py | 71 +++++-------------------------- 1 file changed, 10 insertions(+), 61 deletions(-) (limited to 'client/src') diff --git a/client/src/leap/soledad/client/secrets.py b/client/src/leap/soledad/client/secrets.py index 57a1169a..97dbbaca 100644 --- a/client/src/leap/soledad/client/secrets.py +++ b/client/src/leap/soledad/client/secrets.py @@ -23,7 +23,6 @@ Soledad secrets handling. import os import scrypt -import hmac import logging import binascii import errno @@ -37,7 +36,6 @@ from leap.soledad.common import soledad_assert from leap.soledad.common import soledad_assert_type from leap.soledad.common import document from leap.soledad.common import errors -from leap.soledad.common import crypto from leap.soledad.client import events @@ -104,8 +102,8 @@ class SoledadSecrets(object): REMOTE_STORAGE_SECRET_LENGTH = 512 """ - The length, in bytes, of the secret used to derive an encryption key and a - MAC auth key for remote storage. + The length, in bytes, of the secret used to derive an encryption key for + remote storage. """ SALT_LENGTH = 64 @@ -275,7 +273,7 @@ class SoledadSecrets(object): content = None with open(self._secrets_path, 'r') as f: content = json.loads(f.read()) - _, mac, active_secret = self._import_recovery_document(content) + _, active_secret = self._import_recovery_document(content) # choose first secret if no secret_id was given if self._secret_id is None: if active_secret is None: @@ -291,7 +289,7 @@ class SoledadSecrets(object): self._secrets[self._secret_id] += new_piece enlarged = True # store and save in shared db if needed - if not mac or enlarged: + if enlarged: self._store_secrets() self._put_secrets_in_shared_db() @@ -311,9 +309,7 @@ class SoledadSecrets(object): logger.info( 'Found cryptographic secrets in shared recovery ' 'database.') - _, mac, active_secret = self._import_recovery_document(doc.content) - if mac is False: - self.put_secrets_in_shared_db() + _, active_secret = self._import_recovery_document(doc.content) self._store_secrets() # save new secrets in local file if self._secret_id is None: if active_secret is None: @@ -371,32 +367,20 @@ class SoledadSecrets(object): { 'storage_secrets': { '': { - 'kdf': 'scrypt', - 'kdf_salt': '' - 'kdf_length': 'cipher': 'aes256', 'length': , 'secret': '', }, }, 'active_secret': '', - 'kdf': 'scrypt', - 'kdf_salt': '', - 'kdf_length: , - '_mac_method': 'hmac', - '_mac': '' } Note that multiple storage secrets might be stored in one recovery - document. This method will also calculate a MAC of a string - representation of the secrets dictionary. + document. :return: The recovery document. :rtype: dict """ - # create salt and key for calculating MAC - salt = os.urandom(self.SALT_LENGTH) - key = scrypt.hash(self._passphrase_as_string(), salt, buflen=32) # encrypt secrets encrypted_secrets = {} for secret_id in self._secrets: @@ -406,14 +390,6 @@ class SoledadSecrets(object): data = { self.STORAGE_SECRETS_KEY: encrypted_secrets, self.ACTIVE_SECRET_KEY: self._secret_id, - self.KDF_KEY: self.KDF_SCRYPT, - self.KDF_SALT_KEY: binascii.b2a_base64(salt), - self.KDF_LENGTH_KEY: len(key), - crypto.MAC_METHOD_KEY: crypto.MacMethods.HMAC, - crypto.MAC_KEY: hmac.new( - key, - json.dumps(encrypted_secrets, sort_keys=True), - sha256).hexdigest(), } return data @@ -428,38 +404,11 @@ class SoledadSecrets(object): :param data: The recovery document. :type data: dict - :return: A tuple containing the number of imported secrets, whether - there was MAC information available for authenticating, and - the secret_id of the last active secret. - :rtype: (int, bool) + :return: A tuple containing the number of imported secrets and the + secret_id of the last active secret. + :rtype: (int, str) """ soledad_assert(self.STORAGE_SECRETS_KEY in data) - # check mac of the recovery document - mac = None - if crypto.MAC_KEY in data: - soledad_assert(data[crypto.MAC_KEY] is not None) - soledad_assert(crypto.MAC_METHOD_KEY in data) - soledad_assert(self.KDF_KEY in data) - soledad_assert(self.KDF_SALT_KEY in data) - soledad_assert(self.KDF_LENGTH_KEY in data) - if data[crypto.MAC_METHOD_KEY] == crypto.MacMethods.HMAC: - key = scrypt.hash( - self._passphrase_as_string(), - binascii.a2b_base64(data[self.KDF_SALT_KEY]), - buflen=32) - mac = hmac.new( - key, - json.dumps( - data[self.STORAGE_SECRETS_KEY], sort_keys=True), - sha256).hexdigest() - else: - raise crypto.UnknownMacMethodError('Unknown MAC method: %s.' % - data[crypto.MAC_METHOD_KEY]) - if mac != data[crypto.MAC_KEY]: - raise crypto.WrongMacError( - 'Could not authenticate recovery document\'s ' - 'contents.') - # include secrets in the secret pool. secret_count = 0 secrets = data[self.STORAGE_SECRETS_KEY].items() @@ -477,7 +426,7 @@ class SoledadSecrets(object): except SecretsException as e: logger.error("Failed to decrypt storage secret: %s" % str(e)) - return secret_count, mac, active_secret + return secret_count, active_secret def _get_secrets_from_shared_db(self): """ -- cgit v1.2.3