BinaryFormatter.Deserialize throws many unexpected exceptions

[Metalnem]

The documentation says that the BinaryFormatter.Deserialize method may throw SerializationException or SecurityException, but it can actually throw many more. Here are some of them:

• ArgumentException
• ArgumentOutOfRangeException
• DecoderFallbackException
• FileLoadException
• FormatException
• IndexOutOfRangeException
• IOException
• MemberAccessException
• NullReferenceException
• OverflowException

To reproduce all of these, just run the project from the attached archive.

My environment:

.NET Core SDK (reflecting any global.json):
 Version:   2.2.104
 Commit:    73f036d4ac

Runtime Environment:
 OS Name:     Mac OS X
 OS Version:  10.14
 OS Platform: Darwin
 RID:         osx.10.14-x64
 Base Path:   /usr/local/share/dotnet/sdk/2.2.104/

Found via SharpFuzz.

[danmoseley]

Deserialize runs arbitrary user code - if it doesn't wrap any exceptions from that, then it could throw any exception type.

[joshfree]

@Metalnem do you mind posting the repro code as a gist?

[Metalnem]

@joshfree:

Here's the gist: Program.cs (the important thing when running this is to name the project/assembly as CoreFX.Fuzz, because the assembly name is part of the serialized data).

@danmosemsft:

This is the definition of the class I was using:

[Serializable]
public class Obj
{
  public int A = 0;
  public double B = 0;
  public DateTime C = DateTime.MinValue;
  public bool D = false;
  public List<int> E = null;
  public string[] F = null;
}

As you can see, it's just a plain class with some fields. I would expect that deserializing malformed input stream would result only in SerializationException in this case.

[danmoseley]

Pasted code below

    System.ArgumentException: Object of type 'System.Char' cannot be converted to type 'System.Boolean'.
   at System.RuntimeType.TryChangeType(Object value, Binder binder, CultureInfo culture, Boolean needsSpecialCast)
   at System.Reflection.RtFieldInfo.InternalSetValue(Object obj, Object value, BindingFlags invokeAttr, Binder binder, CultureInfo culture, StackCrawlMark& stackMark)
   at System.Reflection.RtFieldInfo.SetValue(Object obj, Object value, BindingFlags invokeAttr, Binder binder, CultureInfo culture)
   at System.Reflection.FieldInfo.SetValue(Object obj, Object value)
   at System.Runtime.Serialization.FormatterServices.PopulateObjectMembers(Object obj, MemberInfo[] members, Object[] data)
   at System.Runtime.Serialization.Formatters.Binary.ObjectReader.ParseObjectEnd(ParseRecord pr)
   at System.Runtime.Serialization.Formatters.Binary.ObjectReader.Parse(ParseRecord pr)
   at System.Runtime.Serialization.Formatters.Binary.BinaryParser.Run()
   at System.Runtime.Serialization.Formatters.Binary.ObjectReader.Deserialize(BinaryParser serParser, Boolean fCheck)
   at System.Runtime.Serialization.Formatters.Binary.BinaryFormatter.Deserialize(Stream serializationStream, Boolean check)
   at System.Runtime.Serialization.Formatters.Binary.BinaryFormatter.Deserialize(Stream serializationStream)
   at CoreFX.Fuzz.Program.Main(String[] args) in c:\d\fuzz1\Program.cs:line 31
System.ArgumentOutOfRangeException: Object IDs must be greater than zero.
Parameter name: objectToBeFixed
   at System.Runtime.Serialization.ObjectManager.RecordFixup(Int64 objectToBeFixed, MemberInfo member, Int64 objectRequired)
   at System.Runtime.Serialization.Formatters.Binary.ReadObjectInfo.RecordFixup(Int64 objectId, String name, Int64 idRef)
   at System.Runtime.Serialization.Formatters.Binary.ObjectReader.ParseMember(ParseRecord pr)
   at System.Runtime.Serialization.Formatters.Binary.ObjectReader.Parse(ParseRecord pr)
   at System.Runtime.Serialization.Formatters.Binary.BinaryParser.ReadMemberReference()
   at System.Runtime.Serialization.Formatters.Binary.BinaryParser.Run()
   at System.Runtime.Serialization.Formatters.Binary.ObjectReader.Deserialize(BinaryParser serParser, Boolean fCheck)
   at System.Runtime.Serialization.Formatters.Binary.BinaryFormatter.Deserialize(Stream serializationStream, Boolean check)
   at System.Runtime.Serialization.Formatters.Binary.BinaryFormatter.Deserialize(Stream serializationStream)
   at CoreFX.Fuzz.Program.Main(String[] args) in c:\d\fuzz1\Program.cs:line 31
System.Text.DecoderFallbackException: Unable to translate bytes [A0] at index 4 from specified code page to Unicode.
   at System.Text.DecoderExceptionFallbackBuffer.Throw(Byte[] bytesUnknown, Int32 index)
   at System.Text.DecoderExceptionFallbackBuffer.Fallback(Byte[] bytesUnknown, Int32 index)
   at System.Text.DecoderFallbackBuffer.InternalFallback(Byte[] bytes, Byte* pBytes, Char*& chars)
   at System.Text.UTF8Encoding.GetChars(Byte* bytes, Int32 byteCount, Char* chars, Int32 charCount, DecoderNLS baseDecoder)
   at System.Text.DecoderNLS.GetChars(Byte[] bytes, Int32 byteIndex, Int32 byteCount, Char[] chars, Int32 charIndex, Boolean flush)
   at System.Text.DecoderNLS.GetChars(Byte[] bytes, Int32 byteIndex, Int32 byteCount, Char[] chars, Int32 charIndex)
   at System.IO.BinaryReader.ReadString()
   at System.Runtime.Serialization.Formatters.Binary.BinaryObjectWithMapTyped.Read(BinaryParser input)
   at System.Runtime.Serialization.Formatters.Binary.BinaryParser.ReadObjectWithMapTyped(BinaryHeaderEnum binaryHeaderEnum)
   at System.Runtime.Serialization.Formatters.Binary.BinaryParser.Run()
   at System.Runtime.Serialization.Formatters.Binary.ObjectReader.Deserialize(BinaryParser serParser, Boolean fCheck)
   at System.Runtime.Serialization.Formatters.Binary.BinaryFormatter.Deserialize(Stream serializationStream, Boolean check)
   at System.Runtime.Serialization.Formatters.Binary.BinaryFormatter.Deserialize(Stream serializationStream)
   at CoreFX.Fuzz.Program.Main(String[] args) in c:\d\fuzz1\Program.cs:line 31
System.IO.FileLoadException: The given assembly name or codebase was invalid. (Exception from HRESULT: 0x80131047)
   at System.Reflection.AssemblyName.nInit(RuntimeAssembly& assembly, Boolean raiseResolveEvent)
   at System.Reflection.AssemblyName..ctor(String assemblyName)
   at System.TypeNameParser.ResolveAssembly(String asmName, Func`2 assemblyResolver, Boolean throwOnError, StackCrawlMark& stackMark)
   at System.TypeNameParser.ConstructType(Func`2 assemblyResolver, Func`4 typeResolver, Boolean throwOnError, Boolean ignoreCase, StackCrawlMark& stackMark)
   at System.TypeNameParser.ConstructType(Func`2 assemblyResolver, Func`4 typeResolver, Boolean throwOnError, Boolean ignoreCase, StackCrawlMark& stackMark)
   at System.TypeNameParser.GetType(String typeName, Func`2 assemblyResolver, Func`4 typeResolver, Boolean throwOnError, Boolean ignoreCase, StackCrawlMark& stackMark)
   at System.Type.GetType(String typeName, Func`2 assemblyResolver, Func`4 typeResolver, Boolean throwOnError)
   at System.Runtime.Serialization.Formatters.Binary.ObjectReader.GetSimplyNamedTypeFromAssembly(Assembly assm, String typeName, Type& type)
   at System.Runtime.Serialization.Formatters.Binary.ObjectReader.FastBindToType(String assemblyName, String typeName)
   at System.Runtime.Serialization.Formatters.Binary.ObjectReader.Bind(String assemblyString, String typeString)
   at System.Runtime.Serialization.Formatters.Binary.ObjectReader.GetType(BinaryAssemblyInfo assemblyInfo, String name)
   at System.Runtime.Serialization.Formatters.Binary.ObjectMap..ctor(String objectName, String[] memberNames, BinaryTypeEnum[] binaryTypeEnumA, Object[] typeInformationA, Int32[] memberAssemIds, ObjectReader objectReader, Int32 objectId, BinaryAssemblyInfo assemblyInfo, SizedArray assemIdToAssemblyTable)
   at System.Runtime.Serialization.Formatters.Binary.BinaryParser.ReadObjectWithMapTyped(BinaryObjectWithMapTyped record)
   at System.Runtime.Serialization.Formatters.Binary.BinaryParser.ReadObjectWithMapTyped(BinaryHeaderEnum binaryHeaderEnum)
   at System.Runtime.Serialization.Formatters.Binary.BinaryParser.Run()
   at System.Runtime.Serialization.Formatters.Binary.ObjectReader.Deserialize(BinaryParser serParser, Boolean fCheck)
   at System.Runtime.Serialization.Formatters.Binary.BinaryFormatter.Deserialize(Stream serializationStream, Boolean check)
   at System.Runtime.Serialization.Formatters.Binary.BinaryFormatter.Deserialize(Stream serializationStream)
   at CoreFX.Fuzz.Program.Main(String[] args) in c:\d\fuzz1\Program.cs:line 31
System.FormatException: Input string was not in a correct format.
   at System.Number.StringToNumber(ReadOnlySpan`1 str, NumberStyles options, NumberBuffer& number, NumberFormatInfo info, Boolean parseDecimal)
   at System.Number.ParseDecimal(ReadOnlySpan`1 value, NumberStyles options, NumberFormatInfo numfmt)
   at System.Decimal.Parse(String s, IFormatProvider provider)
   at System.Runtime.Serialization.Formatters.Binary.BinaryParser.ReadDecimal()
   at System.Runtime.Serialization.Formatters.Binary.BinaryParser.ReadValue(InternalPrimitiveTypeE code)
   at System.Runtime.Serialization.Formatters.Binary.BinaryParser.ReadMemberPrimitiveUnTyped()
   at System.Runtime.Serialization.Formatters.Binary.BinaryParser.Run()
   at System.Runtime.Serialization.Formatters.Binary.ObjectReader.Deserialize(BinaryParser serParser, Boolean fCheck)
   at System.Runtime.Serialization.Formatters.Binary.BinaryFormatter.Deserialize(Stream serializationStream, Boolean check)
   at System.Runtime.Serialization.Formatters.Binary.BinaryFormatter.Deserialize(Stream serializationStream)
   at CoreFX.Fuzz.Program.Main(String[] args) in c:\d\fuzz1\Program.cs:line 31
System.IndexOutOfRangeException: Index was outside the bounds of the array.
   at System.Runtime.Serialization.Formatters.Binary.SizedArray.set_Item(Int32 index, Object value)
   at System.Runtime.Serialization.Formatters.Binary.BinaryParser.ReadObjectWithMapTyped(BinaryObjectWithMapTyped record)
   at System.Runtime.Serialization.Formatters.Binary.BinaryParser.ReadObjectWithMapTyped(BinaryHeaderEnum binaryHeaderEnum)
   at System.Runtime.Serialization.Formatters.Binary.BinaryParser.Run()
   at System.Runtime.Serialization.Formatters.Binary.ObjectReader.Deserialize(BinaryParser serParser, Boolean fCheck)
   at System.Runtime.Serialization.Formatters.Binary.BinaryFormatter.Deserialize(Stream serializationStream, Boolean check)
   at System.Runtime.Serialization.Formatters.Binary.BinaryFormatter.Deserialize(Stream serializationStream)
   at CoreFX.Fuzz.Program.Main(String[] args) in c:\d\fuzz1\Program.cs:line 31
System.IO.IOException: BinaryReader encountered an invalid string length of -173107588 characters.
   at System.IO.BinaryReader.ReadString()
   at System.Runtime.Serialization.Formatters.Binary.BinaryParser.ReadObjectString(BinaryHeaderEnum binaryHeaderEnum)
   at System.Runtime.Serialization.Formatters.Binary.BinaryParser.Run()
   at System.Runtime.Serialization.Formatters.Binary.ObjectReader.Deserialize(BinaryParser serParser, Boolean fCheck)
   at System.Runtime.Serialization.Formatters.Binary.BinaryFormatter.Deserialize(Stream serializationStream, Boolean check)
   at System.Runtime.Serialization.Formatters.Binary.BinaryFormatter.Deserialize(Stream serializationStream)
   at CoreFX.Fuzz.Program.Main(String[] args) in c:\d\fuzz1\Program.cs:line 31
System.MemberAccessException: Cannot create a type for which Type.ContainsGenericParameters is true.
   at System.Runtime.Serialization.FormatterServices.nativeGetUninitializedObject(RuntimeType type)
   at System.Runtime.Serialization.Formatters.Binary.ObjectReader.ParseObject(ParseRecord pr)
   at System.Runtime.Serialization.Formatters.Binary.ObjectReader.Parse(ParseRecord pr)
   at System.Runtime.Serialization.Formatters.Binary.BinaryParser.ReadObjectWithMapTyped(BinaryHeaderEnum binaryHeaderEnum)
   at System.Runtime.Serialization.Formatters.Binary.BinaryParser.Run()
   at System.Runtime.Serialization.Formatters.Binary.ObjectReader.Deserialize(BinaryParser serParser, Boolean fCheck)
   at System.Runtime.Serialization.Formatters.Binary.BinaryFormatter.Deserialize(Stream serializationStream, Boolean check)
   at System.Runtime.Serialization.Formatters.Binary.BinaryFormatter.Deserialize(Stream serializationStream)
   at CoreFX.Fuzz.Program.Main(String[] args) in c:\d\fuzz1\Program.cs:line 31
System.NullReferenceException: Object reference not set to an instance of an object.
   at System.Runtime.Serialization.Formatters.Binary.ReadObjectInfo.RecordFixup(Int64 objectId, String name, Int64 idRef)
   at System.Runtime.Serialization.Formatters.Binary.ObjectReader.ParseMember(ParseRecord pr)
   at System.Runtime.Serialization.Formatters.Binary.ObjectReader.Parse(ParseRecord pr)
   at System.Runtime.Serialization.Formatters.Binary.BinaryParser.ReadMemberReference()
   at System.Runtime.Serialization.Formatters.Binary.BinaryParser.Run()
   at System.Runtime.Serialization.Formatters.Binary.ObjectReader.Deserialize(BinaryParser serParser, Boolean fCheck)
   at System.Runtime.Serialization.Formatters.Binary.BinaryFormatter.Deserialize(Stream serializationStream, Boolean check)
   at System.Runtime.Serialization.Formatters.Binary.BinaryFormatter.Deserialize(Stream serializationStream)
   at CoreFX.Fuzz.Program.Main(String[] args) in c:\d\fuzz1\Program.cs:line 31
System.OutOfMemoryException: Array dimensions exceeded supported range.
   at System.Runtime.Serialization.Formatters.Binary.ObjectReader.ParseArray(ParseRecord pr)
   at System.Runtime.Serialization.Formatters.Binary.ObjectReader.ParseObject(ParseRecord pr)
   at System.Runtime.Serialization.Formatters.Binary.ObjectReader.Parse(ParseRecord pr)
   at System.Runtime.Serialization.Formatters.Binary.BinaryParser.ReadArray(BinaryHeaderEnum binaryHeaderEnum)
   at System.Runtime.Serialization.Formatters.Binary.BinaryParser.Run()
   at System.Runtime.Serialization.Formatters.Binary.ObjectReader.Deserialize(BinaryParser serParser, Boolean fCheck)
   at System.Runtime.Serialization.Formatters.Binary.BinaryFormatter.Deserialize(Stream serializationStream, Boolean check)
   at System.Runtime.Serialization.Formatters.Binary.BinaryFormatter.Deserialize(Stream serializationStream)
   at CoreFX.Fuzz.Program.Main(String[] args) in c:\d\fuzz1\Program.cs:line 31
System.OverflowException: Array dimensions exceeded supported range.
   at System.Runtime.Serialization.Formatters.Binary.ObjectReader.ParseArray(ParseRecord pr)
   at System.Runtime.Serialization.Formatters.Binary.ObjectReader.ParseObject(ParseRecord pr)
   at System.Runtime.Serialization.Formatters.Binary.ObjectReader.Parse(ParseRecord pr)
   at System.Runtime.Serialization.Formatters.Binary.BinaryParser.ReadArray(BinaryHeaderEnum binaryHeaderEnum)
   at System.Runtime.Serialization.Formatters.Binary.BinaryParser.Run()
   at System.Runtime.Serialization.Formatters.Binary.ObjectReader.Deserialize(BinaryParser serParser, Boolean fCheck)
   at System.Runtime.Serialization.Formatters.Binary.BinaryFormatter.Deserialize(Stream serializationStream, Boolean check)
   at System.Runtime.Serialization.Formatters.Binary.BinaryFormatter.Deserialize(Stream serializationStream)
   at CoreFX.Fuzz.Program.Main(String[] args) in c:\d\fuzz1\Program.cs:line 31

[danmoseley]

@ViktorHofer these seem like expected exceptions in the face of arbitrary input. Do you agree? I guess we should update docs.

[stephentoub]

these seem like expected exceptions in the face of arbitrary input. Do you agree? I guess we should update docs.

NullReferenceExceptions should never happen, even if the input is invalid. That should be fixed.

OutOfMemoryException can happen in many APIs, e.g. File.ReadAllText could OOM, and we don't document that; it's just the nature of the system. That doesn't need to be documented.

The rest of these are because the input data is corrupt and BinarySerializer can't make sense of it. I'd suggest these should be wrapped in a SerializationException, rather than being allowed to escape raw, and should not be documented separately. (I'd also be surprised if these were the only exceptions that could occur from corrupt input.)

[danmoseley]

That makes sense to me, catch and wrap, and fix the NRE.

[danmoseley]

Marked up for grabs to do the wrapping.

[sebastienpl]

Hi, I'd be happy to take that one as a first one.

But before starting doing it I'd like to ensure what kind of fix you'd really expect for it because I see multiple possible approach.

Let's take one first simple example: ArgumentOutOfRangeException. one place where it's thrown is System.Runtime.Serialization.ObjectManager.GetObject
Few ways of fixing:

1. Update ObjectManager.GetObject to replace throwing OutOfRange by Serialization exception.
   Doesn't feel really clean because at that point it's really an argument exception and the caller should have verified that. But as this ObjectManager is only made for that purpose, it might still be a good fix...
2. Update calling methods to check for the value range and throw instead of calling.
   This seems to be the cleanest from my point of view because caller should ensure parameters are clean, the only reason I could see to not do that would be to avoid impacting performances because of redoundant check.
3. Update calling methods to catch and wrap the exception.
   Same logic as above that caller is responsible of passing good parameters (and thus know what exception will be thrown if they are bad) but no additional check impacting perfs (but a lot more small try/catch).
4. Just update BinaryFormatter.Deserialize to catch and wrap no matter where it comes from.
   Doesn't feel really clean to me but we might argue that it's the only viable way to catch everything in one place without impacting multiple pieces of code.

Any thoughts? Maybe even another way of doing that I didn't thought about.

[GrabYourPitchforks]

I realize I'm very late to this party. :)

I'd also be surprised if these were the only exceptions that could occur from corrupt input.

Corrupted input can perform literally any action, including rewriting the x86 code of BinaryFormatter.Deserialize on-the-fly to remove the "wrap all exceptions" logic. So from a strict security perspective, we see no problem with allowing arbitrary exceptions to escape the Deserialize routine. If there's a non-security reason to add the "wrap all exceptions" behavior, go for it.

[birkmose]

I've submitted a suggestion on how to fix to this - which is pretty much what @sebastienpl suggests in step 4. I did not add any new tests for this, as a range of tests (SerializationGuardTests) actually already covers this code path (I had to change those tests as well, as they assumed that the "naked" exception would escape).

It would be a possibility to create new fuzzed tests (using the test namespace/assembly) as well, but I wonder if this is needed? As @stephentoub mentions there can probably be many random exceptions in case of corrupt input.