do not verify the chunk id on every read from an AEAD repo, fixes #9994

docs/changes.rst

@@ -174,6 +174,8 @@ New features:
   - extract: avoid refetching/reparsing repeated chunks, #1678
   - fetch_many: serve all-zero chunks without repository access; also cache
     recently parsed chunks, #1678
+  - do not verify the chunk id on every read from an encrypted repo (the AEAD
+    authentication covers reads), see BORG_ASSERT_ID, #9994, #7362
 - webdav: serve archives via WebDAV / HTTP, including PAX tar downloads - this is a nice
   replacement for `borg mount` in some use cases, #9942
 - mount: expose POSIX ACLs on Linux mounts (not enforced), #1042

docs/internals/security.rst

@@ -191,6 +191,14 @@ Notable:
   This strongly associates the id with the written data (== associates the key with
   the value). When later reading the data for some id, authentication will only
   succeed if what we get was really written by us for that id.
+- Because of that, additionally verifying ``id == MAC(id_key, decompressed)`` after
+  decryption is optional for these modes: it is not what protects against a malicious
+  repository (the authentication tag does that), it only detects chunks whose content
+  does not match their id - which only a malicious or compromised borg client that had
+  the borg key could have written. Borg therefore does not verify that on every read by
+  default, see the ``BORG_ASSERT_ID`` environment variable for details and for the places
+  where borg verifies it by default (e.g. ``borg check --verify-data``, which is the
+  recommended periodic audit for this).
 
 
 Legacy modes

docs/usage/check.rst.inc

@@ -144,6 +144,10 @@ will detect any accidental and malicious corruption. Tamper-resistance is only
 guaranteed for encrypted repositories against attackers without access to the keys.
 You cannot use ``--verify-data`` with ``--repository-only``.
 
+``--verify-data`` also always verifies that each chunk's content matches its chunk id,
+which normal reads do not do by default (see ``BORG_ASSERT_ID``). Running it periodically
+is therefore recommended.
+
 The ``--find-lost-archives`` option will also scan the whole repository, but
 tells Borg to search for lost archive metadata. If Borg encounters any archive
 metadata that does not match an archive directory entry (including

docs/usage/general/environment.rst.inc

@@ -101,6 +101,59 @@ General:
     BORG_FILES_CACHE_TTL
         When set to a numeric value, this determines the maximum "time to live" for the files cache
         entries (default: 2). The files cache is used to determine quickly whether a file is unchanged.
+    BORG_ASSERT_ID
+        Comma-separated list of the places where borg shall verify that a chunk's content matches
+        its chunk id (``chunkid == id_hash(content)``) after decrypting and decompressing it.
+        Verifying costs a full hash pass over everything that is read at such a place.
+
+        Default (variable not set)::
+
+            BORG_ASSERT_ID=repair,transfer,rechunk
+
+        These are the place names that can be listed:
+
+        read
+            Every read that decompresses a chunk: ``borg extract``, ``borg mount``,
+            ``borg export-tar``, ``borg diff``, ... This is by far the most data borg reads, so
+            this place is **not** in the default, see the explanation below.
+        repair
+            ``borg check --repair``. It rebuilds archives from the item metadata stream it reads,
+            re-packing it into new chunks with freshly computed ids, and it recreates manifest and
+            archives directory entries from what it reads.
+        transfer
+            ``borg transfer``, for everything it reads from the source repository. Transferring
+            re-anchors the content in another repository, which is a trust boundary.
+        rechunk
+            ``borg recreate --chunker-params ...``, i.e. re-chunking reads. Re-chunking computes
+            new chunk ids from the content it reads, so a violation would not be noticeable any
+            more afterwards. (Re-chunking in ``borg transfer`` is covered by ``transfer``.)
+
+        An unknown place name is an error. An empty value (``BORG_ASSERT_ID=``) verifies at none of
+        these places, but still where borg always verifies (see below).
+
+        Why ``read`` is not in the default: for encrypted repositories (all the AEAD ciphersuites),
+        the chunk id is part of the AEAD additional authenticated data, so a successful decryption
+        already proves that a holder of the repository key deliberately stored exactly this
+        ciphertext for exactly this chunk id. A malicious or buggy **repository** can therefore not
+        swap, splice or substitute objects, whether the id is verified on read or not. What the id
+        check adds is the detection of chunks whose content does not match their id, which only a
+        malicious or compromised **borg client that had your borg key** could have written (e.g. to
+        poison future deduplication). If that is in your threat model - e.g. because some machines
+        writing into the repository are not fully trusted - add ``read`` to the list::
+
+            BORG_ASSERT_ID=read,repair,transfer,rechunk
+
+        Otherwise, running ``borg check --verify-data`` periodically is recommended: it is the
+        audit that re-certifies the invariant for all chunks in the background, instead of on
+        every read.
+
+        Independent of this variable, borg always verifies the chunk id:
+
+        - in ``borg check --verify-data``. That audit is what makes not verifying elsewhere
+          defensible, so it is not configurable (there is no ``verify_data`` place name).
+        - for ``authenticated`` and ``none`` mode repositories: there is no AEAD there, so the id
+          check *is* the read path's integrity check and switching it off would remove it
+          completely. Same for reading borg 1.x repositories (``borg transfer``).
     BORG_BLAKE3_MT_THRESHOLD
         When set to a numeric value, chunks of at least that many KiB get their id computed by
         multi-threaded BLAKE3, smaller ones single-threaded (default: 256, i.e. 256KiB).

src/borg/archive.py

@@ -1898,6 +1898,14 @@ def check(
         if self.key is None:
             self.key = self.make_key(repository)
         self.repo_objs = RepoObj(self.key)
+        if repair:
+            # --repair re-anchors content: it re-packs the item metadata stream it reads into new chunks
+            # with freshly computed ids (see add_callback in rebuild_archives) and it recreates archives
+            # directory entries from archive metadata content. Just like re-chunking, that would turn a
+            # chunk whose content does not match its id into valid data under a new id, and the violation
+            # could not be noticed afterwards. So everything read here is read at the "repair" place, which
+            # re-certifies chunkid == id_hash(content) by default, see BORG_ASSERT_ID.
+            self.repo_objs.set_assert_id_place("repair")
         if verify_data:
             self.verify_data()
         rebuild_manifest = False
@@ -1985,8 +1993,12 @@ def verify_data(self):
                     defect_chunks.append(chunk_id)
             else:
                 try:
-                    # we must decompress, so it'll call assert_id() in there:
-                    self.repo_objs.parse(chunk_id, encrypted_data, decompress=True, ro_type=ROBJ_DONTCARE)
+                    # we must decompress, so it'll call assert_id() in there.
+                    # this is the audit that re-certifies the id/content invariant, so it reads at its own
+                    # place, which always verifies and can not be switched off, see BORG_ASSERT_ID.
+                    self.repo_objs.parse(
+                        chunk_id, encrypted_data, decompress=True, ro_type=ROBJ_DONTCARE, assert_id_place="verify_data"
+                    )
                 except IntegrityErrorBase as integrity_error:
                     self.error_found = True
                     errors += 1
@@ -2004,8 +2016,14 @@ def verify_data(self):
                     # from the underlying media.
                     try:
                         encrypted_data = self.repository.get(defect_chunk)
-                        # we must decompress, so it'll call assert_id() in there:
-                        self.repo_objs.parse(defect_chunk, encrypted_data, decompress=True, ro_type=ROBJ_DONTCARE)
+                        # we must decompress, so it'll call assert_id() in there (see above):
+                        self.repo_objs.parse(
+                            defect_chunk,
+                            encrypted_data,
+                            decompress=True,
+                            ro_type=ROBJ_DONTCARE,
+                            assert_id_place="verify_data",
+                        )
                     except IntegrityErrorBase:
                         # failed twice -> remove this defect chunk. delete rewrites its pack without it,
                         # keeping the other chunks. update_index=False: finish() rebuilds the index from
@@ -2358,6 +2376,11 @@ def __init__(
         self.rechunkify = chunker_params is not None
         if self.rechunkify:
             logger.debug("Rechunking archives to %s", chunker_params)
+            # Re-chunking computes new ids from the plaintext we read, so a chunk whose content does not match
+            # its id would just silently become a valid chunk under a new id and the violation could not be
+            # noticed any more. Thus we read at the "rechunk" place, which re-certifies the id/content
+            # invariant by default, like borg transfer does, see BORG_ASSERT_ID.
+            self.repo_objs.set_assert_id_place("rechunk")
         self.chunker_params = chunker_params or CHUNKER_PARAMS
         self.compression = compression or CompressionSpec("none")
         self.seen_chunks = set()

src/borg/archiver/check_cmd.py

@@ -143,6 +143,10 @@ def build_parser_check(self, subparsers, common_parser, mid_common_parser):
         guaranteed for encrypted repositories against attackers without access to the keys.
         You cannot use ``--verify-data`` with ``--repository-only``.
 
+        ``--verify-data`` also always verifies that each chunk's content matches its chunk id,
+        which normal reads do not do by default (see ``BORG_ASSERT_ID``). Running it periodically
+        is therefore recommended.
+
         The ``--find-lost-archives`` option will also scan the whole repository, but
         tells Borg to search for lost archive metadata. If Borg encounters any archive
         metadata that does not match an archive directory entry (including

src/borg/archiver/transfer_cmd.py

@@ -143,6 +143,13 @@ def do_transfer(self, args, *, repository, manifest, cache, other_repository=Non
                 "You must use the same chunker secret or deduplication will break. " "Use a related repository!"
             )
 
+        # Transferring re-anchors the content in another repository, so this is the trust boundary where the
+        # chunkid == id_hash(content) invariant should be re-certified (the "transfer" place is verifying by
+        # default, see BORG_ASSERT_ID). This covers everything we read from the source repo: file content
+        # chunks, item metadata streams and the reads feeding --chunker-params re-chunking (there, content
+        # ends up in new chunks under freshly computed ids, so a violation could not be noticed later).
+        other_manifest.repo_objs.set_assert_id_place("transfer")
+
         dry_run = args.dry_run
         archive_infos = other_manifest.archives.list_considering(args)
         count = len(archive_infos)

src/borg/crypto/key.py

@@ -279,6 +279,14 @@ class KeyBase:
     # id_key dummy, needs to be overwritten by subclass
     id_key: bytes = None
 
+    # Is assert_id() the read path's authentication mechanism for this key class?
+    # True (the default, e.g. for the "authenticated" and "none" modes): decrypt() verifies nothing, so the keyed
+    # id hash (resp. the plain sha256 for "none") *is* the only integrity/authenticity check a read has and thus
+    # must never be skipped - skipping would silently demote "authenticated" to "none".
+    # False (the AEAD key classes): every read is authenticated by the AEAD tag, with the chunk id in the AAD,
+    # independently of assert_id() - see AEADKeyBase.assert_id for what assert_id adds on top of that.
+    id_check_is_authentication: ClassVar[bool] = True
+
     # Whether this *particular instance* is encrypted from a practical point of view,
     # i.e. when it's using encryption with a empty passphrase, then
     # that may be *technically* called encryption, but for all intents and purposes
@@ -1106,6 +1114,9 @@ class AEADKeyBase(KeyBase):
     # an AEAD key may be stored as a keyfile or inside the repository (see borg key change-location).
     LOCATION_CONFIGURABLE = True
 
+    # the AEAD tag authenticates every read on its own, assert_id only adds the "evil client" detection below.
+    id_check_is_authentication = False
+
     def assert_id(self, id, data):
         # Comparing the id hash here would not be needed any more for the new AEAD crypto **IF** we
         # could be sure that chunks were created by normal (not tampered, not evil) borg code:
@@ -1119,7 +1130,12 @@ def assert_id(self, id, data):
         # repository using this bad chunkid as key (violating the usual chunkid == id_hash(data)).
         # Later, when reading such a bad chunk, AEAD-auth-and-decrypt would not notice any
         # issue and decrypt successfully.
-        # Thus, to notice such evil borg activity, we must check for such violations here:
+        # Thus, to notice such evil borg activity, we must check for such violations here.
+        #
+        # As only that (rather special) threat is left here, this check is mostly optional for AEAD
+        # keys (it was mandatory from #7362/#7367 until #9994): RepoObj.parse calls it at the places
+        # BORG_ASSERT_ID lists (by default: the places that re-anchor content - check --repair,
+        # transfer, re-chunking), plus always in check --verify-data, see id_check_is_authentication.
         if id and id != Manifest.MANIFEST_ID:
             id_computed = self.id_hash(data)
             if not hmac.compare_digest(id_computed, id):

src/borg/legacy/repoobj.py

@@ -12,6 +12,11 @@
 
 
 class RepoObj1:  # legacy
+    def set_assert_id_place(self, place: str) -> None:
+        # borg 1.x crypto has no AEAD with the chunk id in the AAD, so the id check is a needed part of
+        # the read path here: parse always does it and there is nothing to configure (see BORG_ASSERT_ID).
+        pass
+
     @classmethod
     def extract_crypted_data(cls, data: bytes) -> bytes:
         # used for crypto type detection