fix: NetworkReader ReadBytes/Array<T>/List<T> allocation limit to prevent ClientToServer [SyncVar] allocation attacks [credit: James]

Assets/Mirror/Core/NetworkReader.cs

@@ -45,6 +45,18 @@ public class NetworkReader
         // this is safer. see test: ReadString_InvalidUTF8().
         internal readonly UTF8Encoding encoding = new UTF8Encoding(false, true);
 
+        // while allocation free ReadArraySegment is encouraged,
+        // some functions can allocate a new byte[], List<T>, Texture, etc.
+        // we should keep a reasonable allocation size limit:
+        // -> server won't accidentally allocate 2GB on a mobile device
+        // -> client won't allocate 2GB on server for ClientToServer [SyncVar]s
+        // -> unlike max string length of 64 KB, we need a larger limit here.
+        //    large enough to not break existing projects,
+        //    small enough to reasonably limit allocation attacks.
+        // -> we don't know the exact size of ReadList<T> etc. because <T> is
+        //    managed. instead, this is considered a 'collection length' limit.
+        public const int AllocationLimit = 1024 * 1024 * 16; // 16 MB * sizeof(T)
+
         public NetworkReader(ArraySegment<byte> segment)
         {
             buffer = segment;

Assets/Mirror/Core/NetworkReaderExtensions.cs

@@ -90,6 +90,15 @@ public static byte[] ReadBytesAndSize(this NetworkReader reader)
 
         public static byte[] ReadBytes(this NetworkReader reader, int count)
         {
+            // prevent allocation attacks with a reasonable limit.
+            //   server shouldn't allocate too much on client devices.
+            //   client shouldn't allocate too much on server in ClientToServer [SyncVar]s.
+            if (count > NetworkReader.AllocationLimit)
+            {
+                // throw EndOfStream for consistency with ReadBlittable when out of data
+                throw new EndOfStreamException($"NetworkReader attempted to allocate {count} bytes, which is larger than the allowed limit of {NetworkReader.AllocationLimit} bytes.");
+            }
+
             byte[] bytes = new byte[count];
             reader.ReadBytes(bytes, count);
             return bytes;
@@ -248,6 +257,15 @@ public static List<T> ReadList<T>(this NetworkReader reader)
             // 'null' is encoded as '-1'
             if (length < 0) return null;
 
+            // prevent allocation attacks with a reasonable limit.
+            //   server shouldn't allocate too much on client devices.
+            //   client shouldn't allocate too much on server in ClientToServer [SyncVar]s.
+            if (length > NetworkReader.AllocationLimit)
+            {
+                // throw EndOfStream for consistency with ReadBlittable when out of data
+                throw new EndOfStreamException($"NetworkReader attempted to allocate a List<{typeof(T)}> {length} elements, which is larger than the allowed limit of {NetworkReader.AllocationLimit}.");
+            }
+
             List<T> result = new List<T>(length);
             for (int i = 0; i < length; i++)
             {
@@ -283,16 +301,19 @@ public static T[] ReadArray<T>(this NetworkReader reader)
             // 'null' is encoded as '-1'
             if (length < 0) return null;
 
-            // todo throw an exception for other negative values (we never write them, likely to be attacker)
-
-            // this assumes that a reader for T reads at least 1 bytes
-            // we can't know the exact size of T because it could have a user created reader
-            // NOTE: don't add to length as it could overflow if value is int.max
-            if (length > reader.Remaining)
+            // prevent allocation attacks with a reasonable limit.
+            //   server shouldn't allocate too much on client devices.
+            //   client shouldn't allocate too much on server in ClientToServer [SyncVar]s.
+            if (length > NetworkReader.AllocationLimit)
             {
-                throw new EndOfStreamException($"Received array that is too large: {length}");
+                // throw EndOfStream for consistency with ReadBlittable when out of data
+                throw new EndOfStreamException($"NetworkReader attempted to allocate an Array<{typeof(T)}> with {length} elements, which is larger than the allowed limit of {NetworkReader.AllocationLimit}.");
             }
 
+            // we can't check if reader.Remaining < length,
+            // because we don't know sizeof(T) since it's a managed type.
+            // if (length > reader.Remaining) throw new EndOfStreamException($"Received array that is too large: {length}");
+
             T[] result = new T[length];
             for (int i = 0; i < length; i++)
             {
@@ -309,16 +330,26 @@ public static Uri ReadUri(this NetworkReader reader)
 
         public static Texture2D ReadTexture2D(this NetworkReader reader)
         {
-            // TODO allocation protection when sending textures to server.
-            //      currently can allocate 32k x 32k x 4 byte = 3.8 GB
-
             // support 'null' textures for [SyncVar]s etc.
             // https://github.com/vis2k/Mirror/issues/3144
             short width = reader.ReadShort();
             if (width == -1) return null;
 
             // read height
             short height = reader.ReadShort();
+
+            // prevent allocation attacks with a reasonable limit.
+            //   server shouldn't allocate too much on client devices.
+            //   client shouldn't allocate too much on server in ClientToServer [SyncVar]s.
+            // log an error and return default.
+            // we don't want attackers to be able to trigger exceptions.
+            int totalSize = width * height;
+            if (totalSize > NetworkReader.AllocationLimit)
+            {
+                Debug.LogWarning($"NetworkReader attempted to allocate a Texture2D with total size (width * height) of {totalSize}, which is larger than the allowed limit of {NetworkReader.AllocationLimit}.");
+                return null;
+            }
+
             Texture2D texture2D = new Texture2D(width, height);
 
             // read pixel content

Assets/Mirror/Core/NetworkWriterExtensions.cs

@@ -307,6 +307,10 @@ public static void WriteList<T>(this NetworkWriter writer, List<T> list)
                 return;
             }
 
+            // check if within max size, otherwise Reader can't read it.
+            if (list.Count > NetworkReader.AllocationLimit)
+                throw new IndexOutOfRangeException($"NetworkWriter.WriteList - List<{typeof(T)}> too big: {list.Count} elements. Limit: {NetworkReader.AllocationLimit}");
+
             writer.WriteInt(list.Count);
             for (int i = 0; i < list.Count; i++)
                 writer.Write(list[i]);
@@ -340,6 +344,10 @@ public static void WriteArray<T>(this NetworkWriter writer, T[] array)
                 return;
             }
 
+            // check if within max size, otherwise Reader can't read it.
+            if (array.Length > NetworkReader.AllocationLimit)
+                throw new IndexOutOfRangeException($"NetworkWriter.WriteArray - Array<{typeof(T)}> too big: {array.Length} elements. Limit: {NetworkReader.AllocationLimit}");
+
             writer.WriteInt(array.Length);
             for (int i = 0; i < array.Length; i++)
                 writer.Write(array[i]);
@@ -364,6 +372,11 @@ public static void WriteTexture2D(this NetworkWriter writer, Texture2D texture2D
                 return;
             }
 
+            // check if within max size, otherwise Reader can't read it.
+            int totalSize = texture2D.width * texture2D.height;
+            if (totalSize > NetworkReader.AllocationLimit)
+                throw new IndexOutOfRangeException($"NetworkWriter.WriteTexture2D - Texture2D total size (width*height) too big: {totalSize}. Limit: {NetworkReader.AllocationLimit}");
+
             // write dimensions first so reader can create the texture with size
             // 32k x 32k short is more than enough
             writer.WriteShort((short)texture2D.width);

Assets/Mirror/Tests/Editor/NetworkReaderWriter/NetworkWriterTest.cs

@@ -1388,31 +1388,68 @@ void WriteBadArray()
         }
 
         [Test]
-        [Description("ReadArray should throw if it is trying to read more than length of segment, this is to stop allocation attacks")]
+        [TestCase(20_000)]
+        [TestCase(int.MaxValue)]
+        [TestCase(int.MaxValue - 1)]
+        public void TestReadBytes_LengthIsTooBig(int badLength)
+        {
+            // write bad array
+            NetworkWriter writer = new NetworkWriter();
+            writer.WriteInt(badLength);
+            int[] array = new int[testArraySize] { 1, 2, 3, 4 };
+            for (int i = 0; i < array.Length; i++)
+                writer.Write(array[i]);
+
+            // attempt to read it
+            NetworkReader reader = new NetworkReader(writer.ToArray());
+            EndOfStreamException exception = Assert.Throws<EndOfStreamException>(() =>
+            {
+                _ = reader.ReadBytes(badLength);
+            });
+        }
+
+        [Test]
         [TestCase(testArraySize * sizeof(int) + 1, Description = "min read count is 1 byte, 16 array bytes are writen so 17 should throw error")]
         [TestCase(20_000)]
         [TestCase(int.MaxValue)]
         [TestCase(int.MaxValue - 1)]
-        // todo add fuzzy testing to check more values
-        public void TestArrayThrowsIfLengthIsTooBig(int badLength)
+        public void TestReadList_LengthIsTooBig(int badLength)
         {
+            // write bad array
             NetworkWriter writer = new NetworkWriter();
-            WriteBadArray();
+            writer.WriteInt(badLength);
+            int[] array = new int[testArraySize] { 1, 2, 3, 4 };
+            for (int i = 0; i < array.Length; i++)
+                writer.Write(array[i]);
 
+            // attempt to read it
             NetworkReader reader = new NetworkReader(writer.ToArray());
             EndOfStreamException exception = Assert.Throws<EndOfStreamException>(() =>
             {
-                _ = reader.ReadArray<int>();
+                _ = reader.ReadList<int>();
             });
-            Assert.That(exception, Has.Message.EqualTo($"Received array that is too large: {badLength}"));
+        }
 
-            void WriteBadArray()
+        [Test]
+        [TestCase(testArraySize * sizeof(int) + 1, Description = "min read count is 1 byte, 16 array bytes are writen so 17 should throw error")]
+        [TestCase(20_000)]
+        [TestCase(int.MaxValue)]
+        [TestCase(int.MaxValue - 1)]
+        public void TestReadArray_LengthIsTooBig(int badLength)
+        {
+            // write bad array
+            NetworkWriter writer = new NetworkWriter();
+            writer.WriteInt(badLength);
+            int[] array = new int[testArraySize] { 1, 2, 3, 4 };
+            for (int i = 0; i < array.Length; i++)
+                writer.Write(array[i]);
+
+            // attempt to read it
+            NetworkReader reader = new NetworkReader(writer.ToArray());
+            EndOfStreamException exception = Assert.Throws<EndOfStreamException>(() =>
             {
-                writer.WriteInt(badLength);
-                int[] array = new int[testArraySize] { 1, 2, 3, 4 };
-                for (int i = 0; i < array.Length; i++)
-                    writer.Write(array[i]);
-            }
+                _ = reader.ReadArray<int>();
+            });
         }
 
         [Test]