[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.

2 files changed, 11 insertions, 61 deletions

diff --git a/client/changes/feature_6980_remove-mac-from-secrets-file b/client/changes/feature_6980_remove-mac-from-secrets-file
new file mode 100644
index 00000000..6a424013
--- /dev/null
+++ b/client/changes/feature_6980_remove-mac-from-secrets-file
@@ -0,0 +1 @@
+  o Remove MAC from secrets file. Closes #6980.
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': {
                     '<storage_secret id>': {
-                        'kdf': 'scrypt',
-                        'kdf_salt': '<b64 repr of salt>'
-                        'kdf_length': <key length>
                         'cipher': 'aes256',
                         'length': <secret length>,
                         'secret': '<encrypted storage_secret>',
                     },
                 },
                 'active_secret': '<secret_id>',
-                'kdf': 'scrypt',
-                'kdf_salt': '<b64 repr of salt>',
-                'kdf_length: <key length>,
-                '_mac_method': 'hmac',
-                '_mac': '<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):
         """