iemejia commented on code in PR #3860:
URL: https://github.com/apache/avro/pull/3860#discussion_r3567435614
##########
lang/csharp/src/apache/main/Generic/GenericReader.cs:
##########
@@ -404,11 +404,49 @@ protected virtual object ReadArray(object reuse,
ArraySchema writerSchema, Schem
ArraySchema rs = (ArraySchema)readerSchema;
object result = CreateArray(reuse, rs);
int i = 0;
- for (int n = (int)d.ReadArrayStart(); n != 0; n =
(int)d.ReadArrayNext())
+ long minBytes = MinBytesPerElement(writerSchema.ItemSchema);
+ long total = 0;
+ for (long nl = d.ReadArrayStart(); nl != 0; nl = d.ReadArrayNext())
{
- if (GetArraySize(result) < (i + n)) ResizeArray(ref result, i
+ n);
+ // Reject a block whose element count could not be backed by
the
+ // bytes remaining (or, for zero-byte elements, that exceeds
the
+ // item cap) before allocating for it. Checked on the raw long,
+ // which also avoids the int cast below overflowing.
+ total = EnsureCollectionAvailable(d, total, nl, minBytes);
+ int n = (int)nl;
+ // Preallocate only a bounded amount up front, then grow on
demand
+ // below. On a non-seekable stream EnsureCollectionAvailable
cannot
+ // bound the count, so resizing straight to i+n could allocate
a
+ // huge array before any element is read; a truncated stream
instead
+ // fails within Read() after a bounded growth. Blocks no
larger than
+ // the cap keep the original single-resize fast path.
+ int prealloc = i + Math.Min(n, MaxCollectionPrealloc);
+ if (GetArraySize(result) < prealloc) ResizeArray(ref result,
prealloc);
Review Comment:
Fixed — prealloc is now computed in long and clamped to
MaxCollectionStructural before the int cast, so a large i near the cap can't
overflow the sum.
##########
lang/csharp/src/apache/main/Generic/GenericReader.cs:
##########
@@ -501,6 +544,166 @@ protected virtual object ReadMap(object reuse, MapSchema
writerSchema, Schema re
return result;
}
+ /// <summary>
+ /// Minimum number of bytes a single value of the given schema 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 encodes to zero bytes
+ /// returns 0 (not only <c>null</c>, but also composites that encode to
+ /// nothing, e.g. a record whose fields are all zero-byte), which
disables
+ /// the bytes-remaining check for it (so an array of such elements is
not
+ /// falsely rejected; they are instead bounded by the zero-byte item
cap).
+ /// A depth limit breaks self-referencing schemas.
+ /// </summary>
+ private static int MinBytesPerElement(Schema schema, int depth = 0)
+ {
+ if (schema == null)
+ {
+ return 0;
+ }
+
+ switch (schema.Tag)
+ {
+ case Schema.Type.Null:
+ return 0;
+ case Schema.Type.Float:
+ return 4;
+ case Schema.Type.Double:
+ return 8;
+ case Schema.Type.Fixed:
+ return ((FixedSchema)schema).Size;
+ case Schema.Type.Record:
+ case Schema.Type.Error:
+ if (depth > 64)
+ {
+ // A cyclic or pathologically deep record. Return 1
(not
+ // 0) so the collection check stays enabled; a valid
+ // recursive value always encodes to >= 1 byte. The
depth
+ // guard is applied only here, so zero-byte leaf types
+ // such as null still return 0 regardless of depth.
+ return 1;
+ }
+
+ // Accumulate in a long and clamp so a deeply nested schema
+ // cannot overflow int into a value <= 0, which would
disable
+ // the collection check.
+ long total = 0;
+ foreach (Field f in (RecordSchema)schema)
+ {
+ total += MinBytesPerElement(f.Schema, depth + 1);
+ if (total >= int.MaxValue)
+ {
+ return int.MaxValue;
+ }
+ }
+
+ return (int)total;
+ default:
+ // boolean, int, long, bytes, string, enum, union, array,
map:
+ // all encode to at least one byte.
+ return 1;
+ }
+ }
+
+ // Collection allocation limits, guarding against a block-count DoS.
Both
+ // default to the same values as the other Avro SDKs and can be
overridden
+ // (to a single value capping both) via the AVRO_MAX_COLLECTION_ITEMS
+ // environment variable.
+ private static readonly long MaxCollectionItems =
ReadCollectionLimit(10_000_000L);
+
+ // The largest array the runtime can allocate. Mirrors
+ // BinaryDecoder.MaxDotNetArrayLength: the default reader grows its
backing
+ // array via Array.Resize, which throws
(OutOfMemoryException/OverflowException)
+ // above this length rather than a deterministic AvroException.
+#if NETSTANDARD2_0
+ private const int MaxDotNetArrayLength = 0x3FFFFFFF;
+#else
+ private const int MaxDotNetArrayLength = 0x7FFFFFC7;
+#endif
+
+ // The structural cap is additionally clamped to the runtime's maximum
+ // array length: the callers cast the (cumulative) block count to int
to
+ // size .NET collections, and a limit above the max array length (e.g.
from
+ // a large env override, or int.MaxValue itself) would let a collection
+ // that passes EnsureCollectionAvailable still fault inside
Array.Resize
+ // instead of failing deterministically.
+ private static readonly long MaxCollectionStructural =
+ Math.Min(ReadCollectionLimit(2147483639L), MaxDotNetArrayLength);
+
+ // Upper bound on how many elements the backing array is grown by in a
+ // single step while decoding. The array still grows to hold every
element
+ // actually read; this only avoids resizing to the full (possibly
+ // attacker-declared) block count up front, before any element is read.
+ // That matters most for non-seekable streams, where the
bytes-available
+ // check cannot bound the declared count, so a single Array.Resize to
the
+ // block count could allocate a huge array before the truncated stream
is
+ // detected.
+ private const int MaxCollectionPrealloc = 1024;
+
+ private static long ReadCollectionLimit(long defaultValue)
+ {
+ string env =
Environment.GetEnvironmentVariable("AVRO_MAX_COLLECTION_ITEMS");
+ if (!string.IsNullOrEmpty(env) && long.TryParse(env, out long
value) && value >= 0)
+ {
+ return value;
+ }
+
+ return defaultValue;
+ }
+
+ /// <summary>
+ /// Rejects a collection (array or map) block that could drive an
unbounded
+ /// allocation, before allocating for it. A block whose declared
element
+ /// count could not be backed by the bytes actually remaining is
rejected;
+ /// zero-byte element blocks (where the bytes-remaining check does not
+ /// apply) are bounded by a cumulative item cap; and every collection
is
+ /// bounded by a structural cap. Returns the running total across
blocks.
+ /// </summary>
+ private static long EnsureCollectionAvailable(Decoder d, long total,
long count, long minBytesPerElement)
+ {
+ // A negative count is corrupt/malicious data (it can also arise
from
+ // long.MinValue overflow when negating a negative block count),
and
+ // the callers cast the block count to int; reject it explicitly.
+ if (count < 0)
+ {
+ throw new AvroException($"Invalid negative collection block
count: {count}");
+ }
+
+ // Reject before adding so an oversized block count cannot overflow
+ // `total` (wrapping it negative and bypassing the caps below). The
+ // running total is always <= MaxCollectionStructural on entry (the
+ // invariant this method maintains) and count >= 0, so the
subtraction
+ // cannot underflow or overflow.
+ if (count > MaxCollectionStructural - total)
+ {
+ throw new AvroException(
+ $"Collection block count {count} exceeds the maximum
allowed size of {MaxCollectionStructural}");
Review Comment:
Fixed — the message now reads 'Collection size {total} + {count} exceeds
...' to reflect that the guard fires on the cumulative total, not the block
count alone.
--
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]
