From 87b8d611da7cedf1cc4c8834b1408aeb4e260a77 Mon Sep 17 00:00:00 2001 From: Thomas Waldmann Date: Sat, 1 Aug 2026 15:49:45 +0200 Subject: [PATCH] do not verify the chunk id on every read from an AEAD repo, fixes #9994 For the AEAD encryption modes, the chunk id is part of the AAD, so a successful decryption already proves that a repo key holder deliberately stored this exact ciphertext for this exact chunk id: a malicious repository can not swap, splice or substitute objects, whether we recompute the id hash over the plaintext or not. What assert_id adds there is only the detection of chunks whose content does not match their id, which only a malicious/compromised borg client that had the borg key could have written (see #7362, which made the check mandatory). Whether that extra full-plaintext hash pass is worth it depends on where borg reads, so the new BORG_ASSERT_ID env var takes a comma-separated list of the places that shall verify the chunk id: read the general read path: extract, mount, export-tar, diff, ... repair borg check --repair transfer borg transfer (everything read from the source repo) rechunk borg recreate --chunker-params (re-chunking reads) Default (env var not set): "repair,transfer,rechunk", i.e. every place that re-anchors content and would thus make a violation unnoticeable afterwards, but not the hot read path. Unknown names are an error. BORG_ASSERT_ID=read,repair,transfer,rechunk gives the old behaviour of always verifying. The id is verified no matter what the env var says: - in borg check --verify-data - the audit that re-certifies the id/content invariant for all chunks of the repository is what makes not verifying elsewhere defensible, so it is not configurable (there is no "verify_data" place name, giving one is an error). - for "authenticated" and "none" mode repos and for borg 1.x repo objects: there is no AEAD there, so assert_id IS the read path authentication (KeyBase.id_check_is_authentication) and skipping it would demote "authenticated" to "none". Measured on an Apple M3 Pro (18GB RAM, hw accelerated sha256), reading one 20GiB incompressible file back via "borg export-tar arch - > /dev/null", aes256-ocb, --compression none, 4 runs alternating between the settings: id hash BORG_ASSERT_ID run times [s] median throughput sha256 default 20.43 21.13 20.79 20.93 20.86 982 MiB/s sha256 ...,read 29.42 28.91 28.82 28.05* 28.87 709 MiB/s blake3 default 19.99 19.97 20.00 19.93 19.98 1025 MiB/s blake3 ...,read 24.70 24.95* 24.72 24.60 24.71 829 MiB/s *) repeated run: the original one (31.64s resp. 28.37s) was disturbed by unrelated background load on the test machine. i.e. 28% faster for sha256 id hashes and 19% faster for blake3 ones (the file is much bigger than the RAM, so this includes reading it from the SSD every time). With the default, running borg check --verify-data periodically is the recommended audit that re-certifies the id/content invariant for all chunks. Co-Authored-By: Claude Opus 5 --- docs/changes.rst | 2 + docs/internals/security.rst | 8 ++ docs/usage/check.rst.inc | 4 + docs/usage/general/environment.rst.inc | 53 +++++++ src/borg/archive.py | 31 ++++- src/borg/archiver/check_cmd.py | 4 + src/borg/archiver/transfer_cmd.py | 7 + src/borg/crypto/key.py | 18 ++- src/borg/legacy/repoobj.py | 5 + src/borg/repoobj.py | 84 +++++++++++- src/borg/testsuite/archiver/__init__.py | 24 ++++ src/borg/testsuite/archiver/check_cmd_test.py | 69 +++++++++- .../testsuite/archiver/recreate_cmd_test.py | 28 ++++ .../testsuite/archiver/transfer_cmd_test.py | 39 +++++- src/borg/testsuite/repoobj_test.py | 129 +++++++++++++++++- 15 files changed, 490 insertions(+), 15 deletions(-) diff --git a/docs/changes.rst b/docs/changes.rst index 41e507f7ca..5eaef4331e 100644 --- a/docs/changes.rst +++ b/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 diff --git a/docs/internals/security.rst b/docs/internals/security.rst index d53adda521..079ca2be72 100644 --- a/docs/internals/security.rst +++ b/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 diff --git a/docs/usage/check.rst.inc b/docs/usage/check.rst.inc index fc166373af..5d1ad8fc6b 100644 --- a/docs/usage/check.rst.inc +++ b/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 diff --git a/docs/usage/general/environment.rst.inc b/docs/usage/general/environment.rst.inc index 0689010132..48db13f16c 100644 --- a/docs/usage/general/environment.rst.inc +++ b/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). diff --git a/src/borg/archive.py b/src/borg/archive.py index 8017574b05..f7990a3212 100644 --- a/src/borg/archive.py +++ b/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() diff --git a/src/borg/archiver/check_cmd.py b/src/borg/archiver/check_cmd.py index 9a97d98cf4..682f3a9279 100644 --- a/src/borg/archiver/check_cmd.py +++ b/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 diff --git a/src/borg/archiver/transfer_cmd.py b/src/borg/archiver/transfer_cmd.py index 59671b7c32..406d197864 100644 --- a/src/borg/archiver/transfer_cmd.py +++ b/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) diff --git a/src/borg/crypto/key.py b/src/borg/crypto/key.py index 4a2e27f85e..e635fbfe81 100644 --- a/src/borg/crypto/key.py +++ b/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): diff --git a/src/borg/legacy/repoobj.py b/src/borg/legacy/repoobj.py index 99ac49f174..ab1b5431a6 100644 --- a/src/borg/legacy/repoobj.py +++ b/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 diff --git a/src/borg/repoobj.py b/src/borg/repoobj.py index c1e7974a09..a4295d5692 100644 --- a/src/borg/repoobj.py +++ b/src/borg/repoobj.py @@ -1,14 +1,59 @@ +import os from collections import namedtuple from struct import Struct from .constants import * # NOQA from .helpers import msgpack, workarounds -from .helpers.errors import IntegrityError +from .helpers.errors import Error, IntegrityError from .compress import Compressor, LZ4_COMPRESSOR # Workaround for lost passphrase or key in "authenticated" or "authenticated-blake2" mode AUTHENTICATED_NO_KEY = "authenticated_no_key" in workarounds +# The places where parse() verifies chunkid == id_hash(content) only if the user asks for it. +# The user picks the places via BORG_ASSERT_ID (comma-separated), see the docs of that env var. +ASSERT_ID_PLACES = ( + "read", # the general read path: extract, mount, export-tar, diff, ... + "repair", # borg check --repair + "transfer", # borg transfer (everything it reads from the source repo) + "rechunk", # borg recreate --chunker-params (re-chunking reads) +) +# The places that always verify and thus can not be configured: borg check --verify-data is the +# audit that re-certifies the id/content invariant for all chunks of a repository - it is what makes +# not verifying elsewhere defensible, so it must not be switchable. +ASSERT_ID_PLACES_MANDATORY = ("verify_data",) # borg check --verify-data +# Default value of the BORG_ASSERT_ID env var: verify everywhere except on the (hot) general read +# path - there, the AEAD authentication already covers what a repository could do to us, see +# AEADKeyBase.assert_id. +BORG_ASSERT_ID_DEFAULT = ("repair", "transfer", "rechunk") +# Reads through a RepoObj are attributed to this place unless a command sets another one. +ASSERT_ID_PLACE_DEFAULT = "read" + + +def get_assert_id_places(): + """Determine the configurable places that shall verify the chunk id, see the BORG_ASSERT_ID docs. + + Note: this only decides for keys that authenticate reads independently of the id hash (the AEAD + ciphersuites, see KeyBase.id_check_is_authentication) - for all other keys, the id check is the + read path authentication itself and thus always happens, no matter what is configured here. + The same is true for ASSERT_ID_PLACES_MANDATORY. + """ + value = os.environ.get("BORG_ASSERT_ID") + if value is None: + return frozenset(BORG_ASSERT_ID_DEFAULT) + places = frozenset(place.strip() for place in value.split(",") if place.strip()) + invalid = places - set(ASSERT_ID_PLACES) + if invalid: + problems = [] + mandatory = sorted(invalid & set(ASSERT_ID_PLACES_MANDATORY)) + if mandatory: + problems.append(f"{', '.join(mandatory)}: always verifies, can not be configured") + unknown = sorted(invalid - set(ASSERT_ID_PLACES_MANDATORY)) + if unknown: + problems.append(f"{', '.join(unknown)}: invalid place name(s)") + raise Error(f"BORG_ASSERT_ID: {'; '.join(problems)}. Valid place names are: {', '.join(ASSERT_ID_PLACES)}.") + return places + OBJ_MAGIC = b"BORG_OBJ" @@ -61,6 +106,17 @@ def __init__(self, key): # Some commands write new chunks (e.g. rename) but don't take a --compression argument. This duplicates # the default used by those commands who do take a --compression argument. self.compressor = LZ4_COMPRESSOR + # Where shall the chunk id be verified over the plaintext? See parse() and the BORG_ASSERT_ID docs. + self.assert_id_places = get_assert_id_places() + # The place reads through this RepoObj are attributed to. Commands that are a trust boundary for + # the id/content invariant (transfer, re-chunking, check --repair) set their own place, see + # set_assert_id_place. + self.assert_id_place = ASSERT_ID_PLACE_DEFAULT + + def set_assert_id_place(self, place: str) -> None: + """Attribute all reads through this RepoObj to , see the BORG_ASSERT_ID docs.""" + assert place in ASSERT_ID_PLACES + ASSERT_ID_PLACES_MANDATORY + self.assert_id_place = place def id_hash(self, data: bytes) -> bytes: return self.key.id_hash(data) @@ -134,7 +190,13 @@ def parse_meta(self, id: bytes, cdata: bytes, ro_type: str) -> dict: return meta def parse( - self, id: bytes, cdata: bytes, decompress: bool = True, want_compressed: bool = False, ro_type: str = None + self, + id: bytes, + cdata: bytes, + decompress: bool = True, + want_compressed: bool = False, + ro_type: str = None, + assert_id_place: str = None, ) -> tuple[dict, bytes]: """ Parse a repo object into metadata and data (decrypt it, maybe decompress, maybe verify if the chunk plaintext @@ -145,6 +207,10 @@ def parse( - decompress=True, want_compressed=True: slow, verifying. returns compressed data (caller wants to reuse it). - decompress=False, want_compressed=True: quick, not verifying. returns compressed data (caller wants to reuse). - decompress=False, want_compressed=False: invalid + + assert_id_place gives the place this specific read happens at (see the BORG_ASSERT_ID docs); if not + given, the RepoObj's place is used (see set_assert_id_place). Places in ASSERT_ID_PLACES_MANDATORY + always verify, the others only if the user asked for them. """ assert isinstance(ro_type, str) assert not (not decompress and not want_compressed), "invalid parameter combination!" @@ -186,7 +252,19 @@ def parse( compressor_cls, compression_level = Compressor.detect(compr_hdr) compressor = compressor_cls(level=compression_level) meta, data = compressor.decompress(dict(meta_compressed), data_compressed[:psize]) - if not AUTHENTICATED_NO_KEY: + # For keys where the id check is the read-path authentication ("authenticated" and "none" mode), + # it always has to happen - skipping it would remove all integrity checking from reads. + # For the AEAD keys, the ciphertext is already authenticated for this specific chunk id (the id is + # in the AAD), so the id check only adds detection of chunks whose plaintext does not match their id + # - which only an evil/broken borg client that had the repo key could have written. Whether that + # extra full-plaintext hash pass is worth it depends on the place we read at, see BORG_ASSERT_ID. + place = assert_id_place if assert_id_place is not None else self.assert_id_place + assert_id = ( + self.key.id_check_is_authentication + or place in ASSERT_ID_PLACES_MANDATORY + or place in self.assert_id_places + ) + if assert_id and not AUTHENTICATED_NO_KEY: self.key.assert_id(id, data) else: meta, data = None, None diff --git a/src/borg/testsuite/archiver/__init__.py b/src/borg/testsuite/archiver/__init__.py index 1e2a583fe9..a2a8b74100 100644 --- a/src/borg/testsuite/archiver/__init__.py +++ b/src/borg/testsuite/archiver/__init__.py @@ -202,6 +202,30 @@ def open_archive(repo_path, name): return archive, repository +def read_chunk(archive, repository, chunk_id, *, ro_type=ROBJ_FILE_STREAM): + _, data = archive.repo_objs.parse( + chunk_id, repository.get(chunk_id), ro_type=ro_type, assert_id_place="verify_data" + ) + return data + + +def write_wrong_content_chunk(archive, repository, chunk_id, *, ro_type=ROBJ_FILE_STREAM, wrong_data=None): + """Replace the chunk by one with different content, violating id == id_hash(content). + + This simulates an evil or compromised borg client that had the repo key: the chunk id goes into the AEAD + AAD, so such a chunk decrypts and authenticates just fine, only the id check detects it (see issue #9994). + If wrong_data is not given, the chunk content is bit-flipped, keeping its length (file content chunks + have their size recorded in the item's chunks list). + """ + repo_objs = archive.repo_objs + if wrong_data is None: + data = read_chunk(archive, repository, chunk_id, ro_type=ro_type) + wrong_data = bytes([data[0] ^ 0xFF]) + data[1:] + repository.delete(chunk_id) # put() would not replace an id the repo already has + repository.put(chunk_id, repo_objs.format(chunk_id, {}, wrong_data, ro_type=ro_type)) + repository.flush() + + def open_repository(archiver): if archiver.get_kind() == "remote": return Repository(Location(archiver.repository_location), exclusive=True) diff --git a/src/borg/testsuite/archiver/check_cmd_test.py b/src/borg/testsuite/archiver/check_cmd_test.py index 1e91ea84ba..ef42ad776b 100644 --- a/src/borg/testsuite/archiver/check_cmd_test.py +++ b/src/borg/testsuite/archiver/check_cmd_test.py @@ -8,11 +8,20 @@ from ...archive import ChunkBuffer from ...constants import * # NOQA -from ...helpers import bin_to_hex, msgpack, CommandError +from ...helpers import bin_to_hex, msgpack, CommandError, IntegrityError from ...manifest import Manifest from ...repository import Repository from ..repository_test import fchunk, corrupt_chunk_on_disk -from . import cmd, src_file, create_src_archive, open_archive, generate_archiver_tests, RK_ENCRYPTION +from . import ( + cmd, + src_file, + create_src_archive, + open_archive, + generate_archiver_tests, + read_chunk, + write_wrong_content_chunk, + RK_ENCRYPTION, +) pytest_generate_tests = lambda metafunc: generate_archiver_tests(metafunc, kinds="local,binary") # NOQA @@ -481,6 +490,62 @@ def test_verify_data(archivers, request, init_args): assert f"{src_file}: Missing file chunk detected" in output +def test_verify_data_wrong_chunk_content(archivers, request, monkeypatch): + # a chunk whose content does not match its id (only an evil borg client that had the repo key could + # have written it): the AEAD layer authenticates it just fine, only the id check notices, see #9994. + archiver = request.getfixturevalue(archivers) + if archiver.get_kind() != "local": + pytest.skip("only works locally, patches objects") + + check_cmd_setup(archiver) + archive, repository = open_archive(archiver.repository_path, "archive1") + with repository: + for item in archive.iter_items(): + if item.path.endswith(src_file): + chunk = item.chunks[-1] + break + write_wrong_content_chunk(archive, repository, chunk.id) + + # by default, reads do not check the id/content invariant, so this is not noticed: + monkeypatch.delenv("BORG_ASSERT_ID", raising=False) + cmd(archiver, "extract", "archive1", exit_code=0) + # ... but check --verify-data always checks it: + output = cmd(archiver, "check", "--archives-only", "--verify-data", exit_code=1) + assert f"{bin_to_hex(chunk.id)}, integrity error" in output + assert "id verification failed" in output + + # with "read" in BORG_ASSERT_ID, reads check it too: + monkeypatch.setenv("BORG_ASSERT_ID", "read") + with pytest.raises(IntegrityError): # local (not forked): the Error propagates instead of setting the rc + cmd(archiver, "extract", "archive1") + + +def test_repair_wrong_item_metadata_chunk_content(archivers, request, monkeypatch): + # check --repair re-packs the item metadata stream it reads into new chunks with freshly computed ids, + # so it re-certifies the id/content invariant, even though reads do not check it by default, see #9994. + archiver = request.getfixturevalue(archivers) + if archiver.get_kind() != "local": + pytest.skip("only works locally, patches objects") + + check_cmd_setup(archiver) + archive, repository = open_archive(archiver.repository_path, "archive1") + with repository: + chunk_id = archive.metadata.items[0] # first chunk of the item metadata stream + data = read_chunk(archive, repository, chunk_id, ro_type=ROBJ_ARCHIVE_STREAM) + # append a msgpack nil: the item stream still unpacks (so a read that does not check the id gets + # away with it), but the content does not hash to the chunk id any more. + write_wrong_content_chunk(archive, repository, chunk_id, ro_type=ROBJ_ARCHIVE_STREAM, wrong_data=data + b"\xc0") + + monkeypatch.delenv("BORG_ASSERT_ID", raising=False) + # a normal archives check reads the item metadata stream, but does not check the id: + output = cmd(archiver, "check", "--archives-only", exit_code=1) + assert "id verification failed" not in output + # --repair rebuilds the archive from what it reads, so there it is checked: + output = cmd(archiver, "check", "--repair", "--archives-only", exit_code=0) + assert f"{bin_to_hex(chunk_id)}" in output + assert "id verification failed" in output + + @pytest.mark.parametrize("init_args", [["--encryption=aes256-ocb"], ["--encryption", "none"]]) def test_corrupted_file_chunk(archivers, request, init_args): ## like test_verify_data, but also checks a repository-only check passes after repair and a plain diff --git a/src/borg/testsuite/archiver/recreate_cmd_test.py b/src/borg/testsuite/archiver/recreate_cmd_test.py index c40e3c59fc..14b8e5dada 100644 --- a/src/borg/testsuite/archiver/recreate_cmd_test.py +++ b/src/borg/testsuite/archiver/recreate_cmd_test.py @@ -6,6 +6,7 @@ import pytest from ...constants import * # NOQA +from ...helpers import IntegrityError from .. import changedir, are_hardlinks_supported from . import ( _create_test_caches, @@ -19,6 +20,8 @@ cmd, create_regular_file, create_test_files, + open_archive, + write_wrong_content_chunk, RK_ENCRYPTION, ) @@ -155,6 +158,31 @@ def test_recreate_rechunkify(archivers, request): assert num_chunks1 == num_chunks2 +def test_recreate_rechunkify_wrong_chunk_content(archivers, request, monkeypatch): + # re-chunking computes new chunk ids from the content it reads, so it re-certifies the + # chunkid == id_hash(content) invariant, even though reads do not check it by default, see #9994. + archiver = request.getfixturevalue(archivers) + if archiver.get_kind() != "local": + pytest.skip("only works locally, patches objects") + + create_regular_file(archiver.input_path, "file1", size=100000) + cmd(archiver, "repo-create", RK_ENCRYPTION) + cmd(archiver, "create", "test", "input", "--chunker-params", "7,9,8,127") + archive, repository = open_archive(archiver.repository_path, "test") + with repository: + for item in archive.iter_items(): + if item.path.endswith("file1"): + chunk = item.chunks[-1] + break + write_wrong_content_chunk(archive, repository, chunk.id) + + monkeypatch.delenv("BORG_ASSERT_ID", raising=False) + # without re-chunking, recreate does not read the file content chunks at all: + cmd(archiver, "recreate", "--exclude", "filename_never_matches", "-a", "test") + with pytest.raises(IntegrityError): # local (not forked): the Error propagates instead of setting the rc + cmd(archiver, "recreate", "--chunker-params", "fixed,4096", "-a", "test") + + def test_recreate_fixed_rechunkify(archivers, request): archiver = request.getfixturevalue(archivers) with open(os.path.join(archiver.input_path, "file"), "wb") as fd: diff --git a/src/borg/testsuite/archiver/transfer_cmd_test.py b/src/borg/testsuite/archiver/transfer_cmd_test.py index 8fc64070e1..7db1c93987 100644 --- a/src/borg/testsuite/archiver/transfer_cmd_test.py +++ b/src/borg/testsuite/archiver/transfer_cmd_test.py @@ -11,11 +11,19 @@ import pytest from ...constants import * # NOQA -from ...helpers import open_item +from ...helpers import open_item, IntegrityError from ...helpers.time import parse_timestamp from ...helpers.parseformat import parse_file_size, ChunkerParams from ..platform.platform_test import is_win32 -from . import cmd, create_regular_file, create_test_files, RK_ENCRYPTION, open_archive, generate_archiver_tests +from . import ( + cmd, + create_regular_file, + create_test_files, + RK_ENCRYPTION, + open_archive, + generate_archiver_tests, + write_wrong_content_chunk, +) pytest_generate_tests = lambda metafunc: generate_archiver_tests(metafunc, kinds="local,remote") # NOQA @@ -371,6 +379,33 @@ def check_repo(): check_repo() +@pytest.mark.parametrize("rechunkify", [False, True]) +def test_transfer_wrong_chunk_content(archivers, request, monkeypatch, rechunkify): + # transferring re-anchors the content in another repository, so the chunkid == id_hash(content) + # invariant is re-certified there, even though reads do not check it by default, see #9994. + archiver = request.getfixturevalue(archivers) + if archiver.get_kind() != "local": + pytest.skip("only works locally, patches objects") + + with setup_repos(archiver, monkeypatch) as other_repo1: + create_regular_file(archiver.input_path, "file1", size=100000) + cmd(archiver, "create", "arch1", "input") + archive, repository = open_archive(archiver.repository_path, "arch1") + with repository: + for item in archive.iter_items(): + if item.path.endswith("file1"): + chunk = item.chunks[-1] + break + write_wrong_content_chunk(archive, repository, chunk.id) + + monkeypatch.delenv("BORG_ASSERT_ID", raising=False) + args = ["transfer", other_repo1] + if rechunkify: + args += ["--chunker-params=buzhash,10,23,16,4095"] + with pytest.raises(IntegrityError): # local (not forked): the Error propagates instead of setting the rc + cmd(archiver, *args) + + def test_transfer_archive_metadata(archivers, request, monkeypatch): """Test transfer of archive metadata""" archiver = request.getfixturevalue(archivers) diff --git a/src/borg/testsuite/repoobj_test.py b/src/borg/testsuite/repoobj_test.py index 8a4fdffd27..4b41bacf2a 100644 --- a/src/borg/testsuite/repoobj_test.py +++ b/src/borg/testsuite/repoobj_test.py @@ -1,11 +1,20 @@ import pytest from ..constants import ROBJ_FILE_STREAM, ROBJ_MANIFEST, ROBJ_ARCHIVE_META -from ..crypto.key import PlaintextKey, CHPOKey +from ..crypto.key import PlaintextKey, AuthenticatedKey, CHPOKey from ..helpers import msgpack -from ..helpers.errors import IntegrityError +from ..helpers.errors import Error, IntegrityError from ..repository import Repository -from ..repoobj import OBJ_MAGIC, OBJ_VERSION, OBJ_VERSION_NO_HEADER_AAD, REPOOBJ_HEADER_SIZE, RepoObj +from ..repoobj import ( + ASSERT_ID_PLACES, + ASSERT_ID_PLACES_MANDATORY, + BORG_ASSERT_ID_DEFAULT, + OBJ_MAGIC, + OBJ_VERSION, + OBJ_VERSION_NO_HEADER_AAD, + REPOOBJ_HEADER_SIZE, + RepoObj, +) from ..legacy.repoobj import RepoObj1 from ..compress import LZ4 @@ -20,6 +29,15 @@ def key(repository): return PlaintextKey(repository) +@pytest.fixture +def authenticated_key(repository): + # "authenticated" mode: not encrypted, the id check is the only read path authentication. + key = AuthenticatedKey(repository) + key.init_from_random_data() + key.init_ciphers() + return key + + @pytest.fixture def aead_key(repository): # AEAD key, needed to test header_aad authentication; PlaintextKey is unauthenticated. @@ -271,6 +289,111 @@ def test_untampered_roundtrip_with_aead_key(aead_key): assert got_meta["custom"] == "something" +def wrong_content_object(repo_objs, id): + """Build an object that decrypts fine for , but whose content does not hash to . + + That is what an evil/compromised borg client with the repo key could store (the id goes into the AAD, + so the AEAD layer authenticates such an object just fine) - only assert_id() detects it. + """ + return repo_objs.format(id, {}, b"evil content", ro_type=ROBJ_FILE_STREAM) + + +def test_assert_id_places_default(aead_key, monkeypatch): + monkeypatch.delenv("BORG_ASSERT_ID", raising=False) + repo_objs = RepoObj(aead_key) + assert repo_objs.assert_id_places == frozenset(BORG_ASSERT_ID_DEFAULT) + assert repo_objs.assert_id_place == "read" + id = repo_objs.id_hash(b"foobar" * 10) # id of some other content + cdata = wrong_content_object(repo_objs, id) + + # by default, the id/content invariant is not checked on the "read" place, the AEAD authentication + # is enough there. + _, got_data = repo_objs.parse(id, cdata, ro_type=ROBJ_FILE_STREAM) + assert got_data == b"evil content" + + # the places that re-anchor content are verifying by default: + for place in ("repair", "transfer", "rechunk"): + with pytest.raises(IntegrityError): + repo_objs.parse(id, cdata, ro_type=ROBJ_FILE_STREAM, assert_id_place=place) + + # ... including via the place a command sets for everything it reads (transfer, re-chunking, repair): + repo_objs.set_assert_id_place("transfer") + with pytest.raises(IntegrityError): + repo_objs.parse(id, cdata, ro_type=ROBJ_FILE_STREAM) + + +@pytest.mark.parametrize( + "env_value, verifying", + [ + ("read", {"read"}), + ("read,repair,transfer,rechunk", {"read", "repair", "transfer", "rechunk"}), + ("transfer, rechunk", {"transfer", "rechunk"}), # whitespace is stripped + ("", set()), # verify at none of the configurable places + ], +) +def test_assert_id_places_env_var(aead_key, monkeypatch, env_value, verifying): + monkeypatch.setenv("BORG_ASSERT_ID", env_value) + repo_objs = RepoObj(aead_key) + assert repo_objs.assert_id_places == frozenset(verifying) + id = repo_objs.id_hash(b"foobar" * 10) + cdata = wrong_content_object(repo_objs, id) + + for place in ASSERT_ID_PLACES: + if place in verifying: + with pytest.raises(IntegrityError): + repo_objs.parse(id, cdata, ro_type=ROBJ_FILE_STREAM, assert_id_place=place) + else: + repo_objs.parse(id, cdata, ro_type=ROBJ_FILE_STREAM, assert_id_place=place) + + +@pytest.mark.parametrize("env_value", ["", "read", "repair,transfer,rechunk"]) +def test_assert_id_mandatory_places(aead_key, monkeypatch, env_value): + # borg check --verify-data is the audit that makes not verifying elsewhere defensible, + # so it verifies no matter what the user configured - it is not a configurable place. + monkeypatch.setenv("BORG_ASSERT_ID", env_value) + repo_objs = RepoObj(aead_key) + id = repo_objs.id_hash(b"foobar" * 10) + cdata = wrong_content_object(repo_objs, id) + + for place in ASSERT_ID_PLACES_MANDATORY: + assert place not in repo_objs.assert_id_places + with pytest.raises(IntegrityError): + repo_objs.parse(id, cdata, ro_type=ROBJ_FILE_STREAM, assert_id_place=place) + + +def test_assert_id_places_env_var_invalid(aead_key, monkeypatch): + monkeypatch.setenv("BORG_ASSERT_ID", "read,typo") + with pytest.raises(Error) as exc_info: + RepoObj(aead_key) + assert "typo" in str(exc_info.value) + assert "read" in str(exc_info.value) # it tells the valid place names + + +def test_assert_id_places_env_var_mandatory_place(aead_key, monkeypatch): + # the mandatory places are not accepted in the env var: they can not be switched off, so + # offering to switch them "on" would be misleading. + monkeypatch.setenv("BORG_ASSERT_ID", "read,verify_data") + with pytest.raises(Error) as exc_info: + RepoObj(aead_key) + assert "verify_data: always verifies, can not be configured" in str(exc_info.value) + + +@pytest.mark.parametrize("key_fixture", ["key", "authenticated_key"]) +def test_assert_id_never_skipped_for_non_aead_key(key_fixture, request, monkeypatch): + # PlaintextKey ("none" mode) and AuthenticatedKey ("authenticated" mode): there is no AEAD, so the id + # check is the only integrity check reads have and thus must happen no matter what the user configured. + key = request.getfixturevalue(key_fixture) + monkeypatch.setenv("BORG_ASSERT_ID", "") # verify nowhere - but this is not switchable off + assert key.id_check_is_authentication + repo_objs = RepoObj(key) + id = repo_objs.id_hash(b"foobar" * 10) + cdata = wrong_content_object(repo_objs, id) + + for place in ASSERT_ID_PLACES: + with pytest.raises(IntegrityError): + repo_objs.parse(id, cdata, ro_type=ROBJ_FILE_STREAM, assert_id_place=place) + + def test_version1_object_without_header_aad_still_readable(aead_key): # Builds an OBJ_VERSION_NO_HEADER_AAD object by hand (format() only writes OBJ_VERSION_HEADER_AAD) # and checks that parse()/parse_meta() still decrypt it.