clarify possible IV reuse with multiple Encrypt handles
authorPhilipp Gesang <philipp.gesang@intra2net.com>
Thu, 16 Apr 2020 15:40:08 +0000 (17:40 +0200)
committerPhilipp Gesang <philipp.gesang@intra2net.com>
Tue, 21 Apr 2020 14:47:39 +0000 (16:47 +0200)
Address Deltatar audit item 2.2: Unsafe to create more than 2^32
instances of Encrypt using the same key and salt

Extend the documentation section on handling of IVs with a
paragraph concerning multiple Encrypt handles. Also add a method
to obtain the list of IVs for checks performed by the caller.

deltatar/crypto.py
testing/test_crypto.py

index 3c3a816..388e9c7 100755 (executable)
@@ -70,6 +70,14 @@ e. g. when traversing the same object multiple times. Since the crypto context
 has no notion of a position in a PDT encrypted archive, this condition must be
 sorted out downstream.
 
+When encrypting with more than one Encrypt context special care must be taken
+to prevent accidental reuse of IVs. The builtin protection against reuse is
+only effective for objects encrypted with the same Encrypt handle. If multiple
+Encrypt handles are used to encrypt with the same combination of password and
+salt, the encryption becomes susceptible to birthday attacks (bound = 2^32 due
+to the 64-bit random iv). Thus the use of multiple handles is discouraged.
+
+
 Command Line Utility
 -------------------------------------------------------------------------------
 
@@ -971,7 +979,7 @@ class Crypto (object):
         elif cnt == AES_GCM_IV_CNT_INDEX:
             if self.index_counter_used is True:
                 raise InvalidFileCounter ("attempted to reuse index file "
-                                          " counter %d: must be unique" % cnt)
+                                          "counter %d: must be unique" % cnt)
             self.index_counter_used = True
         if cnt <= AES_GCM_IV_CNT_MAX:
             self.cnt = cnt
@@ -1091,6 +1099,15 @@ class Crypto (object):
         self.enc = None
 
 
+    def get_used_ivs (self):
+        """
+        Get the set of IVs that were used so far during the lifetime of
+        this context. Useful to check for IV reuse if multiple encryption
+        contexts were used independently.
+        """
+        return self.used_ivs
+
+
 class Encrypt (Crypto):
 
     lastinfo     = None
@@ -1103,7 +1120,6 @@ class Encrypt (Crypto):
         The ctor will throw immediately if one of the parameters does not conform
         to our expectations.
 
-                  counter=AES_GCM_IV_CNT_DATA, strict_ivs=True):
         :type      version: int to fit   uint16_t
         :type paramversion: int to fit   uint16_t
         :param    password: mutually exclusive with ``key``
@@ -1117,6 +1133,16 @@ class Encrypt (Crypto):
                             and cannot be reused even with different fixed parts.
         :type   strict_ivs: bool
         :type     insecure: bool, whether to permit passthrough mode
+
+        *Security considerations*: The ``class Encrypt`` handle guarantees that
+        all random parts (first eight bytes) of the IVs used for encrypting
+        objects are unique. This guarantee does *not* apply across handles if
+        multiple handles are used with the same combination of password and
+        salt. Thus, use of multiple handles with the same combination of password
+        and salt is subject to birthday attacks with a bound of 2^32. To avoid
+        collisions, the application should keep the number of handles as low
+        as possible and check for reuse by comparing the set of IVs used of all
+        handles that were created (accessible using the ``get_used_ivs`` method).
         """
         if         password is     None and key is     None \
                 or password is not None and key is not None :
@@ -1333,7 +1359,8 @@ class Decrypt (Crypto):
                             ``AES_GCM_IV_CNT_INDEX`` are unique in each backup set
                             and cannot be reused even with different fixed parts.
         :type   fixedparts: bytes list
-        :type     insecure: bool, whether to process objects encrypted in
+        :type     insecure: bool
+        :param    insecure: whether to process objects encrypted in
                             passthrough mode (*``paramversion`` < 1*)
         """
         if         password is     None and key is     None \
index f47c7c7..3f444f9 100644 (file)
@@ -158,6 +158,56 @@ class AESGCMTest (CryptoLayerTest):
         assert len (header) == crypto.PDTCRYPT_HDR_SIZE
 
 
+    def test_crypto_aes_gcm_enc_multi_ivs_ok_explicit_counter (self):
+        """
+        Access the list of IVs used during encryption and check reuse. Start
+        from explicit counters so the inputs don’t overlap. IVs must not have
+        been reused.
+        """
+
+        def enc (start_count, data):
+            password       = str (os.urandom (42))
+            encryptor      = crypto.Encrypt (TEST_VERSION,
+                                             TEST_PARAMVERSION,
+                                             password=password,
+                                             nacl=TEST_STATIC_NACL,
+                                             counter=start_count)
+
+            for i, blob in enumerate (data, 1):
+                fname = "%s_%d" % (TEST_DUMMY_FILENAME, i)
+                header_dummy   = encryptor.next (fname)
+                _, _           = encryptor.process (blob)
+                _, header, _   = encryptor.done (header_dummy)
+                assert len (encryptor.get_used_ivs ()) == i
+
+            return encryptor.get_used_ivs ()
+
+        ivs1 = enc (0x0042, [TEST_PLAINTEXT, b"none of your business"])
+        ivs2 = enc (0x1337, [b"read me if you can", b"for British eyes only!"])
+
+        # No reuse in general.
+        assert len (ivs1 & ivs2) == 0
+
+        ivs1 = list (ivs1)
+        ivs2 = list (ivs2)
+
+        # Counters of used IVs must match what we passed explicitly.
+        def extract_counters (ivs):
+            def getcount (iv):
+                _, cnt = struct.unpack (crypto.FMT_I2N_IV, iv)
+                return cnt
+            return list (map (getcount, ivs))
+
+        cnt1 = extract_counters (ivs1)
+        cnt2 = extract_counters (ivs2)
+
+        assert 0x0042 in cnt1
+        assert 0x0043 in cnt1
+
+        assert 0x1337 in cnt2
+        assert 0x1338 in cnt2
+
+
     def test_crypto_aes_gcm_enc_chunk_size (self):
         password       = str (os.urandom (42))
         encryptor      = crypto.Encrypt (TEST_VERSION,
@@ -795,35 +845,10 @@ class AESGCMTest (CryptoLayerTest):
             return ct + cnk, header, fixed
 
         ct_1, hdr_1, _____ = enc (orig_pt_1)
-        ct_2, hdr_2, fixed = enc (orig_pt_2)
-
-        mut_hdr_2    = bytearray (hdr_2)
-        mut_hdr_2_vw = memoryview (mut_hdr_2)
-        # get IV
-        iv_lo        = crypto.HDR_OFF_IV
-        iv_hi        = crypto.HDR_OFF_IV + crypto.PDTCRYPT_HDR_SIZE_IV
-        iv_1         = hdr_1 [iv_lo : iv_hi]
-        # transplant into other header
-        mut_hdr_2_vw [iv_lo : iv_hi] = iv_1
-        hdr_2_mod    = bytes (mut_hdr_2)
-        decryptor    = crypto.Decrypt (password=password, fixedparts=fixed,
-                                       strict_ivs=True)
-
-        def dec (hdr, ct):
-            decryptor.next (hdr)
-            off = 0
-            pt  = b""
-            while off < len (ct):
-                upto = min (off + cnksiz, len (ct))
-                cnk = decryptor.process (ct [off:upto])
-                pt += cnk
-                off += cnksiz
-            return pt + decryptor.done ()
+        encryptor.cnt -= 1 # induce error by forcing an identical IV on next step
 
-        decr_pt_1 = dec (hdr_1, ct_1)
-        decr_pt_2 = dec (hdr_2, ct_2) # good header, different IV
-        with self.assertRaises (crypto.DuplicateIV):        # bad header, reuse detected
-            decr_pt_2 = dec (hdr_2_mod, ct_2)
+        with self.assertRaises (crypto.DuplicateIV): # reuse detected
+            ct_2, hdr_2, fixed = enc (orig_pt_2)
 
 
 class HeaderTest (CryptoLayerTest):