From 01c8d6707c958499fd266716b794146d585aaeeb Mon Sep 17 00:00:00 2001 From: Mrityunjay Raj Date: Thu, 13 Aug 2026 03:30:06 +0530 Subject: [PATCH 1/2] 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 0f8d6acec9..5e43d1a23f 100644 --- a/docs/internals/packs.rst +++ b/docs/internals/packs.rst @@ -91,10 +91,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 db7de5cd03..ee4f896a18 100644 --- a/src/borg/archive.py +++ b/src/borg/archive.py @@ -1875,6 +1875,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. @@ -1926,7 +1952,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 e49e3533be..d7746abad4 100644 --- a/src/borg/cache.py +++ b/src/borg/cache.py @@ -852,7 +852,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. @@ -942,7 +948,8 @@ def build_chunkindex_from_repo( # PackReader uses the store directly, so refresh the lock here; a full rebuild can be slow. repository._lock_refresh() 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 d6a851237f..0b73ba38c1 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 e5d083fda6..88a8d311a2 100644 --- a/src/borg/testsuite/archiver/check_cmd_test.py +++ b/src/borg/testsuite/archiver/check_cmd_test.py @@ -688,6 +688,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 drops the index (#10055). diff --git a/src/borg/testsuite/cache_test.py b/src/borg/testsuite/cache_test.py index 872499c197..9944aae820 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 de21668bef..ca05dce01c 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 @@ -1769,6 +1774,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 ff0f7a0efe3e32bf54534786aed9ba6224b280fc Mon Sep 17 00:00:00 2001 From: Mrityunjay Raj Date: Sat, 15 Aug 2026 19:28:07 +0530 Subject: [PATCH 2/2] 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 5e43d1a23f..0fdf58cb59 100644 --- a/docs/internals/packs.rst +++ b/docs/internals/packs.rst @@ -100,19 +100,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 ee4f896a18..59d61d731d 100644 --- a/src/borg/archive.py +++ b/src/borg/archive.py @@ -1878,26 +1878,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 @@ -1952,10 +1948,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 d7746abad4..9f18f051bc 100644 --- a/src/borg/cache.py +++ b/src/borg/cache.py @@ -862,6 +862,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: @@ -948,7 +950,6 @@ def build_chunkindex_from_repo( # PackReader uses the store directly, so refresh the lock here; a full rebuild can be slow. repository._lock_refresh() 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 0b73ba38c1..df2dc792d2 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 88a8d311a2..3734e9ef76 100644 --- a/src/borg/testsuite/archiver/check_cmd_test.py +++ b/src/borg/testsuite/archiver/check_cmd_test.py @@ -689,11 +689,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 9944aae820..a6fdda8685 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 ca05dce01c..f32e9f39d7 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 @@ -1786,9 +1786,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))) @@ -1883,16 +1880,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