Copilot commented on code in PR #3859:
URL: https://github.com/apache/avro/pull/3859#discussion_r3566497673
##########
lang/c++/impl/Generic.cc:
##########
@@ -25,6 +27,151 @@ using std::ostringstream;
using std::string;
using std::vector;
+// Minimum number of bytes a single value of the given schema node can occupy
on
+// the wire. Used to reject an array/map block count that could not be backed
by
+// the bytes remaining. A type that can encode to zero bytes (null) returns 0,
+// which disables the collection check for it (so an array of nulls is not
+// falsely rejected). A depth limit breaks self-referencing (symbolic) schemas.
+static int64_t minBytesPerElement(const NodePtr &node, int depth) {
+ if (!node) {
+ return 0;
+ }
+ switch (node->type()) {
+ case AVRO_NULL:
+ return 0;
+ case AVRO_FLOAT:
+ return 4;
+ case AVRO_DOUBLE:
+ return 8;
+ case AVRO_FIXED: {
+ // fixedSize() is a size_t; clamp to int64_t so a huge fixed size
+ // cannot wrap negative.
+ size_t sz = node->fixedSize();
+ return sz >
static_cast<size_t>(std::numeric_limits<int64_t>::max())
+ ? std::numeric_limits<int64_t>::max()
+ : static_cast<int64_t>(sz);
+ }
+ case AVRO_RECORD: {
+ if (depth > 64) {
+ // Purely a recursion (stack-overflow) safety net for a
+ // pathologically deep schema. A truly cyclic schema never
+ // reaches this: a self-reference is an AVRO_SYMBOLIC node,
+ // handled by the default case below (returning 1 without
+ // following the link), so recursion always terminates. This
+ // guard therefore only trips on a genuinely deep *acyclic*
+ // record, whose true minimum can still be 0 (e.g. a long chain
+ // of records whose only leaves are null). Return 0 rather than
+ // over-estimating, so a valid array/map of such elements is
not
+ // falsely rejected; 0 is always a valid lower bound.
+ return 0;
+ }
+ int64_t total = 0;
+ for (size_t i = 0; i < node->leaves(); ++i) {
+ int64_t fieldMin = minBytesPerElement(node->leafAt(i), depth +
1);
+ // Saturate rather than overflow: a wrapped (negative) total
+ // would disable the collection check.
+ if (fieldMin > std::numeric_limits<int64_t>::max() - total) {
+ return std::numeric_limits<int64_t>::max();
+ }
+ total += fieldMin;
+ }
+ return total;
+ }
+ default:
+ // boolean, int, long, bytes, string, enum, union, array, map and
+ // symbolic references all encode to at least one byte.
+ return 1;
+ }
+}
+
+// Default maximum number of zero-byte-encoded collection elements (e.g. an
+// array of nulls) to allocate from a single decode. Such elements consume no
+// input, so ensureCollectionAvailable cannot bound their count; without a cap
a
+// tiny payload can declare a huge block count and exhaust memory. Overridable
+// via the AVRO_MAX_COLLECTION_ITEMS environment variable.
+static constexpr int64_t kDefaultMaxCollectionItems = 10000000;
+
+// Structural cap on the number of elements in any array or map (an overflow /
+// defense-in-depth guard), matching the historical Integer.MAX_VALUE - 8
limit.
+// Non-zero-byte elements are also bounded by the bytes remaining.
+static constexpr int64_t kDefaultMaxCollectionStructural = 2147483639;
+
+// AVRO_MAX_COLLECTION_ITEMS, when a non-negative integer, caps both limits;
+// otherwise zero-byte elements use the tighter default and all collections use
+// the structural default.
+static void collectionLimits(int64_t &zeroByte, int64_t &structural) {
+ const char *env = std::getenv("AVRO_MAX_COLLECTION_ITEMS");
+ if (env != nullptr && *env != '\0') {
+ char *end = nullptr;
+ long long value = std::strtoll(env, &end, 10);
+ if (*end == '\0' && value >= 0) {
+ zeroByte = structural = static_cast<int64_t>(value);
+ return;
+ }
+ }
+ zeroByte = kDefaultMaxCollectionItems;
+ structural = kDefaultMaxCollectionStructural;
+}
+
+// Reject a collection (array or map) block whose declared element count could
+// not be backed by the bytes actually remaining, before resizing for it.
+// Skipped when the per-element minimum is zero, or when the decoder cannot
+// report how many bytes remain. The comparison divides to avoid overflow.
+static void ensureCollectionAvailable(Decoder &d, size_t existing, size_t
count, int64_t minBytes) {
+ if (count == 0 || minBytes <= 0) {
+ return;
+ }
+ int64_t remaining = d.bytesRemaining();
+ if (remaining >= 0 &&
+ static_cast<uint64_t>(count) >
+ static_cast<uint64_t>(remaining) /
static_cast<uint64_t>(minBytes)) {
+ throw Exception(
+ "Collection claims {} elements with at least {} bytes each, "
+ "but only {} bytes are available",
+ count, minBytes, remaining);
+ }
+ // Structural / overflow guard, also covering decoders that cannot report
the
+ // bytes remaining. Compare without adding so existing + count cannot
overflow.
+ int64_t zeroByte, structural;
+ collectionLimits(zeroByte, structural);
+ const uint64_t limit = static_cast<uint64_t>(structural);
+ if (static_cast<uint64_t>(count) > limit ||
+ static_cast<uint64_t>(existing) > limit -
static_cast<uint64_t>(count)) {
+ throw Exception(
+ "Cannot read a collection of more than {} elements; "
+ "set AVRO_MAX_COLLECTION_ITEMS if this is legitimate",
+ structural);
+ }
Review Comment:
ensureCollectionAvailable() returns early when minBytes<=0, which
unintentionally skips the structural/overflow cap (despite the comment saying
it covers decoders that can’t report remaining bytes). This means array/map
decoding under schema resolution (minBytes forced to 0) won’t apply the
structural limit at all, allowing very large allocations. Also, the current
implementation declares `zeroByte` but never uses it, which will fail the build
under -Werror (set-but-not-used).
##########
lang/c++/impl/BinaryDecoder.cc:
##########
@@ -180,6 +215,18 @@ size_t BinaryDecoder::skipArray() {
auto n = static_cast<size_t>(doDecodeLong());
in_.skipBytes(n);
} else {
Review Comment:
In skipArray(), when the block count is negative, the following “block byte
size” long is cast directly to size_t and passed to skipBytes(). A malicious
input can provide a negative block size, which will convert to a huge size_t
and may cause excessive skipping/reading (DoS) instead of being rejected as
invalid.
##########
lang/c++/impl/BinaryDecoder.cc:
##########
@@ -180,6 +215,18 @@ size_t BinaryDecoder::skipArray() {
auto n = static_cast<size_t>(doDecodeLong());
in_.skipBytes(n);
} else {
+ // Bound the block count: skipping a huge block of zero-byte
elements
+ // would otherwise loop unboundedly (a CPU exhaustion) even though
it
+ // reads/allocates nothing. The decoder has no element schema
here, so
+ // apply the structural cap (AVRO_MAX_COLLECTION_ITEMS, default
+ // Integer.MAX_VALUE - 8).
+ static const int64_t structural = maxCollectionStructural();
Review Comment:
skipArray() caches AVRO_MAX_COLLECTION_ITEMS in a function-local static, so
later runtime changes to the environment variable (including other unit tests
in the same process) won’t be reflected. Generic.cc reads the env var
dynamically each time; doing the same here keeps behavior consistent and avoids
cross-test leakage.
##########
lang/c++/impl/Generic.cc:
##########
@@ -25,6 +27,151 @@ using std::ostringstream;
using std::string;
using std::vector;
+// Minimum number of bytes a single value of the given schema node can occupy
on
+// the wire. Used to reject an array/map block count that could not be backed
by
+// the bytes remaining. A type that can encode to zero bytes (null) returns 0,
+// which disables the collection check for it (so an array of nulls is not
+// falsely rejected). A depth limit breaks self-referencing (symbolic) schemas.
+static int64_t minBytesPerElement(const NodePtr &node, int depth) {
+ if (!node) {
+ return 0;
+ }
+ switch (node->type()) {
+ case AVRO_NULL:
+ return 0;
+ case AVRO_FLOAT:
+ return 4;
+ case AVRO_DOUBLE:
+ return 8;
+ case AVRO_FIXED: {
+ // fixedSize() is a size_t; clamp to int64_t so a huge fixed size
+ // cannot wrap negative.
+ size_t sz = node->fixedSize();
+ return sz >
static_cast<size_t>(std::numeric_limits<int64_t>::max())
+ ? std::numeric_limits<int64_t>::max()
+ : static_cast<int64_t>(sz);
+ }
+ case AVRO_RECORD: {
+ if (depth > 64) {
+ // Purely a recursion (stack-overflow) safety net for a
+ // pathologically deep schema. A truly cyclic schema never
+ // reaches this: a self-reference is an AVRO_SYMBOLIC node,
+ // handled by the default case below (returning 1 without
+ // following the link), so recursion always terminates. This
+ // guard therefore only trips on a genuinely deep *acyclic*
+ // record, whose true minimum can still be 0 (e.g. a long chain
+ // of records whose only leaves are null). Return 0 rather than
+ // over-estimating, so a valid array/map of such elements is
not
+ // falsely rejected; 0 is always a valid lower bound.
+ return 0;
+ }
+ int64_t total = 0;
+ for (size_t i = 0; i < node->leaves(); ++i) {
+ int64_t fieldMin = minBytesPerElement(node->leafAt(i), depth +
1);
+ // Saturate rather than overflow: a wrapped (negative) total
+ // would disable the collection check.
+ if (fieldMin > std::numeric_limits<int64_t>::max() - total) {
+ return std::numeric_limits<int64_t>::max();
+ }
+ total += fieldMin;
+ }
+ return total;
+ }
+ default:
+ // boolean, int, long, bytes, string, enum, union, array, map and
+ // symbolic references all encode to at least one byte.
+ return 1;
+ }
+}
+
+// Default maximum number of zero-byte-encoded collection elements (e.g. an
+// array of nulls) to allocate from a single decode. Such elements consume no
+// input, so ensureCollectionAvailable cannot bound their count; without a cap
a
+// tiny payload can declare a huge block count and exhaust memory. Overridable
+// via the AVRO_MAX_COLLECTION_ITEMS environment variable.
+static constexpr int64_t kDefaultMaxCollectionItems = 10000000;
+
+// Structural cap on the number of elements in any array or map (an overflow /
+// defense-in-depth guard), matching the historical Integer.MAX_VALUE - 8
limit.
+// Non-zero-byte elements are also bounded by the bytes remaining.
+static constexpr int64_t kDefaultMaxCollectionStructural = 2147483639;
+
+// AVRO_MAX_COLLECTION_ITEMS, when a non-negative integer, caps both limits;
+// otherwise zero-byte elements use the tighter default and all collections use
+// the structural default.
+static void collectionLimits(int64_t &zeroByte, int64_t &structural) {
+ const char *env = std::getenv("AVRO_MAX_COLLECTION_ITEMS");
+ if (env != nullptr && *env != '\0') {
+ char *end = nullptr;
+ long long value = std::strtoll(env, &end, 10);
+ if (*end == '\0' && value >= 0) {
+ zeroByte = structural = static_cast<int64_t>(value);
+ return;
+ }
+ }
+ zeroByte = kDefaultMaxCollectionItems;
+ structural = kDefaultMaxCollectionStructural;
+}
+
+// Reject a collection (array or map) block whose declared element count could
+// not be backed by the bytes actually remaining, before resizing for it.
+// Skipped when the per-element minimum is zero, or when the decoder cannot
+// report how many bytes remain. The comparison divides to avoid overflow.
+static void ensureCollectionAvailable(Decoder &d, size_t existing, size_t
count, int64_t minBytes) {
+ if (count == 0 || minBytes <= 0) {
+ return;
+ }
+ int64_t remaining = d.bytesRemaining();
+ if (remaining >= 0 &&
+ static_cast<uint64_t>(count) >
+ static_cast<uint64_t>(remaining) /
static_cast<uint64_t>(minBytes)) {
+ throw Exception(
+ "Collection claims {} elements with at least {} bytes each, "
+ "but only {} bytes are available",
+ count, minBytes, remaining);
+ }
+ // Structural / overflow guard, also covering decoders that cannot report
the
+ // bytes remaining. Compare without adding so existing + count cannot
overflow.
+ int64_t zeroByte, structural;
+ collectionLimits(zeroByte, structural);
+ const uint64_t limit = static_cast<uint64_t>(structural);
+ if (static_cast<uint64_t>(count) > limit ||
+ static_cast<uint64_t>(existing) > limit -
static_cast<uint64_t>(count)) {
+ throw Exception(
+ "Cannot read a collection of more than {} elements; "
+ "set AVRO_MAX_COLLECTION_ITEMS if this is legitimate",
+ structural);
+ }
+}
+
+// Reject a collection of zero-byte elements (e.g. null) whose cumulative count
+// exceeds the configured limit. These elements consume no input, so they
cannot
+// be bounded by the bytes remaining; the count is the only signal.
+static void ensureZeroByteCollectionWithinLimit(size_t existing, size_t count)
{
+ int64_t zeroByte, structural;
+ collectionLimits(zeroByte, structural);
+ const uint64_t limit = static_cast<uint64_t>(zeroByte);
Review Comment:
ensureZeroByteCollectionWithinLimit() declares `structural` but never uses
it, which will fail the build under the project’s -Werror settings
(set-but-not-used).
##########
lang/c++/test/AvailableBytesTests.cc:
##########
@@ -0,0 +1,280 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+// A bytes/string value is encoded as a length prefix followed by that many
+// bytes of data. A malicious or truncated input can declare a huge length with
+// little or no actual data, which would otherwise trigger a correspondingly
+// huge allocation before the shortfall is noticed. When the input stream can
+// report how many bytes remain (a memory-backed stream), the declared length
+// must be rejected before allocating for it.
+
+#include <cstdint>
+#include <cstdlib>
+#include <memory>
+#include <string>
+#include <vector>
+
+#include <boost/test/included/unit_test.hpp>
+
+#include "Compiler.hh"
+#include "Decoder.hh"
+#include "Encoder.hh"
+#include "Exception.hh"
+#include "Generic.hh"
+#include "Stream.hh"
+#include "ValidSchema.hh"
+
+namespace avro {
+
+// Reading a bytes value whose declared length far exceeds the tiny backing
+// buffer must throw instead of attempting a huge allocation.
+static void testDecodeBytesRejectsOversizedLength() {
+ // 0xFE 0x01 is the zig-zag long 127; the buffer carries no data after it.
+ const uint8_t data[] = {0xFE, 0x01};
+ InputStreamPtr in = memoryInputStream(data, sizeof(data));
+ DecoderPtr d = binaryDecoder();
+ d->init(*in);
+ std::vector<uint8_t> value;
+ BOOST_CHECK_THROW(d->decodeBytes(value), Exception);
+}
+
+static void testDecodeStringRejectsOversizedLength() {
+ const uint8_t data[] = {0xFE, 0x01};
+ InputStreamPtr in = memoryInputStream(data, sizeof(data));
+ DecoderPtr d = binaryDecoder();
+ d->init(*in);
+ std::string value;
+ BOOST_CHECK_THROW(d->decodeString(value), Exception);
+}
+
+// A well-formed value whose declared length fits the buffer still decodes.
+static void testDecodeStringWithinLimitStillReads() {
+ // length 3 (zig-zag 6 -> 0x06) followed by "abc".
+ const uint8_t data[] = {0x06, 'a', 'b', 'c'};
+ InputStreamPtr in = memoryInputStream(data, sizeof(data));
+ DecoderPtr d = binaryDecoder();
+ d->init(*in);
+ std::string value;
+ d->decodeString(value);
+ BOOST_CHECK_EQUAL(value, std::string("abc"));
+}
+
+static void testDecodeBytesWithinLimitStillReads() {
+ const uint8_t data[] = {0x06, 0x01, 0x02, 0x03};
+ InputStreamPtr in = memoryInputStream(data, sizeof(data));
+ DecoderPtr d = binaryDecoder();
+ d->init(*in);
+ std::vector<uint8_t> value;
+ d->decodeBytes(value);
+ BOOST_CHECK_EQUAL(value.size(), 3u);
+ BOOST_CHECK_EQUAL(value[0], 1);
+ BOOST_CHECK_EQUAL(value[2], 3);
+}
+
+// An array/map block declares an element count; a malicious or truncated input
+// can declare far more elements than the remaining bytes could hold. The count
+// is validated against the bytes remaining before resizing, using the minimum
+// on-wire size of the element schema (so 0-byte elements like null are not
+// falsely rejected).
+static GenericDatum decodeCollectionHeader(const ValidSchema &s, int64_t
blockCount,
+ bool addEndMarker) {
+ // Keep the output stream alive for the whole decode: memoryInputStream
+ // references the output stream's buffer rather than copying it.
+ std::unique_ptr<OutputStream> os = memoryOutputStream();
+ EncoderPtr e = binaryEncoder();
+ e->init(*os);
+ e->encodeLong(blockCount);
+ if (addEndMarker) {
+ e->encodeLong(0);
+ }
+ e->flush();
+
+ InputStreamPtr in = memoryInputStream(*os);
+ DecoderPtr d = binaryDecoder();
+ d->init(*in);
+ GenericDatum datum;
+ GenericReader::read(*d, datum, s);
+ return datum;
+}
+
+static void testReadArrayRejectsOversizedCount() {
+ ValidSchema s = compileJsonSchemaFromString(
+ "{\"type\":\"array\",\"items\":\"long\"}");
+ // 1,000,000 long elements declared, but no element data follows.
+ BOOST_CHECK_THROW(decodeCollectionHeader(s, 1000000, false), Exception);
+}
+
+static void testReadMapRejectsOversizedCount() {
+ ValidSchema s = compileJsonSchemaFromString(
+ "{\"type\":\"map\",\"values\":\"long\"}");
+ BOOST_CHECK_THROW(decodeCollectionHeader(s, 1000000, false), Exception);
+}
+
+static void testReadArrayOfNullNotFalselyRejected() {
+ ValidSchema s = compileJsonSchemaFromString(
+ "{\"type\":\"array\",\"items\":\"null\"}");
+ // 100,000 nulls (zero bytes each) is legitimate and must decode.
+ GenericDatum datum = decodeCollectionHeader(s, 100000, true);
+ BOOST_CHECK_EQUAL(datum.value<GenericArray>().value().size(), 100000u);
+}
+
+// A deeply but acyclically nested record whose only leaf is null encodes to
+// zero bytes, so the per-element minimum must be 0 and a large array of such
+// records must not be falsely rejected. This exercises the recursion depth
+// guard in minBytesPerElement(), which must yield a conservative lower bound
+// (0) rather than over-estimating for a legitimately deep schema.
+static void testReadArrayOfDeeplyNestedNullNotFalselyRejected() {
+ // Build ~70 nested records: R0 { null f; }, R1 { R0 f; }, ... The nesting
+ // exceeds the depth guard, yet every leaf is null so the true minimum is
0.
+ std::string schema = "\"null\"";
+ for (int i = 0; i < 70; ++i) {
+ schema = "{\"type\":\"record\",\"name\":\"R" + std::to_string(i) +
+ "\",\"fields\":[{\"name\":\"f\",\"type\":" + schema + "}]}";
+ }
+ ValidSchema s = compileJsonSchemaFromString(
+ ("{\"type\":\"array\",\"items\":" + schema + "}").c_str());
+ // 100,000 zero-byte elements is legitimate and must decode.
+ GenericDatum datum = decodeCollectionHeader(s, 100000, true);
+ BOOST_CHECK_EQUAL(datum.value<GenericArray>().value().size(), 100000u);
+}
+
+// Calling bytesRemaining() immediately after init(), before any data has been
+// buffered, must be well-defined (the underlying StreamReader must not
subtract
+// null buffer pointers) and report the whole stream as available.
+static void testBytesRemainingRightAfterInit() {
+ const uint8_t data[] = {0x06, 'a', 'b', 'c'};
+ InputStreamPtr in = memoryInputStream(data, sizeof(data));
+ DecoderPtr d = binaryDecoder();
+ d->init(*in);
+ BOOST_CHECK_EQUAL(d->bytesRemaining(), static_cast<int64_t>(sizeof(data)));
+}
+
+// Zero-byte elements (null, a record with only zero-byte fields) consume no
+// input, so ensureCollectionAvailable cannot bound their count. A huge
declared
+// block count of such elements is capped against a configurable limit.
+static GenericDatum decodeLongs(const ValidSchema &s, const
std::vector<int64_t> &longs) {
+ std::unique_ptr<OutputStream> os = memoryOutputStream();
+ EncoderPtr e = binaryEncoder();
+ e->init(*os);
+ for (int64_t v : longs) {
+ e->encodeLong(v);
+ }
+ e->flush();
+ InputStreamPtr in = memoryInputStream(*os);
+ DecoderPtr d = binaryDecoder();
+ d->init(*in);
+ GenericDatum datum;
+ GenericReader::read(*d, datum, s);
+ return datum;
+}
+
+static void testReadArrayOfNullRejectsCountAboveDefaultLimit() {
+ ValidSchema s = compileJsonSchemaFromString(
+ "{\"type\":\"array\",\"items\":\"null\"}");
+ // The reported exploit: 200,000,000 nulls rejected by the default limit.
+ BOOST_CHECK_THROW(decodeCollectionHeader(s, 200000000, true), Exception);
+}
+
+static void testReadArrayOfAllNullRecordRejectsHugeCount() {
+ ValidSchema s = compileJsonSchemaFromString(
+ "{\"type\":\"array\",\"items\":"
+
"{\"type\":\"record\",\"name\":\"R\",\"fields\":[{\"name\":\"n\",\"type\":\"null\"}]}}");
+ BOOST_CHECK_THROW(decodeCollectionHeader(s, 200000000, true), Exception);
+}
+
+static void testReadArrayOfNullRejectsNegativeCount() {
+ ValidSchema s = compileJsonSchemaFromString(
+ "{\"type\":\"array\",\"items\":\"null\"}");
+ // Negative count (-200M), block byte-size 0, end marker 0: normalized to
+ // 200M and still bounded.
+ BOOST_CHECK_THROW(decodeLongs(s, {-200000000, 0, 0}), Exception);
+}
+
+static void testReadMapOfNullRejectedByAvailableBytes() {
+ ValidSchema s = compileJsonSchemaFromString(
+ "{\"type\":\"map\",\"values\":\"null\"}");
+ // Each map entry carries a >= 1 byte key, so a huge map<null> is bounded
by
+ // the bytes-remaining check.
+ BOOST_CHECK_THROW(decodeCollectionHeader(s, 200000000, false), Exception);
+}
+
+static void testReadArrayOfNullRespectsConfiguredLimit() {
+ ValidSchema s = compileJsonSchemaFromString(
+ "{\"type\":\"array\",\"items\":\"null\"}");
+ setenv("AVRO_MAX_COLLECTION_ITEMS", "1000", 1);
+ // Within the limit decodes; over the limit and cumulative are rejected.
+ GenericDatum ok = decodeCollectionHeader(s, 1000, true);
+ BOOST_CHECK_EQUAL(ok.value<GenericArray>().value().size(), 1000u);
+ BOOST_CHECK_THROW(decodeCollectionHeader(s, 1001, true), Exception);
+ BOOST_CHECK_THROW(decodeLongs(s, {600, 600, 0}), Exception);
+ unsetenv("AVRO_MAX_COLLECTION_ITEMS");
Review Comment:
These tests use setenv()/unsetenv(), which are not available on Windows
builds. Also, unconditionally unsetting the variable can leak state if
AVRO_MAX_COLLECTION_ITEMS was already set in the environment before the test
ran. Prefer a cross-platform set/unset that restores the previous value.
##########
lang/c++/test/AvailableBytesTests.cc:
##########
@@ -0,0 +1,280 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+// A bytes/string value is encoded as a length prefix followed by that many
+// bytes of data. A malicious or truncated input can declare a huge length with
+// little or no actual data, which would otherwise trigger a correspondingly
+// huge allocation before the shortfall is noticed. When the input stream can
+// report how many bytes remain (a memory-backed stream), the declared length
+// must be rejected before allocating for it.
+
+#include <cstdint>
+#include <cstdlib>
+#include <memory>
+#include <string>
+#include <vector>
+
+#include <boost/test/included/unit_test.hpp>
+
+#include "Compiler.hh"
+#include "Decoder.hh"
+#include "Encoder.hh"
+#include "Exception.hh"
+#include "Generic.hh"
+#include "Stream.hh"
+#include "ValidSchema.hh"
+
+namespace avro {
+
+// Reading a bytes value whose declared length far exceeds the tiny backing
+// buffer must throw instead of attempting a huge allocation.
+static void testDecodeBytesRejectsOversizedLength() {
+ // 0xFE 0x01 is the zig-zag long 127; the buffer carries no data after it.
+ const uint8_t data[] = {0xFE, 0x01};
+ InputStreamPtr in = memoryInputStream(data, sizeof(data));
+ DecoderPtr d = binaryDecoder();
+ d->init(*in);
+ std::vector<uint8_t> value;
+ BOOST_CHECK_THROW(d->decodeBytes(value), Exception);
+}
+
+static void testDecodeStringRejectsOversizedLength() {
+ const uint8_t data[] = {0xFE, 0x01};
+ InputStreamPtr in = memoryInputStream(data, sizeof(data));
+ DecoderPtr d = binaryDecoder();
+ d->init(*in);
+ std::string value;
+ BOOST_CHECK_THROW(d->decodeString(value), Exception);
+}
+
+// A well-formed value whose declared length fits the buffer still decodes.
+static void testDecodeStringWithinLimitStillReads() {
+ // length 3 (zig-zag 6 -> 0x06) followed by "abc".
+ const uint8_t data[] = {0x06, 'a', 'b', 'c'};
+ InputStreamPtr in = memoryInputStream(data, sizeof(data));
+ DecoderPtr d = binaryDecoder();
+ d->init(*in);
+ std::string value;
+ d->decodeString(value);
+ BOOST_CHECK_EQUAL(value, std::string("abc"));
+}
+
+static void testDecodeBytesWithinLimitStillReads() {
+ const uint8_t data[] = {0x06, 0x01, 0x02, 0x03};
+ InputStreamPtr in = memoryInputStream(data, sizeof(data));
+ DecoderPtr d = binaryDecoder();
+ d->init(*in);
+ std::vector<uint8_t> value;
+ d->decodeBytes(value);
+ BOOST_CHECK_EQUAL(value.size(), 3u);
+ BOOST_CHECK_EQUAL(value[0], 1);
+ BOOST_CHECK_EQUAL(value[2], 3);
+}
+
+// An array/map block declares an element count; a malicious or truncated input
+// can declare far more elements than the remaining bytes could hold. The count
+// is validated against the bytes remaining before resizing, using the minimum
+// on-wire size of the element schema (so 0-byte elements like null are not
+// falsely rejected).
+static GenericDatum decodeCollectionHeader(const ValidSchema &s, int64_t
blockCount,
+ bool addEndMarker) {
+ // Keep the output stream alive for the whole decode: memoryInputStream
+ // references the output stream's buffer rather than copying it.
+ std::unique_ptr<OutputStream> os = memoryOutputStream();
+ EncoderPtr e = binaryEncoder();
+ e->init(*os);
+ e->encodeLong(blockCount);
+ if (addEndMarker) {
+ e->encodeLong(0);
+ }
+ e->flush();
+
+ InputStreamPtr in = memoryInputStream(*os);
+ DecoderPtr d = binaryDecoder();
+ d->init(*in);
+ GenericDatum datum;
+ GenericReader::read(*d, datum, s);
+ return datum;
+}
+
+static void testReadArrayRejectsOversizedCount() {
+ ValidSchema s = compileJsonSchemaFromString(
+ "{\"type\":\"array\",\"items\":\"long\"}");
+ // 1,000,000 long elements declared, but no element data follows.
+ BOOST_CHECK_THROW(decodeCollectionHeader(s, 1000000, false), Exception);
+}
+
+static void testReadMapRejectsOversizedCount() {
+ ValidSchema s = compileJsonSchemaFromString(
+ "{\"type\":\"map\",\"values\":\"long\"}");
+ BOOST_CHECK_THROW(decodeCollectionHeader(s, 1000000, false), Exception);
+}
+
+static void testReadArrayOfNullNotFalselyRejected() {
+ ValidSchema s = compileJsonSchemaFromString(
+ "{\"type\":\"array\",\"items\":\"null\"}");
+ // 100,000 nulls (zero bytes each) is legitimate and must decode.
+ GenericDatum datum = decodeCollectionHeader(s, 100000, true);
+ BOOST_CHECK_EQUAL(datum.value<GenericArray>().value().size(), 100000u);
+}
+
+// A deeply but acyclically nested record whose only leaf is null encodes to
+// zero bytes, so the per-element minimum must be 0 and a large array of such
+// records must not be falsely rejected. This exercises the recursion depth
+// guard in minBytesPerElement(), which must yield a conservative lower bound
+// (0) rather than over-estimating for a legitimately deep schema.
+static void testReadArrayOfDeeplyNestedNullNotFalselyRejected() {
+ // Build ~70 nested records: R0 { null f; }, R1 { R0 f; }, ... The nesting
+ // exceeds the depth guard, yet every leaf is null so the true minimum is
0.
+ std::string schema = "\"null\"";
+ for (int i = 0; i < 70; ++i) {
+ schema = "{\"type\":\"record\",\"name\":\"R" + std::to_string(i) +
+ "\",\"fields\":[{\"name\":\"f\",\"type\":" + schema + "}]}";
+ }
+ ValidSchema s = compileJsonSchemaFromString(
+ ("{\"type\":\"array\",\"items\":" + schema + "}").c_str());
+ // 100,000 zero-byte elements is legitimate and must decode.
+ GenericDatum datum = decodeCollectionHeader(s, 100000, true);
+ BOOST_CHECK_EQUAL(datum.value<GenericArray>().value().size(), 100000u);
+}
+
+// Calling bytesRemaining() immediately after init(), before any data has been
+// buffered, must be well-defined (the underlying StreamReader must not
subtract
+// null buffer pointers) and report the whole stream as available.
+static void testBytesRemainingRightAfterInit() {
+ const uint8_t data[] = {0x06, 'a', 'b', 'c'};
+ InputStreamPtr in = memoryInputStream(data, sizeof(data));
+ DecoderPtr d = binaryDecoder();
+ d->init(*in);
+ BOOST_CHECK_EQUAL(d->bytesRemaining(), static_cast<int64_t>(sizeof(data)));
+}
+
+// Zero-byte elements (null, a record with only zero-byte fields) consume no
+// input, so ensureCollectionAvailable cannot bound their count. A huge
declared
+// block count of such elements is capped against a configurable limit.
+static GenericDatum decodeLongs(const ValidSchema &s, const
std::vector<int64_t> &longs) {
+ std::unique_ptr<OutputStream> os = memoryOutputStream();
+ EncoderPtr e = binaryEncoder();
+ e->init(*os);
+ for (int64_t v : longs) {
+ e->encodeLong(v);
+ }
+ e->flush();
+ InputStreamPtr in = memoryInputStream(*os);
+ DecoderPtr d = binaryDecoder();
+ d->init(*in);
+ GenericDatum datum;
+ GenericReader::read(*d, datum, s);
+ return datum;
+}
+
+static void testReadArrayOfNullRejectsCountAboveDefaultLimit() {
+ ValidSchema s = compileJsonSchemaFromString(
+ "{\"type\":\"array\",\"items\":\"null\"}");
+ // The reported exploit: 200,000,000 nulls rejected by the default limit.
+ BOOST_CHECK_THROW(decodeCollectionHeader(s, 200000000, true), Exception);
+}
+
+static void testReadArrayOfAllNullRecordRejectsHugeCount() {
+ ValidSchema s = compileJsonSchemaFromString(
+ "{\"type\":\"array\",\"items\":"
+
"{\"type\":\"record\",\"name\":\"R\",\"fields\":[{\"name\":\"n\",\"type\":\"null\"}]}}");
+ BOOST_CHECK_THROW(decodeCollectionHeader(s, 200000000, true), Exception);
+}
+
+static void testReadArrayOfNullRejectsNegativeCount() {
+ ValidSchema s = compileJsonSchemaFromString(
+ "{\"type\":\"array\",\"items\":\"null\"}");
+ // Negative count (-200M), block byte-size 0, end marker 0: normalized to
+ // 200M and still bounded.
+ BOOST_CHECK_THROW(decodeLongs(s, {-200000000, 0, 0}), Exception);
+}
+
+static void testReadMapOfNullRejectedByAvailableBytes() {
+ ValidSchema s = compileJsonSchemaFromString(
+ "{\"type\":\"map\",\"values\":\"null\"}");
+ // Each map entry carries a >= 1 byte key, so a huge map<null> is bounded
by
+ // the bytes-remaining check.
+ BOOST_CHECK_THROW(decodeCollectionHeader(s, 200000000, false), Exception);
+}
+
+static void testReadArrayOfNullRespectsConfiguredLimit() {
+ ValidSchema s = compileJsonSchemaFromString(
+ "{\"type\":\"array\",\"items\":\"null\"}");
+ setenv("AVRO_MAX_COLLECTION_ITEMS", "1000", 1);
+ // Within the limit decodes; over the limit and cumulative are rejected.
+ GenericDatum ok = decodeCollectionHeader(s, 1000, true);
+ BOOST_CHECK_EQUAL(ok.value<GenericArray>().value().size(), 1000u);
+ BOOST_CHECK_THROW(decodeCollectionHeader(s, 1001, true), Exception);
+ BOOST_CHECK_THROW(decodeLongs(s, {600, 600, 0}), Exception);
+ unsetenv("AVRO_MAX_COLLECTION_ITEMS");
+}
+
+// A backed non-zero-byte array that passes the bytes check is still bounded by
+// the structural cap (exercised with a lowered limit).
+static void testReadArrayOfLongRejectedByStructuralCap() {
+ ValidSchema s = compileJsonSchemaFromString(
+ "{\"type\":\"array\",\"items\":\"long\"}");
+ setenv("AVRO_MAX_COLLECTION_ITEMS", "5", 1);
+ // 10 real longs: block count 10, ten 1-byte longs, end marker.
+ std::vector<int64_t> longs = {10, 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 0};
+ BOOST_CHECK_THROW(decodeLongs(s, longs), Exception);
+ unsetenv("AVRO_MAX_COLLECTION_ITEMS");
+}
+
+// Skipping a huge array (via the decoder's skipArray) is bounded by the
+// structural cap.
+static void testSkipArrayRejectsHugeCount() {
+ setenv("AVRO_MAX_COLLECTION_ITEMS", "1000", 1);
+ std::unique_ptr<OutputStream> os = memoryOutputStream();
+ EncoderPtr e = binaryEncoder();
+ e->init(*os);
+ e->encodeLong(2000); // block count 2000, no data needed (skip throws
first)
+ e->flush();
+ InputStreamPtr in = memoryInputStream(*os);
+ DecoderPtr d = binaryDecoder();
+ d->init(*in);
+ BOOST_CHECK_THROW(d->skipArray(), Exception);
+ unsetenv("AVRO_MAX_COLLECTION_ITEMS");
+}
Review Comment:
Same portability/state-leak issue for AVRO_MAX_COLLECTION_ITEMS in this
test: use a cross-platform setter and restore the previous value after the
assertion.
##########
lang/c++/test/AvailableBytesTests.cc:
##########
@@ -0,0 +1,280 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+// A bytes/string value is encoded as a length prefix followed by that many
+// bytes of data. A malicious or truncated input can declare a huge length with
+// little or no actual data, which would otherwise trigger a correspondingly
+// huge allocation before the shortfall is noticed. When the input stream can
+// report how many bytes remain (a memory-backed stream), the declared length
+// must be rejected before allocating for it.
+
+#include <cstdint>
+#include <cstdlib>
+#include <memory>
+#include <string>
+#include <vector>
+
+#include <boost/test/included/unit_test.hpp>
+
+#include "Compiler.hh"
+#include "Decoder.hh"
+#include "Encoder.hh"
+#include "Exception.hh"
+#include "Generic.hh"
+#include "Stream.hh"
+#include "ValidSchema.hh"
+
+namespace avro {
+
+// Reading a bytes value whose declared length far exceeds the tiny backing
+// buffer must throw instead of attempting a huge allocation.
+static void testDecodeBytesRejectsOversizedLength() {
+ // 0xFE 0x01 is the zig-zag long 127; the buffer carries no data after it.
+ const uint8_t data[] = {0xFE, 0x01};
+ InputStreamPtr in = memoryInputStream(data, sizeof(data));
+ DecoderPtr d = binaryDecoder();
+ d->init(*in);
+ std::vector<uint8_t> value;
+ BOOST_CHECK_THROW(d->decodeBytes(value), Exception);
+}
+
+static void testDecodeStringRejectsOversizedLength() {
+ const uint8_t data[] = {0xFE, 0x01};
+ InputStreamPtr in = memoryInputStream(data, sizeof(data));
+ DecoderPtr d = binaryDecoder();
+ d->init(*in);
+ std::string value;
+ BOOST_CHECK_THROW(d->decodeString(value), Exception);
+}
+
+// A well-formed value whose declared length fits the buffer still decodes.
+static void testDecodeStringWithinLimitStillReads() {
+ // length 3 (zig-zag 6 -> 0x06) followed by "abc".
+ const uint8_t data[] = {0x06, 'a', 'b', 'c'};
+ InputStreamPtr in = memoryInputStream(data, sizeof(data));
+ DecoderPtr d = binaryDecoder();
+ d->init(*in);
+ std::string value;
+ d->decodeString(value);
+ BOOST_CHECK_EQUAL(value, std::string("abc"));
+}
+
+static void testDecodeBytesWithinLimitStillReads() {
+ const uint8_t data[] = {0x06, 0x01, 0x02, 0x03};
+ InputStreamPtr in = memoryInputStream(data, sizeof(data));
+ DecoderPtr d = binaryDecoder();
+ d->init(*in);
+ std::vector<uint8_t> value;
+ d->decodeBytes(value);
+ BOOST_CHECK_EQUAL(value.size(), 3u);
+ BOOST_CHECK_EQUAL(value[0], 1);
+ BOOST_CHECK_EQUAL(value[2], 3);
+}
+
+// An array/map block declares an element count; a malicious or truncated input
+// can declare far more elements than the remaining bytes could hold. The count
+// is validated against the bytes remaining before resizing, using the minimum
+// on-wire size of the element schema (so 0-byte elements like null are not
+// falsely rejected).
+static GenericDatum decodeCollectionHeader(const ValidSchema &s, int64_t
blockCount,
+ bool addEndMarker) {
+ // Keep the output stream alive for the whole decode: memoryInputStream
+ // references the output stream's buffer rather than copying it.
+ std::unique_ptr<OutputStream> os = memoryOutputStream();
+ EncoderPtr e = binaryEncoder();
+ e->init(*os);
+ e->encodeLong(blockCount);
+ if (addEndMarker) {
+ e->encodeLong(0);
+ }
+ e->flush();
+
+ InputStreamPtr in = memoryInputStream(*os);
+ DecoderPtr d = binaryDecoder();
+ d->init(*in);
+ GenericDatum datum;
+ GenericReader::read(*d, datum, s);
+ return datum;
+}
+
+static void testReadArrayRejectsOversizedCount() {
+ ValidSchema s = compileJsonSchemaFromString(
+ "{\"type\":\"array\",\"items\":\"long\"}");
+ // 1,000,000 long elements declared, but no element data follows.
+ BOOST_CHECK_THROW(decodeCollectionHeader(s, 1000000, false), Exception);
+}
+
+static void testReadMapRejectsOversizedCount() {
+ ValidSchema s = compileJsonSchemaFromString(
+ "{\"type\":\"map\",\"values\":\"long\"}");
+ BOOST_CHECK_THROW(decodeCollectionHeader(s, 1000000, false), Exception);
+}
+
+static void testReadArrayOfNullNotFalselyRejected() {
+ ValidSchema s = compileJsonSchemaFromString(
+ "{\"type\":\"array\",\"items\":\"null\"}");
+ // 100,000 nulls (zero bytes each) is legitimate and must decode.
+ GenericDatum datum = decodeCollectionHeader(s, 100000, true);
+ BOOST_CHECK_EQUAL(datum.value<GenericArray>().value().size(), 100000u);
+}
+
+// A deeply but acyclically nested record whose only leaf is null encodes to
+// zero bytes, so the per-element minimum must be 0 and a large array of such
+// records must not be falsely rejected. This exercises the recursion depth
+// guard in minBytesPerElement(), which must yield a conservative lower bound
+// (0) rather than over-estimating for a legitimately deep schema.
+static void testReadArrayOfDeeplyNestedNullNotFalselyRejected() {
+ // Build ~70 nested records: R0 { null f; }, R1 { R0 f; }, ... The nesting
+ // exceeds the depth guard, yet every leaf is null so the true minimum is
0.
+ std::string schema = "\"null\"";
+ for (int i = 0; i < 70; ++i) {
+ schema = "{\"type\":\"record\",\"name\":\"R" + std::to_string(i) +
+ "\",\"fields\":[{\"name\":\"f\",\"type\":" + schema + "}]}";
+ }
+ ValidSchema s = compileJsonSchemaFromString(
+ ("{\"type\":\"array\",\"items\":" + schema + "}").c_str());
+ // 100,000 zero-byte elements is legitimate and must decode.
+ GenericDatum datum = decodeCollectionHeader(s, 100000, true);
+ BOOST_CHECK_EQUAL(datum.value<GenericArray>().value().size(), 100000u);
+}
+
+// Calling bytesRemaining() immediately after init(), before any data has been
+// buffered, must be well-defined (the underlying StreamReader must not
subtract
+// null buffer pointers) and report the whole stream as available.
+static void testBytesRemainingRightAfterInit() {
+ const uint8_t data[] = {0x06, 'a', 'b', 'c'};
+ InputStreamPtr in = memoryInputStream(data, sizeof(data));
+ DecoderPtr d = binaryDecoder();
+ d->init(*in);
+ BOOST_CHECK_EQUAL(d->bytesRemaining(), static_cast<int64_t>(sizeof(data)));
+}
+
+// Zero-byte elements (null, a record with only zero-byte fields) consume no
+// input, so ensureCollectionAvailable cannot bound their count. A huge
declared
+// block count of such elements is capped against a configurable limit.
+static GenericDatum decodeLongs(const ValidSchema &s, const
std::vector<int64_t> &longs) {
+ std::unique_ptr<OutputStream> os = memoryOutputStream();
+ EncoderPtr e = binaryEncoder();
+ e->init(*os);
+ for (int64_t v : longs) {
+ e->encodeLong(v);
+ }
+ e->flush();
+ InputStreamPtr in = memoryInputStream(*os);
+ DecoderPtr d = binaryDecoder();
+ d->init(*in);
+ GenericDatum datum;
+ GenericReader::read(*d, datum, s);
+ return datum;
+}
+
+static void testReadArrayOfNullRejectsCountAboveDefaultLimit() {
+ ValidSchema s = compileJsonSchemaFromString(
+ "{\"type\":\"array\",\"items\":\"null\"}");
+ // The reported exploit: 200,000,000 nulls rejected by the default limit.
+ BOOST_CHECK_THROW(decodeCollectionHeader(s, 200000000, true), Exception);
+}
+
+static void testReadArrayOfAllNullRecordRejectsHugeCount() {
+ ValidSchema s = compileJsonSchemaFromString(
+ "{\"type\":\"array\",\"items\":"
+
"{\"type\":\"record\",\"name\":\"R\",\"fields\":[{\"name\":\"n\",\"type\":\"null\"}]}}");
+ BOOST_CHECK_THROW(decodeCollectionHeader(s, 200000000, true), Exception);
+}
+
+static void testReadArrayOfNullRejectsNegativeCount() {
+ ValidSchema s = compileJsonSchemaFromString(
+ "{\"type\":\"array\",\"items\":\"null\"}");
+ // Negative count (-200M), block byte-size 0, end marker 0: normalized to
+ // 200M and still bounded.
+ BOOST_CHECK_THROW(decodeLongs(s, {-200000000, 0, 0}), Exception);
+}
+
+static void testReadMapOfNullRejectedByAvailableBytes() {
+ ValidSchema s = compileJsonSchemaFromString(
+ "{\"type\":\"map\",\"values\":\"null\"}");
+ // Each map entry carries a >= 1 byte key, so a huge map<null> is bounded
by
+ // the bytes-remaining check.
+ BOOST_CHECK_THROW(decodeCollectionHeader(s, 200000000, false), Exception);
+}
+
+static void testReadArrayOfNullRespectsConfiguredLimit() {
+ ValidSchema s = compileJsonSchemaFromString(
+ "{\"type\":\"array\",\"items\":\"null\"}");
+ setenv("AVRO_MAX_COLLECTION_ITEMS", "1000", 1);
+ // Within the limit decodes; over the limit and cumulative are rejected.
+ GenericDatum ok = decodeCollectionHeader(s, 1000, true);
+ BOOST_CHECK_EQUAL(ok.value<GenericArray>().value().size(), 1000u);
+ BOOST_CHECK_THROW(decodeCollectionHeader(s, 1001, true), Exception);
+ BOOST_CHECK_THROW(decodeLongs(s, {600, 600, 0}), Exception);
+ unsetenv("AVRO_MAX_COLLECTION_ITEMS");
+}
+
+// A backed non-zero-byte array that passes the bytes check is still bounded by
+// the structural cap (exercised with a lowered limit).
+static void testReadArrayOfLongRejectedByStructuralCap() {
+ ValidSchema s = compileJsonSchemaFromString(
+ "{\"type\":\"array\",\"items\":\"long\"}");
+ setenv("AVRO_MAX_COLLECTION_ITEMS", "5", 1);
+ // 10 real longs: block count 10, ten 1-byte longs, end marker.
+ std::vector<int64_t> longs = {10, 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 0};
+ BOOST_CHECK_THROW(decodeLongs(s, longs), Exception);
+ unsetenv("AVRO_MAX_COLLECTION_ITEMS");
Review Comment:
Same issue here: setenv()/unsetenv() is POSIX-only and the test should
restore any pre-existing AVRO_MAX_COLLECTION_ITEMS value instead of always
unsetting it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
