Skip to content

Commit 0154a2f

Browse files
adamsitnikadamsitnik
authored
[NRBF] Address issues discovered by Threat Model (#106629)
* introduce ArrayRecord.FlattenedLength * do not include invalid Type or Assembly names in the exception messages, as it's most likely corrupted/tampered/malicious data and could be used as a vector of attack. * It is possible to have binary array records have an element type of array without being marked as jagged
1 parent 550d381 commit 0154a2f

13 files changed

+197
-32
lines changed

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ public abstract partial class ArrayRecord : System.Formats.Nrbf.SerializationRec
1111
internal ArrayRecord() { }
1212
public override System.Formats.Nrbf.SerializationRecordId Id { get { throw null; } }
1313
public abstract System.ReadOnlySpan<int> Lengths { get; }
14+
public virtual long FlattenedLength { get; }
1415
public int Rank { get { throw null; } }
1516
[System.Diagnostics.CodeAnalysis.RequiresDynamicCode("The code for an array of the specified type might not be available.")]
1617
public System.Array GetArray(System.Type expectedArrayType, bool allowNulls = true) { throw null; }

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

Lines changed: 4 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -126,26 +126,23 @@
126126
<data name="Serialization_UnexpectedNullRecordCount" xml:space="preserve">
127127
<value>Unexpected Null Record count.</value>
128128
</data>
129-
<data name="Serialization_MaxArrayLength" xml:space="preserve">
130-
<value>The serialized array length ({0}) was larger than the configured limit {1}.</value>
131-
</data>
132129
<data name="NotSupported_RecordType" xml:space="preserve">
133130
<value>{0} Record Type is not supported by design.</value>
134131
</data>
135132
<data name="Serialization_InvalidReference" xml:space="preserve">
136133
<value>Invalid member reference.</value>
137134
</data>
138135
<data name="Serialization_InvalidTypeName" xml:space="preserve">
139-
<value>Invalid type name: `{0}`.</value>
136+
<value>Invalid type name.</value>
140137
</data>
141138
<data name="Serialization_TypeMismatch" xml:space="preserve">
142139
<value>Expected the array to be of type {0}, but its element type was {1}.</value>
143140
</data>
144141
<data name="Serialization_InvalidTypeOrAssemblyName" xml:space="preserve">
145-
<value>Invalid type or assembly name: `{0},{1}`.</value>
142+
<value>Invalid type or assembly name.</value>
146143
</data>
147144
<data name="Serialization_DuplicateMemberName" xml:space="preserve">
148-
<value>Duplicate member name: `{0}`.</value>
145+
<value>Duplicate member name.</value>
149146
</data>
150147
<data name="Argument_NonSeekableStream" xml:space="preserve">
151148
<value>Stream does not support seeking.</value>
@@ -160,7 +157,7 @@
160157
<value>Only arrays with zero offsets are supported.</value>
161158
</data>
162159
<data name="Serialization_InvalidAssemblyName" xml:space="preserve">
163-
<value>Invalid assembly name: `{0}`.</value>
160+
<value>Invalid assembly name.</value>
164161
</data>
165162
<data name="Serialization_InvalidFormat" xml:space="preserve">
166163
<value>Invalid format.</value>

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

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -25,23 +25,23 @@ internal readonly struct ArrayInfo
2525
internal ArrayInfo(SerializationRecordId id, long totalElementsCount, BinaryArrayType arrayType = BinaryArrayType.Single, int rank = 1)
2626
{
2727
Id = id;
28-
TotalElementsCount = totalElementsCount;
28+
FlattenedLength = totalElementsCount;
2929
ArrayType = arrayType;
3030
Rank = rank;
3131
}
3232

3333
internal SerializationRecordId Id { get; }
3434

35-
internal long TotalElementsCount { get; }
35+
internal long FlattenedLength { get; }
3636

3737
internal BinaryArrayType ArrayType { get; }
3838

3939
internal int Rank { get; }
4040

4141
internal int GetSZArrayLength()
4242
{
43-
Debug.Assert(TotalElementsCount <= MaxArrayLength);
44-
return (int)TotalElementsCount;
43+
Debug.Assert(FlattenedLength <= MaxArrayLength);
44+
return (int)FlattenedLength;
4545
}
4646

4747
internal static ArrayInfo Decode(BinaryReader reader)

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

Lines changed: 13 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@ public abstract class ArrayRecord : SerializationRecord
1818
private protected ArrayRecord(ArrayInfo arrayInfo)
1919
{
2020
ArrayInfo = arrayInfo;
21-
ValuesToRead = arrayInfo.TotalElementsCount;
21+
ValuesToRead = arrayInfo.FlattenedLength;
2222
}
2323

2424
/// <summary>
@@ -27,6 +27,12 @@ private protected ArrayRecord(ArrayInfo arrayInfo)
2727
/// <value>A buffer of integers that represent the number of elements in every dimension.</value>
2828
public abstract ReadOnlySpan<int> Lengths { get; }
2929

30+
/// <summary>
31+
/// When overridden in a derived class, gets the total number of all elements in every dimension.
32+
/// </summary>
33+
/// <value>A number that represent the total number of all elements in every dimension.</value>
34+
public virtual long FlattenedLength => ArrayInfo.FlattenedLength;
35+
3036
/// <summary>
3137
/// Gets the rank of the array.
3238
/// </summary>
@@ -44,7 +50,12 @@ private protected ArrayRecord(ArrayInfo arrayInfo)
4450

4551
internal long ValuesToRead { get; private protected set; }
4652

47-
private protected ArrayInfo ArrayInfo { get; }
53+
internal ArrayInfo ArrayInfo { get; }
54+
55+
internal bool IsJagged
56+
=> ArrayInfo.ArrayType == BinaryArrayType.Jagged
57+
// It is possible to have binary array records have an element type of array without being marked as jagged.
58+
|| TypeName.GetElementType().IsArray;
4859

4960
/// <summary>
5061
/// Allocates an array and fills it with the data provided in the serialized records (in case of primitive types like <see cref="string"/> or <see cref="int"/>) or the serialized records themselves.

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

Lines changed: 78 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,19 +28,38 @@ internal sealed class BinaryArrayRecord : ArrayRecord
2828
];
2929

3030
private TypeName? _typeName;
31+
private long _totalElementsCount;
3132

3233
private BinaryArrayRecord(ArrayInfo arrayInfo, MemberTypeInfo memberTypeInfo)
3334
: base(arrayInfo)
3435
{
3536
MemberTypeInfo = memberTypeInfo;
3637
Values = [];
38+
// We need to parse all elements of the jagged array to obtain total elements count.
39+
_totalElementsCount = -1;
3740
}
3841

3942
public override SerializationRecordType RecordType => SerializationRecordType.BinaryArray;
4043

4144
/// <inheritdoc/>
4245
public override ReadOnlySpan<int> Lengths => new int[1] { Length };
4346

47+
/// <inheritdoc/>
48+
public override long FlattenedLength
49+
{
50+
get
51+
{
52+
if (_totalElementsCount < 0)
53+
{
54+
_totalElementsCount = IsJagged
55+
? GetJaggedArrayFlattenedLength(this)
56+
: ArrayInfo.FlattenedLength;
57+
}
58+
59+
return _totalElementsCount;
60+
}
61+
}
62+
4463
public override TypeName TypeName
4564
=> _typeName ??= MemberTypeInfo.GetArrayTypeName(ArrayInfo);
4665

@@ -174,6 +193,65 @@ internal static ArrayRecord Decode(BinaryReader reader, RecordMap recordMap, Pay
174193
: new BinaryArrayRecord(arrayInfo, memberTypeInfo);
175194
}
176195

196+
private static long GetJaggedArrayFlattenedLength(BinaryArrayRecord jaggedArrayRecord)
197+
{
198+
long result = 0;
199+
Queue<BinaryArrayRecord>? jaggedArrayRecords = null;
200+
201+
do
202+
{
203+
if (jaggedArrayRecords is not null)
204+
{
205+
jaggedArrayRecord = jaggedArrayRecords.Dequeue();
206+
}
207+
208+
Debug.Assert(jaggedArrayRecord.IsJagged);
209+
210+
// In theory somebody could create a payload that would represent
211+
// a very nested array with total elements count > long.MaxValue.
212+
// That is why this method is using checked arithmetic.
213+
result = checked(result + jaggedArrayRecord.Length); // count the arrays themselves
214+
215+
foreach (object value in jaggedArrayRecord.Values)
216+
{
217+
if (value is not SerializationRecord record)
218+
{
219+
continue;
220+
}
221+
222+
if (record.RecordType == SerializationRecordType.MemberReference)
223+
{
224+
record = ((MemberReferenceRecord)record).GetReferencedRecord();
225+
}
226+
227+
switch (record.RecordType)
228+
{
229+
case SerializationRecordType.ArraySinglePrimitive:
230+
case SerializationRecordType.ArraySingleObject:
231+
case SerializationRecordType.ArraySingleString:
232+
case SerializationRecordType.BinaryArray:
233+
ArrayRecord nestedArrayRecord = (ArrayRecord)record;
234+
if (nestedArrayRecord.IsJagged)
235+
{
236+
(jaggedArrayRecords ??= new()).Enqueue((BinaryArrayRecord)nestedArrayRecord);
237+
}
238+
else
239+
{
240+
// Don't call nestedArrayRecord.FlattenedLength to avoid any potential recursion,
241+
// just call nestedArrayRecord.ArrayInfo.FlattenedLength that returns pre-computed value.
242+
result = checked(result + nestedArrayRecord.ArrayInfo.FlattenedLength);
243+
}
244+
break;
245+
default:
246+
break;
247+
}
248+
}
249+
}
250+
while (jaggedArrayRecords is not null && jaggedArrayRecords.Count > 0);
251+
252+
return result;
253+
}
254+
177255
private protected override void AddValue(object value) => Values.Add(value);
178256

179257
internal override (AllowedRecordTypes allowed, PrimitiveType primitiveType) GetAllowedRecordType()

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,7 @@ internal static BinaryLibraryRecord Decode(BinaryReader reader, PayloadOptions o
5050
}
5151
else if (!options.UndoTruncatedTypeNames)
5252
{
53-
ThrowHelper.ThrowInvalidAssemblyName(rawName);
53+
ThrowHelper.ThrowInvalidAssemblyName();
5454
}
5555

5656
return new BinaryLibraryRecord(id, rawName);

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -71,7 +71,7 @@ internal static ClassInfo Decode(BinaryReader reader)
7171
continue;
7272
}
7373
#endif
74-
throw new SerializationException(SR.Format(SR.Serialization_DuplicateMemberName, memberName));
74+
ThrowHelper.ThrowDuplicateMemberName();
7575
}
7676

7777
return new ClassInfo(id, typeName, memberNames);

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

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -193,11 +193,11 @@ internal static RectangularArrayRecord Create(BinaryReader reader, ArrayInfo arr
193193
// to encountering an EOF if we realize later that we actually need to read more bytes in
194194
// order to fully populate the char[,,,...] array. Any such allocation is still linearly
195195
// proportional to the length of the incoming payload, so it's not a DoS vector.
196-
// The multiplication below is guaranteed not to overflow because TotalElementsCount is bounded
197-
// to <= uint.MaxValue (see BinaryArrayRecord.Decode) and sizeOfSingleValue is at most 8.
198-
Debug.Assert(arrayInfo.TotalElementsCount >= 0 && arrayInfo.TotalElementsCount <= long.MaxValue / sizeOfSingleValue);
196+
// The multiplication below is guaranteed not to overflow because FlattenedLength is bounded
197+
// to <= Array.MaxLength (see BinaryArrayRecord.Decode) and sizeOfSingleValue is at most 8.
198+
Debug.Assert(arrayInfo.FlattenedLength >= 0 && arrayInfo.FlattenedLength <= long.MaxValue / sizeOfSingleValue);
199199

200-
long size = arrayInfo.TotalElementsCount * sizeOfSingleValue;
200+
long size = arrayInfo.FlattenedLength * sizeOfSingleValue;
201201
bool? isDataAvailable = reader.IsDataAvailable(size);
202202
if (isDataAvailable.HasValue)
203203
{

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

Lines changed: 10 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -6,28 +6,30 @@
66

77
namespace System.Formats.Nrbf.Utils;
88

9+
// The exception messages do not contain member/type/assembly names on purpose,
10+
// as it's most likely corrupted/tampered/malicious data.
911
internal static class ThrowHelper
1012
{
11-
internal static void ThrowInvalidValue(object value)
13+
internal static void ThrowDuplicateMemberName()
14+
=> throw new SerializationException(SR.Serialization_DuplicateMemberName);
15+
16+
internal static void ThrowInvalidValue(int value)
1217
=> throw new SerializationException(SR.Format(SR.Serialization_InvalidValue, value));
1318

1419
internal static void ThrowInvalidReference()
1520
=> throw new SerializationException(SR.Serialization_InvalidReference);
1621

17-
internal static void ThrowInvalidTypeName(string name)
18-
=> throw new SerializationException(SR.Format(SR.Serialization_InvalidTypeName, name));
22+
internal static void ThrowInvalidTypeName()
23+
=> throw new SerializationException(SR.Serialization_InvalidTypeName);
1924

2025
internal static void ThrowUnexpectedNullRecordCount()
2126
=> throw new SerializationException(SR.Serialization_UnexpectedNullRecordCount);
2227

23-
internal static void ThrowMaxArrayLength(long limit, long actual)
24-
=> throw new SerializationException(SR.Format(SR.Serialization_MaxArrayLength, actual, limit));
25-
2628
internal static void ThrowArrayContainedNulls()
2729
=> throw new SerializationException(SR.Serialization_ArrayContainedNulls);
2830

29-
internal static void ThrowInvalidAssemblyName(string rawName)
30-
=> throw new SerializationException(SR.Format(SR.Serialization_InvalidAssemblyName, rawName));
31+
internal static void ThrowInvalidAssemblyName()
32+
=> throw new SerializationException(SR.Serialization_InvalidAssemblyName);
3133

3234
internal static void ThrowInvalidFormat()
3335
=> throw new SerializationException(SR.Serialization_InvalidFormat);

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -135,7 +135,7 @@ internal static TypeName ParseNonSystemClassRecordTypeName(this string rawName,
135135

136136
if (typeName is null)
137137
{
138-
throw new SerializationException(SR.Format(SR.Serialization_InvalidTypeOrAssemblyName, rawName, libraryRecord.RawLibraryName));
138+
throw new SerializationException(SR.Serialization_InvalidTypeOrAssemblyName);
139139
}
140140

141141
if (typeName.AssemblyName is null)
@@ -183,7 +183,7 @@ private static TypeName With(this TypeName typeName, AssemblyNameInfo assemblyNa
183183
else
184184
{
185185
// BinaryFormatter can not serialize pointers or references.
186-
ThrowHelper.ThrowInvalidTypeName(typeName.FullName);
186+
ThrowHelper.ThrowInvalidTypeName();
187187
}
188188
}
189189

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -141,6 +141,7 @@ private void Test<T>(int size, bool canSeek)
141141
SZArrayRecord<T> arrayRecord = (SZArrayRecord<T>)NrbfDecoder.Decode(stream);
142142

143143
Assert.Equal(size, arrayRecord.Length);
144+
Assert.Equal(size, arrayRecord.FlattenedLength);
144145
T?[] output = arrayRecord.GetArray();
145146
Assert.Equal(input, output);
146147
Assert.Same(output, arrayRecord.GetArray());

0 commit comments

Comments
 (0)