diff --git a/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/AllowedRecordType.cs b/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/AllowedRecordType.cs index 8a3b304610555..063a243078206 100644 --- a/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/AllowedRecordType.cs +++ b/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/AllowedRecordType.cs @@ -3,6 +3,9 @@ namespace System.Formats.Nrbf; +// See [MS-NRBF] Sec. 2.7 for more information. +// https://learn.microsoft.com/openspecs/windows_protocols/ms-nrbf/ca3ad2bc-777b-413a-a72a-9ba6ced76bc3 + [Flags] internal enum AllowedRecordTypes : uint { diff --git a/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/ArrayInfo.cs b/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/ArrayInfo.cs index 4b24b1912e89a..40468c3f8bb3d 100644 --- a/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/ArrayInfo.cs +++ b/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/ArrayInfo.cs @@ -16,7 +16,11 @@ namespace System.Formats.Nrbf; [DebuggerDisplay("{ArrayType}, rank={Rank}")] internal readonly struct ArrayInfo { - internal const int MaxArrayLength = 2147483591; // Array.MaxLength +#if NET8_0_OR_GREATER + internal static int MaxArrayLength => Array.MaxLength; // dynamic lookup in case the value changes in a future runtime +#else + internal const int MaxArrayLength = 2147483591; // hardcode legacy Array.MaxLength for downlevel runtimes +#endif internal ArrayInfo(SerializationRecordId id, long totalElementsCount, BinaryArrayType arrayType = BinaryArrayType.Single, int rank = 1) { @@ -47,7 +51,7 @@ internal static int ParseValidArrayLength(BinaryReader reader) { int length = reader.ReadInt32(); - if (length is < 0 or > MaxArrayLength) + if (length < 0 || length > MaxArrayLength) { ThrowHelper.ThrowInvalidValue(length); } diff --git a/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/ArrayOfClassesRecord.cs b/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/ArrayOfClassesRecord.cs index 46e066bd39dbb..f345292c693a6 100644 --- a/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/ArrayOfClassesRecord.cs +++ b/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/ArrayOfClassesRecord.cs @@ -4,6 +4,7 @@ using System.Collections.Generic; using System.Reflection.Metadata; using System.Formats.Nrbf.Utils; +using System.Diagnostics; namespace System.Formats.Nrbf; @@ -54,6 +55,7 @@ public override TypeName TypeName } int nullCount = ((NullsRecord)actual).NullCount; + Debug.Assert(nullCount > 0, "All implementations of NullsRecord are expected to return a positive value for NullCount."); do { result[resultIndex++] = null; @@ -63,6 +65,8 @@ public override TypeName TypeName } } + Debug.Assert(resultIndex == result.Length, "We should have traversed the entirety of the newly created array."); + return result; } diff --git a/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/ArraySingleObjectRecord.cs b/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/ArraySingleObjectRecord.cs index 37e94842719a9..d0276ff3782e3 100644 --- a/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/ArraySingleObjectRecord.cs +++ b/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/ArraySingleObjectRecord.cs @@ -5,6 +5,7 @@ using System.IO; using System.Reflection.Metadata; using System.Formats.Nrbf.Utils; +using System.Diagnostics; namespace System.Formats.Nrbf; @@ -33,13 +34,15 @@ public override TypeName TypeName { object?[] values = new object?[Length]; - for (int recordIndex = 0, valueIndex = 0; recordIndex < Records.Count; recordIndex++) + int valueIndex = 0; + for (int recordIndex = 0; recordIndex < Records.Count; recordIndex++) { SerializationRecord record = Records[recordIndex]; int nullCount = record is NullsRecord nullsRecord ? nullsRecord.NullCount : 0; if (nullCount == 0) { + // "new object[] { }" is special cased because it allows for storing reference to itself. values[valueIndex++] = record is MemberReferenceRecord referenceRecord && referenceRecord.Reference.Equals(Id) ? values // a reference to self, and a way to get StackOverflow exception ;) : record.GetValue(); @@ -59,6 +62,8 @@ public override TypeName TypeName while (nullCount > 0); } + Debug.Assert(valueIndex == values.Length, "We should have traversed the entirety of the newly created array."); + return values; } diff --git a/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/ArraySinglePrimitiveRecord.cs b/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/ArraySinglePrimitiveRecord.cs index 46693c344bc84..a13507b97015a 100644 --- a/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/ArraySinglePrimitiveRecord.cs +++ b/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/ArraySinglePrimitiveRecord.cs @@ -53,8 +53,32 @@ internal static IReadOnlyList DecodePrimitiveTypes(BinaryReader reader, int c return (List)(object)DecodeDecimals(reader, count); } + // char[] has a unique representation in NRBF streams. Typical strings are transcoded + // to UTF-8 and prefixed with the number of bytes in the UTF-8 representation. char[] + // is also serialized as UTF-8, but it is instead prefixed with the number of chars + // in the UTF-16 representation, not the number of bytes in the UTF-8 representation. + // This number doesn't directly precede the UTF-8 contents in the NRBF stream; it's + // instead contained within the ArrayInfo structure (passed to this method as the + // 'count' argument). + // + // The practical consequence of this is that we don't actually know how many UTF-8 + // bytes we need to consume in order to ensure we've read 'count' chars. We know that + // an n-length UTF-16 string turns into somewhere between [n .. 3n] UTF-8 bytes. + // The best we can do is that when reading an n-element char[], we'll ensure that + // there are at least n bytes remaining in the input stream. We'll still need to + // account for that even with this check, we might hit EOF before fully populating + // the char[]. But from a safety perspective, it does appropriately limit our + // allocations to be proportional to the amount of data present in the input stream, + // which is a sufficient defense against DoS. + long requiredBytes = count; - if (typeof(T) != typeof(char)) // the input is UTF8 + if (typeof(T) == typeof(DateTime) || typeof(T) == typeof(TimeSpan)) + { + // We can't assume DateTime as represented by the runtime is 8 bytes. + // The only assumption we can make is that it's 8 bytes on the wire. + requiredBytes *= 8; + } + else if (typeof(T) != typeof(char)) { requiredBytes *= Unsafe.SizeOf(); } @@ -79,6 +103,10 @@ internal static IReadOnlyList DecodePrimitiveTypes(BinaryReader reader, int c { return (T[])(object)reader.ParseChars(count); } + else if (typeof(T) == typeof(TimeSpan) || typeof(T) == typeof(DateTime)) + { + return DecodeTime(reader, count); + } // It's safe to pre-allocate, as we have ensured there is enough bytes in the stream. T[] result = new T[count]; @@ -130,8 +158,7 @@ internal static IReadOnlyList DecodePrimitiveTypes(BinaryReader reader, int c } #endif } - else if (typeof(T) == typeof(long) || typeof(T) == typeof(ulong) || typeof(T) == typeof(double) - || typeof(T) == typeof(DateTime) || typeof(T) == typeof(TimeSpan)) + else if (typeof(T) == typeof(long) || typeof(T) == typeof(ulong) || typeof(T) == typeof(double)) { Span span = MemoryMarshal.Cast(result); #if NET @@ -145,6 +172,21 @@ internal static IReadOnlyList DecodePrimitiveTypes(BinaryReader reader, int c } } + if (typeof(T) == typeof(bool)) + { + // See DontCastBytesToBooleans test to see what could go wrong. + bool[] booleans = (bool[])(object)result; + resultAsBytes = MemoryMarshal.AsBytes(result); + for (int i = 0; i < booleans.Length; i++) + { + // We don't use the bool array to get the value, as an optimizing compiler or JIT could elide this. + if (resultAsBytes[i] != 0) // it can be any byte different than 0 + { + booleans[i] = true; // set it to 1 in explicit way + } + } + } + return result; } @@ -158,8 +200,34 @@ private static List DecodeDecimals(BinaryReader reader, int count) return values; } + private static T[] DecodeTime(BinaryReader reader, int count) + { + T[] values = new T[count]; + for (int i = 0; i < values.Length; i++) + { + if (typeof(T) == typeof(DateTime)) + { + values[i] = (T)(object)Utils.BinaryReaderExtensions.CreateDateTimeFromData(reader.ReadUInt64()); + } + else if (typeof(T) == typeof(TimeSpan)) + { + values[i] = (T)(object)new TimeSpan(reader.ReadInt64()); + } + else + { + throw new InvalidOperationException(); + } + } + + return values; + } + private static List DecodeFromNonSeekableStream(BinaryReader reader, int count) { + // The count arg could originate from untrusted input, so we shouldn't + // pass it as-is to the ctor's capacity arg. We'll instead rely on + // List.Add's O(1) amortization to keep the entire loop O(count). + List values = new List(Math.Min(count, 4)); for (int i = 0; i < count; i++) { diff --git a/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/ArraySingleStringRecord.cs b/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/ArraySingleStringRecord.cs index 7fed2a494b9b0..42b9eadd97bd5 100644 --- a/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/ArraySingleStringRecord.cs +++ b/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/ArraySingleStringRecord.cs @@ -5,6 +5,7 @@ using System.IO; using System.Reflection.Metadata; using System.Formats.Nrbf.Utils; +using System.Diagnostics; namespace System.Formats.Nrbf; @@ -47,7 +48,8 @@ internal override (AllowedRecordTypes allowed, PrimitiveType primitiveType) GetA { string?[] values = new string?[Length]; - for (int recordIndex = 0, valueIndex = 0; recordIndex < Records.Count; recordIndex++) + int valueIndex = 0; + for (int recordIndex = 0; recordIndex < Records.Count; recordIndex++) { SerializationRecord record = Records[recordIndex]; @@ -73,6 +75,7 @@ record = memberReference.GetReferencedRecord(); } int nullCount = ((NullsRecord)record).NullCount; + Debug.Assert(nullCount > 0, "All implementations of NullsRecord are expected to return a positive value for NullCount."); do { values[valueIndex++] = null; @@ -81,6 +84,8 @@ record = memberReference.GetReferencedRecord(); while (nullCount > 0); } + Debug.Assert(valueIndex == values.Length, "We should have traversed the entirety of the newly created array."); + return values; } } diff --git a/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/BinaryArrayRecord.cs b/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/BinaryArrayRecord.cs index 5aa4878016d9f..cc3210a3e32b7 100644 --- a/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/BinaryArrayRecord.cs +++ b/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/BinaryArrayRecord.cs @@ -6,6 +6,7 @@ using System.IO; using System.Reflection.Metadata; using System.Formats.Nrbf.Utils; +using System.Diagnostics; namespace System.Formats.Nrbf; @@ -84,6 +85,10 @@ private protected override Array Deserialize(Type arrayType, bool allowNulls) case SerializationRecordType.ArraySinglePrimitive: case SerializationRecordType.ArraySingleObject: case SerializationRecordType.ArraySingleString: + + // Recursion depth is bounded by the depth of arrayType, which is + // a trustworthy Type instance. Don't need to worry about stack overflow. + ArrayRecord nestedArrayRecord = (ArrayRecord)record; Array nestedArray = nestedArrayRecord.GetArray(actualElementType, allowNulls); array.SetValue(nestedArray, resultIndex++); @@ -97,6 +102,7 @@ private protected override Array Deserialize(Type arrayType, bool allowNulls) } int nullCount = ((NullsRecord)item).NullCount; + Debug.Assert(nullCount > 0, "All implementations of NullsRecord are expected to return a positive value for NullCount."); do { array.SetValue(null, resultIndex++); @@ -110,6 +116,8 @@ private protected override Array Deserialize(Type arrayType, bool allowNulls) } } + Debug.Assert(resultIndex == array.Length, "We should have traversed the entirety of the newly created array."); + return array; } @@ -122,6 +130,7 @@ internal static ArrayRecord Decode(BinaryReader reader, RecordMap recordMap, Pay bool isRectangular = arrayType is BinaryArrayType.Rectangular; // It is an arbitrary limit in the current CoreCLR type loader. + // Don't change this value without reviewing the loop a few lines below. const int MaxSupportedArrayRank = 32; if (rank < 1 || rank > MaxSupportedArrayRank @@ -132,18 +141,26 @@ internal static ArrayRecord Decode(BinaryReader reader, RecordMap recordMap, Pay } int[] lengths = new int[rank]; // adversary-controlled, but acceptable since upper limit of 32 - long totalElementCount = 1; + long totalElementCount = 1; // to avoid integer overflow during the multiplication below for (int i = 0; i < lengths.Length; i++) { lengths[i] = ArrayInfo.ParseValidArrayLength(reader); totalElementCount *= lengths[i]; + // n.b. This forbids "new T[Array.MaxLength, Array.MaxLength, Array.MaxLength, ..., 0]" + // but allows "new T[0, Array.MaxLength, Array.MaxLength, Array.MaxLength, ...]". But + // that's the same behavior that newarr and Array.CreateInstance exhibit, so at least + // we're consistent. + if (totalElementCount > ArrayInfo.MaxArrayLength) { ThrowHelper.ThrowInvalidValue(lengths[i]); // max array size exceeded } } + // Per BinaryReaderExtensions.ReadArrayType, we do not support nonzero offsets, so + // we don't need to read the NRBF stream 'LowerBounds' field here. + MemberTypeInfo memberTypeInfo = MemberTypeInfo.Decode(reader, 1, options, recordMap); ArrayInfo arrayInfo = new(objectId, totalElementCount, arrayType, rank); @@ -186,6 +203,9 @@ private static Type MapElementType(Type arrayType, out bool isClassRecord) Type elementType = arrayType; int arrayNestingDepth = 0; + // Loop iteration counts are bound by the nesting depth of arrayType, + // which is a trustworthy input. No DoS concerns. + while (elementType.IsArray) { elementType = elementType.GetElementType()!; diff --git a/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/ClassInfo.cs b/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/ClassInfo.cs index 75340b72a4f0d..01645dcb51213 100644 --- a/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/ClassInfo.cs +++ b/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/ClassInfo.cs @@ -50,7 +50,8 @@ internal static ClassInfo Decode(BinaryReader reader) // Use Dictionary instead of List so that searching for member IDs by name // is O(n) instead of O(m * n), where m = memberCount and n = memberNameLength, - // in degenerate cases. + // in degenerate cases. Since memberCount may be hostile, don't allow it to be + // used as the initial capacity in the collection instance. Dictionary memberNames = new(StringComparer.Ordinal); for (int i = 0; i < memberCount; i++) { diff --git a/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/ClassTypeInfo.cs b/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/ClassTypeInfo.cs index b0b7e543fa9b5..dd5ee0e5bdf2c 100644 --- a/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/ClassTypeInfo.cs +++ b/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/ClassTypeInfo.cs @@ -9,7 +9,7 @@ namespace System.Formats.Nrbf; /// -/// Identifies a class by it's name and library id. +/// Identifies a class by its name and library id. /// /// /// ClassTypeInfo structures are described in [MS-NRBF] 2.1.1.8. diff --git a/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/MemberTypeInfo.cs b/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/MemberTypeInfo.cs index 2e4b7e1399be2..57e47a02eec68 100644 --- a/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/MemberTypeInfo.cs +++ b/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/MemberTypeInfo.cs @@ -110,7 +110,7 @@ internal bool ShouldBeRepresentedAsArrayOfClassRecords() { // This library tries to minimize the number of concepts the users need to learn to use it. // Since SZArrays are most common, it provides an SZArrayRecord abstraction. - // Every other array (jagged, multi-dimensional etc) is represented using SZArrayRecord. + // Every other array (jagged, multi-dimensional etc) is represented using ArrayRecord. // The goal of this method is to determine whether given array can be represented as SZArrayRecord. (BinaryType binaryType, object? additionalInfo) = Infos[0]; diff --git a/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/NextInfo.cs b/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/NextInfo.cs index a01a25e60047a..08b1f53dca670 100644 --- a/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/NextInfo.cs +++ b/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/NextInfo.cs @@ -27,7 +27,5 @@ internal NextInfo(AllowedRecordTypes allowed, SerializationRecord parent, internal PrimitiveType PrimitiveType { get; } internal NextInfo With(AllowedRecordTypes allowed, PrimitiveType primitiveType) - => allowed == Allowed && primitiveType == PrimitiveType - ? this // previous record was of the same type - : new(allowed, Parent, Stack, primitiveType); + => new(allowed, Parent, Stack, primitiveType); } diff --git a/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/NrbfDecoder.cs b/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/NrbfDecoder.cs index ef66ec7320f54..a315b37cff023 100644 --- a/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/NrbfDecoder.cs +++ b/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/NrbfDecoder.cs @@ -22,7 +22,7 @@ public static class NrbfDecoder // The header consists of: // - a byte that describes the record type (SerializationRecordType.SerializedStreamHeader) // - four 32 bit integers: - // - root Id (every value is valid) + // - root Id (every value except of 0 is valid) // - header Id (value is ignored) // - major version, it has to be equal 1. // - minor version, it has to be equal 0. diff --git a/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/PayloadOptions.cs b/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/PayloadOptions.cs index 60d1aafcc5291..fdb3ccae4632e 100644 --- a/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/PayloadOptions.cs +++ b/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/PayloadOptions.cs @@ -25,10 +25,17 @@ public PayloadOptions() { } /// /// if truncated type names should be reassembled; otherwise, . /// + /// /// Example: /// TypeName: "Namespace.TypeName`1[[Namespace.GenericArgName" /// LibraryName: "AssemblyName]]" /// Is combined into "Namespace.TypeName`1[[Namespace.GenericArgName, AssemblyName]]" + /// + /// + /// Setting this to can render susceptible to Denial of Service + /// attacks when parsing or handling malicious input. + /// + /// The default value is . /// public bool UndoTruncatedTypeNames { get; set; } } diff --git a/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/RectangularArrayRecord.cs b/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/RectangularArrayRecord.cs index bc286e56ee5c2..b5b15e71aecba 100644 --- a/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/RectangularArrayRecord.cs +++ b/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/RectangularArrayRecord.cs @@ -8,6 +8,7 @@ using System.Runtime.CompilerServices; using System.Runtime.InteropServices; using System.Formats.Nrbf.Utils; +using System.Diagnostics; namespace System.Formats.Nrbf; @@ -55,10 +56,12 @@ private protected override Array Deserialize(Type arrayType, bool allowNulls) #if !NET8_0_OR_GREATER int[] indices = new int[_lengths.Length]; + nuint numElementsWritten = 0; // only for debugging; not used in release builds foreach (object value in _values) { result.SetValue(GetActualValue(value), indices); + numElementsWritten++; int dimension = indices.Length - 1; while (dimension >= 0) @@ -78,6 +81,9 @@ private protected override Array Deserialize(Type arrayType, bool allowNulls) } } + Debug.Assert(numElementsWritten == (uint)_values.Count, "We should have traversed the entirety of the source values collection."); + Debug.Assert(numElementsWritten == (ulong)result.LongLength, "We should have traversed the entirety of the destination array."); + return result; #else // Idea from Array.CoreCLR that maps an array of int indices into @@ -118,6 +124,8 @@ static void CopyTo(List list, Array array) targetElement = (T)GetActualValue(value)!; flattenedIndex++; } + + Debug.Assert(flattenedIndex == (ulong)array.LongLength, "We should have traversed the entirety of the array."); } #endif } @@ -158,7 +166,7 @@ internal static RectangularArrayRecord Create(BinaryReader reader, ArrayInfo arr PrimitiveType.Boolean => sizeof(bool), PrimitiveType.Byte => sizeof(byte), PrimitiveType.SByte => sizeof(sbyte), - PrimitiveType.Char => sizeof(byte), // it's UTF8 + PrimitiveType.Char => sizeof(byte), // it's UTF8 (see comment below) PrimitiveType.Int16 => sizeof(short), PrimitiveType.UInt16 => sizeof(ushort), PrimitiveType.Int32 => sizeof(int), @@ -175,6 +183,20 @@ internal static RectangularArrayRecord Create(BinaryReader reader, ArrayInfo arr if (sizeOfSingleValue > 0) { + // NRBF encodes rectangular char[,,,...] by converting each standalone UTF-16 code point into + // its UTF-8 encoding. This means that surrogate code points (including adjacent surrogate + // pairs) occurring within a char[,,,...] cannot be encoded by NRBF. BinaryReader will detect + // that they're ill-formed and reject them on read. + // + // Per the comment in ArraySinglePrimitiveRecord.DecodePrimitiveTypes, we'll assume best-case + // encoding where 1 UTF-16 char encodes as a single UTF-8 byte, even though this might lead + // to encountering an EOF if we realize later that we actually need to read more bytes in + // order to fully populate the char[,,,...] array. Any such allocation is still linearly + // proportional to the length of the incoming payload, so it's not a DoS vector. + // The multiplication below is guaranteed not to overflow because TotalElementsCount is bounded + // to <= uint.MaxValue (see BinaryArrayRecord.Decode) and sizeOfSingleValue is at most 8. + Debug.Assert(arrayInfo.TotalElementsCount >= 0 && arrayInfo.TotalElementsCount <= long.MaxValue / sizeOfSingleValue); + long size = arrayInfo.TotalElementsCount * sizeOfSingleValue; bool? isDataAvailable = reader.IsDataAvailable(size); if (isDataAvailable.HasValue) diff --git a/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/SerializationRecord.cs b/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/SerializationRecord.cs index 133f507b6f247..8a2d9ad7653b3 100644 --- a/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/SerializationRecord.cs +++ b/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/SerializationRecord.cs @@ -13,7 +13,7 @@ namespace System.Formats.Nrbf; /// /// /// Every instance returned to the end user can be either , -/// a or an . +/// a , or an . /// /// [DebuggerDisplay("{RecordType}, {Id}")] @@ -50,7 +50,20 @@ internal SerializationRecord() // others can't derive from this type /// /// The type to compare against. /// if the serialized type name match provided type; otherwise, . - public bool TypeNameMatches(Type type) => Matches(type, TypeName); + /// is . + public bool TypeNameMatches(Type type) + { +#if NET + ArgumentNullException.ThrowIfNull(type); +#else + if (type is null) + { + throw new ArgumentNullException(nameof(type)); + } +#endif + + return Matches(type, TypeName); + } private static bool Matches(Type type, TypeName typeName) { @@ -61,10 +74,38 @@ private static bool Matches(Type type, TypeName typeName) return false; } + // The TypeName.FullName property getter is recursive and backed by potentially hostile + // input. See comments in that property getter for more information, including what defenses + // are in place to prevent attacks. + // + // Note that the equality comparison below is worst-case O(n) since the adversary could ensure + // that only the last char differs. Even if the strings have equal contents, we should still + // expect the comparison to take O(n) time since RuntimeType.FullName and TypeName.FullName + // will never reference the same string instance with current runtime implementations. + // + // Since a call to Matches could take place within a loop, and since TypeName.FullName could + // be arbitrarily long (it's attacker-controlled and the NRBF protocol allows backtracking via + // the ClassWithId record, providing a form of compression), this presents opportunity + // for an algorithmic complexity attack, where a (2 * l)-length payload has an l-length type + // name and an array with l elements, resulting in O(l^2) total work factor. Protection against + // such attack is provided by the fact that the System.Type object is fully under the app's + // control and is assumed to be trusted and a reasonable length. This brings the cumulative loop + // work factor back down to O(l * RuntimeType.FullName), which is acceptable. + // + // The above statement assumes that "(string)m == (string)n" has worst-case complexity + // O(min(m.Length, n.Length)). This is not stated in string's public docs, but it is + // a guaranteed behavior for all built-in Ordinal string comparisons. + // At first, check the non-allocating properties for mismatch. if (type.IsArray != typeName.IsArray || type.IsConstructedGenericType != typeName.IsConstructedGenericType || type.IsNested != typeName.IsNested - || (type.IsArray && type.GetArrayRank() != typeName.GetArrayRank())) + || (type.IsArray && type.GetArrayRank() != typeName.GetArrayRank()) +#if NET + || type.IsSZArray != typeName.IsSZArray // int[] vs int[*] +#else + || (type.IsArray && type.Name != typeName.Name) +#endif + ) { return false; } @@ -111,6 +152,11 @@ private static bool Matches(Type type, TypeName typeName) /// For reference records, it returns the referenced record. /// For other records, it returns the records themselves. /// + /// + /// Overrides of this method should take care not to allow + /// the introduction of cycles, even in the face of adversarial + /// edges in the object graph. + /// internal virtual object? GetValue() => this; internal virtual void HandleNextRecord(SerializationRecord nextRecord, NextInfo info) diff --git a/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/SerializationRecordId.cs b/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/SerializationRecordId.cs index a7478f5e3ffe0..a8318cb72d11d 100644 --- a/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/SerializationRecordId.cs +++ b/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/SerializationRecordId.cs @@ -31,6 +31,15 @@ internal static SerializationRecordId Decode(BinaryReader reader) { int id = reader.ReadInt32(); + // Many object ids are required to be positive. See: + // - https://learn.microsoft.com/openspecs/windows_protocols/ms-nrbf/8fac763f-e46d-43a1-b360-80eb83d2c5fb + // - https://learn.microsoft.com/openspecs/windows_protocols/ms-nrbf/eb503ca5-e1f6-4271-a7ee-c4ca38d07996 + // - https://learn.microsoft.com/openspecs/windows_protocols/ms-nrbf/7fcf30e1-4ad4-4410-8f1a-901a4a1ea832 (for library id) + // + // Exception: https://learn.microsoft.com/openspecs/windows_protocols/ms-nrbf/0a192be0-58a1-41d0-8a54-9c91db0ab7bf may be negative + // The problem is that input generated with FormatterTypeStyle.XsdString ends up generating negative Ids anyway. + // That information is not reflected in payload in anyway, so we just always allow for negative Ids. + if (id == 0) { ThrowHelper.ThrowInvalidValue(id); diff --git a/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/SerializationRecordType.cs b/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/SerializationRecordType.cs index 57760b8a377fc..b78bae2ac86ca 100644 --- a/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/SerializationRecordType.cs +++ b/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/SerializationRecordType.cs @@ -6,6 +6,9 @@ namespace System.Formats.Nrbf; /// /// Record type. /// +/// +/// SerializationRecordType enumeration is described in [MS-NRBF] 2.1.2.1. +/// public enum SerializationRecordType { /// diff --git a/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/Utils/BinaryReaderExtensions.cs b/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/Utils/BinaryReaderExtensions.cs index a174d11dfffbe..d5baa09dbd8fc 100644 --- a/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/Utils/BinaryReaderExtensions.cs +++ b/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/Utils/BinaryReaderExtensions.cs @@ -1,6 +1,7 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. +using System.Diagnostics; using System.Globalization; using System.IO; using System.Reflection; @@ -30,6 +31,11 @@ internal static SerializationRecordType ReadSerializationRecordType(this BinaryR internal static BinaryArrayType ReadArrayType(this BinaryReader reader) { + // To simplify the behavior and security review of the BinaryArrayRecord type, we + // do not support reading non-zero-offset arrays. If this should change in the + // future, the BinaryArrayRecord.Decode method and supporting infrastructure + // will need re-review. + byte arrayType = reader.ReadByte(); // Rectangular is the last defined value. if (arrayType > (byte)BinaryArrayType.Rectangular) @@ -95,6 +101,11 @@ internal static object ReadPrimitiveValue(this BinaryReader reader, PrimitiveTyp // BinaryFormatter serializes decimals as strings and we can't BinaryReader.ReadDecimal. internal static decimal ParseDecimal(this BinaryReader reader) { + // The spec (MS NRBF 2.1.1.6, https://learn.microsoft.com/en-us/openspecs/windows_protocols/ms-nrbf/10b218f5-9b2b-4947-b4b7-07725a2c8127) + // says that the length of LengthPrefixedString must be of optimal size (using as few bytes as possible). + // BinaryReader.ReadString does not enforce that and we are OK with that, + // as it takes care of handling multiple edge cases and we don't want to re-implement it. + string text = reader.ReadString(); if (!decimal.TryParse(text, NumberStyles.Number, CultureInfo.InvariantCulture, out decimal result)) { @@ -130,6 +141,9 @@ internal static char[] ParseChars(this BinaryReader reader, int count) if (result.Length != count) { + // We might hit EOF before fully reading the requested + // number of chars. This means that ReadChars(count) could return a char[] with + // *fewer* than 'count' elements. ThrowHelper.ThrowEndOfStreamException(); } @@ -200,6 +214,8 @@ static DateTime CreateFromAmbiguousDst(ulong ticks) internal static bool? IsDataAvailable(this BinaryReader reader, long requiredBytes) { + Debug.Assert(requiredBytes >= 0); + if (!reader.BaseStream.CanSeek) { return null; diff --git a/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/Utils/TypeNameHelpers.cs b/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/Utils/TypeNameHelpers.cs index 1c08a5c24eca5..f9a5dc0ef4385 100644 --- a/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/Utils/TypeNameHelpers.cs +++ b/src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/Utils/TypeNameHelpers.cs @@ -117,6 +117,17 @@ internal static TypeName ParseNonSystemClassRecordTypeName(this string rawName, Debug.Assert(payloadOptions.UndoTruncatedTypeNames); Debug.Assert(libraryRecord.RawLibraryName is not null); + // This is potentially a DoS vector, as somebody could submit: + // [1] BinaryLibraryRecord = + // [2] ClassRecord (lib = [1]) + // [3] ClassRecord (lib = [1]) + // ... + // [n] ClassRecord (lib = [1]) + // + // Which means somebody submits a payload of length O(long + n) and tricks us into + // performing O(long * n) work. For this reason, we have marked the UndoTruncatedTypeNames + // property as "keep this disabled unless you trust the input." + // Combining type and library allows us for handling truncated generic type names that may be present in resources. ArraySegment assemblyQualifiedName = RentAssemblyQualifiedName(rawName, libraryRecord.RawLibraryName); TypeName.TryParse(assemblyQualifiedName.AsSpan(), out TypeName? typeName, payloadOptions.TypeNameParseOptions); @@ -148,6 +159,10 @@ internal static TypeName WithCoreLibAssemblyName(this TypeName systemType) private static TypeName With(this TypeName typeName, AssemblyNameInfo assemblyName) { + // This is a recursive method over potentially hostile TypeName arguments. + // We assume the complexity of the TypeName arg was appropriately bounded. + // See comment in TypeName.FullName property getter for more info. + if (!typeName.IsSimple) { if (typeName.IsArray) @@ -186,6 +201,7 @@ private static TypeName ParseWithoutAssemblyName(string rawName, PayloadOptions return typeName; } + // Complexity is O(typeName.Length + libraryName.Length) private static ArraySegment RentAssemblyQualifiedName(string typeName, string libraryName) { int length = typeName.Length + 1 + libraryName.Length; diff --git a/src/libraries/System.Formats.Nrbf/tests/ArraySinglePrimitiveRecordTests.cs b/src/libraries/System.Formats.Nrbf/tests/ArraySinglePrimitiveRecordTests.cs index d4a4b5b3c690a..17155b8ba636d 100644 --- a/src/libraries/System.Formats.Nrbf/tests/ArraySinglePrimitiveRecordTests.cs +++ b/src/libraries/System.Formats.Nrbf/tests/ArraySinglePrimitiveRecordTests.cs @@ -3,6 +3,8 @@ using System.Collections.Generic; using System.IO; +using System.Runtime.Serialization; +using System.Text; using Xunit; namespace System.Formats.Nrbf.Tests; @@ -24,6 +26,51 @@ public static IEnumerable GetCanReadArrayOfAnySizeArgs() } } + [Fact] + public void DontCastBytesToBooleans() + { + using MemoryStream stream = new(); + BinaryWriter writer = new(stream, Encoding.UTF8); + + WriteSerializedStreamHeader(writer); + writer.Write((byte)SerializationRecordType.ArraySinglePrimitive); + writer.Write(1); // object ID + writer.Write(2); // length + writer.Write((byte)PrimitiveType.Boolean); // element type + writer.Write((byte)0x01); + writer.Write((byte)0x02); + writer.Write((byte)SerializationRecordType.MessageEnd); + stream.Position = 0; + + SZArrayRecord serializationRecord = (SZArrayRecord)NrbfDecoder.Decode(stream); + + bool[] bools = serializationRecord.GetArray(); + bool a = bools[0]; + Assert.True(a); + bool b = bools[1]; + Assert.True(b); + bool c = a && b; + Assert.True(c); + } + + [Fact] + public void DontCastBytesToDateTimes() + { + using MemoryStream stream = new(); + BinaryWriter writer = new(stream, Encoding.UTF8); + + WriteSerializedStreamHeader(writer); + writer.Write((byte)SerializationRecordType.ArraySinglePrimitive); + writer.Write(1); // object ID + writer.Write(1); // length + writer.Write((byte)PrimitiveType.DateTime); // element type + writer.Write(ulong.MaxValue); // un-representable DateTime + writer.Write((byte)SerializationRecordType.MessageEnd); + stream.Position = 0; + + Assert.Throws(() => NrbfDecoder.Decode(stream)); + } + [Theory] [MemberData(nameof(GetCanReadArrayOfAnySizeArgs))] public void CanReadArrayOfAnySize_Bool(int size, bool canSeek) => Test(size, canSeek); diff --git a/src/libraries/System.Formats.Nrbf/tests/AttackTests.cs b/src/libraries/System.Formats.Nrbf/tests/AttackTests.cs index d9f7ac05811ad..fe780d94698df 100644 --- a/src/libraries/System.Formats.Nrbf/tests/AttackTests.cs +++ b/src/libraries/System.Formats.Nrbf/tests/AttackTests.cs @@ -154,7 +154,7 @@ public void ArraysOfBytesAreNotBeingPreAllocated() writer.Write((byte)SerializationRecordType.ArraySinglePrimitive); writer.Write(1); // object ID writer.Write(Array.MaxLength); // length - writer.Write((byte)2); // PrimitiveType.Byte + writer.Write((byte)PrimitiveType.Byte); writer.Write((byte)SerializationRecordType.MessageEnd); stream.Position = 0; diff --git a/src/libraries/System.Formats.Nrbf/tests/InvalidInputTests.cs b/src/libraries/System.Formats.Nrbf/tests/InvalidInputTests.cs index 81899523f7d9d..6acb44d03697d 100644 --- a/src/libraries/System.Formats.Nrbf/tests/InvalidInputTests.cs +++ b/src/libraries/System.Formats.Nrbf/tests/InvalidInputTests.cs @@ -429,6 +429,7 @@ public static IEnumerable ThrowsForInvalidPrimitiveType_Arguments() 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)18 }; // used by the spec, but illegal in given context yield return new object[] { recordType, binaryType, (byte)19 }; } } diff --git a/src/libraries/System.Formats.Nrbf/tests/TypeMatchTests.cs b/src/libraries/System.Formats.Nrbf/tests/TypeMatchTests.cs index 6a981197f82f1..e0827c1225b42 100644 --- a/src/libraries/System.Formats.Nrbf/tests/TypeMatchTests.cs +++ b/src/libraries/System.Formats.Nrbf/tests/TypeMatchTests.cs @@ -73,6 +73,34 @@ public void CanRecognizeGenericSystemTypes() Verify(new Dictionary>>()); } + [Fact] + public void ThrowsForNullType() + { + List input = new List(); + + SerializationRecord record = NrbfDecoder.Decode(Serialize(input)); + + Assert.Throws(() => record.TypeNameMatches(type: null)); + } + + [Fact] + public void TakesCustomOffsetsIntoAccount() + { + int[] input = [1, 2, 3]; + + SerializationRecord record = NrbfDecoder.Decode(Serialize(input)); + + Assert.True(record.TypeNameMatches(typeof(int[]))); + + Type nonSzArray = typeof(int).Assembly.GetType("System.Int32[*]"); +#if NET + Assert.False(nonSzArray.IsSZArray); + Assert.True(nonSzArray.IsVariableBoundArray); +#endif + Assert.Equal(1, nonSzArray.GetArrayRank()); + Assert.False(record.TypeNameMatches(nonSzArray)); + } + [Fact] public void TakesGenericTypeDefinitionIntoAccount() { diff --git a/src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/AssemblyNameInfo.cs b/src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/AssemblyNameInfo.cs index bd1febff03659..3ac30c92db57a 100644 --- a/src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/AssemblyNameInfo.cs +++ b/src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/AssemblyNameInfo.cs @@ -81,6 +81,10 @@ internal AssemblyNameInfo(AssemblyNameParser.AssemblyNameParts parts) /// /// Gets the name of the culture associated with the assembly. /// + /// + /// Do not create a instance from this string unless + /// you know the string has originated from a trustworthy source. + /// public string? CultureName { get; } /// @@ -131,6 +135,10 @@ public string FullName /// /// Initializes a new instance of the class based on the stored information. /// + /// + /// Do not create an instance with string unless + /// you know the string has originated from a trustworthy source. + /// public AssemblyName ToAssemblyName() { AssemblyName assemblyName = new(); diff --git a/src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/TypeName.cs b/src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/TypeName.cs index 22ae86f08e6b5..2b620761bb708 100644 --- a/src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/TypeName.cs +++ b/src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/TypeName.cs @@ -95,7 +95,7 @@ private TypeName(string? fullName, /// If returns null, simply returns . /// public string AssemblyQualifiedName - => _assemblyQualifiedName ??= AssemblyName is null ? FullName : $"{FullName}, {AssemblyName.FullName}"; + => _assemblyQualifiedName ??= AssemblyName is null ? FullName : $"{FullName}, {AssemblyName.FullName}"; // see recursion comments in FullName /// /// Returns assembly name which contains this type, or null if this was not @@ -142,6 +142,17 @@ public string FullName { get { + // This is a recursive method over potentially hostile input. Protection against DoS is offered + // via the [Try]Parse method and TypeNameParserOptions.MaxNodes property at construction time. + // This FullName property getter and related methods assume that this TypeName instance has an + // acceptable node count. + // + // The node count controls the total amount of work performed by this method, including: + // - The max possible stack depth due to the recursive methods calls; and + // - The total number of bytes allocated by this function. For a deeply-nested TypeName + // object, the total allocation across the full object graph will be + // O(FullName.Length * GetNodeCount()). + if (_fullName is null) { if (IsConstructedGenericType) @@ -245,6 +256,8 @@ public string Name { get { + // Lookups to Name and FullName might be recursive. See comments in FullName property getter. + if (_name is null) { if (IsConstructedGenericType) @@ -425,6 +438,17 @@ public int GetArrayRank() /// The current type name is not simple. public TypeName WithAssemblyName(AssemblyNameInfo? assemblyName) { + // Recursive method. See comments in FullName property getter for more information + // on how this is protected against attack. + // + // n.b. AssemblyNameInfo could also be hostile. The typical exploit is that a single + // long AssemblyNameInfo is associated with one or more simple TypeName objects, + // leading to an alg. complexity attack (DoS). It's important that TypeName doesn't + // actually *do* anything with the provided AssemblyNameInfo rather than store it. + // For example, don't use it inside a string concat operation unless the caller + // explicitly requested that to happen. If the input is hostile, the caller should + // never perform such concats in a loop. + if (!IsSimple) { TypeNameParserHelpers.ThrowInvalidOperation_NotSimpleName(FullName); diff --git a/src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/TypeNameParser.cs b/src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/TypeNameParser.cs index 97294c8014f2b..08ed944d03108 100644 --- a/src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/TypeNameParser.cs +++ b/src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/TypeNameParser.cs @@ -80,6 +80,8 @@ private TypeNameParser(ReadOnlySpan name, bool throwOnError, TypeNameParse return null; } + // At this point, we have performed O(fullTypeNameLength) total work. + ReadOnlySpan fullTypeName = _inputString.Slice(0, fullTypeNameLength); _inputString = _inputString.Slice(fullTypeNameLength); @@ -142,6 +144,12 @@ private TypeNameParser(ReadOnlySpan name, bool throwOnError, TypeNameParse } } + // At this point, we may have performed O(fullTypeNameLength + _inputString.Length) total work. + // This will be the case if there was whitespace after the full type name in the original input + // string. We could end up looking at these same whitespace chars again later in this method, + // such as when parsing decorators. We rely on the TryDive routine to limit the total number + // of times we might inspect the same character. + // If there was an error stripping the generic args, back up to // before we started processing them, and let the decorator // parser try handling it. @@ -202,6 +210,9 @@ private TypeNameParser(ReadOnlySpan name, bool throwOnError, TypeNameParse result = new(fullName: null, assemblyName, elementOrGenericType: result, declaringType, genericArgs); } + // The loop below is protected by the dive check during the first decorator pass prior + // to assembly name parsing above. + if (previousDecorator != default) // some decorators were recognized { while (TryParseNextDecorator(ref capturedBeforeProcessing, out int parsedModifier)) @@ -245,6 +256,8 @@ private bool TryParseAssemblyName(ref AssemblyNameInfo? assemblyName) return null; } + // The loop below is protected by the dive check in GetFullTypeNameLength. + TypeName? declaringType = null; int nameOffset = 0; foreach (int nestedNameLength in nestedNameLengths) diff --git a/src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/TypeNameParserHelpers.cs b/src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/TypeNameParserHelpers.cs index 93859262eed99..7cafd746b7d17 100644 --- a/src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/TypeNameParserHelpers.cs +++ b/src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/TypeNameParserHelpers.cs @@ -16,6 +16,7 @@ internal static class TypeNameParserHelpers internal const int ByRef = -3; private const char EscapeCharacter = '\\'; #if NET8_0_OR_GREATER + // Keep this in sync with GetFullTypeNameLength/NeedsEscaping private static readonly SearchValues s_endOfFullTypeNameDelimitersSearchValues = SearchValues.Create("[]&*,+\\"); #endif @@ -30,7 +31,7 @@ internal static string GetGenericTypeFullName(ReadOnlySpan fullTypeName, R foreach (TypeName genericArg in genericArgs) { result.Append('['); - result.Append(genericArg.AssemblyQualifiedName); + result.Append(genericArg.AssemblyQualifiedName); // see recursion comments in TypeName.FullName result.Append(']'); result.Append(','); } @@ -97,11 +98,16 @@ static int GetUnescapedOffset(ReadOnlySpan input, int startOffset) return offset; } + // Keep this in sync with s_endOfFullTypeNameDelimitersSearchValues static bool NeedsEscaping(char c) => c is '[' or ']' or '&' or '*' or ',' or '+' or EscapeCharacter; } internal static ReadOnlySpan GetName(ReadOnlySpan fullName) { + // The two-value form of MemoryExtensions.LastIndexOfAny does not suffer + // from the behavior mentioned in the comment at the top of GetFullTypeNameLength. + // It always takes O(m * i) worst-case time and is safe to use here. + int offset = fullName.LastIndexOfAny('.', '+'); if (offset > 0 && fullName[offset - 1] == EscapeCharacter) // this should be very rare (IL Emit & pure IL) @@ -182,6 +188,13 @@ internal static string GetRankOrModifierStringRepresentation(int rankOrModifier, { Debug.Assert(rankOrModifier >= 2); + // O(rank) work, so we have to assume the rank is trusted. We don't put a hard cap on this, + // but within the TypeName parser, we do require the input string to contain the correct number + // of commas. This forces the input string to have at least O(rank) length, so there's no + // alg. complexity attack possible here. Callers can of course pass any arbitrary value to + // TypeName.MakeArrayTypeName, but per first sentence in this comment, we have to assume any + // such arbitrary value which is programmatically fed in originates from a trustworthy source. + builder.Append('['); builder.Append(',', rankOrModifier - 1); builder.Append(']'); @@ -310,6 +323,9 @@ internal static bool TryParseNextDecorator(ref ReadOnlySpan input, out int else if (TryStripFirstCharAndTrailingSpaces(ref input, ',')) { // [,,, ...] + // The runtime restricts arrays to rank 32, but we don't enforce that here. + // Instead, the max rank is controlled by the total number of commas present + // in the array decorator. checked { rank++; } goto ReadNextArrayToken; } diff --git a/src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/TypeNameParserOptions.cs b/src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/TypeNameParserOptions.cs index b7420c40aa9ea..53d6f7f164275 100644 --- a/src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/TypeNameParserOptions.cs +++ b/src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/TypeNameParserOptions.cs @@ -10,6 +10,13 @@ public sealed class TypeNameParseOptions /// /// Limits the maximum value of node count that parser can handle. /// + /// + /// + /// Setting this to a large value can render susceptible to Denial of Service + /// attacks when parsing or handling malicious input. + /// + /// The default value is 20. + /// public int MaxNodes { get => _maxNodes;