GH-43070: [C++][Parquet] Check for valid ciphertext length to prevent segfault (#43071)

### Rationale for this change

See #43070

### What changes are included in this PR?

Checks that the ciphertext length is at least enough to hold the length (if written), nonce and GCM tag for the GCM cipher type.

Also enforces that the input ciphertext length parameter is provided (is > 0) and verifies that the ciphertext size read from the file isn't going to cause reads beyond the end of the ciphertext buffer.

### Are these changes tested?

Yes I've added new unit tests for this.

### Are there any user-facing changes?

No
* GitHub Issue: #43070

Authored-by: Adam Reeve <[email protected]>
Signed-off-by: mwish <[email protected]>

Author: adamreeve

cpp/src/parquet/CMakeLists.txt

@@ -408,6 +408,7 @@ add_parquet_test(arrow-internals-test SOURCES arrow/path_internal_test.cc
 if(PARQUET_REQUIRE_ENCRYPTION)
   add_parquet_test(encryption-test
                    SOURCES
+                   encryption/encryption_internal_test.cc
                    encryption/write_configurations_test.cc
                    encryption/read_configurations_test.cc
                    encryption/properties_test.cc

cpp/src/parquet/column_reader.cc

@@ -512,10 +512,11 @@ std::shared_ptr<Page> SerializedPageReader::NextPage() {
     // Decrypt it if we need to
     if (crypto_ctx_.data_decryptor != nullptr) {
       PARQUET_THROW_NOT_OK(decryption_buffer_->Resize(
-          compressed_len - crypto_ctx_.data_decryptor->CiphertextSizeDelta(),
+          crypto_ctx_.data_decryptor->PlaintextLength(compressed_len),
           /*shrink_to_fit=*/false));
       compressed_len = crypto_ctx_.data_decryptor->Decrypt(
-          page_buffer->data(), compressed_len, decryption_buffer_->mutable_data());
+          page_buffer->span_as<uint8_t>(),
+          decryption_buffer_->mutable_span_as<uint8_t>());
 
       page_buffer = decryption_buffer_;
     }

cpp/src/parquet/encryption/encryption.h

@@ -89,6 +89,14 @@ inline const uint8_t* str2bytes(const std::string& str) {
   return reinterpret_cast<const uint8_t*>(cbytes);
 }
 
+inline ::arrow::util::span<const uint8_t> str2span(const std::string& str) {
+  if (str.empty()) {
+    return {};
+  }
+
+  return {reinterpret_cast<const uint8_t*>(str.data()), str.size()};
+}
+
 class PARQUET_EXPORT ColumnEncryptionProperties {
  public:
   class PARQUET_EXPORT Builder {

cpp/src/parquet/encryption/encryption_internal.cc

@@ -31,6 +31,7 @@
 #include "parquet/encryption/openssl_internal.h"
 #include "parquet/exception.h"
 
+using ::arrow::util::span;
 using parquet::ParquetException;
 
 namespace parquet::encryption {
@@ -315,8 +316,8 @@ class AesDecryptor::AesDecryptorImpl {
 
   ~AesDecryptorImpl() { WipeOut(); }
 
-  int Decrypt(const uint8_t* ciphertext, int ciphertext_len, const uint8_t* key,
-              int key_len, const uint8_t* aad, int aad_len, uint8_t* plaintext);
+  int Decrypt(span<const uint8_t> ciphertext, span<const uint8_t> key,
+              span<const uint8_t> aad, span<uint8_t> plaintext);
 
   void WipeOut() {
     if (nullptr != ctx_) {
@@ -325,25 +326,46 @@ class AesDecryptor::AesDecryptorImpl {
     }
   }
 
-  int ciphertext_size_delta() { return ciphertext_size_delta_; }
+  [[nodiscard]] int PlaintextLength(int ciphertext_len) const {
+    if (ciphertext_len < ciphertext_size_delta_) {
+      std::stringstream ss;
+      ss << "Ciphertext length " << ciphertext_len << " is invalid, expected at least "
+         << ciphertext_size_delta_;
+      throw ParquetException(ss.str());
+    }
+    return ciphertext_len - ciphertext_size_delta_;
+  }
+
+  [[nodiscard]] int CiphertextLength(int plaintext_len) const {
+    if (plaintext_len < 0) {
+      std::stringstream ss;
+      ss << "Negative plaintext length " << plaintext_len;
+      throw ParquetException(ss.str());
+    }
+    return plaintext_len + ciphertext_size_delta_;
+  }
 
  private:
   EVP_CIPHER_CTX* ctx_;
   int aes_mode_;
   int key_length_;
   int ciphertext_size_delta_;
   int length_buffer_length_;
-  int GcmDecrypt(const uint8_t* ciphertext, int ciphertext_len, const uint8_t* key,
-                 int key_len, const uint8_t* aad, int aad_len, uint8_t* plaintext);
 
-  int CtrDecrypt(const uint8_t* ciphertext, int ciphertext_len, const uint8_t* key,
-                 int key_len, uint8_t* plaintext);
+  /// Get the actual ciphertext length, inclusive of the length buffer length,
+  /// and validate that the provided buffer size is large enough.
+  [[nodiscard]] int GetCiphertextLength(span<const uint8_t> ciphertext) const;
+
+  int GcmDecrypt(span<const uint8_t> ciphertext, span<const uint8_t> key,
+                 span<const uint8_t> aad, span<uint8_t> plaintext);
+
+  int CtrDecrypt(span<const uint8_t> ciphertext, span<const uint8_t> key,
+                 span<uint8_t> plaintext);
 };
 
-int AesDecryptor::Decrypt(const uint8_t* plaintext, int plaintext_len, const uint8_t* key,
-                          int key_len, const uint8_t* aad, int aad_len,
-                          uint8_t* ciphertext) {
-  return impl_->Decrypt(plaintext, plaintext_len, key, key_len, aad, aad_len, ciphertext);
+int AesDecryptor::Decrypt(span<const uint8_t> ciphertext, span<const uint8_t> key,
+                          span<const uint8_t> aad, span<uint8_t> plaintext) {
+  return impl_->Decrypt(ciphertext, key, aad, plaintext);
 }
 
 void AesDecryptor::WipeOut() { impl_->WipeOut(); }
@@ -438,56 +460,105 @@ std::shared_ptr<AesDecryptor> AesDecryptor::Make(
   return decryptor;
 }
 
-int AesDecryptor::CiphertextSizeDelta() { return impl_->ciphertext_size_delta(); }
-
-int AesDecryptor::AesDecryptorImpl::GcmDecrypt(const uint8_t* ciphertext,
-                                               int ciphertext_len, const uint8_t* key,
-                                               int key_len, const uint8_t* aad,
-                                               int aad_len, uint8_t* plaintext) {
-  int len;
-  int plaintext_len;
+int AesDecryptor::PlaintextLength(int ciphertext_len) const {
+  return impl_->PlaintextLength(ciphertext_len);
+}
 
-  uint8_t tag[kGcmTagLength];
-  memset(tag, 0, kGcmTagLength);
-  uint8_t nonce[kNonceLength];
-  memset(nonce, 0, kNonceLength);
+int AesDecryptor::CiphertextLength(int plaintext_len) const {
+  return impl_->CiphertextLength(plaintext_len);
+}
 
+int AesDecryptor::AesDecryptorImpl::GetCiphertextLength(
+    span<const uint8_t> ciphertext) const {
   if (length_buffer_length_ > 0) {
+    // Note: length_buffer_length_ must be either 0 or kBufferSizeLength
+    if (ciphertext.size() < static_cast<size_t>(kBufferSizeLength)) {
+      std::stringstream ss;
+      ss << "Ciphertext buffer length " << ciphertext.size()
+         << " is insufficient to read the ciphertext length."
+         << " At least " << kBufferSizeLength << " bytes are required.";
+      throw ParquetException(ss.str());
+    }
+
     // Extract ciphertext length
     int written_ciphertext_len = ((ciphertext[3] & 0xff) << 24) |
                                  ((ciphertext[2] & 0xff) << 16) |
                                  ((ciphertext[1] & 0xff) << 8) | ((ciphertext[0] & 0xff));
 
-    if (ciphertext_len > 0 &&
-        ciphertext_len != (written_ciphertext_len + length_buffer_length_)) {
-      throw ParquetException("Wrong ciphertext length");
+    if (written_ciphertext_len < 0) {
+      std::stringstream ss;
+      ss << "Negative ciphertext length " << written_ciphertext_len;
+      throw ParquetException(ss.str());
+    } else if (ciphertext.size() <
+               static_cast<size_t>(written_ciphertext_len) + length_buffer_length_) {
+      std::stringstream ss;
+      ss << "Serialized ciphertext length "
+         << (written_ciphertext_len + length_buffer_length_)
+         << " is greater than the provided ciphertext buffer length "
+         << ciphertext.size();
+      throw ParquetException(ss.str());
     }
-    ciphertext_len = written_ciphertext_len + length_buffer_length_;
+
+    return written_ciphertext_len + length_buffer_length_;
   } else {
-    if (ciphertext_len == 0) {
-      throw ParquetException("Zero ciphertext length");
+    if (ciphertext.size() > static_cast<size_t>(std::numeric_limits<int32_t>::max())) {
+      std::stringstream ss;
+      ss << "Ciphertext buffer length " << ciphertext.size() << " overflows int32";
+      throw ParquetException(ss.str());
     }
+    return static_cast<int>(ciphertext.size());
+  }
+}
+
+int AesDecryptor::AesDecryptorImpl::GcmDecrypt(span<const uint8_t> ciphertext,
+                                               span<const uint8_t> key,
+                                               span<const uint8_t> aad,
+                                               span<uint8_t> plaintext) {
+  int len;
+  int plaintext_len;
+
+  uint8_t tag[kGcmTagLength];
+  memset(tag, 0, kGcmTagLength);
+  uint8_t nonce[kNonceLength];
+  memset(nonce, 0, kNonceLength);
+
+  int ciphertext_len = GetCiphertextLength(ciphertext);
+
+  if (plaintext.size() < static_cast<size_t>(ciphertext_len) - ciphertext_size_delta_) {
+    std::stringstream ss;
+    ss << "Plaintext buffer length " << plaintext.size() << " is insufficient "
+       << "for ciphertext length " << ciphertext_len;
+    throw ParquetException(ss.str());
+  }
+
+  if (ciphertext_len < length_buffer_length_ + kNonceLength + kGcmTagLength) {
+    std::stringstream ss;
+    ss << "Invalid ciphertext length " << ciphertext_len << ". Expected at least "
+       << length_buffer_length_ + kNonceLength + kGcmTagLength << "\n";
+    throw ParquetException(ss.str());
   }
 
   // Extracting IV and tag
-  std::copy(ciphertext + length_buffer_length_,
-            ciphertext + length_buffer_length_ + kNonceLength, nonce);
-  std::copy(ciphertext + ciphertext_len - kGcmTagLength, ciphertext + ciphertext_len,
-            tag);
+  std::copy(ciphertext.begin() + length_buffer_length_,
+            ciphertext.begin() + length_buffer_length_ + kNonceLength, nonce);
+  std::copy(ciphertext.begin() + ciphertext_len - kGcmTagLength,
+            ciphertext.begin() + ciphertext_len, tag);
 
   // Setting key and IV
-  if (1 != EVP_DecryptInit_ex(ctx_, nullptr, nullptr, key, nonce)) {
+  if (1 != EVP_DecryptInit_ex(ctx_, nullptr, nullptr, key.data(), nonce)) {
     throw ParquetException("Couldn't set key and IV");
   }
 
   // Setting additional authenticated data
-  if ((nullptr != aad) && (1 != EVP_DecryptUpdate(ctx_, nullptr, &len, aad, aad_len))) {
+  if ((!aad.empty()) && (1 != EVP_DecryptUpdate(ctx_, nullptr, &len, aad.data(),
+                                                static_cast<int>(aad.size())))) {
     throw ParquetException("Couldn't set AAD");
   }
 
   // Decryption
   if (!EVP_DecryptUpdate(
-          ctx_, plaintext, &len, ciphertext + length_buffer_length_ + kNonceLength,
+          ctx_, plaintext.data(), &len,
+          ciphertext.data() + length_buffer_length_ + kNonceLength,
           ciphertext_len - length_buffer_length_ - kNonceLength - kGcmTagLength)) {
     throw ParquetException("Failed decryption update");
   }
@@ -500,87 +571,86 @@ int AesDecryptor::AesDecryptorImpl::GcmDecrypt(const uint8_t* ciphertext,
   }
 
   // Finalization
-  if (1 != EVP_DecryptFinal_ex(ctx_, plaintext + len, &len)) {
+  if (1 != EVP_DecryptFinal_ex(ctx_, plaintext.data() + len, &len)) {
     throw ParquetException("Failed decryption finalization");
   }
 
   plaintext_len += len;
   return plaintext_len;
 }
 
-int AesDecryptor::AesDecryptorImpl::CtrDecrypt(const uint8_t* ciphertext,
-                                               int ciphertext_len, const uint8_t* key,
-                                               int key_len, uint8_t* plaintext) {
+int AesDecryptor::AesDecryptorImpl::CtrDecrypt(span<const uint8_t> ciphertext,
+                                               span<const uint8_t> key,
+                                               span<uint8_t> plaintext) {
   int len;
   int plaintext_len;
 
   uint8_t iv[kCtrIvLength];
   memset(iv, 0, kCtrIvLength);
 
-  if (length_buffer_length_ > 0) {
-    // Extract ciphertext length
-    int written_ciphertext_len = ((ciphertext[3] & 0xff) << 24) |
-                                 ((ciphertext[2] & 0xff) << 16) |
-                                 ((ciphertext[1] & 0xff) << 8) | ((ciphertext[0] & 0xff));
+  int ciphertext_len = GetCiphertextLength(ciphertext);
 
-    if (ciphertext_len > 0 &&
-        ciphertext_len != (written_ciphertext_len + length_buffer_length_)) {
-      throw ParquetException("Wrong ciphertext length");
-    }
-    ciphertext_len = written_ciphertext_len;
-  } else {
-    if (ciphertext_len == 0) {
-      throw ParquetException("Zero ciphertext length");
-    }
+  if (plaintext.size() < static_cast<size_t>(ciphertext_len) - ciphertext_size_delta_) {
+    std::stringstream ss;
+    ss << "Plaintext buffer length " << plaintext.size() << " is insufficient "
+       << "for ciphertext length " << ciphertext_len;
+    throw ParquetException(ss.str());
+  }
+
+  if (ciphertext_len < length_buffer_length_ + kNonceLength) {
+    std::stringstream ss;
+    ss << "Invalid ciphertext length " << ciphertext_len << ". Expected at least "
+       << length_buffer_length_ + kNonceLength << "\n";
+    throw ParquetException(ss.str());
   }
 
   // Extracting nonce
-  std::copy(ciphertext + length_buffer_length_,
-            ciphertext + length_buffer_length_ + kNonceLength, iv);
+  std::copy(ciphertext.begin() + length_buffer_length_,
+            ciphertext.begin() + length_buffer_length_ + kNonceLength, iv);
   // Parquet CTR IVs are comprised of a 12-byte nonce and a 4-byte initial
   // counter field.
   // The first 31 bits of the initial counter field are set to 0, the last bit
   // is set to 1.
   iv[kCtrIvLength - 1] = 1;
 
   // Setting key and IV
-  if (1 != EVP_DecryptInit_ex(ctx_, nullptr, nullptr, key, iv)) {
+  if (1 != EVP_DecryptInit_ex(ctx_, nullptr, nullptr, key.data(), iv)) {
     throw ParquetException("Couldn't set key and IV");
   }
 
   // Decryption
-  if (!EVP_DecryptUpdate(ctx_, plaintext, &len,
-                         ciphertext + length_buffer_length_ + kNonceLength,
-                         ciphertext_len - kNonceLength)) {
+  if (!EVP_DecryptUpdate(ctx_, plaintext.data(), &len,
+                         ciphertext.data() + length_buffer_length_ + kNonceLength,
+                         ciphertext_len - length_buffer_length_ - kNonceLength)) {
     throw ParquetException("Failed decryption update");
   }
 
   plaintext_len = len;
 
   // Finalization
-  if (1 != EVP_DecryptFinal_ex(ctx_, plaintext + len, &len)) {
+  if (1 != EVP_DecryptFinal_ex(ctx_, plaintext.data() + len, &len)) {
     throw ParquetException("Failed decryption finalization");
   }
 
   plaintext_len += len;
   return plaintext_len;
 }
 
-int AesDecryptor::AesDecryptorImpl::Decrypt(const uint8_t* ciphertext, int ciphertext_len,
-                                            const uint8_t* key, int key_len,
-                                            const uint8_t* aad, int aad_len,
-                                            uint8_t* plaintext) {
-  if (key_length_ != key_len) {
+int AesDecryptor::AesDecryptorImpl::Decrypt(span<const uint8_t> ciphertext,
+                                            span<const uint8_t> key,
+                                            span<const uint8_t> aad,
+                                            span<uint8_t> plaintext) {
+  if (static_cast<size_t>(key_length_) != key.size()) {
     std::stringstream ss;
-    ss << "Wrong key length " << key_len << ". Should be " << key_length_;
+    ss << "Wrong key length " << key.size() << ". Should be " << key_length_;
     throw ParquetException(ss.str());
   }
 
   if (kGcmMode == aes_mode_) {
-    return GcmDecrypt(ciphertext, ciphertext_len, key, key_len, aad, aad_len, plaintext);
+    return GcmDecrypt(ciphertext, key, aad, plaintext);
   }
 
-  return CtrDecrypt(ciphertext, ciphertext_len, key, key_len, plaintext);
+  return CtrDecrypt(ciphertext, key, plaintext);
 }
 
 static std::string ShortToBytesLe(int16_t input) {