From 84b8a1f572bd364060cdc325286536de4f6209f7 Mon Sep 17 00:00:00 2001 From: Yevgen Trotsan Date: Thu, 21 May 2026 20:18:24 +0100 Subject: [PATCH 1/2] fix(crypto): harden keybag, ASN.1 and key derivation code paths 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) --- ApfsLib/ApfsVolume.cpp | 92 +++++++++++------------ ApfsLib/ApfsVolume.h | 5 ++ ApfsLib/KeyMgmt.cpp | 165 ++++++++++++++++++++++++++--------------- Crypto/Asn1Der.cpp | 10 ++- Crypto/Crypto.cpp | 20 +---- Crypto/Crypto.h | 4 +- 6 files changed, 166 insertions(+), 130 deletions(-) diff --git a/ApfsLib/ApfsVolume.cpp b/ApfsLib/ApfsVolume.cpp index 6c6ffa5..2ca91af 100644 --- a/ApfsLib/ApfsVolume.cpp +++ b/ApfsLib/ApfsVolume.cpp @@ -68,31 +68,8 @@ bool ApfsVolume::Init(paddr_t apsb_paddr) return false; } - if ((m_sb.apfs_fs_flags & 3) != APFS_FS_UNENCRYPTED && !m_container.IsUnencrypted()) - { - uint8_t vek[0x20]; - std::string str; - - std::cout << "Volume " << m_sb.apfs_volname << " is encrypted." << std::endl; - - if (!m_container.GetVolumeKey(vek, m_sb.apfs_vol_uuid)) - { - if (m_container.GetPasswordHint(str, m_sb.apfs_vol_uuid)) - std::cout << "Hint: " << str << std::endl; - - std::cout << "Enter Password: "; - GetPassword(str); - - if (!m_container.GetVolumeKey(vek, m_sb.apfs_vol_uuid, str.c_str())) - { - std::cout << "Wrong password!" << std::endl; - return false; - } - } - - m_aes.SetKey(vek, vek + 0x10); - m_is_encrypted = true; - } + if (!InitVolumeKey()) + return false; if (!m_fs_tree.Init(m_sb.apfs_root_tree_oid, m_sb.apfs_o.o_xid, &m_omap)) std::cerr << "ERROR: root tree init failed" << std::endl; @@ -170,31 +147,8 @@ bool ApfsVolume::MountSnapshot(paddr_t apsb_paddr, xid_t snap_xid) if (m_sb.apfs_magic != APFS_MAGIC) return false; - if ((m_sb.apfs_fs_flags & 3) != APFS_FS_UNENCRYPTED) - { - uint8_t vek[0x20]; - std::string str; - - std::cout << "Volume " << m_sb.apfs_volname << " is encrypted." << std::endl; - - if (!m_container.GetVolumeKey(vek, m_sb.apfs_vol_uuid)) - { - if (m_container.GetPasswordHint(str, m_sb.apfs_vol_uuid)) - std::cout << "Hint: " << str << std::endl; - - std::cout << "Enter Password: "; - GetPassword(str); - - if (!m_container.GetVolumeKey(vek, m_sb.apfs_vol_uuid, str.c_str())) - { - std::cout << "Wrong password!" << std::endl; - return false; - } - } - - m_aes.SetKey(vek, vek + 0x10); - m_is_encrypted = true; - } + if (!InitVolumeKey()) + return false; if (!m_fs_tree.Init(m_sb.apfs_root_tree_oid, m_sb.apfs_o.o_xid, &m_omap)) std::cerr << "WARNING: root tree init failed" << std::endl; @@ -214,6 +168,44 @@ bool ApfsVolume::MountSnapshot(paddr_t apsb_paddr, xid_t snap_xid) return true; } +bool ApfsVolume::InitVolumeKey() +{ + // Software encryption in APFS is per-volume. There is nothing to do if + // this volume is not encrypted, or if the container keybag itself is + // stored unencrypted (in which case no volume key can be derived). + if ((m_sb.apfs_fs_flags & 3) == APFS_FS_UNENCRYPTED || m_container.IsUnencrypted()) + return true; + + uint8_t vek[0x20]; + std::string str; + + std::cout << "Volume " << m_sb.apfs_volname << " is encrypted." << std::endl; + + if (!m_container.GetVolumeKey(vek, m_sb.apfs_vol_uuid)) + { + if (m_container.GetPasswordHint(str, m_sb.apfs_vol_uuid)) + std::cout << "Hint: " << str << std::endl; + + std::cout << "Enter Password: "; + GetPassword(str); + + if (!m_container.GetVolumeKey(vek, m_sb.apfs_vol_uuid, str.c_str())) + { + std::cout << "Wrong password!" << std::endl; + return false; + } + } + + m_aes.SetKey(vek, vek + 0x10); + m_is_encrypted = true; + + // The key has been copied into the AES round keys; don't keep a second + // plaintext copy on the stack. + memset(vek, 0, sizeof(vek)); + + return true; +} + void ApfsVolume::dump(BlockDumper& bd) { std::vector blk; diff --git a/ApfsLib/ApfsVolume.h b/ApfsLib/ApfsVolume.h index a9a8448..f4eab87 100644 --- a/ApfsLib/ApfsVolume.h +++ b/ApfsLib/ApfsVolume.h @@ -54,6 +54,11 @@ class ApfsVolume private: static int CompareSnapMetaKey(const void *skey, size_t skey_len, const void *ekey, size_t ekey_len, void *context); + // Detects whether this volume is encrypted and, if so, obtains the + // volume encryption key (prompting for a password if needed) and + // initialises m_aes. Returns false only on a wrong password. + bool InitVolumeKey(); + ApfsContainer &m_container; apfs_superblock_t m_sb; diff --git a/ApfsLib/KeyMgmt.cpp b/ApfsLib/KeyMgmt.cpp index b2af762..dd37bf9 100644 --- a/ApfsLib/KeyMgmt.cpp +++ b/ApfsLib/KeyMgmt.cpp @@ -1,3 +1,4 @@ +#include #include #include #include @@ -59,14 +60,24 @@ Keybag::~Keybag() bool Keybag::Init(const media_keybag_t * mk, size_t size) { - (void)size; + if (size < sizeof(media_keybag_t)) + return false; if (mk->mk_locker.kl_version != 2) return false; + // kl_nbytes is read from disk and cannot be trusted: it must fit inside + // the buffer that was actually read, and be large enough to contain the + // locker header itself. Otherwise the assign() below reads out of bounds. + const size_t avail = size - offsetof(media_keybag_t, mk_locker); + const uint32_t nbytes = mk->mk_locker.kl_nbytes; + + if (nbytes < sizeof(kb_locker_t) || nbytes > avail) + return false; + const uint8_t *data = reinterpret_cast(&mk->mk_locker); - m_data.assign(data, data + mk->mk_locker.kl_nbytes); + m_data.assign(data, data + nbytes); m_kl = reinterpret_cast(m_data.data()); return true; @@ -88,47 +99,49 @@ const keybag_entry_t * Keybag::GetKey(size_t nr) if (nr >= m_kl->kl_nkeys) return nullptr; - const uint8_t *ptr; + const uint8_t *ptr = m_kl->kl_entries; + const uint8_t *end = m_data.data() + m_data.size(); const keybag_entry_t *kb; size_t len; size_t k; - ptr = m_kl->kl_entries; - - for (k = 0; k < nr; k++) + // Walk the variable-length entry list, checking at every step that both + // the fixed entry header and its key data stay inside the buffer. A + // corrupt keybag must not be able to make us read past m_data. + for (k = 0; k <= nr; k++) { + if (static_cast(end - ptr) < sizeof(keybag_entry_t)) + return nullptr; + kb = reinterpret_cast(ptr); - len = (kb->ke_keylen + sizeof(keybag_entry_t) + 0x0F) & ~0xF; + len = (kb->ke_keylen + sizeof(keybag_entry_t) + 0x0F) & ~static_cast(0xF); + if (len > static_cast(end - ptr)) + return nullptr; + + if (k == nr) + return kb; + ptr += len; } - kb = reinterpret_cast(ptr); - - return kb; + return nullptr; } const keybag_entry_t * Keybag::FindKey(const apfs_uuid_t & uuid, uint16_t type) { - if (!m_kl) - return nullptr; + const size_t cnt = GetKeyCnt(); - const uint8_t *ptr; - const keybag_entry_t *kb; - size_t len; - size_t k; - - ptr = m_kl->kl_entries; - - for (k = 0; k < m_kl->kl_nkeys; k++) + // GetKey() performs all bounds checking; reuse it so the lookup can't + // walk off the end of a malformed keybag. + for (size_t k = 0; k < cnt; k++) { - kb = reinterpret_cast(ptr); + const keybag_entry_t *kb = GetKey(k); + if (!kb) + break; if (memcmp(uuid, kb->ke_uuid, sizeof(apfs_uuid_t)) == 0 && kb->ke_tag == type) return kb; - - len = (kb->ke_keylen + sizeof(keybag_entry_t) + 0x0F) & ~0xF; - ptr += len; } return nullptr; @@ -145,7 +158,6 @@ void Keybag::dump(std::ostream &st, Keybag *cbag, const apfs_uuid_t &vuuid) size_t s; size_t k; const keybag_entry_t *ke; - bagdata_t bd; const char *typestr; st << "Dumping Keybag (" << (cbag ? "recs" : "keys") << ")" << endl; @@ -395,7 +407,7 @@ bool KeyManager::Init(uint64_t block, uint64_t blockcnt, const apfs_uuid_t& cont { bool rc; - rc = LoadKeybag(m_container_bag, 0x6B657973, block, blockcnt, container_uuid); + rc = LoadKeybag(m_container_bag, APFS_KEYBAG_OBJ, block, blockcnt, container_uuid); if (rc) memcpy(m_container_uuid, container_uuid, sizeof(apfs_uuid_t)); else @@ -435,6 +447,11 @@ bool KeyManager::GetVolumeKey(uint8_t* vek, const apfs_uuid_t& volume_uuid, cons { const keybag_entry_t *ke_recs; + // The parameter defaults to nullptr; treat that as an empty passphrase + // so strlen() below can't be called on a null pointer. + if (password == nullptr) + password = ""; + if (g_debug & Dbg_Crypto) { std::cout.setf(std::ios::hex | std::ios::uppercase); @@ -466,7 +483,6 @@ bool KeyManager::GetVolumeKey(uint8_t* vek, const apfs_uuid_t& volume_uuid, cons bool rc = false; const keybag_entry_t *ke_kek; const keybag_entry_t *ke_vek; - bagdata_t bd; kek_entry_t keke; vek_entry_t veke; AES::Mode kek_mode = AES::AES_256; @@ -518,48 +534,55 @@ bool KeyManager::GetVolumeKey(uint8_t* vek, const apfs_uuid_t& volume_uuid, cons { if (g_debug & Dbg_Crypto) std::cout << "Password doesn't work for any key." << std::endl; + memset(dk, 0, sizeof(dk)); + memset(kek, 0, sizeof(kek)); return false; } + rc = false; + ke_vek = m_container_bag.FindKey(volume_uuid, KB_TAG_VOLUME_KEY); - if (!ke_vek) - return false; - if (!DecodeVEK(veke, ke_vek->ke_keydata, ke_vek->ke_keydata + ke_vek->ke_keylen)) - return false; + if (ke_vek && DecodeVEK(veke, ke_vek->ke_keydata, ke_vek->ke_keydata + ke_vek->ke_keylen)) + { + memset(vek, 0, 0x20); - memset(vek, 0, 0x20); + // TODO 1 + if (veke.hdr.info.flags & 2) { + // AES-128. This method is used for FileVault and CoreStorage encrypted + // volumes that have been converted to APFS. + rc = Rfc3394_KeyUnwrap(vek, veke.wrapped_vek, 0x10, kek, kek_mode, &iv); - // TODO 1 - if (veke.hdr.info.flags & 2) { - // AES-128. This method is used for FileVault and CoreStorage encrypted - // volumes that have been converted to APFS. - rc = Rfc3394_KeyUnwrap(vek, veke.wrapped_vek, 0x10, kek, kek_mode, &iv); + if (rc) + { + SHA256 sha; + uint8_t sha_result[0x20]; + sha.Init(); + + // Use (VEK || vek_blob.uuid), then SHA256, then take the first 16 bytes + sha.Update(vek, 0x10); + sha.Update(veke.hdr.uuid, 0x10); + sha.Final(sha_result); + memcpy(vek + 0x10, sha_result, 0x10); + } + } else { + // AES-256. This method is used for wrapping the whole XTS-AES key, + // and applies to non-FileVault encrypted APFS volumes. + rc = Rfc3394_KeyUnwrap(vek, veke.wrapped_vek, 0x20, kek, kek_mode, &iv); + } - if (rc) + if (g_debug & Dbg_Crypto) { - SHA256 sha; - uint8_t sha_result[0x20]; - sha.Init(); - - // Use (VEK || vek_blob.uuid), then SHA256, then take the first 16 bytes - sha.Update(vek, 0x10); - sha.Update(veke.hdr.uuid, 0x10); - sha.Final(sha_result); - memcpy(vek + 0x10, sha_result, 0x10); + std::cout << "VEK Wrpd: " << hexstr(veke.wrapped_vek, 0x28) << std::endl; + std::cout << "VEK : " << hexstr(vek, 0x20) << std::endl; + std::cout << "VEK IV : " << std::setw(16) << iv << std::endl; } - } else { - // AES-256. This method is used for wrapping the whole XTS-AES key, - // and applies to non-FileVault encrypted APFS volumes. - rc = Rfc3394_KeyUnwrap(vek, veke.wrapped_vek, 0x20, kek, kek_mode, &iv); } - if (g_debug & Dbg_Crypto) - { - std::cout << "VEK Wrpd: " << hexstr(veke.wrapped_vek, 0x28) << std::endl; - std::cout << "VEK : " << hexstr(vek, 0x20) << std::endl; - std::cout << "VEK IV : " << std::setw(16) << iv << std::endl; - } + // dk (PBKDF2 output) and kek are key material derived from the user's + // password. Don't leave them lying around on the stack after use. + memset(dk, 0, sizeof(dk)); + memset(kek, 0, sizeof(kek)); return rc; } @@ -609,6 +632,10 @@ void KeyManager::dump(std::ostream &st) bool KeyManager::LoadKeybag(Keybag& bag, uint32_t type, uint64_t block, uint64_t blockcnt, const apfs_uuid_t& uuid) { + // A keybag only ever spans a handful of blocks. Reject an absurd block + // count from a corrupt prange before it becomes a huge allocation. + constexpr uint64_t max_keybag_blocks = 64; + std::vector data; size_t k; const size_t blocksize = m_container.GetBlocksize(); @@ -616,18 +643,36 @@ bool KeyManager::LoadKeybag(Keybag& bag, uint32_t type, uint64_t block, uint64_t if (g_debug & Dbg_Crypto) std::cout << "starting LoadKeybag @ " << std::hex << block << std::endl; + if (blockcnt == 0 || blockcnt > max_keybag_blocks) + { + if (g_debug & Dbg_Errors) + std::cout << "Keybag: invalid block count " << blockcnt << std::endl; + return false; + } + data.resize(blockcnt * blocksize); const media_keybag_t *mk = reinterpret_cast(data.data()); - m_container.ReadBlocks(data.data(), block, blockcnt); + if (!m_container.ReadBlocks(data.data(), block, blockcnt)) + { + if (g_debug & Dbg_Errors) + std::cout << "Keybag: failed to read blocks at " << std::hex << block << std::endl; + return false; + } + if (mk->mk_obj.o_type == type) - m_is_unencrypted = true; + { + // Only the container keybag tells us whether the container is stored + // unencrypted; a per-volume ('recs') keybag must not set this flag. + if (type == APFS_KEYBAG_OBJ) + m_is_unencrypted = true; + } else DecryptBlocks(data.data(), block, blockcnt, uuid); for (k = 0; k < blockcnt; k++) { - if (!VerifyBlock(data.data(), blockcnt * blocksize)) + if (!VerifyBlock(data.data() + k * blocksize, blocksize)) return false; } diff --git a/Crypto/Asn1Der.cpp b/Crypto/Asn1Der.cpp index a66a55e..12f3a85 100644 --- a/Crypto/Asn1Der.cpp +++ b/Crypto/Asn1Der.cpp @@ -46,7 +46,9 @@ const uint8_t* der_decode_len(size_t& len, const uint8_t* der, const uint8_t* de if (nb & 0x80) { s = 0; nb &= 0x7F; - if ((der + nb) >= der_end) return nullptr; + // Reject if the length octets don't fit, without forming an + // out-of-bounds pointer (der + nb may overflow past der_end). + if (nb > static_cast(der_end - der)) return nullptr; for (k = 0; k < nb; k++) s = s << 8 | *der++; len = s; @@ -64,7 +66,11 @@ const uint8_t* der_decode_tl(der_tag_t expected_tag, size_t& len, const uint8_t* if (tag != expected_tag) return nullptr; der = der_decode_len(len, der, der_end); - if (der + len > der_end) + if (der == nullptr) + return nullptr; + // Compare as sizes rather than pointers so an out-of-range len can't + // produce an out-of-bounds pointer (der + len) and undefined behaviour. + if (len > static_cast(der_end - der)) return nullptr; return der; } diff --git a/Crypto/Crypto.cpp b/Crypto/Crypto.cpp index c429779..bad5a60 100644 --- a/Crypto/Crypto.cpp +++ b/Crypto/Crypto.cpp @@ -185,26 +185,20 @@ void HMAC_SHA256(const uint8_t *key, size_t key_len, const uint8_t *data, size_t memset(digest, 0, sizeof(digest)); } -void PBKDF2_HMAC_SHA1(const uint8_t* pw, size_t pw_len, const uint8_t* salt, size_t salt_len, int iterations, uint8_t* derived_key, size_t dk_len) +void PBKDF2_HMAC_SHA1(const uint8_t* pw, size_t pw_len, const uint8_t* salt, size_t salt_len, uint32_t iterations, uint8_t* derived_key, size_t dk_len) { assert(salt_len <= 0x20); assert(dk_len <= 0x20); constexpr size_t h_len = 0x14; - size_t r; - size_t l; uint8_t t[h_len]; uint8_t u[h_len]; uint8_t s[0x24]; size_t k; - int j; + uint32_t j; uint32_t i; size_t n; - r = dk_len % h_len; - l = dk_len / h_len; - if (r > 0) l++; - for (i = 1, k = 0; k < dk_len; i++, k += h_len) { // F(P,S,c,i) @@ -230,7 +224,7 @@ void PBKDF2_HMAC_SHA1(const uint8_t* pw, size_t pw_len, const uint8_t* salt, siz } } -void PBKDF2_HMAC_SHA256(const uint8_t* pw, size_t pw_len, const uint8_t* salt, size_t salt_len, int iterations, uint8_t* derived_key, size_t dk_len) +void PBKDF2_HMAC_SHA256(const uint8_t* pw, size_t pw_len, const uint8_t* salt, size_t salt_len, uint32_t iterations, uint8_t* derived_key, size_t dk_len) { // HMAC_SHA256(pw, key_len, salt, salt_len, mac) @@ -247,20 +241,14 @@ void PBKDF2_HMAC_SHA256(const uint8_t* pw, size_t pw_len, const uint8_t* salt, s assert(dk_len <= 0x20); constexpr size_t h_len = 0x20; - size_t r; - size_t l; uint8_t t[h_len]; uint8_t u[h_len]; uint8_t s[0x14]; size_t k; - int j; + uint32_t j; uint32_t i; size_t n; - r = dk_len % h_len; - l = dk_len / h_len; - if (r > 0) l++; - for (i = 1, k = 0; k < dk_len; i++, k += h_len) { // F(P,S,c,i) diff --git a/Crypto/Crypto.h b/Crypto/Crypto.h index ed166b4..2947b69 100644 --- a/Crypto/Crypto.h +++ b/Crypto/Crypto.h @@ -8,5 +8,5 @@ void Rfc3394_KeyWrap(uint8_t *crypto, const uint8_t *plain, size_t size, const u bool Rfc3394_KeyUnwrap(uint8_t *plain, const uint8_t *crypto, size_t size, const uint8_t *key, AES::Mode aes_mode, uint64_t *iv); void HMAC_SHA1(const uint8_t *key, size_t key_len, const uint8_t *data, size_t data_len, uint8_t *mac); void HMAC_SHA256(const uint8_t *key, size_t key_len, const uint8_t *data, size_t data_len, uint8_t *mac); -void PBKDF2_HMAC_SHA1(const uint8_t* pw, size_t pw_len, const uint8_t* salt, size_t salt_len, int iterations, uint8_t* derived_key, size_t dk_len); -void PBKDF2_HMAC_SHA256(const uint8_t* pw, size_t pw_len, const uint8_t* salt, size_t salt_len, int iterations, uint8_t* derived_key, size_t dk_len); +void PBKDF2_HMAC_SHA1(const uint8_t* pw, size_t pw_len, const uint8_t* salt, size_t salt_len, uint32_t iterations, uint8_t* derived_key, size_t dk_len); +void PBKDF2_HMAC_SHA256(const uint8_t* pw, size_t pw_len, const uint8_t* salt, size_t salt_len, uint32_t iterations, uint8_t* derived_key, size_t dk_len); From 6ee2b10a49d5e9483df8ccaf427eb143cb69ffbe Mon Sep 17 00:00:00 2001 From: Yevgen Trotsan Date: Thu, 21 May 2026 20:23:38 +0100 Subject: [PATCH 2/2] fix(build): add missing include to PList.h 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 explicitly. Co-Authored-By: Claude Opus 4.7 (1M context) --- ApfsLib/PList.h | 1 + 1 file changed, 1 insertion(+) diff --git a/ApfsLib/PList.h b/ApfsLib/PList.h index 645df2d..a752059 100644 --- a/ApfsLib/PList.h +++ b/ApfsLib/PList.h @@ -1,5 +1,6 @@ #pragma once +#include #include #include #include