[NRBF] Address issues discovered by Threat Model by adamsitnik · Pull Request #106629 · dotnet/runtime

adamsitnik · 2024-08-19T13:29:40Z

…es, as it's most likely corrupted/tampered/malicious data and could be used as a vector of attack.

ghost · 2024-08-19T13:29:46Z

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

ghost · 2024-08-19T13:29:47Z

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

adamsitnik · 2024-08-19T13:30:37Z

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

Based on what we have discussed and what @bartonjs wrote here: #103713 (comment)

I believe the type names and assembly names should not be provided in the exception messages.

adamsitnik · 2024-08-19T13:32:58Z

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

+    private static long GetJaggedArrayTotalElementsCount(BinaryArrayRecord jaggedArrayRecord)
+    {
+        long result = 0;
+        Queue<BinaryArrayRecord>? jaggedArrayRecords = null;


The decoder does not perform any recursion when decoding the jagged array. So does this method, but at a cost of potential Queue allocation.

…rray without being marked as jagged

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

- rename TotalElementsCount to FlattenedLength - Ensure lack of recursive call - add a comment explaining null nuances

- don't include member name in the exception message - remove unused resource - make the argument int rather than an object to make it clear it's never a string

adamsitnik · 2024-08-20T17:57:30Z

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

  <data name="Serialization_UnexpectedNullRecordCount" xml:space="preserve">
    <value>Unexpected Null Record count.</value>
  </data>
-  <data name="Serialization_MaxArrayLength" xml:space="preserve">


this resource was not being used for a while

adamsitnik · 2024-08-20T17:58:43Z

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

+        // It is possible to have binary array records have an element type of array without being marked as jagged.
+        || TypeName.GetElementType().IsArray;


@JeremyKuhne this handles the scenario you have mentioned offline (there is also a test for that). Thank you again for pointing this out!

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

bartonjs · 2024-09-04T16:53:15Z

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

+                    case SerializationRecordType.ObjectNullMultiple:
+                        // All nulls need to be included, as it's another form of possible attack.
+                        checked
+                        {
+                            result += ((NullsRecord)item).NullCount;
+                        }
+                        break;
+                    default:
+                        result++;
+                        break;
+                }


Don't the arrays already know their lengths? I'd expect FlattenedLength to be a very simple

long length = here.Length; foreach (element in here.Children) { if (element.IsArray) { length += element.FlattenedLength; } } return length;

(or the queue variant)

And that approach would already have counted the null elements. Ensuring that the number of records matches the length is a different problem (one I feel GetArray tries to solve?).

@bartonjs You are right. This suggestion has greatly simplified the code and solved the other problem (arrays not being included themselves)

bartonjs · 2024-09-04T16:57:38Z

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

+                    case SerializationRecordType.ArraySingleString:
+                        ArrayRecord nestedArrayRecord = (ArrayRecord)record;
+                        if (nestedArrayRecord.IsJagged)
+                        {
+                            (jaggedArrayRecords ??= new()).Enqueue((BinaryArrayRecord)nestedArrayRecord);
+                        }
+                        else
+                        {
+                            Debug.Assert(nestedArrayRecord is not BinaryArrayRecord, "Ensure lack of recursive call");
+                            checked
+                            {
+                                // In theory somebody could create a payload that would represent
+                                // a very nested array with total elements count > long.MaxValue.
+                                result += nestedArrayRecord.FlattenedLength;
+                            }
+                        }
+                        break;


This isn't counting the arrays themselves. Assuming that's intentional, you should have a comment at the top of the method (or before this switch) that explains what does, and what doesn't count.

object[][] objs = new object[1_000_000][]; objs.Fill(Array.Empty<object>());

Doesn't feel like it should have a smaller count than

object[][] objs = new object[1_000_000][]; objs.Fill(null);

And right now it feels like the first one says 0, and the second says a million.

FWIW, I can understand not counting the arrays as answering how many int values might come out of an int[][][]; but then it gets weird when the nulls get counted, because they're not int values.

So I guess there's a bit of "what is this number supposed to mean?". The comment can explain that, and then the code can be judged against the comment.

Good point, I've fixed that.

bartonjs · 2024-09-04T16:59:44Z

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

+                        }
+                        else
+                        {
+                            Debug.Assert(nestedArrayRecord is not BinaryArrayRecord, "Ensure lack of recursive call");


I can read the assert, but I don't understand it. Why can't a BinaryArrayRecord be nested within another BinaryArrayRecord? Why would that necessarily mean that a recursive call happened?

I've eliminated the potential recursive call and added a comment.

# Conflicts: # src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/RectangularArrayRecord.cs

adamsitnik · 2024-09-13T11:19:10Z

@MihuBot fuzz NrbfDecoder

buyaa-n

LGTM and looks @bartonjs's feedbacks addressed as needed

# Conflicts: # src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/RectangularArrayRecord.cs

adamsitnik · 2024-09-16T10:34:16Z

/ba-g DeadLetter is unrelated

* 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

* [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>

* 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 added 5 commits August 19, 2024 14:52

introduce ArrayRecord.TotalElementsCount

d5addcb

remove recursion

38fb5f8

don't allocate the Stack unless it's necessary

f89177c

use Queue (both are fine as the order does not really matter here)

77088c2

do not include invalid Type or Assembly names in the exception messag…

13306fc

…es, as it's most likely corrupted/tampered/malicious data and could be used as a vector of attack.

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

adamsitnik self-assigned this Aug 19, 2024

ghost added the new-api-needs-documentation label Aug 19, 2024

adamsitnik commented Aug 19, 2024

View reviewed changes

It is possible to have binary array records have an element type of a…

297aa4b

…rray without being marked as jagged

adamsitnik mentioned this pull request Aug 19, 2024

[API Proposal]: ArrayRecord.TotalElementsCount #106644

Closed

adamsitnik changed the title ~~[NRB] Address issues discovered by Threat Model~~ [NRBF] Address issues discovered by Threat Model Aug 19, 2024

GrabYourPitchforks reviewed Aug 19, 2024

View reviewed changes

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

GrabYourPitchforks reviewed Aug 19, 2024

View reviewed changes

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

This was referenced Aug 19, 2024

Test failure "RemoteExecutionException : Half-way through waiting for remote process." in System.Threading.ThreadPools.Tests.ThreadPoolTests.IOCompletionPortCountConfigVarTest #106494

Closed

'chrome-GetPropertiesTests' timing out #106625

Closed

adamsitnik added 2 commits August 20, 2024 19:48

address feedback:

ea50be4

- rename TotalElementsCount to FlattenedLength - Ensure lack of recursive call - add a comment explaining null nuances

improve exception messages:

e740b0a

- don't include member name in the exception message - remove unused resource - make the argument int rather than an object to make it clear it's never a string

adamsitnik commented Aug 20, 2024

View reviewed changes

adamsitnik marked this pull request as ready for review August 20, 2024 18:00

adamsitnik requested review from GrabYourPitchforks and JeremyKuhne August 20, 2024 18:01

adamsitnik commented Aug 20, 2024

View reviewed changes

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

add checked to ensure no long overflow for very unusual payloads

140bf92

adamsitnik requested a review from bartonjs August 21, 2024 08:19

build-analysis bot mentioned this pull request Aug 21, 2024

[browser] HalfTests.ExplicitConversion_FromSingle failing due to NaN != NaN #103347

Open

FlattenedLength must include nulls

16394e2

build-analysis bot mentioned this pull request Aug 28, 2024

restarted. Azure DevOps can't recover from restarts. dotnet/dnceng#3879

Closed

3 tasks

bartonjs reviewed Sep 4, 2024

View reviewed changes

adamsitnik added 2 commits September 13, 2024 12:08

Merge remote-tracking branch 'upstream/main' into totalElementsCount

a5a38fb

# Conflicts: # src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/RectangularArrayRecord.cs

address code review feedback: count the arrays themselves

8a61178

adamsitnik force-pushed the totalElementsCount branch from 94f6c04 to 8a61178 Compare September 13, 2024 11:17

MihuBot mentioned this pull request Sep 13, 2024

[Fuzzing] [adamsitnik] [NRBF] Address issues discovered by Threat Model MihuBot/runtime-utils#659

Open

adamsitnik requested a review from bartonjs September 13, 2024 11:23

buyaa-n approved these changes Sep 16, 2024

View reviewed changes

Merge remote-tracking branch 'upstream/main' into totalElementsCount

2556647

# Conflicts: # src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/RectangularArrayRecord.cs

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

Error reported in diagnostic logs. Please examine the log for more details dotnet/dnceng#1928

Open

3 tasks

adamsitnik mentioned this pull request Sep 16, 2024

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

Merged

4 tasks

adamsitnik merged commit 0154a2f into dotnet:main Sep 16, 2024

github-actions bot locked and limited conversation to collaborators Oct 17, 2024

		// It is possible to have binary array records have an element type of array without being marked as jagged.
		\|\| TypeName.GetElementType().IsArray;

Conversation

adamsitnik commented Aug 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Aug 19, 2024

Uh oh!

ghost commented Aug 19, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bartonjs Sep 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bartonjs Sep 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adamsitnik commented Sep 13, 2024

Uh oh!

buyaa-n left a comment

Choose a reason for hiding this comment

Uh oh!

adamsitnik commented Sep 16, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

adamsitnik commented Aug 19, 2024 •

edited

Loading

bartonjs Sep 4, 2024 •

edited

Loading

bartonjs Sep 4, 2024 •

edited

Loading