ARROW-11559: [C++] Use smarter Flatbuffers verification parameters

Flatbuffers is able to encode a virtually unbounded of schema fields in a small buffer size. Verifying that many fields with the Flatbuffers verifier seems to result in potentially unbounded verification times, which is a denial of service risk. To mitigate the risk, impose that a Flatbuffers buffer cannot represent one more than one Flatbuffers table per buffer bit, which should always be true for well-formed Arrow IPC metadata. Indeed, the only recursive table, the `Field` table in Schema.fbs, mandates the presence of its `type` member (though it's not marked as required in the Flatbuffers definition, it's validated by the IPC read routines). TODO: * [ ] Add OSS-Fuzz regression file Closes apache#9447 from pitrou/ARROW-11559-fbb-verification-params Authored-by: Antoine Pitrou <[email protected]> Signed-off-by: Micah Kornfield <[email protected]>
sirensolutions · Jun 7, 2021 · 4dfe438 · 4dfe438
1 parent 2d4509c
commit 4dfe438
Show file tree

Hide file tree

Showing 2 changed files with 17 additions and 7 deletions.
diff --git a/cpp/src/arrow/ipc/metadata_internal.h b/cpp/src/arrow/ipc/metadata_internal.h
@@ -156,10 +156,22 @@ Status GetSparseTensorMetadata(const Buffer& metadata, std::shared_ptr<DataType>
 Status GetKeyValueMetadata(const KVVector* fb_metadata,
                            std::shared_ptr<KeyValueMetadata>* out);
 
+template <typename RootType>
+bool VerifyFlatbuffers(const uint8_t* data, int64_t size) {
+  // Heuristic: tables in a Arrow flatbuffers buffer must take at least 1 bit
+  // each in average (ARROW-11559).
+  // Especially, the only recursive table (the `Field` table in Schema.fbs)
+  // must have a non-empty `type` member.
+  flatbuffers::Verifier verifier(
+      data, static_cast<size_t>(size),
+      /*max_depth=*/128,
+      /*max_tables=*/static_cast<flatbuffers::uoffset_t>(8 * size));
+  return verifier.VerifyBuffer<RootType>(nullptr);
+}
+
 static inline Status VerifyMessage(const uint8_t* data, int64_t size,
                                    const flatbuf::Message** out) {
-  flatbuffers::Verifier verifier(data, size, /*max_depth=*/128);
-  if (!flatbuf::VerifyMessageBuffer(verifier)) {
+  if (!VerifyFlatbuffers<flatbuf::Message>(data, size)) {
     return Status::IOError("Invalid flatbuffers message.");
   }
   *out = flatbuf::GetMessage(data);

diff --git a/cpp/src/arrow/ipc/reader.cc b/cpp/src/arrow/ipc/reader.cc
@@ -1052,11 +1052,9 @@ class RecordBatchFileReaderImpl : public RecordBatchFileReader {
         footer_buffer_,
         file_->ReadAt(footer_offset_ - footer_length - file_end_size, footer_length));
 
-    auto data = footer_buffer_->data();
-    flatbuffers::Verifier verifier(data, footer_buffer_->size(), /*max_depth=*/128,
-                                   /*max_tables=*/UINT_MAX);
-
-    if (!flatbuf::VerifyFooterBuffer(verifier)) {
+    const auto data = footer_buffer_->data();
+    const auto size = footer_buffer_->size();
+    if (!internal::VerifyFlatbuffers<flatbuf::Footer>(data, size)) {
       return Status::IOError("Verification of flatbuffer-encoded Footer failed.");
     }
     footer_ = flatbuf::GetFooter(data);