Skip to content
Prev Previous commit
Next Next commit
bug #3: throw SerializationException rather than FormatException when…
… it's being thrown by BinaryReader (or sth else that we use)
  • Loading branch information
adamsitnik committed Sep 4, 2024
commit bc3f71555d96556806a084f4e898b2a770465fd8
3 changes: 3 additions & 0 deletions src/libraries/System.Formats.Nrbf/src/Resources/Strings.resx
Original file line number Diff line number Diff line change
Expand Up @@ -162,4 +162,7 @@
<data name="Serialization_InvalidAssemblyName" xml:space="preserve">
<value>Invalid assembly name: `{0}`.</value>
</data>
<data name="Serialization_InvalidFormat" xml:space="preserve">
<value>Invalid format.</value>
</data>
</root>
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,14 @@ public static SerializationRecord Decode(Stream payload, out IReadOnlyDictionary
#endif

using BinaryReader reader = new(payload, ThrowOnInvalidUtf8Encoding, leaveOpen: leaveOpen);
return Decode(reader, options ?? new(), out recordMap);
try
{
return Decode(reader, options ?? new(), out recordMap);
}
catch (FormatException) // can be thrown by various BinaryReader methods
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pass to inner exception?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't do that on purpose, to avoid leaking any information from the inner exception that could be attacker controlled (like some weird string that could somehow affect the log file).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

{
throw new SerializationException(SR.Serialization_InvalidFormat);
}
}

/// <summary>
Expand Down
21 changes: 21 additions & 0 deletions src/libraries/System.Formats.Nrbf/tests/InvalidInputTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -516,4 +516,25 @@ public void MissingRootRecord()

Assert.Throws<SerializationException>(() => NrbfDecoder.Decode(stream));
}

[Fact]
public void Invalid7BitEncodedStringLength()
{
// The highest bit of the last byte is set (so it's invalid).
byte[] invalidLength = [byte.MaxValue, byte.MaxValue, byte.MaxValue, byte.MaxValue, byte.MaxValue];

using MemoryStream stream = new();
BinaryWriter writer = new(stream, Encoding.UTF8);

WriteSerializedStreamHeader(writer);
writer.Write((byte)SerializationRecordType.BinaryObjectString);
writer.Write(1); // root record Id
writer.Write(invalidLength); // the length prefix
writer.Write(Encoding.UTF8.GetBytes("theString"));
writer.Write((byte)SerializationRecordType.MessageEnd);

stream.Position = 0;

Assert.Throws<SerializationException>(() => NrbfDecoder.Decode(stream));
}
}