From 555106f9bb47dc08d7286c9c66207cb08e55a0a0 Mon Sep 17 00:00:00 2001 From: Mrityunjay Raj Date: Thu, 13 Aug 2026 03:30:06 +0530 Subject: [PATCH 1/8] check --repair: resync past corrupt object headers when rebuilding the chunks index, #8476 When check --repair rebuilds the chunks index from the packs, a corrupt object header now makes iter_headers resync rather than raise: it takes a validate function and scans forward for the next object, in 1 MiB windows that overlap by one header so a header on a window boundary is still found. Repository-only checks pass no validate and keep raising IntegrityError on a corrupt header. OBJ_MAGIC also occurs inside payloads, so a candidate is accepted only when it authenticates. For AEAD keys, decrypting the metadata authenticates it against the header's magic, version and chunk_id, so the walk confirms a chunk id from a few hundred bytes. Keys that authenticate by chunk_id == id_hash(content) (id_check_is_authentication) read the whole object and parse() at the "repair" id place; validate.needs_data selects between the two. Authentication needs the key, so check --repair makes it before the rebuild with manifest_only=True. A repair that cannot read the manifest has no key and walks without resyncing. --- docs/internals/packs.rst | 28 +++- src/borg/archive.py | 39 ++++- src/borg/cache.py | 11 +- src/borg/repository.py | 103 +++++++++--- src/borg/testsuite/archiver/check_cmd_test.py | 28 ++++ src/borg/testsuite/cache_test.py | 23 ++- src/borg/testsuite/repository_test.py | 147 ++++++++++++++++++ 7 files changed, 350 insertions(+), 29 deletions(-) diff --git a/docs/internals/packs.rst b/docs/internals/packs.rst index 92ed7d8d56..4c85a2ff40 100644 --- a/docs/internals/packs.rst +++ b/docs/internals/packs.rst @@ -94,10 +94,30 @@ A reader locates the next blob by advancing:: next_blob_offset = current_blob_offset + REPOOBJ_HEADER_SIZE + meta_size + data_size -The per-blob magic limits the blast radius of corrupted length fields: if -``meta_size`` or ``data_size`` is damaged, the scanner loses at most one blob. -Once it finds the next ``OBJ_MAGIC`` sequence it resumes. Other corruption -(payload bit flips) is caught by AEAD on that blob without losing position. +``iter_headers()`` checks every header it walks: it must have ``OBJ_MAGIC``, a +supported version, and sizes that keep the blob inside the pack. A header that +fails these checks means a corrupt pack, and ``IntegrityError`` is raised. + +The per-blob magic limits the blast radius of corrupted length fields. The +repair walk (``iter_headers(validate=...)``, used when ``borg check --repair`` +rebuilds the chunks index from the packs) scans forward for the next blob and +resumes there, so the blobs after the damaged part of the pack are still found. + +``OBJ_MAGIC`` occurs inside the payloads as well, and in ``none`` and +``authenticated`` mode the payloads are user content stored as it is, so a +backed up file can contain something shaped like a blob. The scan therefore +accepts a candidate only if it parses. For the AEAD keys it reads the header and +the encrypted metadata, a few hundred bytes: decrypting the metadata +authenticates it together with the header's magic, version and chunk_id, which +are its AAD (additional authenticated data: authenticated with the ciphertext, +but not encrypted). The other keys authenticate by ``chunk_id == id_hash(content)`` +(``KeyBase.id_check_is_authentication``), which needs the blob's data, so for +those the scan reads the whole blob. The key is needed either way; a repair that +cannot read the manifest walks without scanning. + +``data_size`` is not part of that AAD, so accepting a candidate authenticates +its chunk id, and its size only as far as the blob fits into the pack. Bit flips +in the data are caught when the blob is read, on that blob alone. Blobs follow one another contiguously with no padding:: diff --git a/src/borg/archive.py b/src/borg/archive.py index bc45bdad41..c9d2e7f2c7 100644 --- a/src/borg/archive.py +++ b/src/borg/archive.py @@ -2127,6 +2127,32 @@ def __next__(self): return next(self._unpacker) +def resync_validator(repo_objs): + """Return validate(chunk_id, obj): True if obj is the repo object with id chunk_id. + + obj holds an object's header and encrypted metadata, plus its encrypted data when + validate.needs_data is set. For most keys, decrypting the metadata authenticates it against the + header (magic, version, chunk_id), so the metadata alone decides. Keys that authenticate by + chunk_id == id_hash(content) (id_check_is_authentication) need the data; for them + validate.needs_data is set and parse() checks that id at the "repair" id place. + """ + needs_data = repo_objs.key.id_check_is_authentication + + def validate(chunk_id, obj): + try: + if needs_data: + repo_objs.parse(chunk_id, obj, ro_type=ROBJ_DONTCARE, assert_id_place="repair") + else: + repo_objs.parse_meta(chunk_id, obj, ro_type=ROBJ_DONTCARE) + except Exception: + # authentication, id check, msgpack or decompression can each raise on non-object bytes. + return False + return True + + validate.needs_data = needs_data + return validate + + class ArchiveChecker: # Bound how many missing file chunks rebuild_archives buffers for its end-of-run report, # so checking a badly damaged repo with very many missing chunks can not exhaust memory. @@ -2181,7 +2207,18 @@ def check( # so we do not rebuild it from the packs (reading every pack is far too slow for a routine check). # --repair does rebuild from the packs (slow_rebuild=repair), working from the real packs so it # can detect and fix archives that reference chunks whose pack has gone missing. - self.chunks = build_chunkindex_from_repo(self.repository, slow_rebuild=repair, write_immediately=False) + # Under --repair, validate lets the rebuild resync past a corrupt object header (see resync_validator). + # It authenticates objects with the key, so make the key first; manifest_only=True makes make_key use + # the manifest, not self.chunks, which is still unset here. + if self.key is None: + try: + self.key = self.make_key(repository, manifest_only=True) + except IntegrityError as err: + logger.warning(f"{err}. Packs with a corrupt object header can not be repaired.") + validate = resync_validator(RepoObj(self.key)) if repair and self.key is not None else None + self.chunks = build_chunkindex_from_repo( + self.repository, slow_rebuild=repair, validate=validate, write_immediately=False + ) if self.key is None: self.key = self.make_key(repository) self.repo_objs = RepoObj(self.key) diff --git a/src/borg/cache.py b/src/borg/cache.py index 0e4241a99c..37b6603276 100644 --- a/src/borg/cache.py +++ b/src/borg/cache.py @@ -857,7 +857,13 @@ def repack_chunkindex(repository): def build_chunkindex_from_repo( - repository, *, slow_rebuild=False, fragments_only=False, write_immediately=False, init_flags=ChunkIndex.F_USED + repository, + *, + slow_rebuild=False, + fragments_only=False, + validate=None, + write_immediately=False, + init_flags=ChunkIndex.F_USED, ): # fragments_only: build the index from the index/ fragments only, returning None if they cannot be # read completely, and never write to the repo. @@ -952,7 +958,8 @@ def build_chunkindex_from_repo( repository._lock_refresh() pi.show(increase=1) pack_id = hex_to_bin(info.name) - for chunk_id, obj_offset, obj_size in PackReader(repository.store, pack_id).iter_headers(): + # validate makes iter_headers resync past a corrupt object header and index the objects after it. + for chunk_id, obj_offset, obj_size in PackReader(repository.store, pack_id).iter_headers(validate=validate): num_chunks += 1 chunks[chunk_id] = ChunkIndexEntry( flags=init_flags, size=0, pack_id=pack_id, obj_offset=obj_offset, obj_size=obj_size diff --git a/src/borg/repository.py b/src/borg/repository.py index 5b775046fc..75cc4c61b0 100644 --- a/src/borg/repository.py +++ b/src/borg/repository.py @@ -30,7 +30,7 @@ from .storelocking import Lock from .logger import create_logger from .manifest import NoManifestError -from .repoobj import RepoObj, OBJ_MAGIC +from .repoobj import RepoObj, OBJ_MAGIC, SUPPORTED_OBJ_VERSIONS from .crypto.key import is_keyfile logger = create_logger(__name__) @@ -38,6 +38,9 @@ # an object name is its sha256 as 64 lowercase hex digits. _valid_object_name = re.compile(r"[0-9a-f]{64}").fullmatch +# how much of a pack PackReader reads at once when searching for the next object header. +RESYNC_WINDOW_SIZE = 1024 * 1024 + def repo_lister(repository, *, limit=None): marker = None @@ -362,24 +365,73 @@ def read(self, offset, size): return self.store.load(self.key, offset=offset, size=size) def size(self): - """Return the pack size in bytes; for a store-backed pack this is one metadata lookup.""" + """Return the pack size in bytes (a store metadata lookup, unless the pack is in memory).""" if self.pack_contents is not None: return len(self.pack_contents) return self.store.info(self.key).size - def iter_headers(self): + @staticmethod + def _parse_header(hdr_data, offset, pack_size): + """Return the ObjHeader in hdr_data if it is a valid header at offset, None otherwise. + + Valid means: OBJ_MAGIC, a supported version, and an object that fits into the pack. + """ + hdr = RepoObj.ObjHeader(*RepoObj.obj_header.unpack(hdr_data)) + if hdr.magic != OBJ_MAGIC or hdr.version not in SUPPORTED_OBJ_VERSIONS: + return None + if offset + RepoObj.obj_header.size + hdr.meta_size + hdr.data_size > pack_size: + return None + return hdr + + def _find_header(self, offset, pack_size, validate): + """Scan forward from offset for the next object validate accepts, return its offset or None. + + A pack has no framing besides the object headers, so this searches for OBJ_MAGIC. That byte + sequence also occurs inside payloads, so a candidate is accepted only when its header parses + and validate confirms it. + """ + hdr_size = RepoObj.obj_header.size + while offset + hdr_size <= pack_size: + # a window at a time, so the scan costs one store request per RESYNC_WINDOW_SIZE bytes. + buf = bytes(self.read(offset, min(RESYNC_WINDOW_SIZE, pack_size - offset))) + if len(buf) < hdr_size: + break + pos = 0 + while True: + pos = buf.find(OBJ_MAGIC, pos) + if pos < 0 or pos + hdr_size > len(buf): + break # not in this window, or a header overlapping its end: the next window has it + hdr = self._parse_header(buf[pos : pos + hdr_size], offset + pos, pack_size) + if hdr is not None: + obj_size = hdr_size + hdr.meta_size + hdr.data_size + # an object is at most MAX_DATA_SIZE bytes (Repository.put), so a larger candidate is a + # false match on OBJ_MAGIC in a payload. + if obj_size <= MAX_DATA_SIZE: + size = obj_size if validate.needs_data else hdr_size + hdr.meta_size + end = pos + size + # the window holds these bytes, unless the candidate crosses its end. + obj = buf[pos:end] if end <= len(buf) else self.read(offset + pos, size) + if validate(hdr.chunk_id, obj): + return offset + pos + pos += 1 + # step by the window less one header, so a magic straddling the boundary is still found. + offset += max(len(buf) - (hdr_size - 1), 1) + return None + + def iter_headers(self, validate=None): """Yield (chunk_id, offset, size) for each object by walking the fixed object headers. - Only the headers are read, not the payloads, so locating every object costs one short - range read per object (or just a slice, when the pack is already in memory), plus one - store metadata lookup for the pack size. + The walk reads a header per object: one short range read each (or a slice, for a pack in + memory), plus one store metadata lookup for the pack size. + + A header must have OBJ_MAGIC, a supported version and describe an object that fits into + the pack, otherwise the pack is corrupt and IntegrityError is raised. A read shorter than + a header ends the walk: that is the end of the pack. - Each full header must have OBJ_MAGIC and describe an object that fits into the pack, - otherwise the pack is corrupt and IntegrityError is raised. Ending the walk instead - would be worse than raising: the chunks index rebuilt from these headers would just be - missing the rest of the pack, and borg check --repair would then "fix" the archives by - dropping chunks that are there. - A trailing partial header is the clean end of the pack, not corruption. + validate(chunk_id, obj): returns whether obj is a repo object with id chunk_id, where obj is + its header and metadata, plus its data when validate.needs_data is set. When validate is + given, a corrupt header makes the walk resync: it scans for the next object validate accepts + (see _find_header), continues there, and logs the skipped bytes. """ pack_hex = bin_to_hex(self.pack_id) if self.pack_id is not None else "" pack_size = self.size() @@ -389,17 +441,26 @@ def iter_headers(self): hdr_data = self.read(offset, hdr_size) if len(hdr_data) < hdr_size: break # clean EOF, or trailing partial bytes - hdr = RepoObj.ObjHeader(*RepoObj.obj_header.unpack(hdr_data)) - if hdr.magic != OBJ_MAGIC: - raise IntegrityError( - f'pack {pack_hex}: no object header at offset {offset} (pack corruption), run "borg check"' + hdr = self._parse_header(hdr_data, offset, pack_size) + if hdr is None: + if validate is None: + raise IntegrityError( + f'pack {pack_hex}: invalid object header at offset {offset} (pack corruption), run "borg check"' + ) + next_offset = self._find_header(offset + 1, pack_size, validate) + if next_offset is None: + logger.warning( + f"pack {pack_hex}: invalid object header at offset {offset} and none after it, " + f"skipping the remaining {pack_size - offset} bytes." + ) + break + logger.warning( + f"pack {pack_hex}: invalid object header at offset {offset}, " + f"skipping {next_offset - offset} bytes to the next one." ) + offset = next_offset + continue obj_size = hdr_size + hdr.meta_size + hdr.data_size - if offset + obj_size > pack_size: - raise IntegrityError( - f"pack {pack_hex}: object extends past end of file at offset {offset} " - f'(pack corruption), run "borg check"' - ) yield hdr.chunk_id, offset, obj_size offset += obj_size diff --git a/src/borg/testsuite/archiver/check_cmd_test.py b/src/borg/testsuite/archiver/check_cmd_test.py index 9c4b9014bb..bf38f76082 100644 --- a/src/borg/testsuite/archiver/check_cmd_test.py +++ b/src/borg/testsuite/archiver/check_cmd_test.py @@ -718,6 +718,34 @@ def test_extra_chunks(archivers, request): cmd(archiver, "check", "-v", exit_code=0) # check does not deal with orphans anymore +def test_repair_resyncs_pack_with_corrupt_object_header(archivers, request): + """--repair rebuilds the index from a pack whose object header is damaged. + + A damaged header makes the walk lose the object boundaries, so the rebuild scans for the next + object that authenticates and carries on there. That needs the key, which --repair makes before + the rebuild. Repairing the pack itself is a separate step, see #10026. + """ + archiver = request.getfixturevalue(archivers) + if archiver.get_kind() != "local": + pytest.skip("inspects the store directly") + check_cmd_setup(archiver) + cmd(archiver, "check", exit_code=0) + + with Repository(archiver.repository_location, exclusive=True) as repository: + # damage the header of the second object of a pack that holds more than two. + by_pack = {} + for chunk_id, entry in repository.chunks.items(): + by_pack.setdefault(entry.pack_id, []).append((entry.obj_offset, chunk_id)) + pack_id, objs = next((p, sorted(o)) for p, o in by_pack.items() if len(o) > 2) + damaged_offset, _ = objs[1] + key = "packs/" + bin_to_hex(pack_id) + repository.store_store(key, corrupt(repository.store_load(key), damaged_offset)) + + output = cmd(archiver, "check", "--repair", "--debug", exit_code=0) + assert f"invalid object header at offset {damaged_offset}" in output + assert "bytes to the next one" in output # the rebuild resumed at the next object + + def test_repair_finish_flushes_pack_writer(archivers, request): """finish() stores chunks re-added during --repair before it (re)builds the index (#10055). diff --git a/src/borg/testsuite/cache_test.py b/src/borg/testsuite/cache_test.py index 24e64488d8..192d3006dd 100644 --- a/src/borg/testsuite/cache_test.py +++ b/src/borg/testsuite/cache_test.py @@ -26,7 +26,8 @@ ) from ..hashindex import ChunkIndex, ChunkIndexEntry from ..crypto.key import AESOCBKey -from ..helpers import safe_ns +from ..helpers import bin_to_hex, safe_ns +from ..helpers import IntegrityError from ..helpers.msgpack import int_to_timestamp from ..manifest import Manifest from ..repository import Repository @@ -505,6 +506,26 @@ def test_close_consolidates_fragments_across_sessions(tmp_path, monkeypatch): assert cid in index +def test_build_chunkindex_repair_resyncs_after_corrupt_header(tmp_path): + """A corrupt object header fails the rebuild, but with repair=True the rest of the pack is indexed.""" + from .repository_test import accept_all, fchunk + + obj1 = bytearray(fchunk(b"first", chunk_id=H(90))) + obj2 = fchunk(b"second", chunk_id=H(91)) + obj1[0] ^= 0xFF # break the magic of the first object's header + pack_id = H(92) + with Repository(os.fspath(tmp_path / "repository"), exclusive=True, create=True) as repository: + repository.store_store("packs/" + bin_to_hex(pack_id), bytes(obj1) + obj2) + with pytest.raises(IntegrityError): + build_chunkindex_from_repo(repository, slow_rebuild=True) + # accept_all: accepts every candidate, so this exercises the plumbing only. + index = build_chunkindex_from_repo(repository, slow_rebuild=True, validate=accept_all) + assert H(91) in index # found by resyncing past the damaged header + assert H(90) not in index # its header is gone, so the object can not be indexed + assert index[H(91)].pack_id == pack_id + assert index[H(91)].obj_offset == len(obj1) + + def test_repack_leaves_sealed_untouched_and_reconstructs(tmp_path, monkeypatch): """Sealed (>= MIN) fragments survive a repack; build_chunkindex_from_repo reconstructs the index.""" monkeypatch.setattr(cache_mod, "CHUNKINDEX_FRAGMENT_ENTRIES_MIN", 1000) diff --git a/src/borg/testsuite/repository_test.py b/src/borg/testsuite/repository_test.py index 2352e5ad79..11aab716c9 100644 --- a/src/borg/testsuite/repository_test.py +++ b/src/borg/testsuite/repository_test.py @@ -13,6 +13,11 @@ from ..constants import MAX_CLOCK_SKEW from ..helpers import IntegrityError, Location, bin_to_hex from ..hashindex import ChunkIndex +from .. import repository as repository_module +from ..archive import resync_validator +from ..compress import CNONE +from ..constants import ROBJ_FILE_STREAM +from ..crypto.key import CHPOKey, PlaintextKey from ..repository import Repository, MAX_DATA_SIZE, propagate_rsh, rest_serve_command, PackWriter, PackReader from ..repository import PackTracker from ..repoobj import RepoObj, OBJ_MAGIC, OBJ_VERSION @@ -1868,6 +1873,148 @@ def test_pack_reader_raises_on_object_past_end_of_pack_through_store(tmp_path): list(reader.iter_headers()) +def test_pack_reader_raises_on_unsupported_version(): + obj = bytearray(fchunk(b"data", chunk_id=H(7))) + obj[len(OBJ_MAGIC)] = 0xEE # version byte + with pytest.raises(IntegrityError): + list(PackReader(pack_contents=bytes(obj)).iter_headers()) + + +def accept_all(chunk_id, obj): + # validate stand-in: accepts every candidate. + return True + + +accept_all.needs_data = False + + +def test_pack_reader_resync_skips_to_next_object(): + # after a corrupt header the walk continues at the next object. + obj1 = bytearray(fchunk(b"payload-one", meta=b"meta1", chunk_id=H(1))) + obj2 = fchunk(b"payload-two", meta=b"meta2", chunk_id=H(2)) + obj1[0] ^= 0xFF # break the magic of the first object's header + reader = PackReader(pack_contents=bytes(obj1) + obj2) + assert list(reader.iter_headers(validate=accept_all)) == [(H(2), len(obj1), len(obj2))] + + +def test_pack_reader_resync_recovers_from_corrupted_size(): + # a header whose sizes point past the pack, so the next object is found by scanning. + obj1 = bytearray(fchunk(b"payload-one", meta=b"meta1", chunk_id=H(1))) + obj2 = fchunk(b"payload-two", meta=b"meta2", chunk_id=H(2)) + obj3 = fchunk(b"payload-three", chunk_id=H(3)) + # the header's data_size field (magic 8, version 1, chunk_id 32, meta_size 4, data_size 4), + # set to a value reaching far past the end of the pack: + obj1[45:49] = b"\xff\xff\xff\x00" + pack = bytes(obj1) + obj2 + obj3 + reader = PackReader(pack_contents=pack) + assert list(reader.iter_headers(validate=accept_all)) == [ + (H(2), len(obj1), len(obj2)), + (H(3), len(obj1) + len(obj2), len(obj3)), + ] + + +def test_pack_reader_resync_ignores_magic_in_payload(): + # both headers are broken, so the scan runs into the OBJ_MAGIC in obj2's payload before obj3. + obj1 = bytearray(fchunk(b"data", chunk_id=H(1))) + obj2 = bytearray(fchunk(OBJ_MAGIC + b"looks like a header, is not", chunk_id=H(2))) + obj3 = fchunk(b"payload-three", chunk_id=H(3)) + obj1[0] ^= 0xFF + obj2[0] ^= 0xFF + reader = PackReader(pack_contents=bytes(obj1) + bytes(obj2) + obj3) + assert list(reader.iter_headers(validate=accept_all)) == [(H(3), len(obj1) + len(obj2), len(obj3))] + + +def test_pack_reader_resync_finds_header_across_window_boundary(monkeypatch): + # the next header straddles a scan window boundary. + monkeypatch.setattr(repository_module, "RESYNC_WINDOW_SIZE", 64) + obj1 = bytearray(fchunk(b"x" * 100, chunk_id=H(1))) + obj2 = fchunk(b"payload-two", chunk_id=H(2)) + obj1[0] ^= 0xFF + reader = PackReader(pack_contents=bytes(obj1) + obj2) + assert list(reader.iter_headers(validate=accept_all)) == [(H(2), len(obj1), len(obj2))] + + +def test_pack_reader_resync_no_further_header(): + # no object after the damage: the walk ends with what it found. + obj = fchunk(b"data", chunk_id=H(1)) + pack = obj + b"\xaa" * 200 + reader = PackReader(pack_contents=pack) + assert list(reader.iter_headers(validate=accept_all)) == [(H(1), 0, len(obj))] + + +def aead_repo_objs(tmp_path): + # a RepoObj with an AEAD key, whose metadata authenticates on its own. + repository = Repository(str(tmp_path / "repo"), create=True) + key = CHPOKey(repository) + key.init_from_random_data() + key.init_ciphers() + return RepoObj(key) + + +def test_pack_reader_resync_rejects_metadata_that_does_not_authenticate(tmp_path): + # bytes with a well-formed header whose metadata does not decrypt: the scan must walk past them. + repo_objs = aead_repo_objs(tmp_path) + data = b"the real next object" + real_id = repo_objs.id_hash(data) + obj1 = bytearray(repo_objs.format(repo_objs.id_hash(b"first"), {}, b"first", ro_type=ROBJ_FILE_STREAM)) + obj1[0] ^= 0xFF # break obj1's header, so the walk has to resync + garbage = fchunk(b"payload", meta=b"not encrypted metadata", chunk_id=H(9)) + obj2 = repo_objs.format(real_id, {}, data, ro_type=ROBJ_FILE_STREAM) + reader = PackReader(pack_contents=bytes(obj1) + garbage + obj2) + headers = list(reader.iter_headers(validate=resync_validator(repo_objs))) + assert headers == [(real_id, len(obj1) + len(garbage), len(obj2))] + + +def test_pack_reader_resync_accepts_an_object_with_corrupt_data(tmp_path): + # the AEAD keys authenticate the metadata, so the scan resyncs at an object with damaged data. + # Reading that object reports the damage. + repo_objs = aead_repo_objs(tmp_path) + data = b"the real next object" + real_id = repo_objs.id_hash(data) + obj1 = bytearray(repo_objs.format(repo_objs.id_hash(b"first"), {}, b"first", ro_type=ROBJ_FILE_STREAM)) + obj1[0] ^= 0xFF # break obj1's header, so the walk has to resync + obj2 = bytearray(repo_objs.format(real_id, {}, data, ro_type=ROBJ_FILE_STREAM)) + obj2[-1] ^= 0xFF # damage the encrypted data, leaving the header and the metadata intact + reader = PackReader(pack_contents=bytes(obj1) + bytes(obj2)) + headers = list(reader.iter_headers(validate=resync_validator(repo_objs))) + assert headers == [(real_id, len(obj1), len(obj2))] + with pytest.raises(IntegrityError): + repo_objs.parse(real_id, bytes(obj2), ro_type=ROBJ_FILE_STREAM) + + +def test_pack_reader_resync_rejects_user_content_that_looks_like_an_object(tmp_path): + # In "none" mode with no compression, user content lands in the pack as it is, so a backed up + # file can contain something shaped like an object. Those keys authenticate by the id check over + # the content, so the scan reads whole candidates. + repository = Repository(str(tmp_path / "repo"), create=True) + repo_objs = RepoObj(PlaintextKey(repository)) + assert resync_validator(repo_objs).needs_data + repo_objs.compressor = CNONE() + decoy = bytearray(repo_objs.format(repo_objs.id_hash(b"decoy"), {}, b"decoy", ro_type=ROBJ_FILE_STREAM)) + decoy[-1] ^= 0xFF # its content no longer hashes to the id in its header + content = bytes(decoy) # a user stores exactly those bytes in a file + obj1 = bytearray(repo_objs.format(repo_objs.id_hash(content), {}, content, ro_type=ROBJ_FILE_STREAM)) + assert content in obj1 # the decoy is in the pack verbatim + obj1[0] ^= 0xFF # break obj1's header, so the walk resyncs and runs into the decoy + data = b"the real next object" + real_id = repo_objs.id_hash(data) + obj2 = repo_objs.format(real_id, {}, data, ro_type=ROBJ_FILE_STREAM) + reader = PackReader(pack_contents=bytes(obj1) + obj2) + headers = list(reader.iter_headers(validate=resync_validator(repo_objs))) + assert headers == [(real_id, len(obj1), len(obj2))] + + +def test_pack_reader_resync_through_store(tmp_path): + obj1 = bytearray(fchunk(b"FIRST", chunk_id=H(47))) + obj2 = fchunk(b"SECOND", chunk_id=H(48)) + obj1[0] ^= 0xFF + pack_id = H(50) + with Repository(str(tmp_path / "repo"), exclusive=True, create=True) as repository: + repository.store_store("packs/" + bin_to_hex(pack_id), bytes(obj1) + obj2) + reader = PackReader(repository.store, pack_id) + assert list(reader.iter_headers(validate=accept_all)) == [(H(48), len(obj1), len(obj2))] + + def test_pack_reader_size(tmp_path): obj = fchunk(b"data", meta=b"meta", chunk_id=H(6)) assert PackReader(pack_contents=obj).size() == len(obj) From fe06ab792c7e4e90021436a52a6e7a49e673f927 Mon Sep 17 00:00:00 2001 From: Mrityunjay Raj Date: Sat, 15 Aug 2026 19:28:07 +0530 Subject: [PATCH 2/8] check --repair: validate a resync candidate from its metadata slot alone, #8476 Every key mode covers the object header by the metadata slot's AAD, so parse_meta confirms a candidate and validate.needs_data is gone. --- docs/internals/packs.rst | 25 ++++++++--------- src/borg/archive.py | 28 ++++++++----------- src/borg/cache.py | 3 +- src/borg/repository.py | 16 +++++------ src/borg/testsuite/archiver/check_cmd_test.py | 7 ++--- src/borg/testsuite/cache_test.py | 6 ++-- src/borg/testsuite/repository_test.py | 20 ++++++------- 7 files changed, 49 insertions(+), 56 deletions(-) diff --git a/docs/internals/packs.rst b/docs/internals/packs.rst index 4c85a2ff40..9a064f6ac9 100644 --- a/docs/internals/packs.rst +++ b/docs/internals/packs.rst @@ -103,19 +103,18 @@ repair walk (``iter_headers(validate=...)``, used when ``borg check --repair`` rebuilds the chunks index from the packs) scans forward for the next blob and resumes there, so the blobs after the damaged part of the pack are still found. -``OBJ_MAGIC`` occurs inside the payloads as well, and in ``none`` and -``authenticated`` mode the payloads are user content stored as it is, so a -backed up file can contain something shaped like a blob. The scan therefore -accepts a candidate only if it parses. For the AEAD keys it reads the header and -the encrypted metadata, a few hundred bytes: decrypting the metadata -authenticates it together with the header's magic, version and chunk_id, which -are its AAD (additional authenticated data: authenticated with the ciphertext, -but not encrypted). The other keys authenticate by ``chunk_id == id_hash(content)`` -(``KeyBase.id_check_is_authentication``), which needs the blob's data, so for -those the scan reads the whole blob. The key is needed either way; a repair that -cannot read the manifest walks without scanning. - -``data_size`` is not part of that AAD, so accepting a candidate authenticates +``OBJ_MAGIC`` occurs inside the payloads as well, and in the ``none-*`` and +``authenticated-*`` modes the payloads are user content stored as it is, so a +backed up file can contain something shaped like a blob. A candidate is +therefore accepted only when its metadata slot verifies against the header AAD +described above; the header and that slot, a few hundred bytes, are what the +scan reads. Verifying needs the key, so a repair that cannot read the manifest +walks without scanning. + +In the ``none-*`` modes the tag is an unkeyed checksum, so the scan accepts any +well-formed blob, including one a backed up file contains. + +``data_size`` is not part of the AAD, so accepting a candidate authenticates its chunk id, and its size only as far as the blob fits into the pack. Bit flips in the data are caught when the blob is read, on that blob alone. diff --git a/src/borg/archive.py b/src/borg/archive.py index c9d2e7f2c7..69a323c6d0 100644 --- a/src/borg/archive.py +++ b/src/borg/archive.py @@ -2130,26 +2130,22 @@ def __next__(self): def resync_validator(repo_objs): """Return validate(chunk_id, obj): True if obj is the repo object with id chunk_id. - obj holds an object's header and encrypted metadata, plus its encrypted data when - validate.needs_data is set. For most keys, decrypting the metadata authenticates it against the - header (magic, version, chunk_id), so the metadata alone decides. Keys that authenticate by - chunk_id == id_hash(content) (id_check_is_authentication) need the data; for them - validate.needs_data is set and parse() checks that id at the "repair" id place. + obj is an object's header plus its metadata slot. Parsing that slot verifies its tag, which is + computed over the header's magic, version and chunk id as well (AAD, additional authenticated + data: bytes the tag covers without being part of the ciphertext). + + In the "none-*" modes the tag is an unkeyed checksum, so validate accepts any well-formed + object, including one that a backed up file contains. """ - needs_data = repo_objs.key.id_check_is_authentication def validate(chunk_id, obj): try: - if needs_data: - repo_objs.parse(chunk_id, obj, ro_type=ROBJ_DONTCARE, assert_id_place="repair") - else: - repo_objs.parse_meta(chunk_id, obj, ro_type=ROBJ_DONTCARE) + repo_objs.parse_meta(chunk_id, obj, ro_type=ROBJ_DONTCARE) except Exception: - # authentication, id check, msgpack or decompression can each raise on non-object bytes. + # arbitrary bytes fail the tag, the msgpack unpacking or the length checks. return False return True - validate.needs_data = needs_data return validate @@ -2207,10 +2203,10 @@ def check( # so we do not rebuild it from the packs (reading every pack is far too slow for a routine check). # --repair does rebuild from the packs (slow_rebuild=repair), working from the real packs so it # can detect and fix archives that reference chunks whose pack has gone missing. - # Under --repair, validate lets the rebuild resync past a corrupt object header (see resync_validator). - # It authenticates objects with the key, so make the key first; manifest_only=True makes make_key use - # the manifest, not self.chunks, which is still unset here. - if self.key is None: + # --repair also passes validate, which makes the rebuild resync past a corrupt object header. + # Validating needs the key, so read it here. manifest_only=True, because the other source + # make_key reads keys from is self.chunks, which is only built below. + if repair and self.key is None: try: self.key = self.make_key(repository, manifest_only=True) except IntegrityError as err: diff --git a/src/borg/cache.py b/src/borg/cache.py index 37b6603276..3f78b46335 100644 --- a/src/borg/cache.py +++ b/src/borg/cache.py @@ -867,6 +867,8 @@ def build_chunkindex_from_repo( ): # fragments_only: build the index from the index/ fragments only, returning None if they cannot be # read completely, and never write to the repo. + # validate: handed to PackReader.iter_headers when rebuilding from the packs, making it resync + # past a corrupt object header rather than raise IntegrityError. assert not (slow_rebuild and fragments_only) assert not (fragments_only and write_immediately) # fragments_only never writes to the repo # first, try to build a fresh, mostly complete chunk index from centrally stored index fragments: @@ -958,7 +960,6 @@ def build_chunkindex_from_repo( repository._lock_refresh() pi.show(increase=1) pack_id = hex_to_bin(info.name) - # validate makes iter_headers resync past a corrupt object header and index the objects after it. for chunk_id, obj_offset, obj_size in PackReader(repository.store, pack_id).iter_headers(validate=validate): num_chunks += 1 chunks[chunk_id] = ChunkIndexEntry( diff --git a/src/borg/repository.py b/src/borg/repository.py index 75cc4c61b0..3edbae7726 100644 --- a/src/borg/repository.py +++ b/src/borg/repository.py @@ -388,7 +388,7 @@ def _find_header(self, offset, pack_size, validate): A pack has no framing besides the object headers, so this searches for OBJ_MAGIC. That byte sequence also occurs inside payloads, so a candidate is accepted only when its header parses - and validate confirms it. + and validate(chunk_id, obj) confirms the header and metadata slot at that position. """ hdr_size = RepoObj.obj_header.size while offset + hdr_size <= pack_size: @@ -404,10 +404,10 @@ def _find_header(self, offset, pack_size, validate): hdr = self._parse_header(buf[pos : pos + hdr_size], offset + pos, pack_size) if hdr is not None: obj_size = hdr_size + hdr.meta_size + hdr.data_size - # an object is at most MAX_DATA_SIZE bytes (Repository.put), so a larger candidate is a - # false match on OBJ_MAGIC in a payload. + # an object is at most MAX_DATA_SIZE bytes, so a bigger one is a false match on + # OBJ_MAGIC inside a payload. if obj_size <= MAX_DATA_SIZE: - size = obj_size if validate.needs_data else hdr_size + hdr.meta_size + size = hdr_size + hdr.meta_size # the bytes validate looks at end = pos + size # the window holds these bytes, unless the candidate crosses its end. obj = buf[pos:end] if end <= len(buf) else self.read(offset + pos, size) @@ -428,10 +428,10 @@ def iter_headers(self, validate=None): the pack, otherwise the pack is corrupt and IntegrityError is raised. A read shorter than a header ends the walk: that is the end of the pack. - validate(chunk_id, obj): returns whether obj is a repo object with id chunk_id, where obj is - its header and metadata, plus its data when validate.needs_data is set. When validate is - given, a corrupt header makes the walk resync: it scans for the next object validate accepts - (see _find_header), continues there, and logs the skipped bytes. + validate(chunk_id, obj) tells whether obj - an object's header and metadata slot - is the + repo object with id chunk_id. Given one, a corrupt header makes the walk resync instead: + it scans for the next object validate accepts, logs how many bytes that skipped and + continues there. """ pack_hex = bin_to_hex(self.pack_id) if self.pack_id is not None else "" pack_size = self.size() diff --git a/src/borg/testsuite/archiver/check_cmd_test.py b/src/borg/testsuite/archiver/check_cmd_test.py index bf38f76082..6bba0032a1 100644 --- a/src/borg/testsuite/archiver/check_cmd_test.py +++ b/src/borg/testsuite/archiver/check_cmd_test.py @@ -719,11 +719,10 @@ def test_extra_chunks(archivers, request): def test_repair_resyncs_pack_with_corrupt_object_header(archivers, request): - """--repair rebuilds the index from a pack whose object header is damaged. + """--repair rebuilds the chunks index from a pack whose object header is damaged. - A damaged header makes the walk lose the object boundaries, so the rebuild scans for the next - object that authenticates and carries on there. That needs the key, which --repair makes before - the rebuild. Repairing the pack itself is a separate step, see #10026. + A damaged header loses the object boundaries, so the rebuild scans for the next object that + authenticates and continues there. Authenticating needs the key, which --repair reads first. """ archiver = request.getfixturevalue(archivers) if archiver.get_kind() != "local": diff --git a/src/borg/testsuite/cache_test.py b/src/borg/testsuite/cache_test.py index 192d3006dd..cf945247ba 100644 --- a/src/borg/testsuite/cache_test.py +++ b/src/borg/testsuite/cache_test.py @@ -507,7 +507,7 @@ def test_close_consolidates_fragments_across_sessions(tmp_path, monkeypatch): def test_build_chunkindex_repair_resyncs_after_corrupt_header(tmp_path): - """A corrupt object header fails the rebuild, but with repair=True the rest of the pack is indexed.""" + """A corrupt object header fails the rebuild; with validate, the objects after it are indexed.""" from .repository_test import accept_all, fchunk obj1 = bytearray(fchunk(b"first", chunk_id=H(90))) @@ -518,10 +518,10 @@ def test_build_chunkindex_repair_resyncs_after_corrupt_header(tmp_path): repository.store_store("packs/" + bin_to_hex(pack_id), bytes(obj1) + obj2) with pytest.raises(IntegrityError): build_chunkindex_from_repo(repository, slow_rebuild=True) - # accept_all: accepts every candidate, so this exercises the plumbing only. + # accept_all takes any candidate, so this covers the plumbing, not the authentication. index = build_chunkindex_from_repo(repository, slow_rebuild=True, validate=accept_all) assert H(91) in index # found by resyncing past the damaged header - assert H(90) not in index # its header is gone, so the object can not be indexed + assert H(90) not in index # its header is damaged, so its id is unknown assert index[H(91)].pack_id == pack_id assert index[H(91)].obj_offset == len(obj1) diff --git a/src/borg/testsuite/repository_test.py b/src/borg/testsuite/repository_test.py index 11aab716c9..6bd0bf022f 100644 --- a/src/borg/testsuite/repository_test.py +++ b/src/borg/testsuite/repository_test.py @@ -17,7 +17,7 @@ from ..archive import resync_validator from ..compress import CNONE from ..constants import ROBJ_FILE_STREAM -from ..crypto.key import CHPOKey, PlaintextKey +from ..crypto.key import CHPOKey, ChecksumKey from ..repository import Repository, MAX_DATA_SIZE, propagate_rsh, rest_serve_command, PackWriter, PackReader from ..repository import PackTracker from ..repoobj import RepoObj, OBJ_MAGIC, OBJ_VERSION @@ -1885,9 +1885,6 @@ def accept_all(chunk_id, obj): return True -accept_all.needs_data = False - - def test_pack_reader_resync_skips_to_next_object(): # after a corrupt header the walk continues at the next object. obj1 = bytearray(fchunk(b"payload-one", meta=b"meta1", chunk_id=H(1))) @@ -1982,16 +1979,17 @@ def test_pack_reader_resync_accepts_an_object_with_corrupt_data(tmp_path): repo_objs.parse(real_id, bytes(obj2), ro_type=ROBJ_FILE_STREAM) -def test_pack_reader_resync_rejects_user_content_that_looks_like_an_object(tmp_path): - # In "none" mode with no compression, user content lands in the pack as it is, so a backed up - # file can contain something shaped like an object. Those keys authenticate by the id check over - # the content, so the scan reads whole candidates. +def test_pack_reader_resync_rejects_damaged_user_content_without_a_key(tmp_path): + # In "none-*" mode with no compression, user content lands in the pack as it is, so a backed up + # file can contain something shaped like an object. The metadata slot's checksum covers the + # object header, so damaged candidate bytes are still ruled out - what these modes can not rule + # out is an intact object put into a file on purpose, there being no secret to tell them apart. repository = Repository(str(tmp_path / "repo"), create=True) - repo_objs = RepoObj(PlaintextKey(repository)) - assert resync_validator(repo_objs).needs_data + repo_objs = RepoObj(ChecksumKey(repository)) repo_objs.compressor = CNONE() decoy = bytearray(repo_objs.format(repo_objs.id_hash(b"decoy"), {}, b"decoy", ro_type=ROBJ_FILE_STREAM)) - decoy[-1] ^= 0xFF # its content no longer hashes to the id in its header + hdr = RepoObj.ObjHeader(*RepoObj.obj_header.unpack(bytes(decoy[: RepoObj.obj_header.size]))) + decoy[RepoObj.obj_header.size + hdr.meta_size - 1] ^= 0xFF # damage its metadata slot content = bytes(decoy) # a user stores exactly those bytes in a file obj1 = bytearray(repo_objs.format(repo_objs.id_hash(content), {}, content, ro_type=ROBJ_FILE_STREAM)) assert content in obj1 # the decoy is in the pack verbatim From 1844b458ea4a1d6bbbe37c292bd38f67f290bb00 Mon Sep 17 00:00:00 2001 From: Mrityunjay Raj Date: Sun, 23 Aug 2026 18:46:39 +0530 Subject: [PATCH 3/8] check --repair: validate every object header the repair walk accepts, #8476 A meta_size or data_size corrupted to a value that keeps the object inside the pack leaves the header parseable, so a walk that only checks the header follows a wrong offset and loses the intact objects after it. The walk now validates every header it accepts, not only the candidates the resync scan finds: it reads the metadata slot along with the header and checks it. The slot's tag covers the header's magic, version and chunk id and the slot itself, so a corrupted meta_size fails it. data_size, the one header field outside the tag, must equal csize - the data slot's payload size, recorded in the tagged metadata - plus the key's fixed envelope overhead. A header that fails makes the walk scan forward for the next object that validates and resume there. The object with the failed header is dropped. --- docs/internals/packs.rst | 32 ++--- src/borg/archive.py | 19 ++- src/borg/cache.py | 4 +- src/borg/repository.py | 56 +++++---- src/borg/testsuite/archiver/check_cmd_test.py | 18 ++- src/borg/testsuite/cache_test.py | 3 +- src/borg/testsuite/repository_test.py | 119 +++++++++++++++++- 7 files changed, 199 insertions(+), 52 deletions(-) diff --git a/docs/internals/packs.rst b/docs/internals/packs.rst index 9a064f6ac9..5d7b7b2aa4 100644 --- a/docs/internals/packs.rst +++ b/docs/internals/packs.rst @@ -100,23 +100,27 @@ fails these checks means a corrupt pack, and ``IntegrityError`` is raised. The per-blob magic limits the blast radius of corrupted length fields. The repair walk (``iter_headers(validate=...)``, used when ``borg check --repair`` -rebuilds the chunks index from the packs) scans forward for the next blob and -resumes there, so the blobs after the damaged part of the pack are still found. +rebuilds the chunks index from the packs) validates every header it walks, +reading the metadata slot along with it: the slot's tag covers the header AAD +described above and the slot itself, so a corrupted magic, version, chunk id or +``meta_size`` fails it, and ``data_size`` - the one header field outside the +tag - must equal ``csize`` (the data payload size recorded in the tagged +metadata) plus the key's fixed envelope overhead. A header that fails makes the +walk scan for the next blob that validates and resume there, so the blobs after +the damaged one are still found; the damaged blob itself is dropped, it can not +be read back. ``OBJ_MAGIC`` occurs inside the payloads as well, and in the ``none-*`` and ``authenticated-*`` modes the payloads are user content stored as it is, so a -backed up file can contain something shaped like a blob. A candidate is -therefore accepted only when its metadata slot verifies against the header AAD -described above; the header and that slot, a few hundred bytes, are what the -scan reads. Verifying needs the key, so a repair that cannot read the manifest -walks without scanning. - -In the ``none-*`` modes the tag is an unkeyed checksum, so the scan accepts any -well-formed blob, including one a backed up file contains. - -``data_size`` is not part of the AAD, so accepting a candidate authenticates -its chunk id, and its size only as far as the blob fits into the pack. Bit flips -in the data are caught when the blob is read, on that blob alone. +backed up file can contain something shaped like a blob. The scan therefore +accepts a candidate only when it validates like any walked header. Validating +needs the key, so a repair that cannot read the manifest walks without it. + +In the ``none-*`` modes the tag is an unkeyed checksum, so the walk accepts any +well-formed blob, including one a backed up file contains - but it only scans +into a payload after the blob owning it failed to validate. + +Bit flips in the data are caught when the blob is read, on that blob alone. Blobs follow one another contiguously with no padding:: diff --git a/src/borg/archive.py b/src/borg/archive.py index 69a323c6d0..188e9d418f 100644 --- a/src/borg/archive.py +++ b/src/borg/archive.py @@ -2132,19 +2132,25 @@ def resync_validator(repo_objs): obj is an object's header plus its metadata slot. Parsing that slot verifies its tag, which is computed over the header's magic, version and chunk id as well (AAD, additional authenticated - data: bytes the tag covers without being part of the ciphertext). + data: bytes the tag covers without being part of the ciphertext) and over the slot itself, so a + wrong meta_size fails it too. data_size, the one header field the tag does not cover, must + match csize - the data slot's payload size, recorded in the tagged metadata - plus the key's + fixed envelope overhead. In the "none-*" modes the tag is an unkeyed checksum, so validate accepts any well-formed object, including one that a backed up file contains. """ + hdr_size = RepoObj.obj_header.size + overhead = repo_objs.key.PAYLOAD_OVERHEAD # the envelope adds a fixed number of bytes to the payload def validate(chunk_id, obj): try: - repo_objs.parse_meta(chunk_id, obj, ro_type=ROBJ_DONTCARE) + meta = repo_objs.parse_meta(chunk_id, obj, ro_type=ROBJ_DONTCARE) except Exception: # arbitrary bytes fail the tag, the msgpack unpacking or the length checks. return False - return True + data_size = RepoObj.ObjHeader(*RepoObj.obj_header.unpack(obj[:hdr_size])).data_size + return data_size == meta["csize"] + overhead return validate @@ -2747,7 +2753,12 @@ def finish(self): # the packs changed, so the index no longer matches them: rebuild it from the packs # and persist it. logger.info("Rebuilding and writing the repository chunks index.") - build_chunkindex_from_repo(self.repository, slow_rebuild=True, write_immediately=True) + build_chunkindex_from_repo( + self.repository, + slow_rebuild=True, + validate=resync_validator(self.repo_objs), + write_immediately=True, + ) else: # the packs are unchanged, so the index still matches them: persist it as is. logger.info("Writing the rebuilt repository chunks index.") diff --git a/src/borg/cache.py b/src/borg/cache.py index 3f78b46335..33a687d113 100644 --- a/src/borg/cache.py +++ b/src/borg/cache.py @@ -867,8 +867,8 @@ def build_chunkindex_from_repo( ): # fragments_only: build the index from the index/ fragments only, returning None if they cannot be # read completely, and never write to the repo. - # validate: handed to PackReader.iter_headers when rebuilding from the packs, making it resync - # past a corrupt object header rather than raise IntegrityError. + # validate: handed to PackReader.iter_headers when rebuilding from the packs, making it check + # every object header it walks and resync past the ones that fail rather than raise IntegrityError. assert not (slow_rebuild and fragments_only) assert not (fragments_only and write_immediately) # fragments_only never writes to the repo # first, try to build a fresh, mostly complete chunk index from centrally stored index fragments: diff --git a/src/borg/repository.py b/src/borg/repository.py index 3edbae7726..3b6829b0eb 100644 --- a/src/borg/repository.py +++ b/src/borg/repository.py @@ -40,6 +40,9 @@ # how much of a pack PackReader reads at once when searching for the next object header. RESYNC_WINDOW_SIZE = 1024 * 1024 +# how much PackReader reads per object when it validates the headers it walks: the header and, +# for the usual metadata slot size, the slot as well, so validating needs no second read. +VALIDATE_READ_SIZE = 1024 def repo_lister(repository, *, limit=None): @@ -374,12 +377,14 @@ def size(self): def _parse_header(hdr_data, offset, pack_size): """Return the ObjHeader in hdr_data if it is a valid header at offset, None otherwise. - Valid means: OBJ_MAGIC, a supported version, and an object that fits into the pack. + Valid means: OBJ_MAGIC, a supported version, and an object that fits into the pack and is + at most MAX_DATA_SIZE bytes, the limit put() enforces on a whole object. """ hdr = RepoObj.ObjHeader(*RepoObj.obj_header.unpack(hdr_data)) if hdr.magic != OBJ_MAGIC or hdr.version not in SUPPORTED_OBJ_VERSIONS: return None - if offset + RepoObj.obj_header.size + hdr.meta_size + hdr.data_size > pack_size: + obj_size = RepoObj.obj_header.size + hdr.meta_size + hdr.data_size + if obj_size > MAX_DATA_SIZE or offset + obj_size > pack_size: return None return hdr @@ -403,16 +408,12 @@ def _find_header(self, offset, pack_size, validate): break # not in this window, or a header overlapping its end: the next window has it hdr = self._parse_header(buf[pos : pos + hdr_size], offset + pos, pack_size) if hdr is not None: - obj_size = hdr_size + hdr.meta_size + hdr.data_size - # an object is at most MAX_DATA_SIZE bytes, so a bigger one is a false match on - # OBJ_MAGIC inside a payload. - if obj_size <= MAX_DATA_SIZE: - size = hdr_size + hdr.meta_size # the bytes validate looks at - end = pos + size - # the window holds these bytes, unless the candidate crosses its end. - obj = buf[pos:end] if end <= len(buf) else self.read(offset + pos, size) - if validate(hdr.chunk_id, obj): - return offset + pos + size = hdr_size + hdr.meta_size # the bytes validate looks at + end = pos + size + # the window holds these bytes, unless the candidate crosses its end. + obj = buf[pos:end] if end <= len(buf) else self.read(offset + pos, size) + if validate(hdr.chunk_id, obj): + return offset + pos pos += 1 # step by the window less one header, so a magic straddling the boundary is still found. offset += max(len(buf) - (hdr_size - 1), 1) @@ -421,27 +422,34 @@ def _find_header(self, offset, pack_size, validate): def iter_headers(self, validate=None): """Yield (chunk_id, offset, size) for each object by walking the fixed object headers. - The walk reads a header per object: one short range read each (or a slice, for a pack in - memory), plus one store metadata lookup for the pack size. + The walk reads one range per object (or a slice, for a pack in memory), plus one store + metadata lookup for the pack size. - A header must have OBJ_MAGIC, a supported version and describe an object that fits into - the pack, otherwise the pack is corrupt and IntegrityError is raised. A read shorter than - a header ends the walk: that is the end of the pack. + A header that _parse_header does not accept means a corrupt pack and raises IntegrityError. + A read shorter than a header ends the walk: that is the end of the pack. validate(chunk_id, obj) tells whether obj - an object's header and metadata slot - is the - repo object with id chunk_id. Given one, a corrupt header makes the walk resync instead: - it scans for the next object validate accepts, logs how many bytes that skipped and - continues there. + repo object with id chunk_id. Given one, the walk validates every header, reading the + metadata slot along with it, and a header that fails makes the walk resync rather than + raise: it scans from just past that header for the next object validate accepts and + continues there. The object with the failed header is dropped - its id, its extent or its + metadata is wrong, so it can not be read back. """ pack_hex = bin_to_hex(self.pack_id) if self.pack_id is not None else "" pack_size = self.size() hdr_size = RepoObj.obj_header.size + read_size = VALIDATE_READ_SIZE if validate is not None else hdr_size offset = 0 while True: - hdr_data = self.read(offset, hdr_size) - if len(hdr_data) < hdr_size: + buf = self.read(offset, read_size) + if len(buf) < hdr_size: break # clean EOF, or trailing partial bytes - hdr = self._parse_header(hdr_data, offset, pack_size) + hdr = self._parse_header(buf[:hdr_size], offset, pack_size) + if hdr is not None and validate is not None: + size = hdr_size + hdr.meta_size # the bytes validate looks at + obj = buf[:size] if size <= len(buf) else self.read(offset, size) + if not validate(hdr.chunk_id, obj): + hdr = None if hdr is None: if validate is None: raise IntegrityError( @@ -456,7 +464,7 @@ def iter_headers(self, validate=None): break logger.warning( f"pack {pack_hex}: invalid object header at offset {offset}, " - f"skipping {next_offset - offset} bytes to the next one." + f"continuing at the object at offset {next_offset}." ) offset = next_offset continue diff --git a/src/borg/testsuite/archiver/check_cmd_test.py b/src/borg/testsuite/archiver/check_cmd_test.py index 6bba0032a1..34b179285e 100644 --- a/src/borg/testsuite/archiver/check_cmd_test.py +++ b/src/borg/testsuite/archiver/check_cmd_test.py @@ -10,6 +10,7 @@ from ...constants import * # NOQA from ...helpers import bin_to_hex, msgpack, CommandError, Error, IntegrityError, sig_int from ...manifest import Archives, Manifest +from ...repoobj import RepoObj from ...repository import PackTracker, Repository from ..repository_test import fchunk, corrupt_chunk_on_disk from . import ( @@ -718,11 +719,14 @@ def test_extra_chunks(archivers, request): cmd(archiver, "check", "-v", exit_code=0) # check does not deal with orphans anymore -def test_repair_resyncs_pack_with_corrupt_object_header(archivers, request): +@pytest.mark.parametrize("damaged_field", ["magic", "data_size"]) +def test_repair_resyncs_pack_with_corrupt_object_header(archivers, request, damaged_field): """--repair rebuilds the chunks index from a pack whose object header is damaged. A damaged header loses the object boundaries, so the rebuild scans for the next object that authenticates and continues there. Authenticating needs the key, which --repair reads first. + A damaged data_size leaves the header parseable, so the rebuild catches it against the csize + in the authenticated metadata. """ archiver = request.getfixturevalue(archivers) if archiver.get_kind() != "local": @@ -736,13 +740,18 @@ def test_repair_resyncs_pack_with_corrupt_object_header(archivers, request): for chunk_id, entry in repository.chunks.items(): by_pack.setdefault(entry.pack_id, []).append((entry.obj_offset, chunk_id)) pack_id, objs = next((p, sorted(o)) for p, o in by_pack.items() if len(o) > 2) - damaged_offset, _ = objs[1] + damaged_offset, damaged_id = objs[1] + next_offset, next_id = objs[2] + field_offset = {"magic": 0, "data_size": 45}[damaged_field] # magic 8, version 1, chunk_id 32, meta_size 4 key = "packs/" + bin_to_hex(pack_id) - repository.store_store(key, corrupt(repository.store_load(key), damaged_offset)) + repository.store_store(key, corrupt(repository.store_load(key), damaged_offset + field_offset)) output = cmd(archiver, "check", "--repair", "--debug", exit_code=0) assert f"invalid object header at offset {damaged_offset}" in output - assert "bytes to the next one" in output # the rebuild resumed at the next object + assert f"continuing at the object at offset {next_offset}" in output # the rebuild resumed at the next object + with Repository(archiver.repository_location, exclusive=True) as repository: + assert damaged_id not in repository.chunks # the damaged object can not be read back, so it is not indexed + assert repository.chunks[next_id].obj_offset == next_offset # the one after it is def test_repair_finish_flushes_pack_writer(archivers, request): @@ -762,6 +771,7 @@ def test_repair_finish_flushes_pack_writer(archivers, request): checker.repair = True checker.repository = repository checker.key = checker.make_key(repository) + checker.repo_objs = RepoObj(checker.key) checker.manifest = Manifest.load(repository, (Manifest.Operation.CHECK,), key=checker.key) # re-adding a chunk makes the chunks index no longer match the packs, so finish() rebuilds it. checker.chunks_modified = True diff --git a/src/borg/testsuite/cache_test.py b/src/borg/testsuite/cache_test.py index cf945247ba..948f9d23c0 100644 --- a/src/borg/testsuite/cache_test.py +++ b/src/borg/testsuite/cache_test.py @@ -26,8 +26,7 @@ ) from ..hashindex import ChunkIndex, ChunkIndexEntry from ..crypto.key import AESOCBKey -from ..helpers import bin_to_hex, safe_ns -from ..helpers import IntegrityError +from ..helpers import IntegrityError, bin_to_hex, safe_ns from ..helpers.msgpack import int_to_timestamp from ..manifest import Manifest from ..repository import Repository diff --git a/src/borg/testsuite/repository_test.py b/src/borg/testsuite/repository_test.py index 6bd0bf022f..df835fa684 100644 --- a/src/borg/testsuite/repository_test.py +++ b/src/borg/testsuite/repository_test.py @@ -1,6 +1,7 @@ import io import logging import os +import struct import sys import time from collections import namedtuple @@ -17,7 +18,7 @@ from ..archive import resync_validator from ..compress import CNONE from ..constants import ROBJ_FILE_STREAM -from ..crypto.key import CHPOKey, ChecksumKey +from ..crypto.key import AESOCBKey, AuthenticatedKey, CHPOKey, ChecksumKey from ..repository import Repository, MAX_DATA_SIZE, propagate_rsh, rest_serve_command, PackWriter, PackReader from ..repository import PackTracker from ..repoobj import RepoObj, OBJ_MAGIC, OBJ_VERSION @@ -1894,7 +1895,7 @@ def test_pack_reader_resync_skips_to_next_object(): assert list(reader.iter_headers(validate=accept_all)) == [(H(2), len(obj1), len(obj2))] -def test_pack_reader_resync_recovers_from_corrupted_size(): +def test_pack_reader_resync_recovers_from_size_past_the_pack_end(): # a header whose sizes point past the pack, so the next object is found by scanning. obj1 = bytearray(fchunk(b"payload-one", meta=b"meta1", chunk_id=H(1))) obj2 = fchunk(b"payload-two", meta=b"meta2", chunk_id=H(2)) @@ -1910,6 +1911,120 @@ def test_pack_reader_resync_recovers_from_corrupted_size(): ] +def none_repo_objs(): + # a RepoObj with a "none-*" key and no compression: it formats real objects (tagged metadata + # slot, csize) and stores their payload as it is. + repo_objs = RepoObj(ChecksumKey(None)) + repo_objs.compressor = CNONE() + return repo_objs + + +def real_chunk(repo_objs, data): + # (chunk_id, obj) of a real repo object storing data. + chunk_id = repo_objs.id_hash(data) + return chunk_id, repo_objs.format(chunk_id, {}, data, ro_type=ROBJ_FILE_STREAM) + + +@pytest.mark.parametrize("shape", ["into_obj3", "onto_obj3_header", "into_itself"]) +def test_pack_reader_resync_rejects_a_header_with_a_wrong_data_size(shape): + # data_size is the one header field no tag covers. A wrong one that keeps the object inside the + # pack leaves the header parseable, so the walk checks it against csize from the tagged metadata + # and drops obj1. The shapes are where the wrong size points: into obj3, exactly onto obj3's + # header, and back into obj1 itself. + repo_objs = none_repo_objs() + _, obj1 = real_chunk(repo_objs, b"A" * 100) + id2, obj2 = real_chunk(repo_objs, b"B" * 100) + id3, obj3 = real_chunk(repo_objs, b"C" * 100) + true_size = RepoObj.ObjHeader(*RepoObj.obj_header.unpack(obj1[: RepoObj.obj_header.size])).data_size + bad_size = { + "into_obj3": true_size + len(obj2) + 60, + "onto_obj3_header": true_size + len(obj2), + "into_itself": true_size - 60, + }[shape] + obj1 = bytearray(obj1) + obj1[45:49] = struct.pack(" Date: Sun, 23 Aug 2026 18:34:46 +0530 Subject: [PATCH 4/8] check --repair: report a failed object validation apart from a corrupt header, #8476 --- src/borg/archive.py | 12 ++++----- src/borg/cache.py | 4 +-- src/borg/repository.py | 27 ++++++++++--------- src/borg/testsuite/archiver/check_cmd_test.py | 16 ++++++++--- src/borg/testsuite/repository_test.py | 22 +++++++-------- 5 files changed, 47 insertions(+), 34 deletions(-) diff --git a/src/borg/archive.py b/src/borg/archive.py index 188e9d418f..a133063076 100644 --- a/src/borg/archive.py +++ b/src/borg/archive.py @@ -2127,7 +2127,7 @@ def __next__(self): return next(self._unpacker) -def resync_validator(repo_objs): +def object_validator(repo_objs): """Return validate(chunk_id, obj): True if obj is the repo object with id chunk_id. obj is an object's header plus its metadata slot. Parsing that slot verifies its tag, which is @@ -2146,11 +2146,11 @@ def resync_validator(repo_objs): def validate(chunk_id, obj): try: meta = repo_objs.parse_meta(chunk_id, obj, ro_type=ROBJ_DONTCARE) + data_size = RepoObj.ObjHeader(*RepoObj.obj_header.unpack(obj[:hdr_size])).data_size + return data_size == meta["csize"] + overhead except Exception: - # arbitrary bytes fail the tag, the msgpack unpacking or the length checks. + # arbitrary bytes fail the tag, the msgpack unpacking, the length checks or the csize lookup. return False - data_size = RepoObj.ObjHeader(*RepoObj.obj_header.unpack(obj[:hdr_size])).data_size - return data_size == meta["csize"] + overhead return validate @@ -2217,7 +2217,7 @@ def check( self.key = self.make_key(repository, manifest_only=True) except IntegrityError as err: logger.warning(f"{err}. Packs with a corrupt object header can not be repaired.") - validate = resync_validator(RepoObj(self.key)) if repair and self.key is not None else None + validate = object_validator(RepoObj(self.key)) if repair and self.key is not None else None self.chunks = build_chunkindex_from_repo( self.repository, slow_rebuild=repair, validate=validate, write_immediately=False ) @@ -2756,7 +2756,7 @@ def finish(self): build_chunkindex_from_repo( self.repository, slow_rebuild=True, - validate=resync_validator(self.repo_objs), + validate=object_validator(self.repo_objs), write_immediately=True, ) else: diff --git a/src/borg/cache.py b/src/borg/cache.py index 33a687d113..b942bde17c 100644 --- a/src/borg/cache.py +++ b/src/borg/cache.py @@ -867,8 +867,8 @@ def build_chunkindex_from_repo( ): # fragments_only: build the index from the index/ fragments only, returning None if they cannot be # read completely, and never write to the repo. - # validate: handed to PackReader.iter_headers when rebuilding from the packs, making it check - # every object header it walks and resync past the ones that fail rather than raise IntegrityError. + # validate: a repo object validator, handed to PackReader.iter_headers so the rebuild skips the + # objects that fail it. assert not (slow_rebuild and fragments_only) assert not (fragments_only and write_immediately) # fragments_only never writes to the repo # first, try to build a fresh, mostly complete chunk index from centrally stored index fragments: diff --git a/src/borg/repository.py b/src/borg/repository.py index 3b6829b0eb..f1db992cd5 100644 --- a/src/borg/repository.py +++ b/src/borg/repository.py @@ -40,9 +40,9 @@ # how much of a pack PackReader reads at once when searching for the next object header. RESYNC_WINDOW_SIZE = 1024 * 1024 -# how much PackReader reads per object when it validates the headers it walks: the header and, -# for the usual metadata slot size, the slot as well, so validating needs no second read. -VALIDATE_READ_SIZE = 1024 +# how much to read to get an object's header plus, at the usual metadata slot sizes, its metadata +# slot in the same read. +META_READ_SIZE = 1024 def repo_lister(repository, *, limit=None): @@ -438,32 +438,35 @@ def iter_headers(self, validate=None): pack_hex = bin_to_hex(self.pack_id) if self.pack_id is not None else "" pack_size = self.size() hdr_size = RepoObj.obj_header.size - read_size = VALIDATE_READ_SIZE if validate is not None else hdr_size + read_size = META_READ_SIZE if validate is not None else hdr_size offset = 0 while True: buf = self.read(offset, read_size) if len(buf) < hdr_size: break # clean EOF, or trailing partial bytes hdr = self._parse_header(buf[:hdr_size], offset, pack_size) - if hdr is not None and validate is not None: + if hdr is None: + problem = "invalid object header" + elif validate is not None: size = hdr_size + hdr.meta_size # the bytes validate looks at obj = buf[:size] if size <= len(buf) else self.read(offset, size) - if not validate(hdr.chunk_id, obj): - hdr = None - if hdr is None: + problem = None if validate(hdr.chunk_id, obj) else "object does not authenticate" + else: + problem = None + if problem is not None: if validate is None: raise IntegrityError( - f'pack {pack_hex}: invalid object header at offset {offset} (pack corruption), run "borg check"' + f'pack {pack_hex}: {problem} at offset {offset} (pack corruption), run "borg check"' ) next_offset = self._find_header(offset + 1, pack_size, validate) if next_offset is None: logger.warning( - f"pack {pack_hex}: invalid object header at offset {offset} and none after it, " + f"pack {pack_hex}: {problem} at offset {offset} and no object after it, " f"skipping the remaining {pack_size - offset} bytes." ) break logger.warning( - f"pack {pack_hex}: invalid object header at offset {offset}, " + f"pack {pack_hex}: {problem} at offset {offset}, " f"continuing at the object at offset {next_offset}." ) offset = next_offset @@ -1391,7 +1394,7 @@ def get(self, id, read_data=True, raise_missing=True): # RepoObj layout supports separately encrypted metadata and data. # We return enough bytes so the client can decrypt the metadata. hdr_size = RepoObj.obj_header.size - extra_size = 1024 - hdr_size # load a bit more, 1024b, reduces round trips + extra_size = META_READ_SIZE - hdr_size load_size = hdr_size + extra_size # keep the read inside this object: a pack holds neighbouring objects, so don't pull # bytes past obj_size into the next one. (an overshoot would be harmless -- parse_meta diff --git a/src/borg/testsuite/archiver/check_cmd_test.py b/src/borg/testsuite/archiver/check_cmd_test.py index 34b179285e..5a9b025b60 100644 --- a/src/borg/testsuite/archiver/check_cmd_test.py +++ b/src/borg/testsuite/archiver/check_cmd_test.py @@ -2,6 +2,7 @@ from pathlib import Path import re import shutil +import struct from unittest.mock import patch import pytest @@ -742,12 +743,21 @@ def test_repair_resyncs_pack_with_corrupt_object_header(archivers, request, dama pack_id, objs = next((p, sorted(o)) for p, o in by_pack.items() if len(o) > 2) damaged_offset, damaged_id = objs[1] next_offset, next_id = objs[2] - field_offset = {"magic": 0, "data_size": 45}[damaged_field] # magic 8, version 1, chunk_id 32, meta_size 4 key = "packs/" + bin_to_hex(pack_id) - repository.store_store(key, corrupt(repository.store_load(key), damaged_offset + field_offset)) + pack = repository.store_load(key) + if damaged_field == "magic": + pack = corrupt(pack, damaged_offset) + else: + hdr_size = RepoObj.obj_header.size + hdr = RepoObj.ObjHeader(*RepoObj.obj_header.unpack(pack[damaged_offset : damaged_offset + hdr_size])) + # a data_size that keeps the object inside the pack, so the header still parses. + pos = damaged_offset + 45 # magic 8, version 1, chunk_id 32, meta_size 4 + pack = pack[:pos] + struct.pack(" Date: Sun, 23 Aug 2026 19:16:36 +0530 Subject: [PATCH 5/8] check --repair: report which header check failed and cap the metadata read, #8476 --- docs/internals/packs.rst | 12 ++-- src/borg/repository.py | 66 +++++++++++-------- src/borg/testsuite/archiver/check_cmd_test.py | 2 +- src/borg/testsuite/repository_test.py | 28 ++++++-- 4 files changed, 71 insertions(+), 37 deletions(-) diff --git a/docs/internals/packs.rst b/docs/internals/packs.rst index 5d7b7b2aa4..bee8289c5e 100644 --- a/docs/internals/packs.rst +++ b/docs/internals/packs.rst @@ -95,8 +95,9 @@ A reader locates the next blob by advancing:: next_blob_offset = current_blob_offset + REPOOBJ_HEADER_SIZE + meta_size + data_size ``iter_headers()`` checks every header it walks: it must have ``OBJ_MAGIC``, a -supported version, and sizes that keep the blob inside the pack. A header that -fails these checks means a corrupt pack, and ``IntegrityError`` is raised. +supported version, and sizes that keep the blob inside the pack and within +``MAX_DATA_SIZE``. A header that fails these checks means a corrupt pack, and +``IntegrityError`` is raised, naming which check it failed. The per-blob magic limits the blast radius of corrupted length fields. The repair walk (``iter_headers(validate=...)``, used when ``borg check --repair`` @@ -117,8 +118,11 @@ accepts a candidate only when it validates like any walked header. Validating needs the key, so a repair that cannot read the manifest walks without it. In the ``none-*`` modes the tag is an unkeyed checksum, so the walk accepts any -well-formed blob, including one a backed up file contains - but it only scans -into a payload after the blob owning it failed to validate. +well-formed blob, including one a backed up file contains. Such a blob carries +its own chunk id and reads back as itself, so indexing it is harmless. Bytes +crafted to pass the unkeyed checksum are not caught here - authenticating them +is what these modes give up. The scan reaches a payload only after the blob +owning it failed to validate. Bit flips in the data are caught when the blob is read, on that blob alone. diff --git a/src/borg/repository.py b/src/borg/repository.py index f1db992cd5..aa00236ec7 100644 --- a/src/borg/repository.py +++ b/src/borg/repository.py @@ -43,6 +43,9 @@ # how much to read to get an object's header plus, at the usual metadata slot sizes, its metadata # slot in the same read. META_READ_SIZE = 1024 +# the largest metadata slot a validating read fetches. a slot holds a few compression fields, +# packed and encrypted. +MAX_VALIDATED_META_SIZE = 64 * 1024 def repo_lister(repository, *, limit=None): @@ -375,18 +378,37 @@ def size(self): @staticmethod def _parse_header(hdr_data, offset, pack_size): - """Return the ObjHeader in hdr_data if it is a valid header at offset, None otherwise. + """Return (ObjHeader, None) for a valid header at offset, (None, problem) otherwise. Valid means: OBJ_MAGIC, a supported version, and an object that fits into the pack and is - at most MAX_DATA_SIZE bytes, the limit put() enforces on a whole object. + at most MAX_DATA_SIZE bytes, the limit put() enforces on a whole object. problem names + which of these failed. """ hdr = RepoObj.ObjHeader(*RepoObj.obj_header.unpack(hdr_data)) - if hdr.magic != OBJ_MAGIC or hdr.version not in SUPPORTED_OBJ_VERSIONS: - return None + if hdr.magic != OBJ_MAGIC: + return None, "no object header" + if hdr.version not in SUPPORTED_OBJ_VERSIONS: + return None, f"unsupported object version {hdr.version}" obj_size = RepoObj.obj_header.size + hdr.meta_size + hdr.data_size - if obj_size > MAX_DATA_SIZE or offset + obj_size > pack_size: - return None - return hdr + if offset + obj_size > pack_size: + return None, "object extends past end of file" + if obj_size > MAX_DATA_SIZE: + return None, f"object of {obj_size} bytes exceeds the maximum of {MAX_DATA_SIZE}" + return hdr, None + + def _validates(self, hdr, offset, buf, buf_offset, validate): + """Return whether validate accepts the object with header hdr at offset. + + buf holds the pack bytes from buf_offset on; the metadata slot is read separately when buf + does not reach its end. A slot over MAX_VALIDATED_META_SIZE fails without that read. + """ + if hdr.meta_size > MAX_VALIDATED_META_SIZE: + return False + size = RepoObj.obj_header.size + hdr.meta_size + start = offset - buf_offset + end = start + size + obj = buf[start:end] if end <= len(buf) else self.read(offset, size) + return validate(hdr.chunk_id, obj) def _find_header(self, offset, pack_size, validate): """Scan forward from offset for the next object validate accepts, return its offset or None. @@ -406,14 +428,9 @@ def _find_header(self, offset, pack_size, validate): pos = buf.find(OBJ_MAGIC, pos) if pos < 0 or pos + hdr_size > len(buf): break # not in this window, or a header overlapping its end: the next window has it - hdr = self._parse_header(buf[pos : pos + hdr_size], offset + pos, pack_size) - if hdr is not None: - size = hdr_size + hdr.meta_size # the bytes validate looks at - end = pos + size - # the window holds these bytes, unless the candidate crosses its end. - obj = buf[pos:end] if end <= len(buf) else self.read(offset + pos, size) - if validate(hdr.chunk_id, obj): - return offset + pos + hdr, _ = self._parse_header(buf[pos : pos + hdr_size], offset + pos, pack_size) + if hdr is not None and self._validates(hdr, offset + pos, buf, offset, validate): + return offset + pos pos += 1 # step by the window less one header, so a magic straddling the boundary is still found. offset += max(len(buf) - (hdr_size - 1), 1) @@ -425,8 +442,8 @@ def iter_headers(self, validate=None): The walk reads one range per object (or a slice, for a pack in memory), plus one store metadata lookup for the pack size. - A header that _parse_header does not accept means a corrupt pack and raises IntegrityError. - A read shorter than a header ends the walk: that is the end of the pack. + A header that _parse_header does not accept means a corrupt pack: IntegrityError names what + is wrong with it. A read shorter than a header ends the walk: that is the end of the pack. validate(chunk_id, obj) tells whether obj - an object's header and metadata slot - is the repo object with id chunk_id. Given one, the walk validates every header, reading the @@ -438,21 +455,18 @@ def iter_headers(self, validate=None): pack_hex = bin_to_hex(self.pack_id) if self.pack_id is not None else "" pack_size = self.size() hdr_size = RepoObj.obj_header.size + # TODO: objects smaller than META_READ_SIZE make the validating walk read the pack several + # times over. Buffering a window, as _find_header scans with, would suit them; skipping a + # large object stays cheaper with a short read per header. read_size = META_READ_SIZE if validate is not None else hdr_size offset = 0 while True: buf = self.read(offset, read_size) if len(buf) < hdr_size: break # clean EOF, or trailing partial bytes - hdr = self._parse_header(buf[:hdr_size], offset, pack_size) - if hdr is None: - problem = "invalid object header" - elif validate is not None: - size = hdr_size + hdr.meta_size # the bytes validate looks at - obj = buf[:size] if size <= len(buf) else self.read(offset, size) - problem = None if validate(hdr.chunk_id, obj) else "object does not authenticate" - else: - problem = None + hdr, problem = self._parse_header(buf[:hdr_size], offset, pack_size) + if hdr is not None and validate is not None and not self._validates(hdr, offset, buf, offset, validate): + problem = "object does not authenticate" if problem is not None: if validate is None: raise IntegrityError( diff --git a/src/borg/testsuite/archiver/check_cmd_test.py b/src/borg/testsuite/archiver/check_cmd_test.py index 5a9b025b60..dc829d9554 100644 --- a/src/borg/testsuite/archiver/check_cmd_test.py +++ b/src/borg/testsuite/archiver/check_cmd_test.py @@ -756,7 +756,7 @@ def test_repair_resyncs_pack_with_corrupt_object_header(archivers, request, dama repository.store_store(key, pack) output = cmd(archiver, "check", "--repair", "--debug", exit_code=0) - problem = {"magic": "invalid object header", "data_size": "object does not authenticate"}[damaged_field] + problem = {"magic": "no object header", "data_size": "object does not authenticate"}[damaged_field] assert f"{problem} at offset {damaged_offset}" in output assert f"continuing at the object at offset {next_offset}" in output # the rebuild resumed at the next object with Repository(archiver.repository_location, exclusive=True) as repository: diff --git a/src/borg/testsuite/repository_test.py b/src/borg/testsuite/repository_test.py index d10f3f7c02..bdb5b57f4b 100644 --- a/src/borg/testsuite/repository_test.py +++ b/src/borg/testsuite/repository_test.py @@ -20,7 +20,7 @@ from ..constants import ROBJ_FILE_STREAM from ..crypto.key import AESOCBKey, AuthenticatedKey, CHPOKey, ChecksumKey from ..repository import Repository, MAX_DATA_SIZE, propagate_rsh, rest_serve_command, PackWriter, PackReader -from ..repository import PackTracker +from ..repository import PackTracker, MAX_VALIDATED_META_SIZE from ..repoobj import RepoObj, OBJ_MAGIC, OBJ_VERSION from .hashindex_test import H @@ -1840,7 +1840,7 @@ def test_pack_reader_raises_on_bad_magic(): obj2 = bytearray(fchunk(b"d2", meta=b"m2", chunk_id=H(2))) obj2[0] ^= 0xFF # break the magic of the second object's header reader = PackReader(pack_contents=obj1 + bytes(obj2)) - with pytest.raises(IntegrityError): + with pytest.raises(IntegrityError, match="no object header at offset"): list(reader.iter_headers()) @@ -1851,7 +1851,7 @@ def test_pack_reader_raises_on_bad_magic_through_store(tmp_path): with Repository(str(tmp_path / "repo"), exclusive=True, create=True) as repository: repository.store_store("packs/" + bin_to_hex(pack_id), bytes(obj)) reader = PackReader(repository.store, pack_id) - with pytest.raises(IntegrityError): + with pytest.raises(IntegrityError, match="no object header at offset"): list(reader.iter_headers()) @@ -1860,7 +1860,7 @@ def test_pack_reader_raises_on_object_past_end_of_pack(): obj = fchunk(b"data", meta=b"meta", chunk_id=H(5)) pack = obj[:-1] # drop a byte, so the header's data_size no longer fits reader = PackReader(pack_contents=pack) - with pytest.raises(IntegrityError): + with pytest.raises(IntegrityError, match="object extends past end of file at offset"): list(reader.iter_headers()) @@ -1870,17 +1870,33 @@ def test_pack_reader_raises_on_object_past_end_of_pack_through_store(tmp_path): with Repository(str(tmp_path / "repo"), exclusive=True, create=True) as repository: repository.store_store("packs/" + bin_to_hex(pack_id), obj[:-1]) reader = PackReader(repository.store, pack_id) - with pytest.raises(IntegrityError): + with pytest.raises(IntegrityError, match="object extends past end of file at offset"): list(reader.iter_headers()) def test_pack_reader_raises_on_unsupported_version(): obj = bytearray(fchunk(b"data", chunk_id=H(7))) obj[len(OBJ_MAGIC)] = 0xEE # version byte - with pytest.raises(IntegrityError): + with pytest.raises(IntegrityError, match="unsupported object version 238 at offset"): list(PackReader(pack_contents=bytes(obj)).iter_headers()) +def test_pack_reader_rejects_an_object_over_max_data_size(): + # a header claiming more than put() would ever write, in a pack large enough to hold it. + hdr = RepoObj.obj_header.pack(OBJ_MAGIC, OBJ_VERSION, H(8), 0, MAX_DATA_SIZE) + parsed, problem = PackReader._parse_header(hdr, 0, 2 * MAX_DATA_SIZE) + assert parsed is None + assert "exceeds the maximum" in problem + + +def test_pack_reader_does_not_fetch_an_oversized_metadata_slot(): + # an oversized meta_size fails validation on the header alone, with no read of the slot. + hdr = RepoObj.ObjHeader(OBJ_MAGIC, OBJ_VERSION, H(9), MAX_VALIDATED_META_SIZE + 1, 0) + reader = PackReader(pack_contents=b"") + reader.read = lambda offset, size: pytest.fail("the slot was fetched") + assert not reader._validates(hdr, 0, b"", 0, lambda chunk_id, obj: pytest.fail("validate was called")) + + def accept_all(chunk_id, obj): # validate stand-in: accepts every candidate. return True From 97326e86c7595d6506099d1f14f4c7937590dd87 Mon Sep 17 00:00:00 2001 From: Mrityunjay Raj Date: Sat, 29 Aug 2026 21:40:36 +0530 Subject: [PATCH 6/8] check --repair: clamp the header walk to the pack, document blobs from a same-key repo, #8476 --- docs/internals/packs.rst | 22 +++++++++++++++++----- src/borg/archive.py | 4 +++- src/borg/repository.py | 9 +++++---- src/borg/testsuite/repository_test.py | 20 ++++++++++++++++++++ 4 files changed, 45 insertions(+), 10 deletions(-) diff --git a/docs/internals/packs.rst b/docs/internals/packs.rst index bee8289c5e..cb83c419ec 100644 --- a/docs/internals/packs.rst +++ b/docs/internals/packs.rst @@ -118,11 +118,23 @@ accepts a candidate only when it validates like any walked header. Validating needs the key, so a repair that cannot read the manifest walks without it. In the ``none-*`` modes the tag is an unkeyed checksum, so the walk accepts any -well-formed blob, including one a backed up file contains. Such a blob carries -its own chunk id and reads back as itself, so indexing it is harmless. Bytes -crafted to pass the unkeyed checksum are not caught here - authenticating them -is what these modes give up. The scan reaches a payload only after the blob -owning it failed to validate. +well-formed blob, including one a backed up file contains. The +``authenticated-*`` modes accept a blob written with the key the repository +uses: their tag is deterministic and binds a blob to its chunk id alone, so a +blob copied verbatim out of a repository sharing that key validates at any +offset in any pack. Backing up such a repository puts its blobs into the +payloads, as long as compression leaves them as they are - lz4 stores data it +cannot shrink unchanged. + +A blob that arrives this way carries its own chunk id and reads back as itself, +so indexing it adds a chunk nothing references. Its ``data_size`` describes the +blob as it was written, though, and chunking cuts a payload where the content +dictates: a blob whose header and metadata slot fall inside the payload and +whose data is cut off still validates, and the extent it claims covers the blobs +that follow it, which the walk then skips. Bytes crafted to pass an unkeyed +checksum claim an extent the same way - authenticating them is what the +``none-*`` modes give up. The scan reaches a payload only after the blob owning +it failed to validate, so a corrupt header is what makes any of this reachable. Bit flips in the data are caught when the blob is read, on that blob alone. diff --git a/src/borg/archive.py b/src/borg/archive.py index a133063076..5e8e47bb83 100644 --- a/src/borg/archive.py +++ b/src/borg/archive.py @@ -2138,7 +2138,9 @@ def object_validator(repo_objs): fixed envelope overhead. In the "none-*" modes the tag is an unkeyed checksum, so validate accepts any well-formed - object, including one that a backed up file contains. + object, including one that a backed up file contains. The tag binds an object to its chunk id + alone, so the "authenticated-*" modes likewise accept an object copied verbatim from a + repository sharing their key, at any offset in any pack. """ hdr_size = RepoObj.obj_header.size overhead = repo_objs.key.PAYLOAD_OVERHEAD # the envelope adds a fixed number of bytes to the payload diff --git a/src/borg/repository.py b/src/borg/repository.py index aa00236ec7..170b28d018 100644 --- a/src/borg/repository.py +++ b/src/borg/repository.py @@ -443,7 +443,8 @@ def iter_headers(self, validate=None): metadata lookup for the pack size. A header that _parse_header does not accept means a corrupt pack: IntegrityError names what - is wrong with it. A read shorter than a header ends the walk: that is the end of the pack. + is wrong with it. Fewer than a header's bytes left ends the walk: that is the end of the + pack. validate(chunk_id, obj) tells whether obj - an object's header and metadata slot - is the repo object with id chunk_id. Given one, the walk validates every header, reading the @@ -460,10 +461,10 @@ def iter_headers(self, validate=None): # large object stays cheaper with a short read per header. read_size = META_READ_SIZE if validate is not None else hdr_size offset = 0 - while True: - buf = self.read(offset, read_size) + while offset + hdr_size <= pack_size: + buf = self.read(offset, min(read_size, pack_size - offset)) if len(buf) < hdr_size: - break # clean EOF, or trailing partial bytes + break # trailing partial bytes hdr, problem = self._parse_header(buf[:hdr_size], offset, pack_size) if hdr is not None and validate is not None and not self._validates(hdr, offset, buf, offset, validate): problem = "object does not authenticate" diff --git a/src/borg/testsuite/repository_test.py b/src/borg/testsuite/repository_test.py index bdb5b57f4b..6e58050cdb 100644 --- a/src/borg/testsuite/repository_test.py +++ b/src/borg/testsuite/repository_test.py @@ -1941,6 +1941,26 @@ def real_chunk(repo_objs, data): return chunk_id, repo_objs.format(chunk_id, {}, data, ro_type=ROBJ_FILE_STREAM) +def test_pack_reader_reads_no_further_than_the_pack_end(): + # the walk asks for META_READ_SIZE bytes per header, so the last objects of a pack have fewer + # bytes left than that. Every read stays inside the pack and asks for at least a header. + repo_objs = none_repo_objs() + id1, obj1 = real_chunk(repo_objs, b"payload-one") + id2, obj2 = real_chunk(repo_objs, b"payload-two") + pack = bytes(obj1) + bytes(obj2) + reader = PackReader(pack_contents=pack) + reads = [] + read = reader.read + reader.read = lambda offset, size: (reads.append((offset, size)), read(offset, size))[1] + assert list(reader.iter_headers(validate=object_validator(repo_objs))) == [ + (id1, 0, len(obj1)), + (id2, len(obj1), len(obj2)), + ] + assert reads, "the walk did not read anything" + assert all(offset + size <= len(pack) for offset, size in reads), reads + assert all(size > 0 for offset, size in reads), reads + + @pytest.mark.parametrize("shape", ["into_obj3", "onto_obj3_header", "into_itself"]) def test_pack_reader_resync_rejects_a_header_with_a_wrong_data_size(shape): # data_size is the one header field no tag covers. A wrong one that keeps the object inside the From e407a3a5ca6d55b69c2f581d0ba7cd1bf4382e1b Mon Sep 17 00:00:00 2001 From: Mrityunjay Raj Date: Sat, 29 Aug 2026 23:32:55 +0530 Subject: [PATCH 7/8] check --repair: report why a header failed and what a repair leaves behind, #8476 --- docs/internals/packs.rst | 32 ++++++---- src/borg/archive.py | 21 ++++--- src/borg/repository.py | 36 ++++++----- src/borg/testsuite/archiver/check_cmd_test.py | 12 +++- src/borg/testsuite/repository_test.py | 59 +++++++++++++++---- 5 files changed, 115 insertions(+), 45 deletions(-) diff --git a/docs/internals/packs.rst b/docs/internals/packs.rst index cb83c419ec..455b42dbc1 100644 --- a/docs/internals/packs.rst +++ b/docs/internals/packs.rst @@ -78,7 +78,8 @@ Format version ``0x01`` (``OBJ_VERSION_NO_HEADER_AAD``) authenticates ``encrypte writes version ``0x02``; ``parse()``/``parse_meta()`` accept both versions. ``iter_headers()`` (used for pack recovery/compaction, see below) reads the header without -decrypting, so it does not check header AAD authentication. +decrypting, so it does not check header AAD authentication. The repair walk described below is the +exception: given a validator it reads and decrypts each metadata slot, and thus does check it. .. figure:: pack-objheader.png :width: 100% @@ -87,8 +88,8 @@ decrypting, so it does not check header AAD authentication. The fixed 49-byte blob header. ``meta_size`` and ``data_size`` drive traversal; integrity comes from the content-addressed pack name and the - per-blob tag, which authenticates magic/version/chunk_id as additional - authenticated data. + per-blob tag, which at version ``0x02`` authenticates magic/version/chunk_id + as additional authenticated data. A reader locates the next blob by advancing:: @@ -102,14 +103,23 @@ supported version, and sizes that keep the blob inside the pack and within The per-blob magic limits the blast radius of corrupted length fields. The repair walk (``iter_headers(validate=...)``, used when ``borg check --repair`` rebuilds the chunks index from the packs) validates every header it walks, -reading the metadata slot along with it: the slot's tag covers the header AAD -described above and the slot itself, so a corrupted magic, version, chunk id or -``meta_size`` fails it, and ``data_size`` - the one header field outside the -tag - must equal ``csize`` (the data payload size recorded in the tagged -metadata) plus the key's fixed envelope overhead. A header that fails makes the -walk scan for the next blob that validates and resume there, so the blobs after -the damaged one are still found; the damaged blob itself is dropped, it can not -be read back. +reading the metadata slot along with it: the slot's tag covers the slot itself +and the chunk id, and at version ``0x02`` the header AAD described above, so a +corrupted chunk id or ``meta_size`` fails it; a corrupted magic fails the magic +check, and a corrupted version fails because the version decides which AAD the +slot is parsed with. ``data_size`` - the one header field outside the tag at +either version - must equal ``csize`` (the data payload size recorded in the +tagged metadata) plus the key's fixed envelope overhead. A header that fails +makes the walk scan for the next blob that validates and resume there, so the +blobs after the damaged one are still found; the damaged blob itself is dropped, +it can not be read back. + +The walk rebuilds the index. The damaged bytes stay where they are, as a gap no +index entry covers, and a pack is named by the +sha256 of its content, so a pack damaged in the store keeps failing the +store-level check that ``borg check`` runs over ``packs/``: that check keeps +reporting the pack after ``borg check --repair`` has rebuilt the index from it. +Rewriting such a pack is repository-level repair, see :issue:`10026`. ``OBJ_MAGIC`` occurs inside the payloads as well, and in the ``none-*`` and ``authenticated-*`` modes the payloads are user content stored as it is, so a diff --git a/src/borg/archive.py b/src/borg/archive.py index 5e8e47bb83..5ea793262f 100644 --- a/src/borg/archive.py +++ b/src/borg/archive.py @@ -2131,11 +2131,13 @@ def object_validator(repo_objs): """Return validate(chunk_id, obj): True if obj is the repo object with id chunk_id. obj is an object's header plus its metadata slot. Parsing that slot verifies its tag, which is - computed over the header's magic, version and chunk id as well (AAD, additional authenticated - data: bytes the tag covers without being part of the ciphertext) and over the slot itself, so a - wrong meta_size fails it too. data_size, the one header field the tag does not cover, must - match csize - the data slot's payload size, recorded in the tagged metadata - plus the key's - fixed envelope overhead. + computed over the slot itself and over the chunk id, so a wrong meta_size or chunk id fails it. + At object version OBJ_VERSION_HEADER_AAD the magic and the version are covered as well (as AAD, + additional authenticated data: bytes the tag covers without being part of the ciphertext); at + OBJ_VERSION_NO_HEADER_AAD they are not: a wrong magic fails the explicit magic check, and a + wrong version fails because the version selects the AAD the slot is parsed with. data_size, the + one header field outside the tag at either version, must match csize - the data slot's payload + size, recorded in the tagged metadata - plus the key's fixed envelope overhead. In the "none-*" modes the tag is an unkeyed checksum, so validate accepts any well-formed object, including one that a backed up file contains. The tag binds an object to its chunk id @@ -2150,9 +2152,13 @@ def validate(chunk_id, obj): meta = repo_objs.parse_meta(chunk_id, obj, ro_type=ROBJ_DONTCARE) data_size = RepoObj.ObjHeader(*RepoObj.obj_header.unpack(obj[:hdr_size])).data_size return data_size == meta["csize"] + overhead - except Exception: + except (IntegrityErrorBase, msgpack.UnpackException, KeyError, TypeError, IndexError): # arbitrary bytes fail the tag, the msgpack unpacking, the length checks or the csize lookup. return False + except Exception as err: + # anything else is a bug: log it and take the object as invalid. + logger.debug(f"validating an object raised {err!r}, treating it as invalid.") + return False return validate @@ -2753,7 +2759,8 @@ def finish(self): self.repository.flush() if self.chunks_modified: # the packs changed, so the index no longer matches them: rebuild it from the packs - # and persist it. + # and persist it: deleting a defect chunk rewrites its pack and repoints that + # pack's other objects in the repository's index, so our offsets for them are stale. logger.info("Rebuilding and writing the repository chunks index.") build_chunkindex_from_repo( self.repository, diff --git a/src/borg/repository.py b/src/borg/repository.py index 170b28d018..cf6d61fb15 100644 --- a/src/borg/repository.py +++ b/src/borg/repository.py @@ -396,22 +396,26 @@ def _parse_header(hdr_data, offset, pack_size): return None, f"object of {obj_size} bytes exceeds the maximum of {MAX_DATA_SIZE}" return hdr, None - def _validates(self, hdr, offset, buf, buf_offset, validate): - """Return whether validate accepts the object with header hdr at offset. + def _validation_problem(self, hdr, offset, buf, buf_offset, validate): + """Return None if validate accepts the object with header hdr at offset, else the problem. buf holds the pack bytes from buf_offset on; the metadata slot is read separately when buf - does not reach its end. A slot over MAX_VALIDATED_META_SIZE fails without that read. + does not reach its end. A slot over MAX_VALIDATED_META_SIZE is rejected without that read. """ if hdr.meta_size > MAX_VALIDATED_META_SIZE: - return False + return f"metadata slot of {hdr.meta_size} bytes exceeds the maximum of {MAX_VALIDATED_META_SIZE}" size = RepoObj.obj_header.size + hdr.meta_size start = offset - buf_offset end = start + size obj = buf[start:end] if end <= len(buf) else self.read(offset, size) - return validate(hdr.chunk_id, obj) + if not validate(hdr.chunk_id, obj): + return "object does not authenticate" + return None def _find_header(self, offset, pack_size, validate): - """Scan forward from offset for the next object validate accepts, return its offset or None. + """Scan forward from offset for the next object validate accepts, return (offset, header). + + Returns None if the pack holds no such object from offset on. A pack has no framing besides the object headers, so this searches for OBJ_MAGIC. That byte sequence also occurs inside payloads, so a candidate is accepted only when its header parses @@ -429,8 +433,8 @@ def _find_header(self, offset, pack_size, validate): if pos < 0 or pos + hdr_size > len(buf): break # not in this window, or a header overlapping its end: the next window has it hdr, _ = self._parse_header(buf[pos : pos + hdr_size], offset + pos, pack_size) - if hdr is not None and self._validates(hdr, offset + pos, buf, offset, validate): - return offset + pos + if hdr is not None and self._validation_problem(hdr, offset + pos, buf, offset, validate) is None: + return offset + pos, hdr pos += 1 # step by the window less one header, so a magic straddling the boundary is still found. offset += max(len(buf) - (hdr_size - 1), 1) @@ -451,7 +455,7 @@ def iter_headers(self, validate=None): metadata slot along with it, and a header that fails makes the walk resync rather than raise: it scans from just past that header for the next object validate accepts and continues there. The object with the failed header is dropped - its id, its extent or its - metadata is wrong, so it can not be read back. + metadata is wrong, so it can not be read back. The pack keeps the dropped object's bytes. """ pack_hex = bin_to_hex(self.pack_id) if self.pack_id is not None else "" pack_size = self.size() @@ -466,26 +470,26 @@ def iter_headers(self, validate=None): if len(buf) < hdr_size: break # trailing partial bytes hdr, problem = self._parse_header(buf[:hdr_size], offset, pack_size) - if hdr is not None and validate is not None and not self._validates(hdr, offset, buf, offset, validate): - problem = "object does not authenticate" + if hdr is not None and validate is not None: + problem = self._validation_problem(hdr, offset, buf, offset, validate) if problem is not None: if validate is None: raise IntegrityError( f'pack {pack_hex}: {problem} at offset {offset} (pack corruption), run "borg check"' ) - next_offset = self._find_header(offset + 1, pack_size, validate) - if next_offset is None: + found = self._find_header(offset + 1, pack_size, validate) + if found is None: logger.warning( f"pack {pack_hex}: {problem} at offset {offset} and no object after it, " f"skipping the remaining {pack_size - offset} bytes." ) break + next_offset, hdr = found # _find_header validated it, so yield it without re-reading logger.warning( f"pack {pack_hex}: {problem} at offset {offset}, " f"continuing at the object at offset {next_offset}." ) offset = next_offset - continue obj_size = hdr_size + hdr.meta_size + hdr.data_size yield hdr.chunk_id, offset, obj_size offset += obj_size @@ -1287,6 +1291,10 @@ def recorded_ts(info): # the exclusive check lock keeps the pack set fixed, so re-listing packs/ inside # build_chunkindex_from_repo matches this verification. write_immediately persists the # index and drops the corrupt fragments. + # the walk gets no validator: validating needs the key, which a Repository does not + # have. A pack is named by the sha256 of its content, so a pack damaged in the store + # fails verify() above and pack_errors > 0 keeps it out of here. A pack that matches + # its name and still has a bad object header makes iter_headers raise, see #10026. build_chunkindex_from_repo(self, slow_rebuild=True, write_immediately=True) self.invalidate_chunk_index() # the rebuilt index is persisted; drop the in-memory copy index_repaired = True diff --git a/src/borg/testsuite/archiver/check_cmd_test.py b/src/borg/testsuite/archiver/check_cmd_test.py index dc829d9554..70af113497 100644 --- a/src/borg/testsuite/archiver/check_cmd_test.py +++ b/src/borg/testsuite/archiver/check_cmd_test.py @@ -13,7 +13,7 @@ from ...manifest import Archives, Manifest from ...repoobj import RepoObj from ...repository import PackTracker, Repository -from ..repository_test import fchunk, corrupt_chunk_on_disk +from ..repository_test import DATA_SIZE_OFFSET, fchunk, corrupt_chunk_on_disk from . import ( cmd, src_file, @@ -751,7 +751,7 @@ def test_repair_resyncs_pack_with_corrupt_object_header(archivers, request, dama hdr_size = RepoObj.obj_header.size hdr = RepoObj.ObjHeader(*RepoObj.obj_header.unpack(pack[damaged_offset : damaged_offset + hdr_size])) # a data_size that keeps the object inside the pack, so the header still parses. - pos = damaged_offset + 45 # magic 8, version 1, chunk_id 32, meta_size 4 + pos = damaged_offset + DATA_SIZE_OFFSET pack = pack[:pos] + struct.pack(" Date: Sun, 30 Aug 2026 01:16:52 +0530 Subject: [PATCH 8/8] check --repair: move object_validator to repoobj, test the keyless rebuild, #8476 --- docs/internals/packs.rst | 48 ++++++------- src/borg/archive.py | 44 ++---------- src/borg/repoobj.py | 37 +++++++++- src/borg/repository.py | 5 +- src/borg/testsuite/archiver/check_cmd_test.py | 37 +++++++++- src/borg/testsuite/repoobj_test.py | 48 ++++++++++++- src/borg/testsuite/repository_test.py | 67 ++++--------------- 7 files changed, 161 insertions(+), 125 deletions(-) diff --git a/docs/internals/packs.rst b/docs/internals/packs.rst index 455b42dbc1..c7507e6154 100644 --- a/docs/internals/packs.rst +++ b/docs/internals/packs.rst @@ -62,10 +62,12 @@ Format version ``0x02`` (``OBJ_VERSION_HEADER_AAD``) binds the header's first 41 authenticated together with the ciphertext, but not itself encrypted). This applies to all borg 2 modes: the AEAD encryption modes (AES-256-OCB, ChaCha20-Poly1305) authenticate it with their AEAD tag, the ``authenticated-*`` modes with their MAC and the ``none-*`` modes with their (unkeyed) -checksum, see :ref:`tagged_envelope`. ``meta_size`` and ``data_size`` are excluded from -the AAD; tampering with either still fails the check, because it changes the length of the -slice being read. A forged ``chunk_id``, version, or magic byte therefore fails -authentication in ``RepoObj.parse()``/``parse_meta()``. +checksum, see :ref:`tagged_envelope`. ``meta_size`` and ``data_size`` are excluded from the AAD. +``RepoObj.parse()`` reads both slots, so tampering with either size still fails the check, by +changing the length of the slice being read. ``parse_meta()`` reads the metadata slot alone: it +catches a changed ``meta_size`` the same way, but not a changed ``data_size``, which the repair +walk described below pins separately. A forged ``chunk_id``, version, or magic byte fails +authentication in both. ``encrypted_meta`` and ``encrypted_data`` each add a one-byte slot tag on top of the shared header AAD -- ``b"M"`` for ``encrypted_meta``, ``b"D"`` for ``encrypted_data`` -- binding each ciphertext to @@ -121,30 +123,20 @@ store-level check that ``borg check`` runs over ``packs/``: that check keeps reporting the pack after ``borg check --repair`` has rebuilt the index from it. Rewriting such a pack is repository-level repair, see :issue:`10026`. -``OBJ_MAGIC`` occurs inside the payloads as well, and in the ``none-*`` and -``authenticated-*`` modes the payloads are user content stored as it is, so a -backed up file can contain something shaped like a blob. The scan therefore -accepts a candidate only when it validates like any walked header. Validating -needs the key, so a repair that cannot read the manifest walks without it. - -In the ``none-*`` modes the tag is an unkeyed checksum, so the walk accepts any -well-formed blob, including one a backed up file contains. The -``authenticated-*`` modes accept a blob written with the key the repository -uses: their tag is deterministic and binds a blob to its chunk id alone, so a -blob copied verbatim out of a repository sharing that key validates at any -offset in any pack. Backing up such a repository puts its blobs into the -payloads, as long as compression leaves them as they are - lz4 stores data it -cannot shrink unchanged. - -A blob that arrives this way carries its own chunk id and reads back as itself, -so indexing it adds a chunk nothing references. Its ``data_size`` describes the -blob as it was written, though, and chunking cuts a payload where the content -dictates: a blob whose header and metadata slot fall inside the payload and -whose data is cut off still validates, and the extent it claims covers the blobs -that follow it, which the walk then skips. Bytes crafted to pass an unkeyed -checksum claim an extent the same way - authenticating them is what the -``none-*`` modes give up. The scan reaches a payload only after the blob owning -it failed to validate, so a corrupt header is what makes any of this reachable. +``OBJ_MAGIC`` occurs inside the payloads as well, so the scan accepts a candidate +only when it validates like any walked header. Validating needs the key, so a +repair that cannot read the manifest walks without it. + +In the ``none-*`` modes the tag is an unkeyed checksum, and in the +``authenticated-*`` modes it binds a blob to its chunk id and nothing else (see +:ref:`security_structural_auth`), so validating does not establish there that +this repository wrote the blob. Both modes also store payloads as they are, so a +backed up file can contain something that validates - the blobs of a repository +sharing the key, for instance. Such a blob reads back as itself, adding a chunk +nothing references, but the extent its ``data_size`` claims covers whatever +follows it at that offset, which the walk then skips. The scan reaches a payload +only after the blob owning it failed to validate, so a corrupt header is what +makes this reachable. Bit flips in the data are caught when the blob is read, on that blob alone. diff --git a/src/borg/archive.py b/src/borg/archive.py index 5ea793262f..4148e80905 100644 --- a/src/borg/archive.py +++ b/src/borg/archive.py @@ -54,7 +54,7 @@ from . import platform from .platform import acl_get, acl_set, set_flags, get_flags, set_times, swidth from .repository import Repository, NoManifestError -from .repoobj import RepoObj +from .repoobj import RepoObj, object_validator # macOS: SF_DATALESS marks dataless placeholder files (e.g. cloud files not materialized locally). # Reading such files triggers downloading their content. stat.SF_DATALESS is only available @@ -2127,42 +2127,6 @@ def __next__(self): return next(self._unpacker) -def object_validator(repo_objs): - """Return validate(chunk_id, obj): True if obj is the repo object with id chunk_id. - - obj is an object's header plus its metadata slot. Parsing that slot verifies its tag, which is - computed over the slot itself and over the chunk id, so a wrong meta_size or chunk id fails it. - At object version OBJ_VERSION_HEADER_AAD the magic and the version are covered as well (as AAD, - additional authenticated data: bytes the tag covers without being part of the ciphertext); at - OBJ_VERSION_NO_HEADER_AAD they are not: a wrong magic fails the explicit magic check, and a - wrong version fails because the version selects the AAD the slot is parsed with. data_size, the - one header field outside the tag at either version, must match csize - the data slot's payload - size, recorded in the tagged metadata - plus the key's fixed envelope overhead. - - In the "none-*" modes the tag is an unkeyed checksum, so validate accepts any well-formed - object, including one that a backed up file contains. The tag binds an object to its chunk id - alone, so the "authenticated-*" modes likewise accept an object copied verbatim from a - repository sharing their key, at any offset in any pack. - """ - hdr_size = RepoObj.obj_header.size - overhead = repo_objs.key.PAYLOAD_OVERHEAD # the envelope adds a fixed number of bytes to the payload - - def validate(chunk_id, obj): - try: - meta = repo_objs.parse_meta(chunk_id, obj, ro_type=ROBJ_DONTCARE) - data_size = RepoObj.ObjHeader(*RepoObj.obj_header.unpack(obj[:hdr_size])).data_size - return data_size == meta["csize"] + overhead - except (IntegrityErrorBase, msgpack.UnpackException, KeyError, TypeError, IndexError): - # arbitrary bytes fail the tag, the msgpack unpacking, the length checks or the csize lookup. - return False - except Exception as err: - # anything else is a bug: log it and take the object as invalid. - logger.debug(f"validating an object raised {err!r}, treating it as invalid.") - return False - - return validate - - class ArchiveChecker: # Bound how many missing file chunks rebuild_archives buffers for its end-of-run report, # so checking a badly damaged repo with very many missing chunks can not exhaust memory. @@ -2218,13 +2182,17 @@ def check( # --repair does rebuild from the packs (slow_rebuild=repair), working from the real packs so it # can detect and fix archives that reference chunks whose pack has gone missing. # --repair also passes validate, which makes the rebuild resync past a corrupt object header. + # Every object header is validated, since a corrupt data_size parses and points the walk into + # the middle of the pack. Cost: one metadata slot read and one decryption per object. # Validating needs the key, so read it here. manifest_only=True, because the other source # make_key reads keys from is self.chunks, which is only built below. if repair and self.key is None: try: self.key = self.make_key(repository, manifest_only=True) except IntegrityError as err: - logger.warning(f"{err}. Packs with a corrupt object header can not be repaired.") + logger.warning( + f"Could not read the key ({err}), so a pack with a corrupt object header stays as it is." + ) validate = object_validator(RepoObj(self.key)) if repair and self.key is not None else None self.chunks = build_chunkindex_from_repo( self.repository, slow_rebuild=repair, validate=validate, write_immediately=False diff --git a/src/borg/repoobj.py b/src/borg/repoobj.py index 61e5b541d4..2c45b2c160 100644 --- a/src/borg/repoobj.py +++ b/src/borg/repoobj.py @@ -6,6 +6,7 @@ from .helpers import msgpack, workarounds from .helpers.errors import Error, IntegrityError from .compress import Compressor, LZ4_COMPRESSOR +from .crypto.low_level import IntegrityError as IntegrityErrorBase # Workaround for lost passphrase or key in the "authenticated-*" modes AUTHENTICATED_NO_KEY = "authenticated_no_key" in workarounds @@ -75,7 +76,9 @@ def get_assert_id_places(): # Size of the header prefix used as AEAD AAD (additional authenticated data: authenticated together # with the ciphertext, but not itself encrypted) for OBJ_VERSION_HEADER_AAD objects: magic(8) + # version(1) + chunk_id(32). meta_size and data_size are excluded, since they are only known after -# encryption; a change to either still fails authentication, by changing the ciphertext slice length. +# encryption. A change to either changes the ciphertext slice length, so parse(), which reads both +# slots, fails authentication; parse_meta() reads the metadata slot alone and thus does not see a +# changed data_size. REPOOBJ_HEADER_AAD_SIZE = len(OBJ_MAGIC) + 1 + 32 META_AAD_TAG = b"M" @@ -277,5 +280,37 @@ def parse( return meta_compressed if want_compressed else meta, data_compressed if want_compressed else data +def object_validator(repo_objs): + """Return validate(chunk_id, obj): True if obj is the repo object with id chunk_id. + + obj is an object's header plus its metadata slot. Parsing that slot verifies its tag, which is + computed over the slot itself and over the chunk id, so a wrong meta_size or chunk id fails it. + At object version OBJ_VERSION_HEADER_AAD the magic and the version are covered as well (as AAD, + additional authenticated data: bytes the tag covers without being part of the ciphertext); at + OBJ_VERSION_NO_HEADER_AAD they are not: a wrong magic fails the explicit magic check, and a + wrong version fails because the version selects the AAD the slot is parsed with. data_size, the + one header field outside the tag at either version, must match csize - the data slot's payload + size, recorded in the tagged metadata - plus the key's fixed envelope overhead. + + In the "none-*" modes the tag is an unkeyed checksum, and in the "authenticated-*" modes it is + deterministic and binds an object to its chunk id alone. Both therefore accept an object that a + backed up file contains, at any offset in any pack. + """ + hdr_size = RepoObj.obj_header.size + overhead = repo_objs.key.PAYLOAD_OVERHEAD # the envelope adds a fixed number of bytes to the payload + + def validate(chunk_id, obj): + try: + meta = repo_objs.parse_meta(chunk_id, obj, ro_type=ROBJ_DONTCARE) + data_size = RepoObj.ObjHeader(*RepoObj.obj_header.unpack(obj[:hdr_size])).data_size + return data_size == meta["csize"] + overhead + except (IntegrityErrorBase, msgpack.UnpackException, KeyError, TypeError, IndexError): + # arbitrary bytes fail the tag, the msgpack unpacking, the length checks or the csize lookup. + # Anything else is a bug and propagates: dropping an object over it would lose data. + return False + + return validate + + # Backward compatibility: RepoObj1 has moved to borg.legacy.repoobj from .legacy.repoobj import RepoObj1 # noqa: F401 diff --git a/src/borg/repository.py b/src/borg/repository.py index cf6d61fb15..792fe657e1 100644 --- a/src/borg/repository.py +++ b/src/borg/repository.py @@ -43,8 +43,9 @@ # how much to read to get an object's header plus, at the usual metadata slot sizes, its metadata # slot in the same read. META_READ_SIZE = 1024 -# the largest metadata slot a validating read fetches. a slot holds a few compression fields, -# packed and encrypted. +# the largest metadata slot a validating read fetches. a slot holds a few compression fields, packed +# and encrypted, i.e. some tens of bytes. MAX_DATA_SIZE bounds a slot already; this caps what a +# corrupt meta_size can make the read fetch to far less than that. MAX_VALIDATED_META_SIZE = 64 * 1024 diff --git a/src/borg/testsuite/archiver/check_cmd_test.py b/src/borg/testsuite/archiver/check_cmd_test.py index 70af113497..61d8691963 100644 --- a/src/borg/testsuite/archiver/check_cmd_test.py +++ b/src/borg/testsuite/archiver/check_cmd_test.py @@ -7,13 +7,15 @@ import pytest +from ... import archive as archive_module from ...archive import ArchiveChecker, ChunkBuffer from ...constants import * # NOQA from ...helpers import bin_to_hex, msgpack, CommandError, Error, IntegrityError, sig_int from ...manifest import Archives, Manifest from ...repoobj import RepoObj from ...repository import PackTracker, Repository -from ..repository_test import DATA_SIZE_OFFSET, fchunk, corrupt_chunk_on_disk +from ..repoobj_test import DATA_SIZE_OFFSET +from ..repository_test import fchunk, corrupt_chunk_on_disk from . import ( cmd, src_file, @@ -772,6 +774,39 @@ def test_repair_resyncs_pack_with_corrupt_object_header(archivers, request, dama assert f"Store object packs/{bin_to_hex(pack_id)} is corrupted" in output +def test_repair_without_the_key_rebuilds_without_validating(archivers, request, monkeypatch): + """--repair that can not read the key says so and rebuilds the chunks index without validating. + + make_key gives up with an IntegrityError when the manifest yields no key, which a badly damaged + repository can do. The rebuild then walks the object headers alone, so a pack with a corrupt + object header keeps its damage. + """ + archiver = request.getfixturevalue(archivers) + if archiver.get_kind() != "local": + pytest.skip("patches in-process archive internals") + check_cmd_setup(archiver) + real_make_key = ArchiveChecker.make_key + + def make_key(self, repository, manifest_only=False): + if manifest_only: # the read that yields the validator's key; the later full read succeeds + raise IntegrityError("no key") + return real_make_key(self, repository, manifest_only=manifest_only) + + real_build = archive_module.build_chunkindex_from_repo + validators = [] + + def build_chunkindex_from_repo(repository, **kwargs): + validators.append(kwargs.get("validate")) + return real_build(repository, **kwargs) + + monkeypatch.setattr(ArchiveChecker, "make_key", make_key) + monkeypatch.setattr(archive_module, "build_chunkindex_from_repo", build_chunkindex_from_repo) + output = cmd(archiver, "check", "--repair", exit_code=0) + assert "Could not read the key (" in output + assert validators[0] is None # the rebuild got no validator, so it walked the headers alone + cmd(archiver, "list", "archive1", exit_code=0) # the archives are still readable + + def test_repair_finish_flushes_pack_writer(archivers, request): """finish() stores chunks re-added during --repair before it (re)builds the index (#10055). diff --git a/src/borg/testsuite/repoobj_test.py b/src/borg/testsuite/repoobj_test.py index db98ff6b6c..b723b9cede 100644 --- a/src/borg/testsuite/repoobj_test.py +++ b/src/borg/testsuite/repoobj_test.py @@ -1,8 +1,8 @@ import pytest from ..constants import ROBJ_FILE_STREAM, ROBJ_MANIFEST, ROBJ_ARCHIVE_META -from ..crypto.key import ChecksumKey, AuthenticatedKey, CHPOKey, LegacyPlaintextKey -from ..helpers import msgpack +from ..crypto.key import AESOCBKey, ChecksumKey, AuthenticatedKey, CHPOKey, LegacyPlaintextKey +from ..helpers import CompressionSpec, msgpack from ..helpers.errors import Error, IntegrityError from ..repository import Repository from ..repoobj import ( @@ -14,10 +14,15 @@ OBJ_VERSION_NO_HEADER_AAD, REPOOBJ_HEADER_SIZE, RepoObj, + object_validator, ) from ..legacy.repoobj import RepoObj1 from ..compress import LZ4 +# offsets of the size fields in the object header. +META_SIZE_OFFSET = len(OBJ_MAGIC) + 1 + 32 # the magic, the version byte and the chunk id precede it +DATA_SIZE_OFFSET = META_SIZE_OFFSET + 4 + @pytest.fixture def repository(tmpdir): @@ -500,3 +505,42 @@ def test_version1_object_without_header_aad_still_readable(aead_key): assert got_meta["type"] == ROBJ_FILE_STREAM got_meta, got_data = repo_objs.parse(id, cdata, ro_type=ROBJ_FILE_STREAM) assert got_data == data + + +def validator_input(repo_objs, data): + # (chunk_id, head) of a real repo object storing data: head is its object header plus its + # metadata slot, which is what validate takes. + chunk_id = repo_objs.id_hash(data) + obj = repo_objs.format(chunk_id, {}, data, ro_type=ROBJ_FILE_STREAM) + hdr_size = RepoObj.obj_header.size + meta_size = RepoObj.ObjHeader(*RepoObj.obj_header.unpack(obj[:hdr_size])).meta_size + return chunk_id, obj[: hdr_size + meta_size] + + +@pytest.mark.parametrize("key_class", [ChecksumKey, AuthenticatedKey, CHPOKey, AESOCBKey]) +def test_object_validator_checks_the_sizes_for_every_envelope(key_class): + # data_size == csize + the envelope overhead must hold for each key family; changing meta_size + # or data_size must fail validation. + key = key_class(None) + if hasattr(key, "init_from_random_data"): + key.init_from_random_data() + key.init_ciphers() + repo_objs = RepoObj(key) + chunk_id, head = validator_input(repo_objs, b"payload" * 100) + validate = object_validator(repo_objs) + assert validate(chunk_id, head) + for field_offset in (META_SIZE_OFFSET, DATA_SIZE_OFFSET): + bad = bytearray(head) + bad[field_offset] ^= 0x01 + assert not validate(chunk_id, bytes(bad)) + + +@pytest.mark.parametrize("compression", ["none", "lz4", "zstd,3", "obfuscate,2,lz4"]) +def test_object_validator_accepts_every_compression(compression): + # data_size == csize + the envelope overhead is what pins data_size, so every compressor must + # record the whole payload it produced as csize. The obfuscating one pads the payload and + # records the padded size. + repo_objs = RepoObj(ChecksumKey(None)) + repo_objs.compressor = CompressionSpec(compression).compressor + chunk_id, head = validator_input(repo_objs, b"payload" * 100) + assert object_validator(repo_objs)(chunk_id, head) diff --git a/src/borg/testsuite/repository_test.py b/src/borg/testsuite/repository_test.py index 234262e561..4596d2e325 100644 --- a/src/borg/testsuite/repository_test.py +++ b/src/borg/testsuite/repository_test.py @@ -10,19 +10,18 @@ import pytest from borghash import HashTableNT -from ..cache import write_chunkindex_invalid -from ..constants import MAX_CLOCK_SKEW -from ..helpers import CompressionSpec, IntegrityError, Location, bin_to_hex -from ..hashindex import ChunkIndex from .. import repository as repository_module -from ..archive import object_validator +from ..cache import write_chunkindex_invalid from ..compress import CNONE -from ..constants import ROBJ_FILE_STREAM -from ..crypto.key import AESOCBKey, AuthenticatedKey, CHPOKey, ChecksumKey -from ..repository import Repository, MAX_DATA_SIZE, propagate_rsh, rest_serve_command, PackWriter, PackReader -from ..repository import PackTracker, MAX_VALIDATED_META_SIZE -from ..repoobj import RepoObj, OBJ_MAGIC, OBJ_VERSION, REPOOBJ_HEADER_AAD_SIZE +from ..constants import MAX_CLOCK_SKEW, ROBJ_FILE_STREAM +from ..crypto.key import CHPOKey, ChecksumKey +from ..helpers import IntegrityError, Location, bin_to_hex +from ..hashindex import ChunkIndex +from ..repository import Repository, MAX_DATA_SIZE, MAX_VALIDATED_META_SIZE, propagate_rsh, rest_serve_command +from ..repository import PackWriter, PackReader, PackTracker +from ..repoobj import RepoObj, OBJ_MAGIC, OBJ_VERSION, object_validator from .hashindex_test import H +from .repoobj_test import DATA_SIZE_OFFSET, META_SIZE_OFFSET def test_rest_serve_command_local(): @@ -87,11 +86,6 @@ def reopen(repository, exclusive: bool | None = True, create=False): raise TypeError(f"Invalid argument type. Expected 'Repository', received '{type(repository).__name__}'.") -# offsets of the object header's size fields, for the tests that damage one of them. -META_SIZE_OFFSET = REPOOBJ_HEADER_AAD_SIZE # magic, version and chunk id precede it -DATA_SIZE_OFFSET = META_SIZE_OFFSET + 4 - - def fchunk(data, meta=b"", chunk_id=b"\x00" * 32): # Build a raw chunk with a valid RepoObj layout but no encryption or compression. Pass a unique # chunk_id when objects must not share a pack: identical bytes hash to the same sha256 pack id @@ -1889,9 +1883,10 @@ def test_pack_reader_raises_on_unsupported_version(): def test_pack_reader_rejects_an_object_over_max_data_size(): # a header claiming more than put() would ever write, in a pack large enough to hold it. hdr = RepoObj.obj_header.pack(OBJ_MAGIC, OBJ_VERSION, H(8), 0, MAX_DATA_SIZE) - parsed, problem = PackReader._parse_header(hdr, 0, 2 * MAX_DATA_SIZE) - assert parsed is None - assert "exceeds the maximum" in problem + reader = PackReader(pack_contents=hdr) + reader.size = lambda: 2 * MAX_DATA_SIZE # the object fits the pack, so only the limit rejects it + with pytest.raises(IntegrityError, match="exceeds the maximum"): + list(reader.iter_headers()) def test_pack_reader_does_not_fetch_an_oversized_metadata_slot(): @@ -1906,7 +1901,7 @@ def test_pack_reader_does_not_fetch_an_oversized_metadata_slot(): ) -def test_pack_reader_resync_validates_the_object_it_resumes_at_once(monkeypatch): +def test_pack_reader_resync_validates_the_object_it_resumes_at_once(): # the scan validated the header it found, so the walk yields it without validating it again. obj1 = bytearray(fchunk(b"payload-one", chunk_id=H(1))) obj2 = fchunk(b"payload-two", chunk_id=H(2)) @@ -2051,40 +2046,6 @@ def test_pack_reader_resync_starts_past_the_last_validated_object(): ] -@pytest.mark.parametrize("key_class", [ChecksumKey, AuthenticatedKey, CHPOKey, AESOCBKey]) -def test_object_validator_checks_the_sizes_for_every_envelope(key_class): - # data_size == csize + the envelope overhead must hold for each key family; changing meta_size - # or data_size must fail validation. - key = key_class(None) - if hasattr(key, "init_from_random_data"): - key.init_from_random_data() - key.init_ciphers() - repo_objs = RepoObj(key) - chunk_id, obj = real_chunk(repo_objs, b"payload" * 100) - hdr_size = RepoObj.obj_header.size - meta_size = RepoObj.ObjHeader(*RepoObj.obj_header.unpack(obj[:hdr_size])).meta_size - head = obj[: hdr_size + meta_size] # what the walk hands to validate - validate = object_validator(repo_objs) - assert validate(chunk_id, head) - for field_offset in (META_SIZE_OFFSET, DATA_SIZE_OFFSET): - bad = bytearray(head) - bad[field_offset] ^= 0x01 - assert not validate(chunk_id, bytes(bad)) - - -@pytest.mark.parametrize("compression", ["none", "lz4", "zstd,3", "obfuscate,2,lz4"]) -def test_object_validator_accepts_every_compression(compression): - # data_size == csize + the envelope overhead is what pins data_size, so every compressor must - # record the whole payload it produced as csize. The obfuscating one pads the payload and - # records the padded size. - repo_objs = RepoObj(ChecksumKey(None)) - repo_objs.compressor = CompressionSpec(compression).compressor - chunk_id, obj = real_chunk(repo_objs, b"payload" * 100) - hdr = RepoObj.ObjHeader(*RepoObj.obj_header.unpack(obj[: RepoObj.obj_header.size])) - head = obj[: RepoObj.obj_header.size + hdr.meta_size] # what the walk hands to validate - assert object_validator(repo_objs)(chunk_id, head) - - def test_pack_reader_resync_keeps_the_object_before_a_corrupt_header(): # the object before the damage validated, so it is kept and the scan starts past it. obj1 = fchunk(b"payload-one", meta=b"meta1", chunk_id=H(1))