[NRBF] Comments and bug fixes from security review #107735

adamsitnik · 2024-09-12T12:27:30Z

This PR does the following:

copies all comments authored by @GrabYourPitchforks in our internal repo (f3f87a3)
changes a const that represented max array length to a field (because it may change in the future and should not be hardocded), it was also @GrabYourPitchforks suggestion (8a42d8b)
adds missing comments based (8a42d8b)
changes SerializationRecord.TypeNameMatches to throw ArgumentNullException for null argument (969a2ef)
makes SerializationRecord.TypeNameMatches aware of int[] vs int[*] difference (both are single dimension, but the latter is not zero-indexed, 61a9b14)
stops casting raw bytes to booleans, please see this example from @GrabYourPitchforks
to understand why (fcb3c8a)
stops casting raw bytes to DateTime in order to get them properly validated (88504ef)

… may change in the future

…entNullException for null Type argument

…ifference between SZArray and single-dimension, non-zero offset arrays (example: int[] and int[*])

dotnet-policy-service · 2024-09-12T12:27:47Z

Tagging subscribers to 'binaryformatter-migration': @adamsitnik, @bartonjs, @jeffhandley, @terrajobst

adamsitnik · 2024-09-12T12:27:52Z

@MihuBot fuzz NrbfDecoder

jkotas · 2024-09-12T14:26:34Z

src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/TypeName.cs

+                // - 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()).


I think that this is a perf and DoS bug in the implementation. It does not make sense for formatting back to the string to be quadratic when we go to a great lengths to ensure that the parsing is linear.

Maybe file an issue for this?

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

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

GrabYourPitchforks · 2024-09-12T19:23:00Z

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

        long requiredBytes = count;
-        if (typeof(T) != typeof(char)) // the input is UTF8
+        if (typeof(T) != typeof(char))
        {
            requiredBytes *= Unsafe.SizeOf<T>();


Unreliable. See offline comments. (I missed this in the initial review.)

NIT: I might be missing something, but this doesn't look like addressed

NIT: I might be missing something, but this doesn't look like addressed

I've addressed it by not using Unsafe.SizeOf<T>() for DateTime and TimeSpan:

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

Lines 75 to 81 in d68a423

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))

adamsitnik · 2024-09-13T07:23:41Z

@MihuBot fuzz NrbfDecoder

buyaa-n · 2024-09-16T03:52:20Z

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

@@ -6,6 +6,9 @@ namespace System.Formats.Nrbf;
 /// <summary>
 /// Specifies record types.
 /// </summary>
+/// <remarks>
+/// SerializationRecordType enumeration is described in <see href="https://learn.microsoft.com/en-us/openspecs/windows_protocols/ms-nrbf/954a0657-b901-4813-9398-4ec732fe8b32">[MS-NRBF] 2.1.2.1</see>.


NIT:

Suggested change

/// SerializationRecordType enumeration is described in <see href="https://learn.microsoft.com/en-us/openspecs/windows_protocols/ms-nrbf/954a0657-b901-4813-9398-4ec732fe8b32">[MS-NRBF] 2.1.2.1</see>.

/// SerializationRecordType enumeration is described in <see href="https://learn.microsoft.com/openspecs/windows_protocols/ms-nrbf/954a0657-b901-4813-9398-4ec732fe8b32">[MS-NRBF] 2.1.2.1</see>.

buyaa-n · 2024-09-16T03:52:42Z

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

@@ -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)


NIT:

Suggested change

// The spec (MS NRBF 2.1.1.6, https://learn.microsoft.com/en-us/openspecs/windows_protocols/ms-nrbf/10b218f5-9b2b-4947-b4b7-07725a2c8127)

// The spec (MS NRBF 2.1.1.6, https://learn.microsoft.com/openspecs/windows_protocols/ms-nrbf/10b218f5-9b2b-4947-b4b7-07725a2c8127)

buyaa-n

Left some NITs that could addressed later, overall LGTM

* copy comments and asserts from Levis internal code review * apply Levis suggestion: don't store Array.MaxLength as a const, as it may change in the future * add missing and fix some of the existing comments * first bug fix: SerializationRecord.TypeNameMatches should throw ArgumentNullException for null Type argument * second bug fix: SerializationRecord.TypeNameMatches should know the difference between SZArray and single-dimension, non-zero offset arrays (example: int[] and int[*]) * third bug fix: don't cast bytes to booleans * fourth bug fix: don't cast bytes to DateTimes * add one test case that I've forgot in previous PR # Conflicts: # src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/SerializationRecord.cs

* copy comments and asserts from Levis internal code review * apply Levis suggestion: don't store Array.MaxLength as a const, as it may change in the future * add missing and fix some of the existing comments * first bug fix: SerializationRecord.TypeNameMatches should throw ArgumentNullException for null Type argument * second bug fix: SerializationRecord.TypeNameMatches should know the difference between SZArray and single-dimension, non-zero offset arrays (example: int[] and int[*]) * third bug fix: don't cast bytes to booleans * fourth bug fix: don't cast bytes to DateTimes * add one test case that I've forgot in previous PR

* [NRBF] Don't use Unsafe.As when decoding DateTime(s) (#105749) * Add NrbfDecoder Fuzzer (#107385) * [NRBF] Fix bugs discovered by the fuzzer (#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) # Conflicts: # src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/NrbfDecoder.cs * [NRBF] throw SerializationException when a surrogate character is read (#107532) (so far an ArgumentException was thrown) * [NRBF] Fuzzing non-seekable stream input (#107605) * [NRBF] More bug fixes (#107682) - Don't use `Debug.Fail` not followed by an exception (it may cause problems for apps deployed in Debug) - avoid Int32 overflow - throw for unexpected enum values just in case parsing has not rejected them - validate the number of chars read by BinaryReader.ReadChars - pass serialization record id to ex message - return false rather than throw EndOfStreamException when provided Stream has not enough data - don't restore the position in finally - limit max SZ and MD array length to Array.MaxLength, stop using LinkedList<T> as List<T> will be able to hold all elements now - remove internal enum values that were always illegal, but needed to be handled everywhere - Fix DebuggerDisplay * [NRBF] Comments and bug fixes from internal code review (#107735) * copy comments and asserts from Levis internal code review * apply Levis suggestion: don't store Array.MaxLength as a const, as it may change in the future * add missing and fix some of the existing comments * first bug fix: SerializationRecord.TypeNameMatches should throw ArgumentNullException for null Type argument * second bug fix: SerializationRecord.TypeNameMatches should know the difference between SZArray and single-dimension, non-zero offset arrays (example: int[] and int[*]) * third bug fix: don't cast bytes to booleans * fourth bug fix: don't cast bytes to DateTimes * add one test case that I've forgot in previous PR # Conflicts: # src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/SerializationRecord.cs * [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 --------- Co-authored-by: Buyaa Namnan <bunamnan@microsoft.com>

adamsitnik added 8 commits September 12, 2024 13:25

copy comments and asserts from Levis internal code review

f3f87a3

apply Levis suggestion: don't store Array.MaxLength as a const, as it…

8a42d8b

… may change in the future

add missing and fix some of the existing comments

88b9dba

first bug fix: SerializationRecord.TypeNameMatches should throw Argum…

969a2ef

…entNullException for null Type argument

second bug fix: SerializationRecord.TypeNameMatches should know the d…

61a9b14

…ifference between SZArray and single-dimension, non-zero offset arrays (example: int[] and int[*])

third bug fix: don't cast bytes to booleans

fcb3c8a

fourth bug fix: don't cast bytes to DateTimes

88504ef

add one test case that I've forgot in previous PR

581c1fc

adamsitnik added binaryformatter-migration Issues related to the removal of BinaryFormatter and migrations away from it area-System.Formats.Nrbf labels Sep 12, 2024

adamsitnik requested review from GrabYourPitchforks and buyaa-n September 12, 2024 12:27

dotnet-policy-service bot assigned adamsitnik Sep 12, 2024

MihuBot mentioned this pull request Sep 12, 2024

[Fuzzing] [adamsitnik] [NRBF] Comments and bug fixes from security review MihuBot/runtime-utils#656

Open

jkotas reviewed Sep 12, 2024

View reviewed changes

src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/SerializationRecordId.cs Outdated Show resolved Hide resolved

bartonjs reviewed Sep 12, 2024

View reviewed changes

src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/ArraySinglePrimitiveRecord.cs Show resolved Hide resolved

bartonjs reviewed Sep 12, 2024

View reviewed changes

src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/ArraySinglePrimitiveRecord.cs Outdated Show resolved Hide resolved

GrabYourPitchforks reviewed Sep 12, 2024

View reviewed changes

src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/ArraySinglePrimitiveRecord.cs Outdated Show resolved Hide resolved

GrabYourPitchforks reviewed Sep 12, 2024

View reviewed changes

address code review feedback

d68a423

adamsitnik requested review from GrabYourPitchforks and bartonjs September 13, 2024 07:23

MihuBot mentioned this pull request Sep 13, 2024

[Fuzzing] [adamsitnik] [NRBF] Comments and bug fixes from security review MihuBot/runtime-utils#658

Open

build-analysis bot mentioned this pull request Sep 13, 2024

SIGKILL (OOM?) while running LibraryImportGenerator.Tests w/o actionable log messages or artifacts dotnet/dnceng#2496

Open

3 tasks

buyaa-n reviewed Sep 16, 2024

View reviewed changes

buyaa-n approved these changes Sep 16, 2024

View reviewed changes

adamsitnik merged commit ad7b02a into dotnet:main Sep 16, 2024
83 of 85 checks passed

adamsitnik mentioned this pull request Sep 16, 2024

[release/9.0-rc2] NRBF Fuzzer and bug fixes #107788

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[NRBF] Comments and bug fixes from security review #107735

[NRBF] Comments and bug fixes from security review #107735

adamsitnik commented Sep 12, 2024 •

edited

Loading

dotnet-policy-service bot commented Sep 12, 2024

adamsitnik commented Sep 12, 2024

jkotas Sep 12, 2024

buyaa-n Sep 16, 2024

GrabYourPitchforks Sep 12, 2024

buyaa-n Sep 16, 2024

adamsitnik Sep 16, 2024

adamsitnik commented Sep 13, 2024

buyaa-n Sep 16, 2024

buyaa-n Sep 16, 2024

buyaa-n left a comment

	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))

	/// SerializationRecordType enumeration is described in <see href="https://learn.microsoft.com/en-us/openspecs/windows_protocols/ms-nrbf/954a0657-b901-4813-9398-4ec732fe8b32">[MS-NRBF] 2.1.2.1</see>.
	/// SerializationRecordType enumeration is described in <see href="https://learn.microsoft.com/openspecs/windows_protocols/ms-nrbf/954a0657-b901-4813-9398-4ec732fe8b32">[MS-NRBF] 2.1.2.1</see>.

	// The spec (MS NRBF 2.1.1.6, https://learn.microsoft.com/en-us/openspecs/windows_protocols/ms-nrbf/10b218f5-9b2b-4947-b4b7-07725a2c8127)
	// The spec (MS NRBF 2.1.1.6, https://learn.microsoft.com/openspecs/windows_protocols/ms-nrbf/10b218f5-9b2b-4947-b4b7-07725a2c8127)

[NRBF] Comments and bug fixes from security review #107735

[NRBF] Comments and bug fixes from security review #107735

Conversation

adamsitnik commented Sep 12, 2024 • edited Loading

dotnet-policy-service bot commented Sep 12, 2024

adamsitnik commented Sep 12, 2024

jkotas Sep 12, 2024

Choose a reason for hiding this comment

buyaa-n Sep 16, 2024

Choose a reason for hiding this comment

GrabYourPitchforks Sep 12, 2024

Choose a reason for hiding this comment

buyaa-n Sep 16, 2024

Choose a reason for hiding this comment

adamsitnik Sep 16, 2024

Choose a reason for hiding this comment

adamsitnik commented Sep 13, 2024

buyaa-n Sep 16, 2024

Choose a reason for hiding this comment

buyaa-n Sep 16, 2024

Choose a reason for hiding this comment

buyaa-n left a comment

Choose a reason for hiding this comment

adamsitnik commented Sep 12, 2024 •

edited

Loading