fix: fastbufferreader string deserialization int overflow (#4052)

* fix

This fixes an edge case scenario with string reading where if user code has caused the character count to have been already read prior to attempting to read a string value (safely or unsafely) and then reading the string can result in the signed integer size to roll over to a negative value and thus causing the reader to attempt to read into restricted memory outside of the application domain which results in the editor crashing.

The fix catches this scenario and throws an overflow exception prior to attempting to read into negative memory space relative to the application domain.

* test

This test validates the fix.

* update

Adding changelog entry.

* update

Making the reader use the already calculated readSize.

* test

Updating test to unsafely read a string under the same condition.

* fix

Fixing an issue where the unsafe string read needs to create an empty string based on the character count and not the byte count.

* refactor

Based on Paolo's suggestion on combining the string size, in bytes, validation script to a single in-lined method shared between the safe and unsafe string read methods.
Also combined the actual reading of the string data into a single in-lined method.

* style

Removing the auto-added (and not used) UnityEngine.UIElements using directive.

* style

updated comments and renamed CheckIfValidStringLength to ValidateStringByteCount.

* test

Removing the added 3 bytes used for earlier debugging purposes.

* style

spelling/typo fix.
whitespace after sentence in comment fix.

* refactor

Based on Emma's suggestion, removing the `SizeOfLengthField` method and `TryBeginReadInternal` check within the ReadValueSafe (string) method and replacing that with `ReadLengthSafe`.

* Update com.unity.netcode.gameobjects/Runtime/Serialization/FastBufferReader.cs

Co-authored-by: Emma <emma.mcmillan@unity3d.com>

* update

Based on suggestion from Emma, removing InBitwiseContext check since this is done in both ReadLengthSafe and TryBeginReadInternal.

---------

Co-authored-by: Emma <emma.mcmillan@unity3d.com>

Author: NoelStephensUnity

com.unity.netcode.gameobjects/CHANGELOG.md

@@ -21,6 +21,8 @@ Additional documentation and release notes are available at [Multiplayer Documen
 
 
 ### Fixed
+
+- Issue when FastBufferReader is attempting to read a string and all or a portion of the character count has already been read by user script, it could read a character length that results in a negative byte length which could result in an editor crash. (#4052)
 - Issue where NetworkRigidbodyBase was not applying rotation correctly when using Rigidbody2D. (#4012)
 - Issue where NetworkRigidbodyBase was always checking the 3D rigid body's interpolation mode when determining if it is kinematic and needs to put the rigid body to sleep and then switch to interpolation. (#4012)
 

com.unity.netcode.gameobjects/Runtime/Serialization/FastBufferReader.cs

@@ -564,14 +564,36 @@ public void ReadNetworkSerializableInPlace<T>(ref T value) where T : INetworkSer
         }
 
         /// <summary>
-        /// Reads a string
-        /// NOTE: ALLOCATES
+        /// Validates the string's total byte count based on whether we are
+        /// using one or two byte characters.
+        /// </summary>
+        /// <remarks>
+        /// Will throw an overflow exception if the size is greater than <see cref="int.MaxValue"/>.
+        /// </remarks>
+        /// <param name="length">Character count</param>
+        /// <param name="oneByteChars">If false(default) 2 byte characters and if true 1 byte characters</param>
+        /// <returns>total size in bytes to read</returns>
+        /// <exception cref="OverflowException"></exception>
+        [MethodImpl(MethodImplOptions.AggressiveInlining)]
+        private int ValidateStringByteCount(int length, bool oneByteChars)
+        {
+            var readSize = oneByteChars ? length : length * sizeof(char);
+            if (int.MaxValue < (uint)readSize)
+            {
+                throw new OverflowException($"Invalid reader position detected when trying to read a string of size {(uint)readSize}! This can result from an error in the serialization. Ensure deserialization exactly matches what was serialized!");
+            }
+            return readSize;
+        }
+
+        /// <summary>
+        /// Commonly shared string read method between <see cref="ReadValue"/>.
         /// </summary>
-        /// <param name="s">Stores the read string</param>
-        /// <param name="oneByteChars">Whether or not to use one byte per character. This will only allow ASCII</param>
-        public unsafe void ReadValue(out string s, bool oneByteChars = false)
+        /// <param name="s">The output of the string read.</param>
+        /// <param name="length">The number of characters in the string.</param>
+        /// <param name="oneByteChars">If false(default) 2 byte characters and if true 1 byte characters.</param>
+        [MethodImpl(MethodImplOptions.AggressiveInlining)]
+        private unsafe void ReadString(out string s, int length, bool oneByteChars)
         {
-            ReadLength(out int length);
             s = "".PadRight(length);
             int target = s.Length;
             fixed (char* native = s)
@@ -592,56 +614,50 @@ public unsafe void ReadValue(out string s, bool oneByteChars = false)
         }
 
         /// <summary>
-        /// Reads a string.
-        /// NOTE: ALLOCATES
-        ///
-        /// "Safe" version - automatically performs bounds checking. Less efficient than bounds checking
-        /// for multiple reads at once by calling TryBeginRead.
+        /// Reads a string without bounds checking.
+        /// NOTE: This method ALLOCATES memory.
         /// </summary>
+        /// <remarks>
+        /// This is the un-safe string read which requires invoking <see cref="TryBeginRead(int)"/> prior to invoking this method.<br />
+        /// Using one byte characters only allows ASCII characters.
+        /// </remarks>
         /// <param name="s">Stores the read string</param>
-        /// <param name="oneByteChars">Whether or not to use one byte per character. This will only allow ASCII</param>
-        public unsafe void ReadValueSafe(out string s, bool oneByteChars = false)
+        /// <param name="oneByteChars">If false(default) 2 byte characters and if true 1 byte characters.</param>
+        public unsafe void ReadValue(out string s, bool oneByteChars = false)
         {
-#if DEBUG
-            if (Handle->InBitwiseContext)
-            {
-                throw new InvalidOperationException(
-                    "Cannot use BufferReader in bytewise mode while in a bitwise context.");
-            }
-#endif
+            ReadLength(out int length);
 
-            if (!TryBeginReadInternal(SizeOfLengthField()))
-            {
-                throw new OverflowException("Reading past the end of the buffer");
-            }
+            // Validate the string's byte count based on the character count.
+            ValidateStringByteCount(length, oneByteChars);
 
-            ReadLength(out int length);
+            // Read the string
+            ReadString(out s, length, oneByteChars);
+        }
+
+        /// <summary>
+        /// Reads a string after it performs bounds checking automatically.
+        /// NOTE: This method ALLOCATES memory.
+        /// </summary>
+        /// <remarks>
+        /// This is the safe string read which invokes <see cref = "TryBeginReadInternal(int)"/> prior to reading the string.<br />
+        /// Using one byte characters only allows ASCII characters.
+        /// </remarks>
+        /// <param name="s">The string re the read string</param>
+        /// <param name="oneByteChars">If false(default) 2 byte characters and if true 1 byte characters.</param>
+        public unsafe void ReadValueSafe(out string s, bool oneByteChars = false)
+        {
+            ReadLengthSafe(out int length);
 
-            if (!TryBeginReadInternal(length * (oneByteChars ? 1 : sizeof(char))))
+            // Validate the string's byte count based on the character count and if it is valid begin reading based on the returned
+            // byte count.
+            if (!TryBeginReadInternal(ValidateStringByteCount(length, oneByteChars)))
             {
                 throw new OverflowException("Reading past the end of the buffer");
             }
-            s = "".PadRight(length);
-            int target = s.Length;
-            fixed (char* native = s)
-            {
-                if (oneByteChars)
-                {
-                    for (int i = 0; i < target; ++i)
-                    {
-                        ReadByte(out byte b);
-                        native[i] = (char)b;
-                    }
-                }
-                else
-                {
-                    ReadBytes((byte*)native, target * sizeof(char));
-                }
-            }
-        }
 
-        [MethodImpl(MethodImplOptions.AggressiveInlining)]
-        private static int SizeOfLengthField() => sizeof(uint);
+            // Read the string
+            ReadString(out s, length, oneByteChars);
+        }
 
         [MethodImpl(MethodImplOptions.AggressiveInlining)]
         private void ReadLengthSafe(out uint length) => ReadUnmanagedSafe(out length);

com.unity.netcode.gameobjects/Tests/Editor/Serialization/FastBufferReaderTests.cs

@@ -924,6 +924,50 @@ public void GivenFastBufferWriterContainingValue_WhenReadingString_ValueMatchesW
             }
         }
 
+        /// <summary>
+        /// This validates that <see cref="FastBufferReader"/> catches a potential
+        /// scenario where the string's character count value has already been read
+        /// due to an error within user script and the resultant character count
+        /// multiplied times 2 (when using 2 bytes vs 1) causes the length to roll over
+        /// to a negative value which, in turn, causes the reader to attempt to read
+        /// into restricted memory and causes the editor to crash.
+        /// </summary>
+        [Test]
+        public void ReadingStringAfterStringLengthHasAlreadyBeenRead([Values] bool isSafeRead)
+        {
+            // This was an issue uncovered in UUM-145752 that resulted
+            // in the below text to result in a length that when using
+            // 2 bytes per character would cause the skewed size to roll
+            // over into a negative value causing the editor to crash
+            // when it attempted to read a large negative offset value.
+            string valueToTest = "true";
+
+            var serializedValueSize = FastBufferWriter.GetWriteSize(valueToTest);
+
+            using var writer = new FastBufferWriter(serializedValueSize, Allocator.Temp);
+            writer.WriteValueSafe(valueToTest);
+
+            using var reader = new FastBufferReader(writer, Allocator.Temp);
+
+            // Read the value of the character count before trying to read the string
+            // This mocks user code having read too far into a stream causing the position to be skewed such that
+            // the string reader reads the some of the bytes for the actual text as the length.
+            reader.ReadByteSafe(out byte count);
+            Assert.True(count == valueToTest.Length, $"Count ({count}) is not the expected size of {valueToTest.Length}!");
+            if (isSafeRead)
+            {
+                // This should throw an overflow exception but should not crash the editor.
+                Assert.Throws<OverflowException>(() => reader.ReadValueSafe(out string valueRead));
+            }
+            else
+            {
+                // Assume user does a pre-calculation of the size to be read:
+                Assert.IsTrue(reader.TryBeginRead(count), "Reader denied read permission");
+
+                // This should throw an overflow exception but should not crash the editor.
+                Assert.Throws<OverflowException>(() => reader.ReadValue(out string valueRead));
+            }
+        }
 
         [TestCase(1, 0)]
         [TestCase(2, 0)]