You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@arrow.apache.org by uw...@apache.org on 2018/04/21 14:35:15 UTC
[arrow] branch master updated: ARROW-2222: handle untrusted inputs
This is an automated email from the ASF dual-hosted git repository.
uwe pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/arrow.git
The following commit(s) were added to refs/heads/master by this push:
new 1ba7d51 ARROW-2222: handle untrusted inputs
1ba7d51 is described below
commit 1ba7d51b577c258652425099f2ef278649316c68
Author: Marco Neumann <ma...@blue-yonder.com>
AuthorDate: Sat Apr 21 15:35:17 2018 +0100
ARROW-2222: handle untrusted inputs
This is a proof of concept of handling untrusted inputs. The error messages aren't filled yet and tests aren't there, but this is everything the fuzzer found after running over an hour. It basically includes the following parts:
## Flatbuffer validation
This may be the only thing that could be optional and where we should test the performance impact. It's a single, simple change in `Message::ReadFrom`.
## nullptr validation
It seems, that even when we validate the flatbuffer, nullptrs are still legit in many places and we just ignore them. I've added a bunch of checks. We may want to introduce a simple macro for that, that includes error message generation and the return w/ an `IOError`.
## Integer handling in `BufferReader`
The `nbytes` argument can be negative since it's a signed integer. I've seen at least one case where the fuzzer tried to produce a negative body size of a message that led to a negative `nbytes` argument, a subsequent negative `position_` and a out of bounds read. I think we should catch the negative body length somewhere else and produce a proper error message, but I'm not sure if that's the only way of provoking "negative reads". For robustness, the checks should be there. If someon [...]
Author: Marco Neumann <ma...@blue-yonder.com>
Closes #1763 from crepererum/ARROW-2222 and squashes the following commits:
826dcc2c <Marco Neumann> increase flatbuffers verifier max depth to 128
9bd80fd3 <Marco Neumann> catch nullptr in flatbuffer-encoded data
d9be6b1b <Marco Neumann> validate flatbuffer message before reading
9a2faf83 <Marco Neumann> catch negative reads in BufferReader::Read
---
cpp/src/arrow/io/memory.cc | 6 ++++++
cpp/src/arrow/ipc/message.cc | 7 ++++++-
cpp/src/arrow/ipc/metadata-internal.cc | 23 ++++++++++++++++++++++-
cpp/src/arrow/ipc/reader.cc | 6 ++++++
4 files changed, 40 insertions(+), 2 deletions(-)
diff --git a/cpp/src/arrow/io/memory.cc b/cpp/src/arrow/io/memory.cc
index 45d6bdb..512e7f5 100644
--- a/cpp/src/arrow/io/memory.cc
+++ b/cpp/src/arrow/io/memory.cc
@@ -257,6 +257,9 @@ Status BufferReader::Tell(int64_t* position) const {
bool BufferReader::supports_zero_copy() const { return true; }
Status BufferReader::Read(int64_t nbytes, int64_t* bytes_read, void* buffer) {
+ if (nbytes < 0) {
+ return Status::IOError("Cannot read a negative number of bytes from BufferReader.");
+ }
*bytes_read = std::min(nbytes, size_ - position_);
if (*bytes_read) {
memcpy(buffer, data_ + position_, *bytes_read);
@@ -266,6 +269,9 @@ Status BufferReader::Read(int64_t nbytes, int64_t* bytes_read, void* buffer) {
}
Status BufferReader::Read(int64_t nbytes, std::shared_ptr<Buffer>* out) {
+ if (nbytes < 0) {
+ return Status::IOError("Cannot read a negative number of bytes from BufferReader.");
+ }
int64_t size = std::min(nbytes, size_ - position_);
if (size > 0 && buffer_ != nullptr) {
diff --git a/cpp/src/arrow/ipc/message.cc b/cpp/src/arrow/ipc/message.cc
index c14fb49..0a5bcdc 100644
--- a/cpp/src/arrow/ipc/message.cc
+++ b/cpp/src/arrow/ipc/message.cc
@@ -136,7 +136,12 @@ bool Message::Equals(const Message& other) const {
Status Message::ReadFrom(const std::shared_ptr<Buffer>& metadata, io::InputStream* stream,
std::unique_ptr<Message>* out) {
- auto fb_message = flatbuf::GetMessage(metadata->data());
+ auto data = metadata->data();
+ flatbuffers::Verifier verifier(data, metadata->size(), 128);
+ if (!flatbuf::VerifyMessageBuffer(verifier)) {
+ return Status::IOError("Invalid flatbuffers message.");
+ }
+ auto fb_message = flatbuf::GetMessage(data);
int64_t body_length = fb_message->bodyLength();
diff --git a/cpp/src/arrow/ipc/metadata-internal.cc b/cpp/src/arrow/ipc/metadata-internal.cc
index af1d6c8..d543239 100644
--- a/cpp/src/arrow/ipc/metadata-internal.cc
+++ b/cpp/src/arrow/ipc/metadata-internal.cc
@@ -817,6 +817,9 @@ static Status VisitField(const flatbuf::Field* field, DictionaryTypeMap* id_to_f
if (dict_metadata == nullptr) {
// Field is not dictionary encoded. Visit children
auto children = field->children();
+ if (children == nullptr) {
+ return Status::IOError("Children-pointer of flatbuffer-encoded Field is null.");
+ }
for (flatbuffers::uoffset_t i = 0; i < children->size(); ++i) {
RETURN_NOT_OK(VisitField(children->Get(i), id_to_field));
}
@@ -832,9 +835,16 @@ static Status VisitField(const flatbuf::Field* field, DictionaryTypeMap* id_to_f
Status GetDictionaryTypes(const void* opaque_schema, DictionaryTypeMap* id_to_field) {
auto schema = static_cast<const flatbuf::Schema*>(opaque_schema);
+ if (schema->fields() == nullptr) {
+ return Status::IOError("Fields-pointer of flatbuffer-encoded Schema is null.");
+ }
int num_fields = static_cast<int>(schema->fields()->size());
for (int i = 0; i < num_fields; ++i) {
- RETURN_NOT_OK(VisitField(schema->fields()->Get(i), id_to_field));
+ auto field = schema->fields()->Get(i);
+ if (field == nullptr) {
+ return Status::IOError("Field-pointer of flatbuffer-encoded Schema is null.");
+ }
+ RETURN_NOT_OK(VisitField(field, id_to_field));
}
return Status::OK();
}
@@ -842,6 +852,9 @@ Status GetDictionaryTypes(const void* opaque_schema, DictionaryTypeMap* id_to_fi
Status GetSchema(const void* opaque_schema, const DictionaryMemo& dictionary_memo,
std::shared_ptr<Schema>* out) {
auto schema = static_cast<const flatbuf::Schema*>(opaque_schema);
+ if (schema->fields() == nullptr) {
+ return Status::IOError("Fields-pointer of flatbuffer-encoded Schema is null.");
+ }
int num_fields = static_cast<int>(schema->fields()->size());
std::vector<std::shared_ptr<Field>> fields(num_fields);
@@ -855,6 +868,14 @@ Status GetSchema(const void* opaque_schema, const DictionaryMemo& dictionary_mem
if (fb_metadata != nullptr) {
metadata->reserve(fb_metadata->size());
for (const auto& pair : *fb_metadata) {
+ if (pair->key() == nullptr) {
+ return Status::IOError(
+ "Key-pointer in custom metadata of flatbuffer-encoded Schema is null.");
+ }
+ if (pair->value() == nullptr) {
+ return Status::IOError(
+ "Value-pointer in custom metadata of flatbuffer-encoded Schema is null.");
+ }
metadata->Append(pair->key()->str(), pair->value()->str());
}
}
diff --git a/cpp/src/arrow/ipc/reader.cc b/cpp/src/arrow/ipc/reader.cc
index 6441bc3..5ffbe6f 100644
--- a/cpp/src/arrow/ipc/reader.cc
+++ b/cpp/src/arrow/ipc/reader.cc
@@ -328,6 +328,9 @@ Status ReadRecordBatch(const Buffer& metadata, const std::shared_ptr<Schema>& sc
if (message->header_type() != flatbuf::MessageHeader_RecordBatch) {
DCHECK_EQ(message->header_type(), flatbuf::MessageHeader_RecordBatch);
}
+ if (message->header() == nullptr) {
+ return Status::IOError("Header-pointer of flatbuffer-encoded Message is null.");
+ }
auto batch = reinterpret_cast<const flatbuf::RecordBatch*>(message->header());
return ReadRecordBatch(batch, schema, max_recursion_depth, file, out);
}
@@ -426,6 +429,9 @@ class RecordBatchStreamReader::RecordBatchStreamReaderImpl {
RETURN_NOT_OK(
ReadMessageAndValidate(message_reader_.get(), Message::SCHEMA, false, &message));
+ if (message->header() == nullptr) {
+ return Status::IOError("Header-pointer of flatbuffer-encoded Message is null.");
+ }
RETURN_NOT_OK(internal::GetDictionaryTypes(message->header(), &dictionary_types_));
// TODO(wesm): In future, we may want to reconcile the ids in the stream with
--
To stop receiving notification emails like this one, please contact
uwe@apache.org.