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

adamsitnik · 2024-09-13T11:46:51Z

This PR combines the PRs that added Fuzzing for NrbfDecoder with the most recent bug fixes.

It contains of:

[NRBF] Don't use Unsafe.As when decoding DateTime(s) #105749 where we have stopped using unsafe cast from ulong to DateTime and are now compliant
Add NrbfDecoderFuzzer #107385 where we have added an excellent Fuzzer that has discovered plenty of edge case bugs
[NRBF] Fix bugs discovered by the fuzzer #107368 where we have fixed some of the bugs discovered by the Fuzzer, mainly around enum parsing and throwing documented exceptions
[NRBF] throw SerializationException when a surrogate character is read #107532 where we have started throwing SerializationException rather than ArgumentException for surrogate characters
[NRBF] Fuzzing non-seekable stream input #107605 where we have extended the Fuzzer to cover non-seekable Stream code path
[NRBF] More bug fixes #107682 where we have removed int32 overflow, stopped using Debug.Fail and limited the max array length to Array.MaxLength (so far we were doing that only for single dimension arrays, now we do it for multi dimensional arrays as well)
[NRBF] Comments and bug fixes from security review #107735 where we have stopped doing unsafe tricks to read arrays of boolean and DateTime, added null handling to SerializationRecord.TypeNameMatches copied a lot of valuable comments from the internal code review
[NRBF] Address issues discovered by Threat Model #106629 where we have addressed issues discovered by Threat Model by exposing a new API that allows the users to check the total number of elements of any array record, including potentially hostile jagged arrays.

Customer Impact

Customer reported
Found internally

Half of the bugs were discovered by the Fuzzer, the other half were reported internally by @GrabYourPitchforks.

Regression

Yes
No

[If yes, specify when the regression was introduced. Provide the PR or commit if known.]

Testing

All bugs have been turned into unit tests (and of course are passing now).

Risk

The bug fixes present in this PR were relatively simple and each of them individually represents a low risk. But the fact that there are so many increases the risk. Because of this risk, we decided to Mark the System.Formats.Nrbf assembly as [Experimental] with SYSLIB5005 (#107905).

* 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

dotnet#107532) (so far an ArgumentException was thrown)

- 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

* 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

* 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

adamsitnik · 2024-09-16T13:08:34Z

@MihuBot fuzz NrbfDecoder

bartonjs · 2024-09-16T22:53:41Z

...ibraries/System.Formats.Nrbf/src/System/Formats/Nrbf/SystemClassWithMembersAndTypesRecord.cs

@@ -75,29 +75,30 @@ ulong value when TypeNameMatches(typeof(UIntPtr)) => Create(new UIntPtr(value)),
                    _ => this
                };
            }
-            else if (HasMember("_ticks") && MemberValues[0] is long ticks && TypeNameMatches(typeof(TimeSpan)))
+            else if (HasMember("_ticks") && GetRawValue("_ticks") is long ticks && TypeNameMatches(typeof(TimeSpan)))


This is double-dipping the dictionary. Ideally it would be enhanced to use a TryGetValue approach.

That doubles the perf cost of this, and "double" isn't a threat, so it's not critical to fix, but would maybe make sense to do for vNext/main.

I was going to comment that the HasMember call was unnecessary, but it looks like GetMember (called by GetRawValue) will throw for an unknown name.

carlossanlop · 2024-09-16T23:04:32Z

@adamsitnik this won't make it before the RC2, which I will do in a few minutes.

Please retarget this PR to the release/9.0-rc2 branch and send an email to Tactics requesting approval.

bartonjs · 2024-09-16T23:58:02Z

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

+        Assert.True(a);
+        bool b = bools[1];
+        Assert.True(b);
+        bool c = a && b;


I believe you need to use single-ampersand to test what you were trying to test (reliably)

Sharplab shows that sometimes the compiler writes down the IL instruction "and" (which turns into the x86 "and") for both operations, and sometimes it uses brfalse for "&&".

The only time I've seen it use brfalse involved spans, e.g. this example on sharplab...

but & always ends up as IL:and.

Probably doesn't super matter, as this probably uses IL:and.

After finding some boundary examples, I believe that Roslyn turns expr1 && expr2 into and if expr2 is a literal-, local-, or parameter- expression, and brfalse otherwise. (In my linked example it used brfalse because the RHS was an indexer-expression).

If I'm right, then your && here will use the IL and instruction, hence the CPU and instruction, so it is a valid test.

But, wow, that took a long time to "prove".

bartonjs · 2024-09-17T00:10:01Z

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

+            writer.Write((byte)PrimitiveType.Char);
+        }
+
+        writer.Write((byte)0xC0); // a surrogate character


Are there also tests that show that a valid surrogate pair work? Or does BinaryFormatter not support characters outside the BMP?

bartonjs · 2024-09-17T00:15:47Z

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

+        {
+            result = reader.ReadChars(count);
+        }
+        catch (ArgumentException) // A surrogate character was read.


For a single character I can maybe see it throwing for a surrogate half; but I would expect a multiple character read to work for correctly paired surrogates.

Is the comment wrong, and it meant "an unpaired surrogate character was read" (emphasis on "unpaired") or does whatever calls this not support characters outside the BMP? (question is slightly repeated in the tests)

bartonjs · 2024-09-17T00:38:18Z

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

+
+        SerializationRecord record = NrbfDecoder.Decode(Serialize(input));
+
+        Assert.Throws<ArgumentNullException>(() => record.TypeNameMatches(type: null));


Suggested change

Assert.Throws<ArgumentNullException>(() => record.TypeNameMatches(type: null));

AssertExtensions.Throws<ArgumentNullException>("type", () => record.TypeNameMatches(type: null));

Is a better test, as it ensures that the parameter name is correct.

bartonjs · 2024-09-17T00:39:53Z

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

            throw new SerializationException(ex.Message, ex);
        }

-        return Unsafe.As<long, DateTime>(ref data);
+        [MethodImpl(MethodImplOptions.NoInlining)]
+        static DateTime CreateFromAmbiguousDst(ulong ticks)


I don't see new tests for this code. Hopefully there are existing ones.

bartonjs

I don't see anything that would preclude backporting; but there are a few "room for improvement" things for main/vNext.

jeffhandley · 2024-09-17T18:58:12Z

We need to retarget this PR to the release/9.0-rc2 branch. Before we close this PR and replace it with one against the correct target (where we'd break the association with the review comments), we're going to try retargeting it to see if it will cleanly retarget without pulling undesired commits in with it.

dotnet-policy-service · 2024-09-18T16:28:55Z

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

adamsitnik and others added 6 commits September 13, 2024 13:31

[NRBF] Don't use Unsafe.As when decoding DateTime(s) (dotnet#105749)

2d93a7d

Add NrbfDecoder Fuzzer (dotnet#107385)

af1d673

[NRBF] throw SerializationException when a surrogate character is read (

02e8bb0

dotnet#107532) (so far an ArgumentException was thrown)

[NRBF] Fuzzing non-seekable stream input (dotnet#107605)

198c3b6

dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Sep 13, 2024

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

adamsitnik added area-System.Formats.Nrbf and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Sep 13, 2024

adamsitnik changed the title ~~[release/9.0] Backport NRBF fixes and Fuzzer~~ [release/9.0] NRBF Fuzzer and bug fixes Sep 16, 2024

adamsitnik marked this pull request as ready for review September 16, 2024 13:08

MihuBot mentioned this pull request Sep 16, 2024

[Fuzzing] [adamsitnik] [release/9.0] NRBF Fuzzer and bug fixes MihuBot/runtime-utils#667

Open

adamsitnik requested review from jeffhandley, GrabYourPitchforks, bartonjs and buyaa-n September 16, 2024 16:27

adamsitnik added this to the 9.0.0 milestone Sep 16, 2024

buyaa-n approved these changes Sep 16, 2024

View reviewed changes

bartonjs reviewed Sep 16, 2024

View reviewed changes

bartonjs reviewed Sep 17, 2024

View reviewed changes

bartonjs approved these changes Sep 17, 2024

View reviewed changes

adamsitnik changed the base branch from release/9.0 to release/9.0-rc2 September 17, 2024 18:58

jeffhandley changed the title ~~[release/9.0] NRBF Fuzzer and bug fixes~~ [release/9.0-rc2] NRBF Fuzzer and bug fixes Sep 17, 2024

jeffhandley added the Servicing-consider Issue for next servicing release review label Sep 17, 2024

jeffhandley approved these changes Sep 17, 2024

View reviewed changes

jeffhandley added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Sep 17, 2024

carlossanlop merged commit fde8a3b into dotnet:release/9.0-rc2 Sep 17, 2024
86 of 93 checks passed

jeffhandley added the binaryformatter-migration Issues related to the removal of BinaryFormatter and migrations away from it label Sep 18, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

adamsitnik commented Sep 13, 2024 •

edited by jeffhandley

Loading

adamsitnik commented Sep 16, 2024

bartonjs Sep 16, 2024

carlossanlop commented Sep 16, 2024

bartonjs Sep 16, 2024

bartonjs Sep 17, 2024

bartonjs Sep 17, 2024

bartonjs Sep 17, 2024

bartonjs Sep 17, 2024

bartonjs Sep 17, 2024

bartonjs left a comment

jeffhandley commented Sep 17, 2024

dotnet-policy-service bot commented Sep 18, 2024


		SerializationRecord record = NrbfDecoder.Decode(Serialize(input));

		Assert.Throws<ArgumentNullException>(() => record.TypeNameMatches(type: null));

	Assert.Throws<ArgumentNullException>(() => record.TypeNameMatches(type: null));
	AssertExtensions.Throws<ArgumentNullException>("type", () => record.TypeNameMatches(type: null));

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

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

Conversation

adamsitnik commented Sep 13, 2024 • edited by jeffhandley Loading

Customer Impact

Regression

Testing

Risk

adamsitnik commented Sep 16, 2024

bartonjs Sep 16, 2024

Choose a reason for hiding this comment

carlossanlop commented Sep 16, 2024

bartonjs Sep 16, 2024

Choose a reason for hiding this comment

bartonjs Sep 17, 2024

Choose a reason for hiding this comment

bartonjs Sep 17, 2024

Choose a reason for hiding this comment

bartonjs Sep 17, 2024

Choose a reason for hiding this comment

bartonjs Sep 17, 2024

Choose a reason for hiding this comment

bartonjs Sep 17, 2024

Choose a reason for hiding this comment

bartonjs left a comment

Choose a reason for hiding this comment

jeffhandley commented Sep 17, 2024

dotnet-policy-service bot commented Sep 18, 2024

adamsitnik commented Sep 13, 2024 •

edited by jeffhandley

Loading