Skip to content

fix(crypto): harden keybag, ASN.1 and key derivation code paths - #219

Open
BuenGenio wants to merge 3 commits into
sgan81:masterfrom
BuenGenio:master
Open

fix(crypto): harden keybag, ASN.1 and key derivation code paths#219
BuenGenio wants to merge 3 commits into
sgan81:masterfrom
BuenGenio:master

Conversation

@BuenGenio

@BuenGenio BuenGenio commented May 21, 2026

Copy link
Copy Markdown

Summary

Audit and hardening of the existing software-encryption code path (keybag parsing, ASN.1/DER decoding, PBKDF2, volume key handling). All changes are on the read path — behaviour is unchanged for a well-formed encrypted volume; the goal is to make corrupt or hostile input fail safely instead of reading out of bounds or crashing.

##Bugs fixed

  • OOB reads on a corrupt keybag — Keybag::Init trusted kl_nbytes from disk as the assign() size; Keybag::GetKey/FindKey walked the variable-length entry list with no bounds checks. Both can read past m_data. Now validated/bounds-checked.
  • Wrong block verification — LoadKeybag verified the whole buffer blockcnt times with identical arguments instead of verifying each block. Now verifies each block individually.
  • LoadKeybag robustness — ignored ReadBlocks return value; unbounded resize() from a corrupt block count; m_is_unencrypted could be set by a volume (recs) keybag. All fixed.
  • ASN.1 parser UB — der_decode_len/der_decode_tl formed out-of-bounds pointers and could dereference nullptr. Replaced pointer arithmetic with size comparisons.
  • PBKDF2 iteration truncation — iterations was int, but the keybag value is uint64_t, so a large count truncated to a negative int and silently derived the wrong key. Changed to uint32_t.
  • GetVolumeKey null-deref — the password parameter defaults to nullptr but the body called strlen() on it. Now treated as empty.

Hardening / cleanup

  • Zero derived key material (dk, kek, vek) from the stack after use.
  • Extract duplicated volume-encryption setup into ApfsVolume::InitVolumeKey(), fixing a divergent guard between Init and MountSnapshot.
  • Remove dead variables.

Build fix (second commit)

fix(build): add missing include to PList.h — pre-existing and unrelated to crypto, but required for the project to build at all on GCC 15 / libstdc++ 15, which no longer leaks transitively.

Testing

Built with cmake + make (GCC 15, Release):

crypto library — built (covers Asn1Der.cpp, Crypto.cpp)
libapfs.a — built (covers KeyMgmt.cpp, ApfsVolume.cpp)
apfsutil, apfs-dump, apfs-dump-quick — compiled and linked
All modified files compile with no new warnings under -Wall -Wextra

BuenGenio and others added 3 commits May 21, 2026 20:18
Audit and harden the existing software-encryption code. All changes are
on the read path; behaviour is unchanged for a well-formed encrypted
volume.

Keybag parsing (ApfsLib/KeyMgmt.cpp):
- Validate kl_nbytes against the buffer that was actually read so a
  corrupt value can no longer drive an out-of-bounds heap read in
  Keybag::Init.
- Bounds-check the variable-length entry walk in Keybag::GetKey and
  reuse it from FindKey, so a malformed keybag cannot walk past m_data.
- Verify each keybag block individually instead of verifying the whole
  buffer blockcnt times with identical arguments.
- Check the ReadBlocks return value, cap the keybag block count to
  reject an absurd allocation, and only let the container keybag set
  m_is_unencrypted.

ASN.1/DER parser (Crypto/Asn1Der.cpp):
- Avoid undefined behaviour from out-of-bounds pointer arithmetic and a
  possible null-pointer dereference in der_decode_len/der_decode_tl by
  comparing sizes instead of pointers.

Key derivation (Crypto/Crypto.cpp, Crypto/Crypto.h):
- Change the PBKDF2 iteration count from int to uint32_t; the keybag
  value is uint64_t and a large count truncated to a negative int,
  silently producing the wrong key. Drop dead variables.

Volume key handling (ApfsLib/KeyMgmt.cpp, ApfsLib/ApfsVolume.cpp):
- Treat a null password as empty in GetVolumeKey to avoid a strlen()
  null dereference.
- Zero derived key material (dk, kek, vek) from the stack after use.
- Extract the duplicated volume-encryption setup into
  ApfsVolume::InitVolumeKey() so Init and MountSnapshot behave
  consistently.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
PList.h uses uint8_t and uint32_t but relied on them being pulled in
transitively by another standard header. libstdc++ 15 (GCC 15) no
longer does so, which broke the build with errors such as
"'uint8_t' does not name a type". Include <cstdint> explicitly.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
fix(crypto): harden keybag, ASN.1 and key derivation code paths
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.

1 participant