validate object headers when walking a pack (PackReader.iter_headers)

[mr-raj12]

Noticed this while looking at #8476, which wants the chunks index rebuilt by iterating only over the object headers of the packs. That walk is PackReader.iter_headers (used by build_chunkindex_from_repo) and it did not check the headers at all.

It unpacks each fixed header and takes the next offset from the sizes in it:

hdr = RepoObj.ObjHeader(*RepoObj.obj_header.unpack(hdr_data))
obj_size = hdr_size + hdr.meta_size + hdr.data_size
yield hdr.chunk_id, offset, obj_size
offset += obj_size

So on a corrupt header obj_size is wrong and the walk either keeps going on payload bytes and yields garbage (chunk_id, offset, size) tuples, or skips past the end of the pack, where the short read hits the len(hdr_data) < hdr_size branch and looks like a clean EOF. The index rebuilt from that is wrong either way, and nothing says so.

This PR is now just the check. Each header must have OBJ_MAGIC, a supported version and sizes that keep the object inside the pack, otherwise IntegrityError naming the pack, like check_pack_objects does. superseded_gap_ranges already does the same check when it walks headers in the gaps. The bounds check needs the pack size, which PackReader did not have, so there is now PackReader.size(): len(pack_contents) in memory, store.info(key).size otherwise, one metadata lookup per iter_headers call. No per-object roundtrip.

Tests

Corrupt magic, unsupported version, and an object declared past the end of the pack all raise, in memory and through the store.

Split off: repair-time resync

The two resync commits that were here (scan forward to the next object header, accept a candidate only if it authenticates) have been dropped from this PR and kept for a follow-up, because they depend on questions about the pack format itself rather than on this check:

• The scan hands each candidate's full bytes to the validator. Authenticating only the encrypted metadata is enough (the AAD covers magic, version and chunk_id) and turns up to MAX_DATA_SIZE per candidate into ~130 bytes. But then data_size is authenticated by nothing, and parse_meta() does no id check, so none/authenticated lose the one check that rejects a decoy object stored verbatim inside a file's content.
• Independently of corruption, iter_headers costs one store range read per object. Rebuilding an index over ssh/rest is that many roundtrips per pack.

Both go away if a pack carries an authenticated directory of its objects, or if the header itself is authenticated. Happy to work on either; the follow-up is easier to judge once that is decided.

What this does not fix

The pack stays damaged. check --repair drops the index in finish(), so the next command rebuilds from the packs and raises on the same header again. Before this PR that rebuild would have silently produced a wrong index instead, so this is not a regression, but a repo is only really usable again once the pack itself is repaired (#10026).

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 86.82%. Comparing base (0b451c1) to head (0ccf95b).
⚠️ Report is 12 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #10083      +/-   ##
==========================================
+ Coverage   86.77%   86.82%   +0.04%     
==========================================
  Files          98       99       +1     
  Lines       17277    17342      +65     
  Branches     2622     2631       +9     
==========================================
+ Hits        14992    15057      +65     
- Misses       1587     1589       +2     
+ Partials      698      696       -2     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

[ThomasWaldmann]

raising is ok for normal usage, so users can notice corruption.

for repair usage, if length is corrupted, guess we need to re-sync using a scan for the magic.

[ThomasWaldmann]

The magic scan should be combined with authentication / id check:

• if the length is obviously wrong, do the magic scan.
• if the magic scan has located a repo object, do the crypto authentication / id check for the meta / the data and accept the object only if that succeeds. otherwise the magic scan could confuse user content (the magic could appear in user content, esp. if no encryption and no compression is used) with pack structure.

[mr-raj12]

Agreed on the auth/id check. For none/authenticated mode it has to be the id check on the decompressed plaintext: PlaintextKey.decrypt and AuthenticatedKeyBase.decrypt only strip the type byte, so decrypting the meta would happily accept anything there.

Before implementing it I got stuck on an ordering problem. The validation needs the key, but ArchiveChecker.check() builds the chunks index before it has one (build_chunkindex_from_repo(...), and self.key = self.make_key(repository) only after that), and make_key() iterates self.chunks when the manifest can not be read, which is exactly a repair case. So the key creation can not simply move in front of the rebuild.

Two ways out that I see. One is make_key(repository, manifest_only=True) before the rebuild: if that gives a key the resync validates, and if it does not, we do not resync at all and iter_headers raises like it does now. The other is two passes: rebuild without validation and remember the pack ids that needed a resync, then create the key (which can use the index by then) and re-walk only those packs with validation, replacing their entries. The first is much less code, but a repo whose manifest is gone gets no resync at all, which is the case where it would help most. The second always validates and its second walk only touches the damaged packs, so I lean that way, but it is your call.

Also, if no usable key can be obtained at all: should the resync be refused, or done unvalidated with a warning?

[ThomasWaldmann]

I'll check make_key ...

Update: It can be used with manifest_only=True, then it does not need the chunks index.

Guess for now, this is an acceptable limitation to unblock progress here. Later, we can also try to decrypt all sorts of other stuff (index, cache, ...) without needing a chunks index.

Thinking about it: making the key should be one of the very first steps. we'll try to have most stuff encrypted in the repo and almost everything is running client-side now (in contrast to borg 1.x, where Repository code ran server-side and there is no key on the server).

[ThomasWaldmann]

ping?

[mr-raj12]

Pushed, rebased on master. make_key(manifest_only=True) runs before the rebuild now, and the scan parses a candidate before accepting it.

No new BORG_ASSERT_ID place was needed: id_check_is_authentication already forces the id check for none/authenticated, and "repair" is in BORG_ASSERT_ID_DEFAULT for the AEAD keys. I also dropped the resync flag, iter_headers scans exactly when it gets a validate function, so scanning without validating can not be expressed.

The pack itself stays damaged though, and finish() drops the index, so the next command rebuilds from the packs and raises on the same header again. It silently built a wrong index before, so nothing got worse, but the repo is only really fixed by repairing the pack (#10026). Want that in this PR or separately?

[ThomasWaldmann]

Hmm, guess we need another change:

• scanning and relying only on the unauthenticated headers (magic, lengths, id) is not good enough, obviously
• the metadata after that header is (in the best case) AEAD encrypted/authenicated AND it isn't much to read / transfer / process. I guess we just won't care for the encryption=none case - not our fault if people want it unsafe. We can at least get an authenticated chunkid that way.
• reading the data (and authenticating it on the client-side) though would mean to transfer the whole repository content to the client (that is basically as expensive as --verify-data, but we throw the result away afterwards) - we shouldn't do that just to rebuild the index.

[mr-raj12]

Agreed on both points, so I took the resync commits back out. This PR is now only the header check, which stands on its own; the description is updated.

On authenticating the metadata instead of the whole object: that is clearly right, parse_meta() on the header plus the encrypted metadata is ~130 bytes against up to MAX_DATA_SIZE, and the AAD (magic, version, chunk_id) is exactly what the walk yields. Two things do not survive the change, though:

• data_size is then covered by nothing. meta_size is covered indirectly, it sets the ciphertext length, but obj_size = hdr + meta_size + data_size is what goes into the chunks index and what the walk advances by. It could be cross-checked against meta["csize"] plus the AEAD overhead, which would need that overhead as a constant, it is currently inline in AEADKeyBase.encrypt.
• parse_meta() does no id check, so for none/authenticated* the scan would accept anything whose metadata unpacks. That is the mode where a backed up file can contain something shaped like an object, so it is the mode that needs the check most.

Which is the same thing you said about the format: with the current one a candidate is either cheap to check or safe to check, not both. And the cost is not only in the corrupt case, the healthy walk is one store range read per object, so an index rebuild over ssh/rest is that many roundtrips per pack.

Two ways out I can see:

1. An authenticated object directory at the end of a pack, (chunk_id, offset, size) per object, sealed with the pack id as AAD, and a fixed-size footer giving its length. An index rebuild is then one read per pack and the result is authenticated, and there is nothing to scan for as long as the directory is intact. Needs a format version bump and a writer change, and a damaged directory still needs some fallback, which is where a scan would remain.
2. A keyed MAC over the 49-byte header. 16 bytes per object, headers verify without reading anything else, which covers data_size and the decoy case as well. Smaller change, but the walk still costs one read per object.

I would rather build the follow-up on whichever of those you want than tune the scan on the current format. Do you have a preference? Same question decides whether repairing the pack itself (#10026) belongs in that follow-up.

[mr-raj12]

The resync work is now #10094, stacked on this one.

[ThomasWaldmann]

LGTM.

[ThomasWaldmann]

1. Would solve efficient index rebuilding. If we use (chunkid, offset, metasize, datasize) that would also enable an all-at-once Store.gather call for all objects' metadata inside a pack. Needs implementing in borgstore first, but is easy. But: server-side defrag can't build an authenticated object directory.
2. Maybe the full header can be authenticated as AAD (with the metadata, with the data).