[NRBF] Fix bugs discovered by the fuzzer (dotnet#107368)

* bug #1: don't allow for values out of the SerializationRecordType enum range

* bug dotnet#2: throw SerializationException rather than KeyNotFoundException when the referenced record is missing or it points to a record of different type

* bug dotnet#3: throw SerializationException rather than FormatException when it's being thrown by BinaryReader (or sth else that we use)

* bug dotnet#4: document the fact that IOException can be thrown

* bug dotnet#5: throw SerializationException rather than OverflowException when parsing the decimal fails

* bug dotnet#6: 0 and 17 are illegal values for PrimitiveType enum

* bug dotnet#7: throw SerializationException when a surrogate character is read (so far an ArgumentException was thrown)

src/libraries/System.Formats.Nrbf/src/Resources/Strings.resx

@@ -133,7 +133,7 @@
     <value>{0} Record Type is not supported by design.</value>
   </data>
   <data name="Serialization_InvalidReference" xml:space="preserve">
-    <value>Member reference was pointing to a record of unexpected type.</value>
+    <value>Invalid member reference.</value>
   </data>
   <data name="Serialization_InvalidTypeName" xml:space="preserve">
     <value>Invalid type name: `{0}`.</value>
@@ -162,4 +162,10 @@
   <data name="Serialization_InvalidAssemblyName" xml:space="preserve">
     <value>Invalid assembly name: `{0}`.</value>
   </data>
+  <data name="Serialization_InvalidFormat" xml:space="preserve">
+    <value>Invalid format.</value>
+  </data>
+  <data name="Serialization_SurrogateCharacter" xml:space="preserve">
+    <value>A surrogate character was read.</value>
+  </data>
 </root>

src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/ArraySinglePrimitiveRecord.cs

@@ -171,12 +171,17 @@ private static List<decimal> DecodeDecimals(BinaryReader reader, int count)
 
             reader.BaseStream.ReadExactly(buffer.Slice(0, stringLength));
 
-            values.Add(decimal.Parse(buffer.Slice(0, stringLength), CultureInfo.InvariantCulture));
+            if (!decimal.TryParse(buffer.Slice(0, stringLength), NumberStyles.Number, CultureInfo.InvariantCulture, out decimal value))
+            {
+                ThrowHelper.ThrowInvalidFormat();
+            }
+
+            values.Add(value);
         }
 #else
         for (int i = 0; i < count; i++)
         {
-            values.Add(decimal.Parse(reader.ReadString(), CultureInfo.InvariantCulture));
+            values.Add(reader.ParseDecimal());
         }
 #endif
         return values;

src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/ClassTypeInfo.cs

@@ -26,7 +26,7 @@ internal static ClassTypeInfo Decode(BinaryReader reader, PayloadOptions options
         string rawName = reader.ReadString();
         SerializationRecordId libraryId = SerializationRecordId.Decode(reader);
 
-        BinaryLibraryRecord library = (BinaryLibraryRecord)recordMap[libraryId];
+        BinaryLibraryRecord library = recordMap.GetRecord<BinaryLibraryRecord>(libraryId);
 
         return new ClassTypeInfo(rawName.ParseNonSystemClassRecordTypeName(library, options));
     }

src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/ClassWithIdRecord.cs

@@ -34,10 +34,7 @@ internal static ClassWithIdRecord Decode(
         SerializationRecordId id = SerializationRecordId.Decode(reader);
         SerializationRecordId metadataId = SerializationRecordId.Decode(reader);
 
-        if (recordMap[metadataId] is not ClassRecord referencedRecord)
-        {
-            throw new SerializationException(SR.Serialization_InvalidReference);
-        }
+        ClassRecord referencedRecord = recordMap.GetRecord<ClassRecord>(metadataId);
 
         return new ClassWithIdRecord(id, referencedRecord);
     }

src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/ClassWithMembersAndTypesRecord.cs

@@ -27,7 +27,7 @@ internal static ClassWithMembersAndTypesRecord Decode(BinaryReader reader, Recor
         MemberTypeInfo memberTypeInfo = MemberTypeInfo.Decode(reader, classInfo.MemberNames.Count, options, recordMap);
         SerializationRecordId libraryId = SerializationRecordId.Decode(reader);
 
-        BinaryLibraryRecord library = (BinaryLibraryRecord)recordMap[libraryId];
+        BinaryLibraryRecord library = recordMap.GetRecord<BinaryLibraryRecord>(libraryId);
         classInfo.LoadTypeName(library, options);
 
         return new ClassWithMembersAndTypesRecord(classInfo, memberTypeInfo);

src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/MemberReferenceRecord.cs

@@ -38,5 +38,5 @@ private MemberReferenceRecord(SerializationRecordId reference, RecordMap recordM
     internal static MemberReferenceRecord Decode(BinaryReader reader, RecordMap recordMap)
         => new(SerializationRecordId.Decode(reader), recordMap);
 
-    internal SerializationRecord GetReferencedRecord() => RecordMap[Reference];
+    internal SerializationRecord GetReferencedRecord() => RecordMap.GetRecord(Reference);
 }

src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/NrbfDecoder.cs

@@ -46,6 +46,7 @@ public static bool StartsWithPayloadHeader(ReadOnlySpan<byte> bytes)
     /// <exception cref="ArgumentNullException"><paramref name="stream" /> is <see langword="null" />.</exception>
     /// <exception cref="NotSupportedException">The stream does not support reading or seeking.</exception>
     /// <exception cref="ObjectDisposedException">The stream was closed.</exception>
+    /// <exception cref="IOException">An I/O error occurred.</exception>
     /// <remarks>When this method returns, <paramref name="stream" /> is restored to its original position.</remarks>
     public static bool StartsWithPayloadHeader(Stream stream)
     {
@@ -107,6 +108,7 @@ public static bool StartsWithPayloadHeader(Stream stream)
     /// <exception cref="ArgumentNullException"><paramref name="payload"/> is <see langword="null" />.</exception>
     /// <exception cref="ArgumentException"><paramref name="payload"/> does not support reading or is already closed.</exception>
     /// <exception cref="SerializationException">Reading from <paramref name="payload"/> encountered invalid NRBF data.</exception>
+    /// <exception cref="IOException">An I/O error occurred.</exception>
     /// <exception cref="NotSupportedException">
     /// Reading from <paramref name="payload"/> encountered unsupported records,
     /// for example, arrays with non-zero offset or unsupported record types
@@ -142,7 +144,14 @@ public static SerializationRecord Decode(Stream payload, out IReadOnlyDictionary
 #endif
 
         using BinaryReader reader = new(payload, ThrowOnInvalidUtf8Encoding, leaveOpen: leaveOpen);
-        return Decode(reader, options ?? new(), out recordMap);
+        try
+        {
+            return Decode(reader, options ?? new(), out recordMap);
+        }
+        catch (FormatException) // can be thrown by various BinaryReader methods
+        {
+            throw new SerializationException(SR.Serialization_InvalidFormat);
+        }
     }
 
     /// <summary>
@@ -213,12 +222,7 @@ private static SerializationRecord Decode(BinaryReader reader, PayloadOptions op
     private static SerializationRecord DecodeNext(BinaryReader reader, RecordMap recordMap,
         AllowedRecordTypes allowed, PayloadOptions options, out SerializationRecordType recordType)
     {
-        byte nextByte = reader.ReadByte();
-        if (((uint)allowed & (1u << nextByte)) == 0)
-        {
-            ThrowHelper.ThrowForUnexpectedRecordType(nextByte);
-        }
-        recordType = (SerializationRecordType)nextByte;
+        recordType = reader.ReadSerializationRecordType(allowed);
 
         SerializationRecord record = recordType switch
         {
@@ -254,7 +258,7 @@ private static SerializationRecord DecodeMemberPrimitiveTypedRecord(BinaryReader
             PrimitiveType.Boolean => new MemberPrimitiveTypedRecord<bool>(reader.ReadBoolean()),
             PrimitiveType.Byte => new MemberPrimitiveTypedRecord<byte>(reader.ReadByte()),
             PrimitiveType.SByte => new MemberPrimitiveTypedRecord<sbyte>(reader.ReadSByte()),
-            PrimitiveType.Char => new MemberPrimitiveTypedRecord<char>(reader.ReadChar()),
+            PrimitiveType.Char => new MemberPrimitiveTypedRecord<char>(reader.ParseChar()),
             PrimitiveType.Int16 => new MemberPrimitiveTypedRecord<short>(reader.ReadInt16()),
             PrimitiveType.UInt16 => new MemberPrimitiveTypedRecord<ushort>(reader.ReadUInt16()),
             PrimitiveType.Int32 => new MemberPrimitiveTypedRecord<int>(reader.ReadInt32()),
@@ -263,7 +267,7 @@ private static SerializationRecord DecodeMemberPrimitiveTypedRecord(BinaryReader
             PrimitiveType.UInt64 => new MemberPrimitiveTypedRecord<ulong>(reader.ReadUInt64()),
             PrimitiveType.Single => new MemberPrimitiveTypedRecord<float>(reader.ReadSingle()),
             PrimitiveType.Double => new MemberPrimitiveTypedRecord<double>(reader.ReadDouble()),
-            PrimitiveType.Decimal => new MemberPrimitiveTypedRecord<decimal>(decimal.Parse(reader.ReadString(), CultureInfo.InvariantCulture)),
+            PrimitiveType.Decimal => new MemberPrimitiveTypedRecord<decimal>(reader.ParseDecimal()),
             PrimitiveType.DateTime => new MemberPrimitiveTypedRecord<DateTime>(Utils.BinaryReaderExtensions.CreateDateTimeFromData(reader.ReadUInt64())),
             // String is handled with a record, never on it's own
             _ => new MemberPrimitiveTypedRecord<TimeSpan>(new TimeSpan(reader.ReadInt64())),

src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/RecordMap.cs

@@ -63,7 +63,8 @@ internal void Add(SerializationRecord record)
 
     internal SerializationRecord GetRootRecord(SerializedStreamHeaderRecord header)
     {
-        SerializationRecord rootRecord = _map[header.RootId];
+        SerializationRecord rootRecord = GetRecord(header.RootId);
+
         if (rootRecord is SystemClassWithMembersAndTypesRecord systemClass)
         {
             // update the record map, so it's visible also to those who access it via Id
@@ -72,4 +73,14 @@ internal SerializationRecord GetRootRecord(SerializedStreamHeaderRecord header)
 
         return rootRecord;
     }
+
+    internal SerializationRecord GetRecord(SerializationRecordId recordId)
+        => _map.TryGetValue(recordId, out SerializationRecord? record)
+            ? record
+            : throw new SerializationException(SR.Serialization_InvalidReference);
+
+    internal T GetRecord<T>(SerializationRecordId recordId) where T : SerializationRecord
+        => _map.TryGetValue(recordId, out SerializationRecord? record) && record is T casted
+            ? casted
+            : throw new SerializationException(SR.Serialization_InvalidReference);
 }

src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/Utils/BinaryReaderExtensions.cs

@@ -15,6 +15,19 @@ internal static class BinaryReaderExtensions
 {
     private static object? s_baseAmbiguousDstDateTime;
 
+    internal static SerializationRecordType ReadSerializationRecordType(this BinaryReader reader, AllowedRecordTypes allowed)
+    {
+        byte nextByte = reader.ReadByte();
+        if (nextByte > (byte)SerializationRecordType.MethodReturn // MethodReturn is the last defined value.
+            || (nextByte > (byte)SerializationRecordType.ArraySingleString && nextByte < (byte)SerializationRecordType.MethodCall) // not part of the spec
+            || ((uint)allowed & (1u << nextByte)) == 0) // valid, but not allowed
+        {
+            ThrowHelper.ThrowForUnexpectedRecordType(nextByte);
+        }
+
+        return (SerializationRecordType)nextByte;
+    }
+
     internal static BinaryArrayType ReadArrayType(this BinaryReader reader)
     {
         byte arrayType = reader.ReadByte();
@@ -48,7 +61,7 @@ internal static PrimitiveType ReadPrimitiveType(this BinaryReader reader)
     {
         byte primitiveType = reader.ReadByte();
         // String is the last defined value, 4 is not used at all.
-        if (primitiveType is 4 or > (byte)PrimitiveType.String)
+        if (primitiveType is 0 or 4 or (byte)PrimitiveType.Null or > (byte)PrimitiveType.String)
         {
             ThrowHelper.ThrowInvalidValue(primitiveType);
         }
@@ -64,7 +77,7 @@ internal static object ReadPrimitiveValue(this BinaryReader reader, PrimitiveTyp
             PrimitiveType.Boolean => reader.ReadBoolean(),
             PrimitiveType.Byte => reader.ReadByte(),
             PrimitiveType.SByte => reader.ReadSByte(),
-            PrimitiveType.Char => reader.ReadChar(),
+            PrimitiveType.Char => reader.ParseChar(),
             PrimitiveType.Int16 => reader.ReadInt16(),
             PrimitiveType.UInt16 => reader.ReadUInt16(),
             PrimitiveType.Int32 => reader.ReadInt32(),
@@ -73,11 +86,35 @@ internal static object ReadPrimitiveValue(this BinaryReader reader, PrimitiveTyp
             PrimitiveType.UInt64 => reader.ReadUInt64(),
             PrimitiveType.Single => reader.ReadSingle(),
             PrimitiveType.Double => reader.ReadDouble(),
-            PrimitiveType.Decimal => decimal.Parse(reader.ReadString(), CultureInfo.InvariantCulture),
+            PrimitiveType.Decimal => reader.ParseDecimal(),
             PrimitiveType.DateTime => CreateDateTimeFromData(reader.ReadUInt64()),
             _ => new TimeSpan(reader.ReadInt64()),
         };
 
+    // BinaryFormatter serializes decimals as strings and we can't BinaryReader.ReadDecimal.
+    internal static decimal ParseDecimal(this BinaryReader reader)
+    {
+        string text = reader.ReadString();
+        if (!decimal.TryParse(text, NumberStyles.Number, CultureInfo.InvariantCulture, out decimal result))
+        {
+            ThrowHelper.ThrowInvalidFormat();
+        }
+
+        return result;
+    }
+
+    internal static char ParseChar(this BinaryReader reader)
+    {
+        try
+        {
+            return reader.ReadChar();
+        }
+        catch (ArgumentException) // A surrogate character was read.
+        {
+            throw new SerializationException(SR.Serialization_SurrogateCharacter);
+        }
+    }
+
     /// <summary>
     ///  Creates a <see cref="DateTime"/> object from raw data with validation.
     /// </summary>

src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/Utils/ThrowHelper.cs

@@ -29,6 +29,9 @@ internal static void ThrowArrayContainedNulls()
     internal static void ThrowInvalidAssemblyName(string rawName)
         => throw new SerializationException(SR.Format(SR.Serialization_InvalidAssemblyName, rawName));
 
+    internal static void ThrowInvalidFormat()
+        => throw new SerializationException(SR.Serialization_InvalidFormat);
+
     internal static void ThrowEndOfStreamException()
         => throw new EndOfStreamException();
 

src/libraries/System.Formats.Nrbf/tests/InvalidInputTests.cs

@@ -426,7 +426,9 @@ public static IEnumerable<object[]> ThrowsForInvalidPrimitiveType_Arguments()
         {
             foreach (byte binaryType in new byte[] { (byte)0 /* BinaryType.Primitive */, (byte)7 /* BinaryType.PrimitiveArray */ })
             {
+                yield return new object[] { recordType, binaryType, (byte)0 }; // value not used by the spec
                 yield return new object[] { recordType, binaryType, (byte)4 }; // value not used by the spec
+                yield return new object[] { recordType, binaryType, (byte)17 }; // used by the spec, but illegal in given context
                 yield return new object[] { recordType, binaryType, (byte)19 };
             }
         }
@@ -478,4 +480,109 @@ public void ThrowsOnInvalidArrayType()
         stream.Position = 0;
         Assert.Throws<SerializationException>(() => NrbfDecoder.Decode(stream));
     }
+
+    [Theory]
+    [InlineData(18, typeof(NotSupportedException))] // not part of the spec, but still less than max allowed value (22)
+    [InlineData(19, typeof(NotSupportedException))] // same as above
+    [InlineData(20, typeof(NotSupportedException))] // same as above
+    [InlineData(23, typeof(SerializationException))] // not part of the spec and more than max allowed value (22)
+    [InlineData(64, typeof(SerializationException))] // same as above but also matches AllowedRecordTypes.SerializedStreamHeader
+    public void InvalidSerializationRecordType(byte recordType, Type expectedException)
+    {
+        using MemoryStream stream = new();
+        BinaryWriter writer = new(stream, Encoding.UTF8);
+
+        WriteSerializedStreamHeader(writer);
+        writer.Write(recordType); // SerializationRecordType
+        writer.Write((byte)SerializationRecordType.MessageEnd);
+
+        stream.Position = 0;
+
+        Assert.Throws(expectedException, () => NrbfDecoder.Decode(stream));
+    }
+
+    [Fact]
+    public void MissingRootRecord()
+    {
+        const int RootRecordId = 1;
+        using MemoryStream stream = new();
+        BinaryWriter writer = new(stream, Encoding.UTF8);
+
+        WriteSerializedStreamHeader(writer, rootId: RootRecordId);
+        writer.Write((byte)SerializationRecordType.BinaryObjectString);
+        writer.Write(RootRecordId + 1); // a different ID
+        writer.Write("theString");
+        writer.Write((byte)SerializationRecordType.MessageEnd);
+
+        stream.Position = 0;
+
+        Assert.Throws<SerializationException>(() => NrbfDecoder.Decode(stream));
+    }
+
+    [Fact]
+    public void Invalid7BitEncodedStringLength()
+    {
+        // The highest bit of the last byte is set (so it's invalid).
+        byte[] invalidLength = [byte.MaxValue, byte.MaxValue, byte.MaxValue, byte.MaxValue, byte.MaxValue];
+
+        using MemoryStream stream = new();
+        BinaryWriter writer = new(stream, Encoding.UTF8);
+
+        WriteSerializedStreamHeader(writer);
+        writer.Write((byte)SerializationRecordType.BinaryObjectString);
+        writer.Write(1); // root record Id
+        writer.Write(invalidLength); // the length prefix
+        writer.Write(Encoding.UTF8.GetBytes("theString"));
+        writer.Write((byte)SerializationRecordType.MessageEnd);
+
+        stream.Position = 0;
+
+        Assert.Throws<SerializationException>(() => NrbfDecoder.Decode(stream));
+    }
+
+    [Theory]
+    [InlineData("79228162514264337593543950336")] // invalid format (decimal.MaxValue + 1)
+    [InlineData("1111111111111111111111111111111111111111111111111")] // overflow
+    public void InvalidDecimal(string textRepresentation)
+    {
+        using MemoryStream stream = new();
+        BinaryWriter writer = new(stream, Encoding.UTF8);
+
+        WriteSerializedStreamHeader(writer);
+        writer.Write((byte)SerializationRecordType.SystemClassWithMembersAndTypes);
+        writer.Write(1); // root record Id
+        writer.Write("ClassWithDecimalField"); // type name
+        writer.Write(1); // member count
+        writer.Write("memberName");
+        writer.Write((byte)BinaryType.Primitive);
+        writer.Write((byte)PrimitiveType.Decimal);
+        writer.Write(textRepresentation);
+        writer.Write((byte)SerializationRecordType.MessageEnd);
+
+        stream.Position = 0;
+
+        Assert.Throws<SerializationException>(() => NrbfDecoder.Decode(stream));
+    }
+
+    [Fact]
+    public void SurrogateCharacter()
+    {
+        using MemoryStream stream = new();
+        BinaryWriter writer = new(stream, Encoding.UTF8);
+
+        WriteSerializedStreamHeader(writer);
+        writer.Write((byte)SerializationRecordType.SystemClassWithMembersAndTypes);
+        writer.Write(1); // root record Id
+        writer.Write("ClassWithCharField"); // type name
+        writer.Write(1); // member count
+        writer.Write("memberName");
+        writer.Write((byte)BinaryType.Primitive);
+        writer.Write((byte)PrimitiveType.Char);
+        writer.Write((byte)0xC0); // a surrogate character
+        writer.Write((byte)SerializationRecordType.MessageEnd);
+
+        stream.Position = 0;
+
+        Assert.Throws<SerializationException>(() => NrbfDecoder.Decode(stream));
+    }
 }

src/libraries/System.Formats.Nrbf/tests/ReadTests.cs

@@ -45,10 +45,10 @@ protected static BinaryFormatter CreateBinaryFormatter()
         };
 #pragma warning restore SYSLIB0011 // Type or member is obsolete
 
-    protected static void WriteSerializedStreamHeader(BinaryWriter writer, int major = 1, int minor = 0)
+    protected static void WriteSerializedStreamHeader(BinaryWriter writer, int major = 1, int minor = 0, int rootId = 1)
     {
         writer.Write((byte)SerializationRecordType.SerializedStreamHeader);
-        writer.Write(1); // root ID
+        writer.Write(rootId); // root ID
         writer.Write(1); // header ID
         writer.Write(major); // major version
         writer.Write(minor); // minor version

src/libraries/System.Formats.Nrbf/tests/System.Formats.Nrbf.Tests.csproj

@@ -7,6 +7,8 @@
 
   <ItemGroup>
     <Compile Include="..\src\System\Formats\Nrbf\BinaryArrayType.cs" Link="BinaryArrayType.cs" />
+    <Compile Include="..\src\System\Formats\Nrbf\BinaryType.cs" Link="BinaryType.cs" />
+    <Compile Include="..\src\System\Formats\Nrbf\PrimitiveType.cs" Link="PrimitiveType.cs" />
   </ItemGroup>
 
   <ItemGroup>