fix: networkanimator does no bounds check when reading parameters (#4090)

* fix

Adding bounds check for reading NetworkAnimator parameters.

* test

Adding a unit test to validate the bounds check.

* update

adding change log entry

* style

adding additional comments for clarity on what the test is doing.

Author: NoelStephensUnity

com.unity.netcode.gameobjects/CHANGELOG.md

@@ -22,6 +22,7 @@ Additional documentation and release notes are available at [Multiplayer Documen
 
 ### Fixed
 
+- Issue where `NetworkAnimator` did no bounds check on the parameter index read prior to obtaining a pointer to the location within the array. (#4090)
 - Issue where scene migration in a distributed authority session would throw an exception on clients migrating their owned `NetworkObject` instances to a different scene.(#4086)
 - Issue where migrating a dynamically spawned or in-scene placed `NetworkObject` into a different scene during awake could result in that spawned instance to not be synchronized. (#4086)
 - Issue where a NullReferenceException was thrown when a non-authority failed to spawn a NetworkObject. (#4067)

com.unity.netcode.gameobjects/Runtime/Components/NetworkAnimator.cs

@@ -1377,6 +1377,13 @@ private unsafe void ReadParameters(FastBufferReader reader)
             while (totalParametersRead < totalParametersToRead)
             {
                 ByteUnpacker.ReadValuePacked(reader, out uint parameterIndex);
+
+                // Do bounds check prior to getting the element as a reference at that index.
+                if (parameterIndex >= m_CachedAnimatorParameters.Length)
+                {
+                    NetworkManager.Log.ErrorServer(new Logging.Context(LogLevel.Error, $"[{nameof(NetworkAnimator)}][{name}] Invalid index of {parameterIndex} was received when there are only {m_CachedAnimatorParameters.Length} parameters. Ignoring the remainger of this {nameof(ParametersUpdateMessage)}!"));
+                    return;
+                }
                 ref var cacheValue = ref UnsafeUtility.ArrayElementAsRef<AnimatorParamCache>(m_CachedAnimatorParameters.GetUnsafePtr(), (int)parameterIndex);
                 var hash = cacheValue.Hash;
                 if (cacheValue.Type == AnimationParamEnumWrapper.AnimatorControllerParameterInt)

testproject/Assets/Tests/Runtime/Animation/NetworkAnimatorTests.cs

@@ -3,9 +3,11 @@
 using System.Linq;
 using NUnit.Framework;
 using Unity.Netcode;
+using Unity.Netcode.Components;
 using Unity.Netcode.TestHelpers.Runtime;
 using UnityEngine;
 using UnityEngine.TestTools;
+using static Unity.Netcode.Components.NetworkAnimator;
 
 
 namespace TestProject.RuntimeTests
@@ -336,7 +338,40 @@ public void ParameterExcludedTests()
             VerboseDebug($" ------------------ Parameter Test [{m_OwnerShipMode}] Stopping ------------------ ");
         }
 
+        private unsafe void MockWritingParameters(ref FastBufferWriter writer)
+        {
+            writer.Seek(0);
+            writer.Truncate();
+
+            // Write out how many parameter entries to read
+            BytePacker.WriteValuePacked(writer, (uint)1);
+            // Write an invalid index level (anything would be invalid with no parameters)
+            BytePacker.WriteValuePacked(writer, (uint)1000);
+            // Write some value for the invalid parameter
+            BytePacker.WriteValuePacked(writer, (uint)10);
+        }
+
+        [Test]
+        public void ParameterBoundsCheck()
+        {
+            var gameObject = new GameObject();
+            gameObject.AddComponent<NetworkObject>();
+            var networkAnimator = gameObject.AddComponent<NetworkAnimator>();
 
+            var writer = new FastBufferWriter(40, Unity.Collections.Allocator.TempJob);
+
+            // Mock a parameter update with an invalid index
+            MockWritingParameters(ref writer);
+
+            var invalidParameters = new ParametersUpdateMessage()
+            {
+                Parameters = writer.ToArray()
+            };
+            // Expect an error message
+            LogAssert.Expect(LogType.Error, new System.Text.RegularExpressions.Regex($"parameters. Ignoring the remainger of this {nameof(ParametersUpdateMessage)}!"));
+            // Pass in the invalid ParametersUpdateMessage
+            networkAnimator.UpdateParameters(ref invalidParameters);
+        }
 
         private bool AllTriggersDetected(OwnerShipMode ownerShipMode)
         {