Refactor `Pl_AES_PDF` and `Pl_RC4` constructors to simplify key handling

Replaced raw key pointer and length parameters with a `std::string` in `Pl_AES_PDF` and `Pl_RC4` constructors for improved safety and clarity. Updated all usage sites accordingly, reducing reliance on manual memory management and redundant conversions.

Refactor `Pl_AES_PDF` and `Pl_RC4` constructors to simplify key handling
Replaced raw key pointer and length parameters with a `std::string` in `Pl_AES_PDF` and `Pl_RC4` constructors for improved safety and clarity. Updated all usage sites accordingly, reducing reliance on manual memory management and redundant conversions.
m-holger
1 parent 4278a557
Showing 8 changed files with 27 additions and 70 deletions
libqpdf/Pl_AES_PDF.cc
libqpdf/Pl_RC4.cc
libqpdf/QPDFWriter.cc
libqpdf/QPDF_encryption.cc
libqpdf/qpdf/Pl_AES_PDF.hh
libqpdf/qpdf/Pl_RC4.hh
libtests/aes.cc
libtests/rc4.cc
@@ -9,25 +9,18 @@
 bool Pl_AES_PDF::use_static_iv = false;
-Pl_AES_PDF::Pl_AES_PDF(
-    char const* identifier,
-    Pipeline* next,
-    bool encrypt,
-    unsigned char const* key,
-    size_t key_bytes) :
+Pl_AES_PDF::Pl_AES_PDF(char const* identifier, Pipeline* next, bool encrypt, std::string key) :
     Pipeline(identifier, next),
+    key(key),
     crypto(QPDFCryptoProvider::getImpl()),
-    encrypt(encrypt),
-    key_bytes(key_bytes)
+    encrypt(encrypt)
 {
     if (!next) {
         throw std::logic_error("Attempt to create Pl_AES_PDF with nullptr as next");
     }
-    if (!(key_bytes == 32 || key_bytes == 16)) {
+    if (!(key.size() == 32 || key.size() == 16)) {
         throw std::runtime_error("unsupported key length");
     }
-    this->key = std::make_unique<unsigned char[]>(key_bytes);
-    std::memcpy(this->key.get(), key, key_bytes);
     std::memset(this->inbuf, 0, this->buf_size);
     std::memset(this->outbuf, 0, this->buf_size);
     std::memset(this->cbc_block, 0, this->buf_size);
@@ -170,7 +163,12 @@ Pl_AES_PDF::flush(bool strip_padding)
                 return_after_init = true;
             }
         }
-        crypto->rijndael_init(encrypt, key.get(), key_bytes, cbc_mode, cbc_block);
+        crypto->rijndael_init(
+            encrypt,
+            reinterpret_cast<const unsigned char*>(key.data()),
+            key.size(),
+            cbc_mode,
+            cbc_block);
         if (return_after_init) {
             return;
         }
@@ -2,15 +2,10 @@
 #include <qpdf/QUtil.hh>
-Pl_RC4::Pl_RC4(
-    char const* identifier,
-    Pipeline* next,
-    unsigned char const* key_data,
-    int key_len,
-    size_t out_bufsize) :
+Pl_RC4::Pl_RC4(char const* identifier, Pipeline* next, std::string key, size_t out_bufsize) :
     Pipeline(identifier, next),
     out_bufsize(out_bufsize),
-    rc4(key_data, key_len)
+    rc4(reinterpret_cast<unsigned char const*>(key.data()), static_cast<int>(key.size()))
 {
     if (!next) {
         throw std::logic_error("Attempt to create Pl_RC4 with nullptr as next");
@@ -1109,18 +1109,9 @@ QPDFWriter::write_encrypted(std::string_view str)
     if (!(m->encryption && !m->cur_data_key.empty())) {
         write(str);
     } else if (m->encrypt_use_aes) {
-        write(
-            pl::pipe<Pl_AES_PDF>(
-                str,
-                true,
-                QUtil::unsigned_char_pointer(m->cur_data_key),
-                m->cur_data_key.length()));
+        write(pl::pipe<Pl_AES_PDF>(str, true, m->cur_data_key));
     } else {
-        write(
-            pl::pipe<Pl_RC4>(
-                str,
-                QUtil::unsigned_char_pointer(m->cur_data_key),
-                QIntC::to_int(m->cur_data_key.length())));
+        write(pl::pipe<Pl_RC4>(str, m->cur_data_key));
     }
     return *this;
@@ -1654,12 +1645,7 @@ QPDFWriter::unparseObject(
             val = object.getStringValue();
             if (m->encrypt_use_aes) {
                 Pl_Buffer bufpl("encrypted string");
-                Pl_AES_PDF pl(
-                    "aes encrypt string",
-                    &bufpl,
-                    true,
-                    QUtil::unsigned_char_pointer(m->cur_data_key),
-                    m->cur_data_key.length());
+                Pl_AES_PDF pl("aes encrypt string", &bufpl, true, m->cur_data_key);
                 pl.writeString(val);
                 pl.finish();
                 val = QPDF_String(bufpl.getString()).unparse(true);
@@ -230,8 +230,7 @@ process_with_aes(
     size_t iv_length = 0)
 {
     Pl_Buffer buffer("buffer");
-    Pl_AES_PDF aes(
-        "aes", &buffer, encrypt, QUtil::unsigned_char_pointer(key), QIntC::to_uint(key.length()));
+    Pl_AES_PDF aes("aes", &buffer, encrypt, key);
     if (iv) {
         aes.setIV(iv, iv_length);
     } else {
@@ -902,12 +901,7 @@ QPDF::decryptString(std::string&amp; str, QPDFObjGen og)
         if (use_aes) {
             QTC::TC("qpdf", "QPDF_encryption aes decode string");
             Pl_Buffer bufpl("decrypted string");
-            Pl_AES_PDF pl(
-                "aes decrypt string",
-                &bufpl,
-                false,
-                QUtil::unsigned_char_pointer(key),
-                key.length());
+            Pl_AES_PDF pl("aes decrypt string", &bufpl, false, key);
             pl.writeString(str);
             pl.finish();
             str = bufpl.getString();
@@ -1028,19 +1022,11 @@ QPDF::decryptStream(
     std::string key = getKeyForObject(encp, og, use_aes);
     if (use_aes) {
         QTC::TC("qpdf", "QPDF_encryption aes decode stream");
-        decrypt_pipeline = std::make_unique<Pl_AES_PDF>(
-            "AES stream decryption",
-            pipeline,
-            false,
-            QUtil::unsigned_char_pointer(key),
-            key.length());
+        decrypt_pipeline =
+            std::make_unique<Pl_AES_PDF>("AES stream decryption", pipeline, false, key);
     } else {
         QTC::TC("qpdf", "QPDF_encryption rc4 decode stream");
-        decrypt_pipeline = std::make_unique<Pl_RC4>(
-            "RC4 stream decryption",
-            pipeline,
-            QUtil::unsigned_char_pointer(key),
-            toI(key.length()));
+        decrypt_pipeline = std::make_unique<Pl_RC4>("RC4 stream decryption", pipeline, key);
     }
     pipeline = decrypt_pipeline.get();
 }
@@ -11,12 +11,7 @@ class Pl_AES_PDF final: public Pipeline
 {
   public:
     // key should be a pointer to key_bytes bytes of data
-    Pl_AES_PDF(
-        char const* identifier,
-        Pipeline* next,
-        bool encrypt,
-        unsigned char const* key,
-        size_t key_bytes);
+    Pl_AES_PDF(char const* identifier, Pipeline* next, bool encrypt, std::string key);
     ~Pl_AES_PDF() final = default;
     void write(unsigned char const* data, size_t len) final;
@@ -42,13 +37,12 @@ class Pl_AES_PDF final: public Pipeline
     static unsigned int const buf_size = QPDFCryptoImpl::rijndael_buf_size;
     static bool use_static_iv;
+    std::string key;
     std::shared_ptr<QPDFCryptoImpl> crypto;
     bool encrypt;
     bool cbc_mode{true};
     bool first{true};
     size_t offset{0}; // offset into memory buffer
-    std::unique_ptr<unsigned char[]> key;
-    size_t key_bytes{0};
     unsigned char inbuf[buf_size];
     unsigned char outbuf[buf_size];
     unsigned char cbc_block[buf_size];
@@ -12,11 +12,7 @@ class Pl_RC4 final: public Pipeline
     // key_len of -1 means treat key_data as a null-terminated string
     Pl_RC4(
-        char const* identifier,
-        Pipeline* next,
-        unsigned char const* key_data,
-        int key_len = -1,
-        size_t out_bufsize = def_bufsize);
+        char const* identifier, Pipeline* next, std::string key, size_t out_bufsize = def_bufsize);
     ~Pl_RC4() final = default;
     void write(unsigned char const* data, size_t len) final;
@@ -84,8 +84,9 @@ main(int argc, char* argv[])
         key[i / 2] = static_cast<unsigned char>(val);
     }
+    std::string keystr(reinterpret_cast<char const*>(key), keylen);
     auto* out = new Pl_StdioFile("stdout", outfile);
-    auto* aes = new Pl_AES_PDF("aes_128_cbc", out, encrypt, key, keylen);
+    auto* aes = new Pl_AES_PDF("aes_128_cbc", out, encrypt, keystr);
     delete[] key;
     key = nullptr;
     if (!cbc_mode) {
@@ -58,7 +58,8 @@ main(int argc, char* argv[])
     FILE* outfile = QUtil::safe_fopen(outfilename, "wb");
     auto* out = new Pl_StdioFile("stdout", outfile);
     // Use a small buffer size (64) for testing
-    auto* rc4 = new Pl_RC4("rc4", out, key, QIntC::to_int(keylen), 64U);
+    std::string keystr(reinterpret_cast<char const*>(key), keylen);
+    auto* rc4 = new Pl_RC4("rc4", out, keystr, 64U);
     delete[] key;
     // 64 < buffer size < 512, buffer_size is not a power of 2 for testing