Copilot commented on code in PR #3860:
URL: https://github.com/apache/avro/pull/3860#discussion_r3565068925
##########
lang/csharp/src/apache/main/IO/BinaryDecoder.cs:
##########
@@ -264,11 +264,48 @@ public void SkipFixed(int len)
// Read p bytes into a new byte buffer
private byte[] read(long p)
{
+ EnsureAvailableBytes(p);
byte[] buffer = new byte[p];
Read(buffer, 0, buffer.Length);
return buffer;
}
+ /// <summary>
+ /// When the underlying stream can report its length, verifies that at
+ /// least <paramref name="length"/> bytes remain before the caller
+ /// allocates a buffer of that size. This guards against an
+ /// out-of-memory attack from a malicious or truncated input that
+ /// declares a huge length prefix but carries little actual data. The
+ /// check is skipped for non-seekable streams, whose remaining length
is
+ /// unknown.
+ /// </summary>
+ /// <param name="length">Number of bytes about to be read.</param>
+ internal void EnsureAvailableBytes(long length)
+ {
+ if (length > 0)
+ {
+ long remaining = RemainingBytes();
+ if (remaining >= 0 && length > remaining)
+ {
+ throw new AvroException(
+ $"Cannot read {length} bytes, only {remaining} bytes
remaining in the stream");
+ }
+ }
+ }
Review Comment:
EnsureAvailableBytes() only runs when length > 0, so a negative length
(e.g., from ReadBytes()) bypasses validation and can flow into array
allocation, resulting in runtime exceptions unrelated to Avro input validation.
It should reject negative lengths explicitly (even if ReadString already guards
its own length).
##########
lang/csharp/src/apache/main/Generic/GenericReader.cs:
##########
@@ -501,6 +511,71 @@ 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 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 schemas.
+ /// </summary>
+ private static int MinBytesPerElement(Schema schema, int depth = 0)
+ {
+ if (schema == null || depth > 64)
+ {
+ 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:
+ int total = 0;
+ foreach (Field f in (RecordSchema)schema)
+ {
+ total += MinBytesPerElement(f.Schema, depth + 1);
+ }
+
+ return total;
+ default:
+ // boolean, int, long, bytes, string, enum, union, array,
map:
+ // all encode to at least one byte.
+ return 1;
+ }
+ }
+
+ /// <summary>
+ /// Rejects a collection (array or map) block whose declared element
count
+ /// could not be backed by the bytes actually remaining, before
+ /// allocating. Skipped when the per-element minimum is zero, or when
the
+ /// decoder cannot report how many bytes remain.
+ /// </summary>
+ private static void EnsureCollectionAvailable(Decoder d, long count,
int minBytesPerElement)
+ {
+ if (count <= 0 || minBytesPerElement <= 0)
+ {
+ return;
+ }
+
+ if (d is BinaryDecoder bd)
+ {
+ long remaining = bd.RemainingBytes();
+ if (remaining >= 0 && count > remaining / minBytesPerElement)
+ {
+ throw new AvroException(
+ $"Collection claims {count} elements with at least
{minBytesPerElement} bytes each, but only {remaining} bytes are available");
+ }
+ }
+ }
Review Comment:
EnsureCollectionAvailable() returns early when minBytesPerElement <= 0
(e.g., arrays of null), but ReadArray/ReadMap still cast the raw long block
count to int. A crafted block count > int.MaxValue will overflow that cast and
can lead to negative sizes/incorrect allocations or infinite/incorrect loops.
Add an explicit int.MaxValue guard on the raw long count before any casts
(independent of minBytesPerElement).
##########
lang/csharp/src/apache/main/Generic/GenericReader.cs:
##########
@@ -501,6 +511,71 @@ 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 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 schemas.
+ /// </summary>
+ private static int MinBytesPerElement(Schema schema, int depth = 0)
+ {
+ if (schema == null || depth > 64)
+ {
+ 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:
+ int total = 0;
+ foreach (Field f in (RecordSchema)schema)
+ {
+ total += MinBytesPerElement(f.Schema, depth + 1);
+ }
+
+ return total;
+ default:
Review Comment:
MinBytesPerElement() sums record field minima into an int, which can
overflow and become <= 0. Because EnsureCollectionAvailable() treats
minBytesPerElement <= 0 as “skip checks”, an attacker-controlled schema with
enough fixed/record nesting could bypass the block-size validation. Use a long
accumulator and clamp to int.MaxValue to avoid overflow-induced bypass.
##########
lang/csharp/src/apache/test/IO/BinaryCodecTests.cs:
##########
@@ -430,5 +435,124 @@ public void TestFixed(int size)
TestSkip(b, (Decoder d) => d.SkipFixed(size),
(Encoder e, byte[] t) => e.WriteFixed(t), size);
}
+
+ // A bytes/string value is a length prefix followed by that many bytes.
+ // A malicious or truncated input can declare a huge length with little
+ // actual data; on a seekable stream the reader must reject it before
+ // allocating, rather than attempting a huge allocation.
+ [Test]
+ public void TestReadBytesRejectsLengthBeyondStream()
+ {
+ MemoryStream ms = new MemoryStream();
+ Encoder e = new BinaryEncoder(ms);
+ e.WriteLong(1_000_000); // declares 1,000,000 bytes...
+ ms.Position = 0; // ...but no data follows
+ Decoder d = new BinaryDecoder(ms);
+ Assert.Throws<AvroException>(() => d.ReadBytes());
+ }
+
+ [Test]
+ public void TestReadStringRejectsLengthBeyondStream()
+ {
+ MemoryStream ms = new MemoryStream();
+ Encoder e = new BinaryEncoder(ms);
+ e.WriteLong(1_000_000); // declares 1,000,000 bytes...
+ ms.Position = 0; // ...but no data follows
+ Decoder d = new BinaryDecoder(ms);
+ Assert.Throws<AvroException>(() => d.ReadString());
+ }
+
+ // A well-formed value whose declared length fits the stream still
reads.
+ [Test]
+ public void TestReadBytesWithinStreamStillReads()
+ {
+ byte[] payload = Encoding.UTF8.GetBytes("hello");
+ MemoryStream ms = new MemoryStream();
+ Encoder e = new BinaryEncoder(ms);
+ e.WriteBytes(payload);
+ ms.Position = 0;
+ Decoder d = new BinaryDecoder(ms);
+ Assert.AreEqual(payload, d.ReadBytes());
+ }
+
+ // On a non-seekable stream the remaining length is unknown, so the
+ // pre-check is skipped and a valid value still decodes.
+ [Test]
+ public void TestReadBytesNonSeekableStreamStillReads()
+ {
+ byte[] payload = Encoding.UTF8.GetBytes("hello");
+ MemoryStream backing = new MemoryStream();
+ Encoder e = new BinaryEncoder(backing);
+ e.WriteBytes(payload);
+ byte[] encoded = backing.ToArray();
+
+ using (var ns = new NonSeekableStream(new MemoryStream(encoded)))
+ {
+ Decoder d = new BinaryDecoder(ns);
+ Assert.AreEqual(payload, d.ReadBytes());
+ }
+ }
+
+ // 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
+ // allocating, using the minimum on-wire size of the element schema (so
+ // 0-byte elements like null are not falsely rejected).
+ [Test]
+ public void TestReadArrayRejectsCountBeyondStream()
+ {
+ var schema =
Avro.Schema.Parse("{\"type\":\"array\",\"items\":\"long\"}");
+ var ms = new MemoryStream();
+ new BinaryEncoder(ms).WriteLong(1000000); // 1,000,000 longs, no
data
+ ms.Position = 0;
+ var reader = new GenericReader<object>(schema, schema);
+ Assert.Throws<AvroException>(() => reader.Read(null, new
BinaryDecoder(ms)));
+ }
+
+ [Test]
+ public void TestReadMapRejectsCountBeyondStream()
+ {
+ var schema =
Avro.Schema.Parse("{\"type\":\"map\",\"values\":\"long\"}");
+ var ms = new MemoryStream();
+ new BinaryEncoder(ms).WriteLong(1000000);
+ ms.Position = 0;
+ var reader = new GenericReader<object>(schema, schema);
+ Assert.Throws<AvroException>(() => reader.Read(null, new
BinaryDecoder(ms)));
+ }
+
+ [Test]
+ public void TestReadArrayOfNullNotFalselyRejected()
+ {
+ var schema =
Avro.Schema.Parse("{\"type\":\"array\",\"items\":\"null\"}");
+ var ms = new MemoryStream();
+ var enc = new BinaryEncoder(ms);
+ enc.WriteLong(100000); // one block of 100,000 nulls (zero bytes
each)
+ enc.WriteLong(0); // end-of-array marker
+ ms.Position = 0;
+ var reader = new GenericReader<object>(schema, schema);
+ var result = (Array)reader.Read(null, new BinaryDecoder(ms));
+ Assert.AreEqual(100000, result.Length);
+ }
+
+ // Minimal read-only, forward-only stream wrapper reporting
CanSeek=false.
+ private sealed class NonSeekableStream : Stream
+ {
+ private readonly Stream inner;
+ public NonSeekableStream(Stream inner) { this.inner = inner; }
+ public override bool CanRead => true;
+ public override bool CanSeek => false;
+ public override bool CanWrite => false;
+ public override long Length => throw new NotSupportedException();
+ public override long Position
+ {
+ get => throw new NotSupportedException();
+ set => throw new NotSupportedException();
+ }
+ public override void Flush() { }
+ public override int Read(byte[] buffer, int offset, int count) =>
inner.Read(buffer, offset, count);
+ public override long Seek(long offset, SeekOrigin origin) => throw
new NotSupportedException();
+ public override void SetLength(long value) => throw new
NotSupportedException();
+ public override void Write(byte[] buffer, int offset, int count)
=> throw new NotSupportedException();
Review Comment:
NonSeekableStream is used in a using block, but it doesn't dispose the
wrapped inner stream. Overriding Dispose(bool) to dispose the inner stream
avoids leaking resources (and matches typical wrapper-stream behavior).
--
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]
