From a4c6b045a10b3ba2a4a3fdf35861f8353a406de1 Mon Sep 17 00:00:00 2001 From: Andreas Stefl Date: Mon, 5 Oct 2026 06:40:34 +0200 Subject: [PATCH 1/2] fix(pdf): validate the complete AES padding trailer --- CHANGELOG.md | 3 +++ src/odr/internal/pdf/pdf_encryption.cpp | 21 ++++++++++++--------- test/src/internal/pdf/pdf_encryption.cpp | 21 +++++++++++++++++++++ 3 files changed, 36 insertions(+), 9 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c37deb18a..e26f3f515 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 +- PDF AES decryption validates every padding byte before removing a trailer, + preserving data with malformed padding. + - `ODR_CLANG_TIDY=ON` runs clang-tidy during C++ compilation and fails configuration if the executable is unavailable. diff --git a/src/odr/internal/pdf/pdf_encryption.cpp b/src/odr/internal/pdf/pdf_encryption.cpp index f8b516318..21d0f419d 100644 --- a/src/odr/internal/pdf/pdf_encryption.cpp +++ b/src/odr/internal/pdf/pdf_encryption.cpp @@ -45,14 +45,16 @@ std::string pad_password(const std::string &password) { return pw; } -/// Strip PKCS#7 padding (1-16 bytes) from decrypted AES data; leave the input -/// untouched if the trailer is not valid padding (lenient — real files vary). +/// Strip valid AES padding (ISO 32000-1 7.6.2); preserve an invalid trailer. std::string strip_pkcs7(std::string data) { if (data.empty()) { return data; } const auto n = static_cast(data.back()); - if (n >= 1 && n <= aes_block && n <= data.size()) { + if (n >= 1 && n <= aes_block && n <= data.size() && + std::ranges::all_of( + std::string_view(data).substr(data.size() - n), + [n](const char c) { return static_cast(c) == n; })) { data.resize(data.size() - n); } return data; @@ -84,7 +86,7 @@ std::string recover_user_password(const std::string &owner_password, // /O to recover the user password. std::string hash = crypto::util::md5(pad_password(owner_password)); if (r >= 3) { - for (int i = 0; i < 50; ++i) { + for (std::uint32_t i = 0; i < 50; ++i) { hash = crypto::util::md5(hash.substr(0, key_length)); } } @@ -94,7 +96,7 @@ std::string recover_user_password(const std::string &owner_password, if (r == 2) { return crypto::util::rc4(rc4_key, user); } - for (int i = 19; i >= 0; --i) { + for (std::int32_t i = 19; i >= 0; --i) { user = crypto::util::rc4(xor_key(rc4_key, static_cast(i)), user); } @@ -110,7 +112,7 @@ std::string hash_r6(const std::string &password, const std::string &salt, // rounds; stop once the last byte of E is small enough. std::string k = crypto::util::sha256(password + salt + udata); std::string e; - for (int round = 0; + for (std::uint32_t round = 0; round < 64 || static_cast(e.back()) > round - 32; ++round) { std::string block; @@ -120,12 +122,13 @@ std::string hash_r6(const std::string &password, const std::string &salt, block += udata; std::string k1; k1.reserve(block.size() * 64); - for (int i = 0; i < 64; ++i) { + for (std::uint32_t i = 0; i < 64; ++i) { k1 += block; } e = crypto::util::encrypt_aes_cbc(k.substr(0, 16), k.substr(16, 16), k1); - int mod = 0; // the first 16 bytes of E as a big-endian integer, mod 3 + std::uint32_t mod = + 0; // the first 16 bytes of E as a big-endian integer, mod 3 for (std::size_t i = 0; i < 16; ++i) { mod = (mod * 256 + static_cast(e[i])) % 3; } @@ -155,7 +158,7 @@ std::string standard_security::compute_key_r2_r4( } std::string hash = crypto::util::md5(input); if (r >= 3) { - for (int i = 0; i < 50; ++i) { + for (std::uint32_t i = 0; i < 50; ++i) { hash = crypto::util::md5(hash.substr(0, key_length)); } } diff --git a/test/src/internal/pdf/pdf_encryption.cpp b/test/src/internal/pdf/pdf_encryption.cpp index 3bdc2861b..fff1673b3 100644 --- a/test/src/internal/pdf/pdf_encryption.cpp +++ b/test/src/internal/pdf/pdf_encryption.cpp @@ -224,3 +224,24 @@ TEST(PdfEncryption, qpdf_aes_128_r4_owner_password) { EXPECT_NE(decryptor_owner->decrypt_stream(kContentRef, stream).find(kMarker), std::string::npos); } + +TEST(PdfEncryption, aes_padding_requires_every_trailer_byte) { + const std::string key(32, 'k'); + const std::string iv(16, 'i'); + const Decryptor decryptor(key, EncryptionMethod::aes_v3, + EncryptionMethod::aes_v3); + for (std::uint8_t length = 1; length <= 16; ++length) { + const std::string message(16 - length, 'x'); + std::string padded = + message + std::string(length, static_cast(length)); + const auto decode = [&] { + return decryptor.decrypt_stream({1, 0}, + iv + encrypt_aes_cbc(key, iv, padded)); + }; + EXPECT_EQ(decode(), message); + if (length > 1) { + padded[message.size()] = 'x'; + EXPECT_EQ(decode(), padded); + } + } +} From ab3e889cfa8b535e590db310447cbf92ac0d4c97 Mon Sep 17 00:00:00 2001 From: Andreas Stefl Date: Tue, 6 Oct 2026 16:04:26 +0200 Subject: [PATCH 2/2] refactor(pdf): keep the mod comment above its declaration Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01MxyTMutqSUJRGfxA8CyzMc --- src/odr/internal/pdf/pdf_encryption.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/odr/internal/pdf/pdf_encryption.cpp b/src/odr/internal/pdf/pdf_encryption.cpp index 21d0f419d..834604ed2 100644 --- a/src/odr/internal/pdf/pdf_encryption.cpp +++ b/src/odr/internal/pdf/pdf_encryption.cpp @@ -127,8 +127,8 @@ std::string hash_r6(const std::string &password, const std::string &salt, } e = crypto::util::encrypt_aes_cbc(k.substr(0, 16), k.substr(16, 16), k1); - std::uint32_t mod = - 0; // the first 16 bytes of E as a big-endian integer, mod 3 + // the first 16 bytes of E as a big-endian integer, mod 3 + std::uint32_t mod = 0; for (std::size_t i = 0; i < 16; ++i) { mod = (mod * 256 + static_cast(e[i])) % 3; }