Bind pack object header into AEAD authentication

[mr-raj12]

Pack object headers (magic, version, chunk_id) are stored unencrypted ahead of the encrypted meta and data. Only chunk_id was included in AEAD authentication as additional authenticated data (AAD); magic and version were not. A tampered magic byte was still rejected by the existing structural check (hdr.magic != OBJ_MAGIC) before decrypt ever ran; a tampered version byte between the two accepted values was not caught by any check.

Format version 0x02 (OBJ_VERSION_HEADER_AAD) binds the header's first 41 bytes (magic, version, chunk_id) into the AEAD authentication of encrypted_meta and encrypted_data. format() writes version 0x02; parse()/parse_meta() still accept version 0x01 objects, so nothing needs to be rewritten on disk. encrypted_meta and encrypted_data each add a one-byte slot tag on top of the shared AAD, so swapping the two ciphertexts (with matching meta_size/data_size adjustments) fails authentication instead of decrypting under the wrong slot.

Renamed the header= parameter on KeyBase.encrypt/decrypt to aad=, since the low-level cipher already has its own header= meaning a different thing (the stored envelope prefix).

Also fixed AEADKeyBase.decrypt catching helpers.IntegrityError, a subclass of the low_level.IntegrityError actually raised by the cipher, so the except clause never matched. Existing callers catch the common base class, so this did not let failures through unnoticed; it only meant the chunk-id-annotated error message was never attached.

Added tests for a tampered chunk_id, a tampered magic byte, AAD tampering at the key layer, a meta/data slot swap, and reading a version 0x01 object.

[codecov]

Codecov Report

❌ Patch coverage is 89.28571% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 85.74%. Comparing base (e5ed8f6) to head (9e11923).
⚠️ Report is 50 commits behind head on master.

Files with missing lines	Patch %	Lines
src/borg/repoobj.py	86.36%	0 Missing and 3 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #9946      +/-   ##
==========================================
+ Coverage   85.49%   85.74%   +0.25%     
==========================================
  Files          93       95       +2     
  Lines       16122    16828     +706     
  Branches     2466     2577     +111     
==========================================
+ Hits        13783    14430     +647     
- Misses       1632     1664      +32     
- Partials      707      734      +27     

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

[ThomasWaldmann]

Guess the exception class fix should be moved to a separate PR (and this PR could then be rebased on that PR).

[ThomasWaldmann]

Meta and data are encrypted with the same key and same AAD, so an attacker controlling the repo can swap encrypted_meta and encrypted_data (fixing up the two size fields) and both slots still authenticate — in v1 and in the new v2.

This is pre-existing (master has it too), but if we're minting an object format v2 for AAD reasons, slot domain separation (e.g. header_aad + b"M" / + b"D") belongs in it — otherwise we'll need a v3 for it later.

[mr-raj12]

Guess the exception class fix should be moved to a separate PR (and this PR could then be rebased on that PR).

added a TODO , will raise a separate PR for after this

[mr-raj12]

Meta and data are encrypted with the same key and same AAD, so an attacker controlling the repo can swap encrypted_meta and encrypted_data (fixing up the two size fields) and both slots still authenticate — in v1 and in the new v2.

This is pre-existing (master has it too), but if we're minting an object format v2 for AAD reasons, slot domain separation (e.g. header_aad + b"M" / + b"D") belongs in it — otherwise we'll need a v3 for it later.

added slot tags (b"M"/b"D") into the AAD for meta and data, added a test with that swap case

[ThomasWaldmann]

review by claude fable5

Thanks — with the second commit this is in good shape. I reviewed both commits, ran the test suite locally (repoobj/key/repository/packs units plus the archiver end-to-end subset: check, compact, transfer, extract, debug — all green, matching CI), and verified the crypto claims experimentally. Summary below.

What I verified

• The struct layout <8sB32sII guarantees obj[:41] is exactly magic + version + chunk_id, matching the header_aad built in format().
• Version-flip downgrade fails in both directions: a v2 object rewritten to version 0x01 is decrypted with the v1 AAD (id only) while its tag covers header_aad + slot_tag + id, so authentication fails; same for v1 rewritten to v2. Legit v1 objects stay readable.
• The slot tags close a real gap: before the second commit, encrypted_meta and encrypted_data authenticated under the same key and AAD, so an attacker controlling repo storage could swap the two ciphertexts (fixing up meta_size/data_size) without failing authentication — decryption succeeded and parse() only died later on a raw msgpack UnpackException (not IntegrityError, so it escaped the IntegrityErrorBase handlers). I reproduced that on the first commit and confirmed the second commit makes the swap fail cleanly with IntegrityError at the AEAD layer. Since no structural check can know which slot a ciphertext was written for, this is a guarantee only the AAD can provide — it is the strongest justification for the format version bump.
• The AAD is unambiguous: all components are fixed-width for v2 (41 + 1 + 32 bytes), so no concatenation ambiguity between versions or slots.
• The exception-class fix is correct: helpers.IntegrityError is a subclass of low_level.IntegrityError, and the ciphers raise the base class, so the old except IntegrityError: (helpers) never matched and the "Chunk : Could not decrypt" wrap was dead code. Catching low_level.IntegrityError is exactly right, and re-raising the helpers class keeps every downstream except IntegrityErrorBase site working while adding the chunk id and proper exit-code behavior. (Note the impact was smaller than the PR description suggests: the raw exception was still caught by the IntegrityErrorBase handlers, so failures were never silently accepted — the loss was the missing chunk-id context. Same for the header claims: a tampered magic or version byte was already rejected by the structural checks; what was genuinely unauthenticated before was hdr.chunk_id and the slot assignment. Please adjust the PR description accordingly.)

On compatibility

The version bump means older borg2 betas fail on new objects with "unsupported object version: 2". That is accepted — we don't care for older betas, no repo-level gating needed.

Requested changes

1. Resolve the TODOs before merge. Either actually extract the two except low_level.IntegrityError hunks into their own small PR rebased before this one (as your TODOs propose), or keep them here and just drop the TODO comments — they are PR-management notes and shouldn't land in key.py. Either way is fine.
2. Rename the header= kwarg on key.encrypt()/key.decrypt() to aad=: it now carries AAD (header prefix + slot tag), and header collides confusingly with the low-level ciphers' header= parameter, which is the envelope prefix — a different thing, as your own envelope_header rename in AEADKeyBase.encrypt highlights.
3. Add assert len(id) == 32 in format(): struct 32s silently pads/truncates, so a non-32-byte id would produce a v2 object whose on-disk header differs from the AAD it was encrypted with, making it unreadable by its own rules. Theoretical (all id_hashes are 32 bytes), but cheap insurance.

Nits (take or leave)

• REPOOBJ_HEADER_AAD_SIZE could be derived (len(OBJ_MAGIC) + 1 + 32) instead of a literal.
• Move the from ..repoobj import ... statements in the tests to module top (project style: imports at top unless avoiding a circular import).
• packs.rst implies version 0x02 guarantees header authentication, but plaintext/authenticated key modes ignore the AAD entirely — worth one sentence noting the binding applies to the AEAD encryption modes.
• test_tampered_header_magic_detected exercises the structural magic check (as its comment says), which behaves the same on master — fine as regression coverage, just not evidence for the AAD binding.

Possible follow-up (separate issue, pre-existing): for v1 objects and plaintext/authenticated modes, corruption that reaches msgpack.unpackb in parse()/parse_meta() surfaces as UnpackException instead of IntegrityError; wrapping unpack failures would make those paths degrade gracefully too.