Skip to content

Commit e79426e

Browse files
adamsitnikadamsitnik
authored
[NRBF] Fix bugs discovered by the fuzzer (dotnet#107368)
* bug #1: don't allow for values out of the SerializationRecordType enum range * bug #2: throw SerializationException rather than KeyNotFoundException when the referenced record is missing or it points to a record of different type * bug #3: throw SerializationException rather than FormatException when it's being thrown by BinaryReader (or sth else that we use) * bug #4: document the fact that IOException can be thrown * bug #5: throw SerializationException rather than OverflowException when parsing the decimal fails * bug #6: 0 and 17 are illegal values for PrimitiveType enum * bug #7: throw SerializationException when a surrogate character is read (so far an ArgumentException was thrown)
1 parent dc5dbab commit e79426e

13 files changed

+197
-25
lines changed

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

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -133,7 +133,7 @@
133133
<value>{0} Record Type is not supported by design.</value>
134134
</data>
135135
<data name="Serialization_InvalidReference" xml:space="preserve">
136-
<value>Member reference was pointing to a record of unexpected type.</value>
136+
<value>Invalid member reference.</value>
137137
</data>
138138
<data name="Serialization_InvalidTypeName" xml:space="preserve">
139139
<value>Invalid type name: `{0}`.</value>
@@ -162,4 +162,10 @@
162162
<data name="Serialization_InvalidAssemblyName" xml:space="preserve">
163163
<value>Invalid assembly name: `{0}`.</value>
164164
</data>
165+
<data name="Serialization_InvalidFormat" xml:space="preserve">
166+
<value>Invalid format.</value>
167+
</data>
168+
<data name="Serialization_SurrogateCharacter" xml:space="preserve">
169+
<value>A surrogate character was read.</value>
170+
</data>
165171
</root>

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

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -171,12 +171,17 @@ private static List<decimal> DecodeDecimals(BinaryReader reader, int count)
171171

172172
reader.BaseStream.ReadExactly(buffer.Slice(0, stringLength));
173173

174-
values.Add(decimal.Parse(buffer.Slice(0, stringLength), CultureInfo.InvariantCulture));
174+
if (!decimal.TryParse(buffer.Slice(0, stringLength), NumberStyles.Number, CultureInfo.InvariantCulture, out decimal value))
175+
{
176+
ThrowHelper.ThrowInvalidFormat();
177+
}
178+
179+
values.Add(value);
175180
}
176181
#else
177182
for (int i = 0; i < count; i++)
178183
{
179-
values.Add(decimal.Parse(reader.ReadString(), CultureInfo.InvariantCulture));
184+
values.Add(reader.ParseDecimal());
180185
}
181186
#endif
182187
return values;

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,7 @@ internal static ClassTypeInfo Decode(BinaryReader reader, PayloadOptions options
2626
string rawName = reader.ReadString();
2727
SerializationRecordId libraryId = SerializationRecordId.Decode(reader);
2828

29-
BinaryLibraryRecord library = (BinaryLibraryRecord)recordMap[libraryId];
29+
BinaryLibraryRecord library = recordMap.GetRecord<BinaryLibraryRecord>(libraryId);
3030

3131
return new ClassTypeInfo(rawName.ParseNonSystemClassRecordTypeName(library, options));
3232
}

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

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -34,10 +34,7 @@ internal static ClassWithIdRecord Decode(
3434
SerializationRecordId id = SerializationRecordId.Decode(reader);
3535
SerializationRecordId metadataId = SerializationRecordId.Decode(reader);
3636

37-
if (recordMap[metadataId] is not ClassRecord referencedRecord)
38-
{
39-
throw new SerializationException(SR.Serialization_InvalidReference);
40-
}
37+
ClassRecord referencedRecord = recordMap.GetRecord<ClassRecord>(metadataId);
4138

4239
return new ClassWithIdRecord(id, referencedRecord);
4340
}

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,7 @@ internal static ClassWithMembersAndTypesRecord Decode(BinaryReader reader, Recor
2727
MemberTypeInfo memberTypeInfo = MemberTypeInfo.Decode(reader, classInfo.MemberNames.Count, options, recordMap);
2828
SerializationRecordId libraryId = SerializationRecordId.Decode(reader);
2929

30-
BinaryLibraryRecord library = (BinaryLibraryRecord)recordMap[libraryId];
30+
BinaryLibraryRecord library = recordMap.GetRecord<BinaryLibraryRecord>(libraryId);
3131
classInfo.LoadTypeName(library, options);
3232

3333
return new ClassWithMembersAndTypesRecord(classInfo, memberTypeInfo);

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -38,5 +38,5 @@ private MemberReferenceRecord(SerializationRecordId reference, RecordMap recordM
3838
internal static MemberReferenceRecord Decode(BinaryReader reader, RecordMap recordMap)
3939
=> new(SerializationRecordId.Decode(reader), recordMap);
4040

41-
internal SerializationRecord GetReferencedRecord() => RecordMap[Reference];
41+
internal SerializationRecord GetReferencedRecord() => RecordMap.GetRecord(Reference);
4242
}

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

Lines changed: 13 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,7 @@ public static bool StartsWithPayloadHeader(ReadOnlySpan<byte> bytes)
4646
/// <exception cref="ArgumentNullException"><paramref name="stream" /> is <see langword="null" />.</exception>
4747
/// <exception cref="NotSupportedException">The stream does not support reading or seeking.</exception>
4848
/// <exception cref="ObjectDisposedException">The stream was closed.</exception>
49+
/// <exception cref="IOException">An I/O error occurred.</exception>
4950
/// <remarks>When this method returns, <paramref name="stream" /> is restored to its original position.</remarks>
5051
public static bool StartsWithPayloadHeader(Stream stream)
5152
{
@@ -107,6 +108,7 @@ public static bool StartsWithPayloadHeader(Stream stream)
107108
/// <exception cref="ArgumentNullException"><paramref name="payload"/> is <see langword="null" />.</exception>
108109
/// <exception cref="ArgumentException"><paramref name="payload"/> does not support reading or is already closed.</exception>
109110
/// <exception cref="SerializationException">Reading from <paramref name="payload"/> encountered invalid NRBF data.</exception>
111+
/// <exception cref="IOException">An I/O error occurred.</exception>
110112
/// <exception cref="NotSupportedException">
111113
/// Reading from <paramref name="payload"/> encountered unsupported records,
112114
/// for example, arrays with non-zero offset or unsupported record types
@@ -142,7 +144,14 @@ public static SerializationRecord Decode(Stream payload, out IReadOnlyDictionary
142144
#endif
143145

144146
using BinaryReader reader = new(payload, ThrowOnInvalidUtf8Encoding, leaveOpen: leaveOpen);
145-
return Decode(reader, options ?? new(), out recordMap);
147+
try
148+
{
149+
return Decode(reader, options ?? new(), out recordMap);
150+
}
151+
catch (FormatException) // can be thrown by various BinaryReader methods
152+
{
153+
throw new SerializationException(SR.Serialization_InvalidFormat);
154+
}
146155
}
147156

148157
/// <summary>
@@ -213,12 +222,7 @@ private static SerializationRecord Decode(BinaryReader reader, PayloadOptions op
213222
private static SerializationRecord DecodeNext(BinaryReader reader, RecordMap recordMap,
214223
AllowedRecordTypes allowed, PayloadOptions options, out SerializationRecordType recordType)
215224
{
216-
byte nextByte = reader.ReadByte();
217-
if (((uint)allowed & (1u << nextByte)) == 0)
218-
{
219-
ThrowHelper.ThrowForUnexpectedRecordType(nextByte);
220-
}
221-
recordType = (SerializationRecordType)nextByte;
225+
recordType = reader.ReadSerializationRecordType(allowed);
222226

223227
SerializationRecord record = recordType switch
224228
{
@@ -254,7 +258,7 @@ private static SerializationRecord DecodeMemberPrimitiveTypedRecord(BinaryReader
254258
PrimitiveType.Boolean => new MemberPrimitiveTypedRecord<bool>(reader.ReadBoolean()),
255259
PrimitiveType.Byte => new MemberPrimitiveTypedRecord<byte>(reader.ReadByte()),
256260
PrimitiveType.SByte => new MemberPrimitiveTypedRecord<sbyte>(reader.ReadSByte()),
257-
PrimitiveType.Char => new MemberPrimitiveTypedRecord<char>(reader.ReadChar()),
261+
PrimitiveType.Char => new MemberPrimitiveTypedRecord<char>(reader.ParseChar()),
258262
PrimitiveType.Int16 => new MemberPrimitiveTypedRecord<short>(reader.ReadInt16()),
259263
PrimitiveType.UInt16 => new MemberPrimitiveTypedRecord<ushort>(reader.ReadUInt16()),
260264
PrimitiveType.Int32 => new MemberPrimitiveTypedRecord<int>(reader.ReadInt32()),
@@ -263,7 +267,7 @@ private static SerializationRecord DecodeMemberPrimitiveTypedRecord(BinaryReader
263267
PrimitiveType.UInt64 => new MemberPrimitiveTypedRecord<ulong>(reader.ReadUInt64()),
264268
PrimitiveType.Single => new MemberPrimitiveTypedRecord<float>(reader.ReadSingle()),
265269
PrimitiveType.Double => new MemberPrimitiveTypedRecord<double>(reader.ReadDouble()),
266-
PrimitiveType.Decimal => new MemberPrimitiveTypedRecord<decimal>(decimal.Parse(reader.ReadString(), CultureInfo.InvariantCulture)),
270+
PrimitiveType.Decimal => new MemberPrimitiveTypedRecord<decimal>(reader.ParseDecimal()),
267271
PrimitiveType.DateTime => new MemberPrimitiveTypedRecord<DateTime>(Utils.BinaryReaderExtensions.CreateDateTimeFromData(reader.ReadUInt64())),
268272
// String is handled with a record, never on it's own
269273
_ => new MemberPrimitiveTypedRecord<TimeSpan>(new TimeSpan(reader.ReadInt64())),

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

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -63,7 +63,8 @@ internal void Add(SerializationRecord record)
6363

6464
internal SerializationRecord GetRootRecord(SerializedStreamHeaderRecord header)
6565
{
66-
SerializationRecord rootRecord = _map[header.RootId];
66+
SerializationRecord rootRecord = GetRecord(header.RootId);
67+
6768
if (rootRecord is SystemClassWithMembersAndTypesRecord systemClass)
6869
{
6970
// 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)
7273

7374
return rootRecord;
7475
}
76+
77+
internal SerializationRecord GetRecord(SerializationRecordId recordId)
78+
=> _map.TryGetValue(recordId, out SerializationRecord? record)
79+
? record
80+
: throw new SerializationException(SR.Serialization_InvalidReference);
81+
82+
internal T GetRecord<T>(SerializationRecordId recordId) where T : SerializationRecord
83+
=> _map.TryGetValue(recordId, out SerializationRecord? record) && record is T casted
84+
? casted
85+
: throw new SerializationException(SR.Serialization_InvalidReference);
7586
}

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

Lines changed: 40 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,19 @@ internal static class BinaryReaderExtensions
1515
{
1616
private static object? s_baseAmbiguousDstDateTime;
1717

18+
internal static SerializationRecordType ReadSerializationRecordType(this BinaryReader reader, AllowedRecordTypes allowed)
19+
{
20+
byte nextByte = reader.ReadByte();
21+
if (nextByte > (byte)SerializationRecordType.MethodReturn // MethodReturn is the last defined value.
22+
|| (nextByte > (byte)SerializationRecordType.ArraySingleString && nextByte < (byte)SerializationRecordType.MethodCall) // not part of the spec
23+
|| ((uint)allowed & (1u << nextByte)) == 0) // valid, but not allowed
24+
{
25+
ThrowHelper.ThrowForUnexpectedRecordType(nextByte);
26+
}
27+
28+
return (SerializationRecordType)nextByte;
29+
}
30+
1831
internal static BinaryArrayType ReadArrayType(this BinaryReader reader)
1932
{
2033
byte arrayType = reader.ReadByte();
@@ -48,7 +61,7 @@ internal static PrimitiveType ReadPrimitiveType(this BinaryReader reader)
4861
{
4962
byte primitiveType = reader.ReadByte();
5063
// String is the last defined value, 4 is not used at all.
51-
if (primitiveType is 4 or > (byte)PrimitiveType.String)
64+
if (primitiveType is 0 or 4 or (byte)PrimitiveType.Null or > (byte)PrimitiveType.String)
5265
{
5366
ThrowHelper.ThrowInvalidValue(primitiveType);
5467
}
@@ -64,7 +77,7 @@ internal static object ReadPrimitiveValue(this BinaryReader reader, PrimitiveTyp
6477
PrimitiveType.Boolean => reader.ReadBoolean(),
6578
PrimitiveType.Byte => reader.ReadByte(),
6679
PrimitiveType.SByte => reader.ReadSByte(),
67-
PrimitiveType.Char => reader.ReadChar(),
80+
PrimitiveType.Char => reader.ParseChar(),
6881
PrimitiveType.Int16 => reader.ReadInt16(),
6982
PrimitiveType.UInt16 => reader.ReadUInt16(),
7083
PrimitiveType.Int32 => reader.ReadInt32(),
@@ -73,11 +86,35 @@ internal static object ReadPrimitiveValue(this BinaryReader reader, PrimitiveTyp
7386
PrimitiveType.UInt64 => reader.ReadUInt64(),
7487
PrimitiveType.Single => reader.ReadSingle(),
7588
PrimitiveType.Double => reader.ReadDouble(),
76-
PrimitiveType.Decimal => decimal.Parse(reader.ReadString(), CultureInfo.InvariantCulture),
89+
PrimitiveType.Decimal => reader.ParseDecimal(),
7790
PrimitiveType.DateTime => CreateDateTimeFromData(reader.ReadUInt64()),
7891
_ => new TimeSpan(reader.ReadInt64()),
7992
};
8093

94+
// BinaryFormatter serializes decimals as strings and we can't BinaryReader.ReadDecimal.
95+
internal static decimal ParseDecimal(this BinaryReader reader)
96+
{
97+
string text = reader.ReadString();
98+
if (!decimal.TryParse(text, NumberStyles.Number, CultureInfo.InvariantCulture, out decimal result))
99+
{
100+
ThrowHelper.ThrowInvalidFormat();
101+
}
102+
103+
return result;
104+
}
105+
106+
internal static char ParseChar(this BinaryReader reader)
107+
{
108+
try
109+
{
110+
return reader.ReadChar();
111+
}
112+
catch (ArgumentException) // A surrogate character was read.
113+
{
114+
throw new SerializationException(SR.Serialization_SurrogateCharacter);
115+
}
116+
}
117+
81118
/// <summary>
82119
/// Creates a <see cref="DateTime"/> object from raw data with validation.
83120
/// </summary>

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

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,9 @@ internal static void ThrowArrayContainedNulls()
2929
internal static void ThrowInvalidAssemblyName(string rawName)
3030
=> throw new SerializationException(SR.Format(SR.Serialization_InvalidAssemblyName, rawName));
3131

32+
internal static void ThrowInvalidFormat()
33+
=> throw new SerializationException(SR.Serialization_InvalidFormat);
34+
3235
internal static void ThrowEndOfStreamException()
3336
=> throw new EndOfStreamException();
3437

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

Lines changed: 107 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -426,7 +426,9 @@ public static IEnumerable<object[]> ThrowsForInvalidPrimitiveType_Arguments()
426426
{
427427
foreach (byte binaryType in new byte[] { (byte)0 /* BinaryType.Primitive */, (byte)7 /* BinaryType.PrimitiveArray */ })
428428
{
429+
yield return new object[] { recordType, binaryType, (byte)0 }; // value not used by the spec
429430
yield return new object[] { recordType, binaryType, (byte)4 }; // value not used by the spec
431+
yield return new object[] { recordType, binaryType, (byte)17 }; // used by the spec, but illegal in given context
430432
yield return new object[] { recordType, binaryType, (byte)19 };
431433
}
432434
}
@@ -478,4 +480,109 @@ public void ThrowsOnInvalidArrayType()
478480
stream.Position = 0;
479481
Assert.Throws<SerializationException>(() => NrbfDecoder.Decode(stream));
480482
}
483+
484+
[Theory]
485+
[InlineData(18, typeof(NotSupportedException))] // not part of the spec, but still less than max allowed value (22)
486+
[InlineData(19, typeof(NotSupportedException))] // same as above
487+
[InlineData(20, typeof(NotSupportedException))] // same as above
488+
[InlineData(23, typeof(SerializationException))] // not part of the spec and more than max allowed value (22)
489+
[InlineData(64, typeof(SerializationException))] // same as above but also matches AllowedRecordTypes.SerializedStreamHeader
490+
public void InvalidSerializationRecordType(byte recordType, Type expectedException)
491+
{
492+
using MemoryStream stream = new();
493+
BinaryWriter writer = new(stream, Encoding.UTF8);
494+
495+
WriteSerializedStreamHeader(writer);
496+
writer.Write(recordType); // SerializationRecordType
497+
writer.Write((byte)SerializationRecordType.MessageEnd);
498+
499+
stream.Position = 0;
500+
501+
Assert.Throws(expectedException, () => NrbfDecoder.Decode(stream));
502+
}
503+
504+
[Fact]
505+
public void MissingRootRecord()
506+
{
507+
const int RootRecordId = 1;
508+
using MemoryStream stream = new();
509+
BinaryWriter writer = new(stream, Encoding.UTF8);
510+
511+
WriteSerializedStreamHeader(writer, rootId: RootRecordId);
512+
writer.Write((byte)SerializationRecordType.BinaryObjectString);
513+
writer.Write(RootRecordId + 1); // a different ID
514+
writer.Write("theString");
515+
writer.Write((byte)SerializationRecordType.MessageEnd);
516+
517+
stream.Position = 0;
518+
519+
Assert.Throws<SerializationException>(() => NrbfDecoder.Decode(stream));
520+
}
521+
522+
[Fact]
523+
public void Invalid7BitEncodedStringLength()
524+
{
525+
// The highest bit of the last byte is set (so it's invalid).
526+
byte[] invalidLength = [byte.MaxValue, byte.MaxValue, byte.MaxValue, byte.MaxValue, byte.MaxValue];
527+
528+
using MemoryStream stream = new();
529+
BinaryWriter writer = new(stream, Encoding.UTF8);
530+
531+
WriteSerializedStreamHeader(writer);
532+
writer.Write((byte)SerializationRecordType.BinaryObjectString);
533+
writer.Write(1); // root record Id
534+
writer.Write(invalidLength); // the length prefix
535+
writer.Write(Encoding.UTF8.GetBytes("theString"));
536+
writer.Write((byte)SerializationRecordType.MessageEnd);
537+
538+
stream.Position = 0;
539+
540+
Assert.Throws<SerializationException>(() => NrbfDecoder.Decode(stream));
541+
}
542+
543+
[Theory]
544+
[InlineData("79228162514264337593543950336")] // invalid format (decimal.MaxValue + 1)
545+
[InlineData("1111111111111111111111111111111111111111111111111")] // overflow
546+
public void InvalidDecimal(string textRepresentation)
547+
{
548+
using MemoryStream stream = new();
549+
BinaryWriter writer = new(stream, Encoding.UTF8);
550+
551+
WriteSerializedStreamHeader(writer);
552+
writer.Write((byte)SerializationRecordType.SystemClassWithMembersAndTypes);
553+
writer.Write(1); // root record Id
554+
writer.Write("ClassWithDecimalField"); // type name
555+
writer.Write(1); // member count
556+
writer.Write("memberName");
557+
writer.Write((byte)BinaryType.Primitive);
558+
writer.Write((byte)PrimitiveType.Decimal);
559+
writer.Write(textRepresentation);
560+
writer.Write((byte)SerializationRecordType.MessageEnd);
561+
562+
stream.Position = 0;
563+
564+
Assert.Throws<SerializationException>(() => NrbfDecoder.Decode(stream));
565+
}
566+
567+
[Fact]
568+
public void SurrogateCharacter()
569+
{
570+
using MemoryStream stream = new();
571+
BinaryWriter writer = new(stream, Encoding.UTF8);
572+
573+
WriteSerializedStreamHeader(writer);
574+
writer.Write((byte)SerializationRecordType.SystemClassWithMembersAndTypes);
575+
writer.Write(1); // root record Id
576+
writer.Write("ClassWithCharField"); // type name
577+
writer.Write(1); // member count
578+
writer.Write("memberName");
579+
writer.Write((byte)BinaryType.Primitive);
580+
writer.Write((byte)PrimitiveType.Char);
581+
writer.Write((byte)0xC0); // a surrogate character
582+
writer.Write((byte)SerializationRecordType.MessageEnd);
583+
584+
stream.Position = 0;
585+
586+
Assert.Throws<SerializationException>(() => NrbfDecoder.Decode(stream));
587+
}
481588
}

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -45,10 +45,10 @@ protected static BinaryFormatter CreateBinaryFormatter()
4545
};
4646
#pragma warning restore SYSLIB0011 // Type or member is obsolete
4747

48-
protected static void WriteSerializedStreamHeader(BinaryWriter writer, int major = 1, int minor = 0)
48+
protected static void WriteSerializedStreamHeader(BinaryWriter writer, int major = 1, int minor = 0, int rootId = 1)
4949
{
5050
writer.Write((byte)SerializationRecordType.SerializedStreamHeader);
51-
writer.Write(1); // root ID
51+
writer.Write(rootId); // root ID
5252
writer.Write(1); // header ID
5353
writer.Write(major); // major version
5454
writer.Write(minor); // minor version

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

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,8 @@
77

88
<ItemGroup>
99
<Compile Include="..\src\System\Formats\Nrbf\BinaryArrayType.cs" Link="BinaryArrayType.cs" />
10+
<Compile Include="..\src\System\Formats\Nrbf\BinaryType.cs" Link="BinaryType.cs" />
11+
<Compile Include="..\src\System\Formats\Nrbf\PrimitiveType.cs" Link="PrimitiveType.cs" />
1012
</ItemGroup>
1113

1214
<ItemGroup>

0 commit comments

Comments
 (0)