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.

[ThomasWaldmann]

as you now have renamed the param to aad, there is no need to rename header.

[ThomasWaldmann]

Please file a ticket, that we drop this later, before rc1. Strictly taken, there is no need to support old betas.

But if we do it (and it works, so nothing else must break compatibility), we at least must remember to remove the b22-legacy-support again later, before we go rc.

[ThomasWaldmann]

sizes only known after encryption - is that true?