From 1f0756529604f5dfb3a50907976014def81ea006 Mon Sep 17 00:00:00 2001 From: Andreas Stefl Date: Sun, 4 Oct 2026 22:41:56 +0200 Subject: [PATCH 1/2] fix: validate cipher blocks and derivation parameters --- CHANGELOG.md | 3 + src/odr/internal/crypto/crypto_argon2.cpp | 6 +- src/odr/internal/crypto/crypto_util.cpp | 62 +++++++++---------- test/src/internal/crypto/crypto_util_test.cpp | 32 ++++++++++ 4 files changed, 69 insertions(+), 34 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 03f75df6c..9eaa0e86d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,9 @@ The release run heads these entries with the version and opens a fresh ## Unreleased +- Block-cipher helpers reject incomplete blocks before calling Crypto++, and + password derivation rejects parameters that would be narrowed or truncated. + - CFB streams retain their entry metadata after the file wrapper is released. Nested directory walks finish children before resuming outer siblings. diff --git a/src/odr/internal/crypto/crypto_argon2.cpp b/src/odr/internal/crypto/crypto_argon2.cpp index 96cce03c5..0e2f457fd 100644 --- a/src/odr/internal/crypto/crypto_argon2.cpp +++ b/src/odr/internal/crypto/crypto_argon2.cpp @@ -218,8 +218,10 @@ std::string argon2::id(const std::size_t tag_size, if (tag_size < 4 || tag_size > max_uint32) { throw std::invalid_argument("argon2id: tag size out of range"); } - if (salt.size() < 8) { - throw std::invalid_argument("argon2id: salt too short"); + if (salt.size() < 8 || salt.size() > max_uint32 || + password.size() > max_uint32) { + throw std::invalid_argument( + "argon2id: password or salt length out of range"); } if (iterations < 1 || iterations > max_uint32) { throw std::invalid_argument("argon2id: iterations out of range"); diff --git a/src/odr/internal/crypto/crypto_util.cpp b/src/odr/internal/crypto/crypto_util.cpp index 6b4b4493d..646d1b33c 100644 --- a/src/odr/internal/crypto/crypto_util.cpp +++ b/src/odr/internal/crypto/crypto_util.cpp @@ -5,6 +5,7 @@ #include #include #include +#include #include // RC4 and MD5 are retained in Crypto++ only for legacy formats (here: the PDF @@ -31,6 +32,22 @@ namespace odr::internal::crypto { using byte = std::uint8_t; +namespace { + +std::string process(CryptoPP::StreamTransformation &cipher, + const std::string_view input) { + if (input.size() % cipher.MandatoryBlockSize() != 0) { + throw std::invalid_argument("cipher input is not a whole number of blocks"); + } + std::string result(input.size(), '\0'); + cipher.ProcessData(reinterpret_cast(result.data()), + reinterpret_cast(input.data()), + input.size()); + return result; +} + +} // namespace + std::string util::base64_encode(const std::string_view in) { std::string out; CryptoPP::Base64Encoder b(new CryptoPP::StringSink(out), false); @@ -119,25 +136,26 @@ std::string util::sha512(const std::string_view in) { std::string util::rc4(const std::string_view key, const std::string_view input) { - std::string result(input.size(), '\0'); CryptoPP::Weak::ARC4 rc4(reinterpret_cast(key.data()), key.size()); - rc4.ProcessData(reinterpret_cast(result.data()), - reinterpret_cast(input.data()), input.size()); - return result; + return process(rc4, input); } std::string util::pbkdf2(const std::size_t key_size, const std::string_view start_key, const std::string_view salt, const std::size_t iteration_count) { + if (iteration_count == 0 || + iteration_count > std::numeric_limits::max()) { + throw std::invalid_argument("PBKDF2 iteration count out of range"); + } std::string result(key_size, '\0'); const CryptoPP::PKCS5_PBKDF2_HMAC pbkdf2; pbkdf2.DeriveKey(reinterpret_cast(result.data()), result.size(), false, reinterpret_cast(start_key.data()), start_key.size(), reinterpret_cast(salt.data()), salt.size(), - iteration_count); + static_cast(iteration_count)); return result; } @@ -151,41 +169,29 @@ std::string util::argon2id(const std::size_t key_size, std::string util::decrypt_aes_ecb(const std::string_view key, const std::string_view input) { - std::string result(input.size(), '\0'); CryptoPP::ECB_Mode::Decryption decryption; decryption.SetKey(reinterpret_cast(key.data()), key.size()); - decryption.ProcessData(reinterpret_cast(result.data()), - reinterpret_cast(input.data()), - input.size()); - return result; + return process(decryption, input); } std::string util::decrypt_aes_cbc(const std::string_view key, const std::string_view iv, const std::string_view input) { - std::string result(input.size(), '\0'); CryptoPP::CBC_Mode::Decryption decryption; decryption.SetKeyWithIV(reinterpret_cast(key.data()), key.size(), reinterpret_cast(iv.data()), iv.size()); - decryption.ProcessData(reinterpret_cast(result.data()), - reinterpret_cast(input.data()), - input.size()); - return result; + return process(decryption, input); } std::string util::encrypt_aes_cbc(const std::string_view key, const std::string_view iv, const std::string_view input) { - std::string result(input.size(), '\0'); CryptoPP::CBC_Mode::Encryption encryption; encryption.SetKeyWithIV(reinterpret_cast(key.data()), key.size(), reinterpret_cast(iv.data()), iv.size()); - encryption.ProcessData(reinterpret_cast(result.data()), - reinterpret_cast(input.data()), - input.size()); - return result; + return process(encryption, input); } std::string util::decrypt_aes_gcm(const std::string_view key, @@ -231,29 +237,21 @@ std::string util::decrypt_aes_gcm(const std::string_view key, std::string util::decrypt_triple_des(const std::string_view key, const std::string_view iv, const std::string_view input) { - std::string result(input.size(), '\0'); CryptoPP::CBC_Mode::Decryption decryption; decryption.SetKeyWithIV(reinterpret_cast(key.data()), key.size(), reinterpret_cast(iv.data()), iv.size()); - decryption.ProcessData(reinterpret_cast(result.data()), - reinterpret_cast(input.data()), - input.size()); - return result; + return process(decryption, input); } std::string util::decrypt_blowfish(const std::string_view key, const std::string_view iv, const std::string_view input) { - std::string result(input.size(), '\0'); CryptoPP::CFB_Mode::Decryption decryption; decryption.SetKeyWithIV(reinterpret_cast(key.data()), key.size(), reinterpret_cast(iv.data()), iv.size()); - decryption.ProcessData(reinterpret_cast(result.data()), - reinterpret_cast(input.data()), - input.size()); - return result; + return process(decryption, input); } namespace { @@ -263,7 +261,7 @@ class MyInflator final : public CryptoPP::Inflator { explicit MyInflator(BufferedTransformation *attachment = nullptr) : Inflator(attachment, false, -1) {} - std::uint32_t GetPadding() const { return m_padding; } + std::size_t GetPadding() const { return m_padding; } protected: void ProcessPoststreamTail() override { @@ -272,7 +270,7 @@ class MyInflator final : public CryptoPP::Inflator { } private: - std::uint32_t m_padding{0}; + std::size_t m_padding{0}; }; } // namespace diff --git a/test/src/internal/crypto/crypto_util_test.cpp b/test/src/internal/crypto/crypto_util_test.cpp index f757bcbeb..778678459 100644 --- a/test/src/internal/crypto/crypto_util_test.cpp +++ b/test/src/internal/crypto/crypto_util_test.cpp @@ -2,12 +2,44 @@ #include +#include #include +#include #include #include using namespace odr::internal::crypto::util; +TEST(CryptoUtil, block_ciphers_reject_partial_blocks) { + const std::string key(16, '\0'); + const std::string iv(16, '\0'); + for (const std::size_t size : std::array{1, 15, 17}) { + const std::string input(size, '\0'); + EXPECT_THROW(decrypt_aes_ecb(key, input), std::invalid_argument); + EXPECT_THROW(decrypt_aes_cbc(key, iv, input), std::invalid_argument); + EXPECT_THROW(encrypt_aes_cbc(key, iv, input), std::invalid_argument); + EXPECT_THROW( + decrypt_triple_des(std::string(24, '\0'), std::string(8, '\0'), input), + std::invalid_argument); + } + const std::string cipher = hex_decode("66e94bd4ef8a2c3b884cfa59ca342b2e"); + EXPECT_EQ(encrypt_aes_cbc(key, iv, std::string(16, '\0')), cipher); + EXPECT_EQ(decrypt_aes_cbc(key, iv, cipher), std::string(16, '\0')); + EXPECT_EQ(decrypt_aes_ecb(key, cipher), std::string(16, '\0')); +} + +TEST(CryptoUtil, pbkdf2_rejects_narrowed_iteration_counts) { + EXPECT_THROW(pbkdf2(16, "password", "salt", 0), std::invalid_argument); + if constexpr (sizeof(std::size_t) > sizeof(unsigned int)) { + EXPECT_THROW( + pbkdf2(16, "password", "salt", + std::size_t{std::numeric_limits::max()} + 1), + std::invalid_argument); + } + EXPECT_EQ(hex_encode(pbkdf2(20, "password", "salt", 1)), + "0c60c80f961f0e71f3a9b524af6012062fe037a6"); +} + // RFC 1321, appendix A.5. TEST(CryptoUtil, md5) { EXPECT_EQ(hex_encode(md5("")), "d41d8cd98f00b204e9800998ecf8427e"); From 44e177f25bb1f7965073d6d21a70e2f2b7d2f930 Mon Sep 17 00:00:00 2001 From: Andreas Stefl Date: Sun, 4 Oct 2026 23:18:23 +0200 Subject: [PATCH 2/2] fix(pdf): decrypt the whole AES blocks of a damaged string or stream The shared block check now throws for a partial last block. In a PDF, that error is not caught per object, so one damaged string stopped the whole document. Before, Crypto++ decrypted the whole blocks and left the partial block as zero bytes. The decryptor now passes only the whole blocks after the IV, so the out-of-bounds read stays fixed and the whole blocks still decrypt. The AES-V2 and AES-V3 paths share the helper. A crypto test also confirms that Blowfish CFB, a stream mode, accepts any length, because an ODF Blowfish stream has no block alignment. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01Gb1fLafqqzehpqPuU6uBfn --- src/odr/internal/pdf/pdf_encryption.cpp | 25 +++++++++++-------- test/src/internal/crypto/crypto_util_test.cpp | 4 +++ test/src/internal/pdf/pdf_encryption.cpp | 14 +++++++++++ 3 files changed, 32 insertions(+), 11 deletions(-) diff --git a/src/odr/internal/pdf/pdf_encryption.cpp b/src/odr/internal/pdf/pdf_encryption.cpp index f6c92f200..8acadce74 100644 --- a/src/odr/internal/pdf/pdf_encryption.cpp +++ b/src/odr/internal/pdf/pdf_encryption.cpp @@ -61,6 +61,17 @@ std::string strip_pkcs7(std::string data) { return data; } +/// AES-CBC data with its IV in front (7.6.3). A trailing partial block of a +/// damaged file is dropped, so the whole blocks still decrypt. +std::string decrypt_aes(const std::string &key, const std::string &data) { + if (data.size() < aes_block) { + return data; + } + const std::size_t whole = (data.size() - aes_block) / aes_block * aes_block; + return strip_pkcs7(crypto::util::decrypt_aes_cbc( + key, data.substr(0, aes_block), data.substr(aes_block, whole))); +} + } // namespace // Standard-security algorithms with no callers outside this file (the rest are @@ -363,21 +374,13 @@ std::string Decryptor::decrypt(const ObjectReference &reference, } if (method == EncryptionMethod::aes_v3) { - // V 5: the file key is used directly; first 16 bytes are the IV. - if (data.size() < aes_block) { - return data; - } - return strip_pkcs7(crypto::util::decrypt_aes_cbc( - m_key, data.substr(0, aes_block), data.substr(aes_block))); + // V 5: the file key is used directly. + return decrypt_aes(m_key, data); } const std::string key = object_key(reference, method); if (method == EncryptionMethod::aes_v2) { - if (data.size() < aes_block) { - return data; - } - return strip_pkcs7(crypto::util::decrypt_aes_cbc( - key, data.substr(0, aes_block), data.substr(aes_block))); + return decrypt_aes(key, data); } return crypto::util::rc4(key, data); // Method::rc4 diff --git a/test/src/internal/crypto/crypto_util_test.cpp b/test/src/internal/crypto/crypto_util_test.cpp index 778678459..7e140afee 100644 --- a/test/src/internal/crypto/crypto_util_test.cpp +++ b/test/src/internal/crypto/crypto_util_test.cpp @@ -22,6 +22,10 @@ TEST(CryptoUtil, block_ciphers_reject_partial_blocks) { decrypt_triple_des(std::string(24, '\0'), std::string(8, '\0'), input), std::invalid_argument); } + // CFB is a stream mode: an ODF Blowfish stream has any length. + EXPECT_EQ( + decrypt_blowfish(key, std::string(8, '\0'), std::string(13, '\0')).size(), + 13); const std::string cipher = hex_decode("66e94bd4ef8a2c3b884cfa59ca342b2e"); EXPECT_EQ(encrypt_aes_cbc(key, iv, std::string(16, '\0')), cipher); EXPECT_EQ(decrypt_aes_cbc(key, iv, cipher), std::string(16, '\0')); diff --git a/test/src/internal/pdf/pdf_encryption.cpp b/test/src/internal/pdf/pdf_encryption.cpp index f2a292338..3bdc2861b 100644 --- a/test/src/internal/pdf/pdf_encryption.cpp +++ b/test/src/internal/pdf/pdf_encryption.cpp @@ -159,6 +159,20 @@ TEST(PdfEncryption, qpdf_aes_128_r4) { EXPECT_NE(content.find(kMarker), std::string::npos); } +// A damaged file can end an AES string inside a block; the whole blocks before +// it still decrypt. +TEST(PdfEncryption, aes_drops_a_trailing_partial_block) { + const std::string key(32, '\0'); + const std::string iv(16, '\0'); + const std::string plaintext = "0123456789abcde"; + const std::string data = + iv + encrypt_aes_cbc(key, iv, plaintext + '\x01') + "xyz"; + const Decryptor decryptor(key, EncryptionMethod::aes_v3, + EncryptionMethod::aes_v3); + EXPECT_EQ(decryptor.decrypt_string(ObjectReference(1, 0), data), plaintext); + EXPECT_EQ(decryptor.decrypt_string(ObjectReference(1, 0), iv + "xyz"), ""); +} + // EncryptMetadata false adds the 0xFFFFFFFF salt to the key derivation // (Algorithm 2, step f). TEST(PdfEncryption, qpdf_aes_128_r4_cleartext_metadata) {