Skip to content

Do not decrypt a container key bag that is already plaintext - #91

Open
willmcginnis wants to merge 1 commit into
libyal:mainfrom
willmcginnis:plaintext-container-key-bag
Open

willmcginnis wants to merge 1 commit into
libyal:mainfrom
willmcginnis:plaintext-container-key-bag

Conversation

@willmcginnis

@willmcginnis willmcginnis commented Sep 14, 2026

Copy link
Copy Markdown
Contributor

Fixes #90.

A container key bag can be stored unencrypted on disk. libfsapfs_container_key_bag_read_file_io_handle decrypted unconditionally, so on those containers a valid plaintext key bag was turned into noise and the parse then failed on the object type.

On the error path you pointed me at: the failure is self-disguising, and libfsapfs_container_is_locked is exactly where it surfaces. libfsapfs_internal_container_open_read treats a 0 return from the key bag read as "locked" and sets key_bag->is_locked, so libfsapfs_container_is_locked reports locked for what is really a parse failure on an intact key bag, and fsapfsinfo prints "Is locked (uses hardware encryption)". I have not added a separate error path, because once the plaintext case is read correctly is_locked stops being set spuriously for it. To be precise about the scope: this prevents valid plaintext container key bags from being reported as locked, while other object-type failures retain the existing behaviour, so it removes one cause of a misleading lock status rather than making that status authoritative. If you would rather distinguish the two causes explicitly, say so and I will add it.

The change

A read-only probe, run before decrypting: object type 0x6b657973, subtype 0, and a Fletcher-64 over the block. It inspects the buffer in place and touches no container key bag state, so a negative result leaves nothing to roll back. read_file_io_handle then either takes ownership of the buffer as read or allocates and decrypts as before. The single read_data call is unchanged, so entries are never appended twice.

This is the same case apfs-fuse models with m_is_unencrypted (b5955511).

Testing

Twelve key bag blocks captured from APFS containers across macOS 10.15.7, 12.7.6 and 13.7.8, covering born-encrypted, encrypted-volume and CoreStorage-converted provenances: six on-disk encrypted blocks and the six corresponding plaintext key bags.

The probe returns 0 for all six encrypted blocks and 1 for all six plaintext key bags, called as the exported library function rather than a copy of it. The expected answers came from an independent Fletcher-64 implementation over the same blocks, and the two agree 12 of 12.

The same change was also built and run end to end against two FileVault container images, with a before/after control. On the image whose container key bag is plaintext on disk, the stock build prints "Is locked (uses hardware encryption)" and never parses the key bag, while the patched build parses it and proceeds to open volumes. To be complete about what that patched run shows, because it is not a clean pass: having got past the container key bag it then hits the identical unconditional decrypt in libfsapfs_volume_key_bag_read_file_io_handle, fails with invalid object type on volume 5, and prints 4 volumes where stock printed 6. That second reader is the other half of #90 and this PR does not touch it. I would rather show you that than describe the run as a clean success, since it localises the next fix.

On the image whose container key bag is genuinely encrypted, the patched build's stdout and stderr are byte-identical to stock, so the encrypted path is provably untouched. "Stock" there was the patch's parent commit rather than upstream main, to isolate this change alone.

One scoping note on that end-to-end run: it was performed before the rebase onto current main. The code it exercised is not merely similar to this commit, it is the same: the probe function, read_file_io_handle through its on_error label, and the header are byte-identical between the tested tree and 58eeb22. The only difference anywhere in the file is your own bounds-checking added in 6aecf24.

The encrypted arm is the one that matters for regression risk: if the probe ever returned 1 there, a genuinely encrypted key bag would be passed through undecrypted. It does not, on any of the six.

The container key bag can be stored unencrypted on disk. read_file_io_handle
decrypted unconditionally, so on those containers a valid plaintext key bag was
turned into noise and the parse then failed on the object type.

The failure is self-disguising. libfsapfs_internal_container_open_read treats a
0 return from the key bag read as "locked" and sets key_bag->is_locked, so
fsapfsinfo reports "Is locked (uses hardware encryption)" for what is really a
parse failure on an intact key bag, and libfsapfs_container_is_locked reports
that same value. This removes one cause of that misleading status; an object
type mismatch after a successful decrypt still sets it, as before.

Adds a read-only probe run before decrypting: object type 0x6b657973, subtype
0, and a Fletcher-64 over the block. It inspects the buffer in place and
touches no container key bag state, so a negative result leaves nothing to roll
back. read_file_io_handle then either takes ownership of the buffer as read or
allocates and decrypts as before. The single read_data call is unchanged, so
entries are never appended twice.

The probe rejects a size that is not a multiple of 4 rather than passing it to
libfsapfs_checksum_calculate_fletcher64, which returns an error for it; such a
size is a "not an unencrypted container key bag" case rather than an error.
Unreachable through the current caller, where the size is a multiple of the
block size, but the function is declared in the internal header.

Tested against twelve key bag blocks captured from APFS containers across
macOS 10.15.7, 12.7.6 and 13.7.8, covering born-encrypted, encrypted-volume and
CoreStorage-converted provenances: six on-disk encrypted blocks and the six
corresponding plaintext key bags. The probe returns 0 for all six encrypted and
1 for all six plaintext, matching an independent Fletcher-64 implementation on
the same blocks.

This is the same case apfs-fuse models with m_is_unencrypted (b5955511).
@willmcginnis
willmcginnis force-pushed the plaintext-container-key-bag branch from 58eeb22 to 4a14a18 Compare September 14, 2026 01:35
@codecov

codecov Bot commented Sep 14, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 0% with 50 lines in your changes missing coverage. Please review.
✅ Project coverage is 29.27%. Comparing base (6aecf24) to head (4a14a18).

Files with missing lines Patch % Lines
libfsapfs/libfsapfs_container_key_bag.c 0.00% 50 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #91      +/-   ##
==========================================
+ Coverage   29.24%   29.27%   +0.02%     
==========================================
  Files          74       74              
  Lines       16010    15984      -26     
  Branches     3681     3688       +7     
==========================================
- Hits         4682     4679       -3     
+ Misses      10122    10099      -23     
  Partials     1206     1206              

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Key bags are decrypted unconditionally, which destroys key bags that are stored in the clear

1 participant