check --repair: resync past corrupt object headers when rebuilding the chunks index

docs/internals/packs.rst

@@ -91,10 +91,37 @@ 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 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``
+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. 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. 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.
 
 Blobs follow one another contiguously with no padding::
 

src/borg/archive.py

@@ -2081,6 +2081,34 @@ 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 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.
+
+    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:
+            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, the length checks or the csize lookup.
+            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.
@@ -2135,7 +2163,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)
+        # --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:
+                logger.warning(f"{err}. Packs with a corrupt object header can not be repaired.")
+        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
+        )
         if self.key is None:
             self.key = self.make_key(repository)
         self.repo_objs = RepoObj(self.key)
@@ -2668,7 +2707,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=object_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.")

src/borg/cache.py

@@ -857,10 +857,18 @@ 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.
+    # 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:
@@ -952,7 +960,7 @@ 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():
+        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

src/borg/repository.py

@@ -30,14 +30,23 @@
 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__)
 
 # 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
+# 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):
     marker = None
@@ -362,44 +371,121 @@ 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 (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. problem names
+        which of these failed.
+        """
+        hdr = RepoObj.ObjHeader(*RepoObj.obj_header.unpack(hdr_data))
+        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 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.
+
+        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(chunk_id, obj) confirms the header and metadata slot at that position.
+        """
+        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 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)
+        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 one range per object (or a slice, for a pack in memory), plus one store
+        metadata lookup for the pack size.
 
-        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.
+        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
+        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 "<no id>"
         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:
-            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 = 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, 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(
+                        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}: {problem} at offset {offset} and no object after it, "
+                        f"skipping the remaining {pack_size - offset} bytes."
+                    )
+                    break
+                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
-            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
 
@@ -1320,7 +1406,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