fix: validate cipher blocks and derivation parameters

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.
 

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");

src/odr/internal/crypto/crypto_util.cpp

@@ -5,6 +5,7 @@
 #include <array>
 #include <cstdint>
 #include <cstring>
+#include <limits>
 #include <stdexcept>
 
 // 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<byte *>(result.data()),
+                     reinterpret_cast<const byte *>(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<const byte *>(key.data()),
                            key.size());
-  rc4.ProcessData(reinterpret_cast<byte *>(result.data()),
-                  reinterpret_cast<const byte *>(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<unsigned int>::max()) {
+    throw std::invalid_argument("PBKDF2 iteration count out of range");
+  }
   std::string result(key_size, '\0');
   const CryptoPP::PKCS5_PBKDF2_HMAC<CryptoPP::SHA1> pbkdf2;
   pbkdf2.DeriveKey(reinterpret_cast<byte *>(result.data()), result.size(),
                    false, reinterpret_cast<const byte *>(start_key.data()),
                    start_key.size(),
                    reinterpret_cast<const byte *>(salt.data()), salt.size(),
-                   iteration_count);
+                   static_cast<unsigned int>(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<CryptoPP::AES>::Decryption decryption;
   decryption.SetKey(reinterpret_cast<const byte *>(key.data()), key.size());
-  decryption.ProcessData(reinterpret_cast<byte *>(result.data()),
-                         reinterpret_cast<const byte *>(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<CryptoPP::AES>::Decryption decryption;
   decryption.SetKeyWithIV(reinterpret_cast<const byte *>(key.data()),
                           key.size(), reinterpret_cast<const byte *>(iv.data()),
                           iv.size());
-  decryption.ProcessData(reinterpret_cast<byte *>(result.data()),
-                         reinterpret_cast<const byte *>(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<CryptoPP::AES>::Encryption encryption;
   encryption.SetKeyWithIV(reinterpret_cast<const byte *>(key.data()),
                           key.size(), reinterpret_cast<const byte *>(iv.data()),
                           iv.size());
-  encryption.ProcessData(reinterpret_cast<byte *>(result.data()),
-                         reinterpret_cast<const byte *>(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<CryptoPP::DES_EDE3>::Decryption decryption;
   decryption.SetKeyWithIV(reinterpret_cast<const byte *>(key.data()),
                           key.size(), reinterpret_cast<const byte *>(iv.data()),
                           iv.size());
-  decryption.ProcessData(reinterpret_cast<byte *>(result.data()),
-                         reinterpret_cast<const byte *>(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<CryptoPP::Blowfish>::Decryption decryption;
   decryption.SetKeyWithIV(reinterpret_cast<const byte *>(key.data()),
                           key.size(), reinterpret_cast<const byte *>(iv.data()),
                           iv.size());
-  decryption.ProcessData(reinterpret_cast<byte *>(result.data()),
-                         reinterpret_cast<const byte *>(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
 

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

test/src/internal/crypto/crypto_util_test.cpp

@@ -2,12 +2,48 @@
 
 #include <gtest/gtest.h>
 
+#include <array>
 #include <cstdint>
+#include <limits>
 #include <stdexcept>
 #include <string>
 
 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<std::size_t, 3>{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);
+  }
+  // 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'));
+  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<unsigned int>::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");

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) {