GH-48858: [C++][Parquet] Avoid re-serializing footer for signature verification (#48859)

pitrou · web-flow · commit b5e495dd6747 · 2026-01-21T09:21:31.000+01:00
### Rationale for this change When reading an encrypted Parquet file with a plaintext footer, the Parquet reader is able to verify footer integrity by comparing the signature in the file with the one computed by encrypting the footer. However, the way it does this is to first re-serializes the deserialized footer using Thrift. This has several issues: 1. it's inefficient 2. it's not obvious that it will always produce the same Thrift encoding as the original, leading to spurious signature verification failures 3. if the original footer deserializes to invalid enum values, attempting to serialize it again will lead to undefined behavior Reason 3 is what allowed this to be uncovered by OSS-Fuzz (see https://oss-fuzz.com/testcase-detail/4740205688193024). This PR switches to reusing the original serialized metadata. ### Are these changes tested? Yes, by existing tests and new fuzz regression file. ### Are there any user-facing changes? No. * GitHub Issue: #48858 Authored-by: Antoine Pitrou <antoine@python.org> Signed-off-by: Antoine Pitrou <antoine@python.org>
diff --git a/cpp/src/parquet/file_reader.cc b/cpp/src/parquet/file_reader.cc
@@ -55,6 +55,10 @@ using arrow::internal::AddWithOverflow;
 
 namespace parquet {
 
+using ::arrow::Future;
+using ::arrow::Result;
+using ::arrow::Status;
+
 namespace {
 bool IsColumnChunkFullyDictionaryEncoded(const ColumnChunkMetaData& col) {
   // Check the encoding_stats to see if all data pages are dictionary encoded.
@@ -398,7 +402,7 @@ class SerializedFile : public ParquetFileReader::Contents {
     PARQUET_THROW_NOT_OK(cached_source_->Cache(ranges));
   }
 
-  ::arrow::Result<std::vector<::arrow::io::ReadRange>> GetReadRanges(
+  Result<std::vector<::arrow::io::ReadRange>> GetReadRanges(
       const std::vector<int>& row_groups, const std::vector<int>& column_indices,
       int64_t hole_size_limit, int64_t range_size_limit) {
     std::vector<::arrow::io::ReadRange> ranges;
@@ -413,10 +417,10 @@ class SerializedFile : public ParquetFileReader::Contents {
                                                      range_size_limit);
   }
 
-  ::arrow::Future<> WhenBuffered(const std::vector<int>& row_groups,
-                                 const std::vector<int>& column_indices) const {
+  Future<> WhenBuffered(const std::vector<int>& row_groups,
+                        const std::vector<int>& column_indices) const {
     if (!cached_source_) {
-      return ::arrow::Status::Invalid("Must call PreBuffer before WhenBuffered");
+      return Status::Invalid("Must call PreBuffer before WhenBuffered");
     }
     std::vector<::arrow::io::ReadRange> ranges;
     for (int row : row_groups) {
@@ -465,23 +469,8 @@ class SerializedFile : public ParquetFileReader::Contents {
       // Fall through
     }
 
-    const uint32_t read_metadata_len = ParseUnencryptedFileMetadata(
-        metadata_buffer, metadata_len, std::move(file_decryptor));
-    auto file_decryption_properties = properties_.file_decryption_properties();
-    if (is_encrypted_footer) {
-      // Nothing else to do here.
-      return;
-    } else if (!file_metadata_->is_encryption_algorithm_set()) {  // Non encrypted file.
-      if (file_decryption_properties != nullptr) {
-        if (!file_decryption_properties->plaintext_files_allowed()) {
-          throw ParquetException("Applying decryption properties on plaintext file");
-        }
-      }
-    } else {
-      // Encrypted file with plaintext footer mode.
-      ParseMetaDataOfEncryptedFileWithPlaintextFooter(
-          file_decryption_properties, metadata_buffer, metadata_len, read_metadata_len);
-    }
+    ParseMetaDataFinal(std::move(metadata_buffer), metadata_len, is_encrypted_footer,
+                       std::move(file_decryptor));
   }
 
   // Validate the source size and get the initial read size.
@@ -522,16 +511,15 @@ class SerializedFile : public ParquetFileReader::Contents {
   }
 
   // Does not throw.
-  ::arrow::Future<> ParseMetaDataAsync() {
+  Future<> ParseMetaDataAsync() {
     int64_t footer_read_size;
     BEGIN_PARQUET_CATCH_EXCEPTIONS
     footer_read_size = GetFooterReadSize();
     END_PARQUET_CATCH_EXCEPTIONS
     // Assumes this is kept alive externally
     return source_->ReadAsync(source_size_ - footer_read_size, footer_read_size)
-        .Then([this,
-               footer_read_size](const std::shared_ptr<::arrow::Buffer>& footer_buffer)
-                  -> ::arrow::Future<> {
+        .Then([this, footer_read_size](
+                  const std::shared_ptr<::arrow::Buffer>& footer_buffer) -> Future<> {
           uint32_t metadata_len;
           BEGIN_PARQUET_CATCH_EXCEPTIONS
           metadata_len = ParseFooterLength(footer_buffer, footer_read_size);
@@ -557,7 +545,7 @@ class SerializedFile : public ParquetFileReader::Contents {
   }
 
   // Continuation
-  ::arrow::Future<> ParseMaybeEncryptedMetaDataAsync(
+  Future<> ParseMaybeEncryptedMetaDataAsync(
       std::shared_ptr<::arrow::Buffer> footer_buffer,
       std::shared_ptr<::arrow::Buffer> metadata_buffer, int64_t footer_read_size,
       uint32_t metadata_len) {
@@ -580,26 +568,30 @@ class SerializedFile : public ParquetFileReader::Contents {
                  file_decryptor = std::move(file_decryptor)](
                     const std::shared_ptr<::arrow::Buffer>& metadata_buffer) {
             // Continue and read the file footer
-            return ParseMetaDataFinal(metadata_buffer, metadata_len, is_encrypted_footer,
-                                      file_decryptor);
+            BEGIN_PARQUET_CATCH_EXCEPTIONS
+            ParseMetaDataFinal(metadata_buffer, metadata_len, is_encrypted_footer,
+                               file_decryptor);
+            END_PARQUET_CATCH_EXCEPTIONS
+            return Status::OK();
           });
     }
-    return ParseMetaDataFinal(std::move(metadata_buffer), metadata_len,
-                              is_encrypted_footer, std::move(file_decryptor));
+    BEGIN_PARQUET_CATCH_EXCEPTIONS
+    ParseMetaDataFinal(std::move(metadata_buffer), metadata_len, is_encrypted_footer,
+                       std::move(file_decryptor));
+    END_PARQUET_CATCH_EXCEPTIONS
+    return Status::OK();
   }
 
   // Continuation
-  ::arrow::Status ParseMetaDataFinal(
-      std::shared_ptr<::arrow::Buffer> metadata_buffer, uint32_t metadata_len,
-      const bool is_encrypted_footer,
-      std::shared_ptr<InternalFileDecryptor> file_decryptor) {
-    BEGIN_PARQUET_CATCH_EXCEPTIONS
+  void ParseMetaDataFinal(std::shared_ptr<::arrow::Buffer> metadata_buffer,
+                          uint32_t metadata_len, const bool is_encrypted_footer,
+                          std::shared_ptr<InternalFileDecryptor> file_decryptor) {
     const uint32_t read_metadata_len = ParseUnencryptedFileMetadata(
         metadata_buffer, metadata_len, std::move(file_decryptor));
     auto file_decryption_properties = properties_.file_decryption_properties();
     if (is_encrypted_footer) {
       // Nothing else to do here.
-      return ::arrow::Status::OK();
+      return;
     } else if (!file_metadata_->is_encryption_algorithm_set()) {  // Non encrypted file.
       if (file_decryption_properties != nullptr) {
         if (!file_decryption_properties->plaintext_files_allowed()) {
@@ -611,8 +603,6 @@ class SerializedFile : public ParquetFileReader::Contents {
       ParseMetaDataOfEncryptedFileWithPlaintextFooter(
           file_decryption_properties, metadata_buffer, metadata_len, read_metadata_len);
     }
-    END_PARQUET_CATCH_EXCEPTIONS
-    return ::arrow::Status::OK();
   }
 
  private:
@@ -707,20 +697,16 @@ void SerializedFile::ParseMetaDataOfEncryptedFileWithPlaintextFooter(
     auto file_decryptor = std::make_shared<InternalFileDecryptor>(
         file_decryption_properties, file_aad, algo.algorithm,
         file_metadata_->footer_signing_key_metadata(), properties_.memory_pool());
-    // set the InternalFileDecryptor in the metadata as well, as it's used
-    // for signature verification and for ColumnChunkMetaData creation.
-    file_metadata_->set_file_decryptor(std::move(file_decryptor));
+    // Set the InternalFileDecryptor in the metadata as well, as it's used
+    // for ColumnChunkMetaData creation.
+    file_metadata_->set_file_decryptor(file_decryptor);
 
     if (file_decryption_properties->check_plaintext_footer_integrity()) {
-      if (metadata_len - read_metadata_len !=
-          (parquet::encryption::kGcmTagLength + parquet::encryption::kNonceLength)) {
-        throw ParquetInvalidOrCorruptedFileException(
-            "Failed reading metadata for encryption signature (requested ",
-            parquet::encryption::kGcmTagLength + parquet::encryption::kNonceLength,
-            " bytes but have ", metadata_len - read_metadata_len, " bytes)");
-      }
-
-      if (!file_metadata_->VerifySignature(metadata_buffer->data() + read_metadata_len)) {
+      auto serialized_metadata =
+          metadata_buffer->span_as<uint8_t>().subspan(0, read_metadata_len);
+      auto signature = metadata_buffer->span_as<uint8_t>().subspan(read_metadata_len);
+      if (!FileMetaData::VerifySignature(serialized_metadata, signature,
+                                         file_decryptor.get())) {
         throw ParquetInvalidOrCorruptedFileException(
             "Parquet crypto signature verification failed");
       }
@@ -804,7 +790,7 @@ std::unique_ptr<ParquetFileReader::Contents> ParquetFileReader::Contents::Open(
   return result;
 }
 
-::arrow::Future<std::unique_ptr<ParquetFileReader::Contents>>
+Future<std::unique_ptr<ParquetFileReader::Contents>>
 ParquetFileReader::Contents::OpenAsync(std::shared_ptr<ArrowInputFile> source,
                                        const ReaderProperties& props,
                                        std::shared_ptr<FileMetaData> metadata) {
@@ -815,7 +801,7 @@ ParquetFileReader::Contents::OpenAsync(std::shared_ptr<ArrowInputFile> source,
   if (metadata == nullptr) {
     // TODO(ARROW-12259): workaround since we have Future<(move-only type)>
     struct {
-      ::arrow::Result<std::unique_ptr<ParquetFileReader::Contents>> operator()() {
+      Result<std::unique_ptr<ParquetFileReader::Contents>> operator()() {
         return std::move(result);
       }
 
@@ -825,7 +811,7 @@ ParquetFileReader::Contents::OpenAsync(std::shared_ptr<ArrowInputFile> source,
     return file->ParseMetaDataAsync().Then(std::move(Continuation));
   } else {
     file->set_metadata(std::move(metadata));
-    return ::arrow::Future<std::unique_ptr<ParquetFileReader::Contents>>::MakeFinished(
+    return Future<std::unique_ptr<ParquetFileReader::Contents>>::MakeFinished(
         std::move(result));
   }
   END_PARQUET_CATCH_EXCEPTIONS
@@ -855,24 +841,24 @@ std::unique_ptr<ParquetFileReader> ParquetFileReader::OpenFile(
   return Open(std::move(source), props, std::move(metadata));
 }
 
-::arrow::Future<std::unique_ptr<ParquetFileReader>> ParquetFileReader::OpenAsync(
+Future<std::unique_ptr<ParquetFileReader>> ParquetFileReader::OpenAsync(
     std::shared_ptr<::arrow::io::RandomAccessFile> source, const ReaderProperties& props,
     std::shared_ptr<FileMetaData> metadata) {
   BEGIN_PARQUET_CATCH_EXCEPTIONS
   auto fut = SerializedFile::OpenAsync(std::move(source), props, std::move(metadata));
   // TODO(ARROW-12259): workaround since we have Future<(move-only type)>
-  auto completed = ::arrow::Future<std::unique_ptr<ParquetFileReader>>::Make();
-  fut.AddCallback([fut, completed](
-                      const ::arrow::Result<std::unique_ptr<ParquetFileReader::Contents>>&
-                          contents) mutable {
-    if (!contents.ok()) {
-      completed.MarkFinished(contents.status());
-      return;
-    }
-    std::unique_ptr<ParquetFileReader> result = std::make_unique<ParquetFileReader>();
-    result->Open(fut.MoveResult().MoveValueUnsafe());
-    completed.MarkFinished(std::move(result));
-  });
+  auto completed = Future<std::unique_ptr<ParquetFileReader>>::Make();
+  fut.AddCallback(
+      [fut, completed](
+          const Result<std::unique_ptr<ParquetFileReader::Contents>>& contents) mutable {
+        if (!contents.ok()) {
+          completed.MarkFinished(contents.status());
+          return;
+        }
+        std::unique_ptr<ParquetFileReader> result = std::make_unique<ParquetFileReader>();
+        result->Open(fut.MoveResult().MoveValueUnsafe());
+        completed.MarkFinished(std::move(result));
+      });
   return completed;
   END_PARQUET_CATCH_EXCEPTIONS
 }
@@ -919,7 +905,7 @@ void ParquetFileReader::PreBuffer(const std::vector<int>& row_groups,
   file->PreBuffer(row_groups, column_indices, ctx, options);
 }
 
-::arrow::Result<std::vector<::arrow::io::ReadRange>> ParquetFileReader::GetReadRanges(
+Result<std::vector<::arrow::io::ReadRange>> ParquetFileReader::GetReadRanges(
     const std::vector<int>& row_groups, const std::vector<int>& column_indices,
     int64_t hole_size_limit, int64_t range_size_limit) {
   // Access private methods here
@@ -929,8 +915,8 @@ ::arrow::Result<std::vector<::arrow::io::ReadRange>> ParquetFileReader::GetReadR
                              range_size_limit);
 }
 
-::arrow::Future<> ParquetFileReader::WhenBuffered(
-    const std::vector<int>& row_groups, const std::vector<int>& column_indices) const {
+Future<> ParquetFileReader::WhenBuffered(const std::vector<int>& row_groups,
+                                         const std::vector<int>& column_indices) const {
   // Access private methods here
   SerializedFile* file =
       ::arrow::internal::checked_cast<SerializedFile*>(contents_.get());
diff --git a/cpp/src/parquet/metadata.cc b/cpp/src/parquet/metadata.cc
@@ -1169,6 +1169,42 @@ void FileMetaData::WriteTo(::arrow::io::OutputStream* dst,
   return impl_->WriteTo(dst, encryptor);
 }
 
+bool FileMetaData::VerifySignature(std::span<const uint8_t> serialized_metadata,
+                                   std::span<const uint8_t> signature,
+                                   InternalFileDecryptor* file_decryptor) {
+  DCHECK_NE(file_decryptor, nullptr);
+
+  // In plaintext footer, the "signature" is the concatenation of the nonce used
+  // for GCM encryption, and the authentication tag obtained after GCM encryption.
+  if (signature.size() != encryption::kGcmTagLength + encryption::kNonceLength) {
+    throw ParquetInvalidOrCorruptedFileException(
+        "Invalid footer encryption signature (expected ",
+        encryption::kGcmTagLength + encryption::kNonceLength, " bytes, got ",
+        signature.size(), ")");
+  }
+
+  // Encrypt plaintext serialized metadata so as to compute its signature
+  auto nonce = signature.subspan(0, encryption::kNonceLength);
+  auto tag = signature.subspan(encryption::kNonceLength);
+  const SecureString& key = file_decryptor->GetFooterKey();
+  const std::string& aad = encryption::CreateFooterAad(file_decryptor->file_aad());
+
+  auto aes_encryptor = encryption::AesEncryptor::Make(
+      file_decryptor->algorithm(), static_cast<int>(key.size()), /*metadata=*/true,
+      /*write_length=*/false);
+
+  std::shared_ptr<Buffer> encrypted_buffer =
+      AllocateBuffer(file_decryptor->pool(),
+                     aes_encryptor->CiphertextLength(serialized_metadata.size()));
+  int32_t encrypted_len = aes_encryptor->SignedFooterEncrypt(
+      serialized_metadata, key.as_span(), str2span(aad), nonce,
+      encrypted_buffer->mutable_span_as<uint8_t>());
+  DCHECK_EQ(encrypted_len, encrypted_buffer->size());
+  // Check computed signature against expected
+  return 0 == memcmp(encrypted_buffer->data() + encrypted_len - encryption::kGcmTagLength,
+                     tag.data(), encryption::kGcmTagLength);
+}
+
 class FileCryptoMetaData::FileCryptoMetaDataImpl {
  public:
   FileCryptoMetaDataImpl() = default;
diff --git a/cpp/src/parquet/metadata.h b/cpp/src/parquet/metadata.h
@@ -21,6 +21,7 @@
 #include <map>
 #include <memory>
 #include <optional>
+#include <span>
 #include <string>
 #include <vector>
 
@@ -322,8 +323,8 @@ class PARQUET_EXPORT FileMetaData {
   EncryptionAlgorithm encryption_algorithm() const;
   const std::string& footer_signing_key_metadata() const;
 
-  /// \brief Verify signature of FileMetaData when file is encrypted but footer
-  /// is not encrypted (plaintext footer).
+  PARQUET_DEPRECATED(
+      "Deprecated in 24.0.0. If you need this functionality, please report an issue.")
   bool VerifySignature(const void* signature);
 
   void WriteTo(::arrow::io::OutputStream* dst,
@@ -383,6 +384,11 @@ class PARQUET_EXPORT FileMetaData {
   void set_file_decryptor(std::shared_ptr<InternalFileDecryptor> file_decryptor);
   const std::shared_ptr<InternalFileDecryptor>& file_decryptor() const;
 
+  // Verify the signature of a plaintext footer.
+  static bool VerifySignature(std::span<const uint8_t> serialized_metadata,
+                              std::span<const uint8_t> signature,
+                              InternalFileDecryptor* file_decryptor);
+
   // PIMPL Idiom
   FileMetaData();
   class FileMetaDataImpl;
