Consider removing ISerializable support

[bradwilson]

Update: July 6, 2022

The solution I landed on includes removing the type-unsafe serialization format in favor of a mostly lookup table-based type system, while still supporting serialization of enums and IXunitSerializable objects with embedded type information. This limits the type-embedded serialization to just those two cases, and we verify that we are only creating enums or classes that implement IXunitSerializable when deserializing. Since this interface was designed explicitly only for unit testing with xUnit.net (and it's not an arbitrary serialization interface like ISerializable), this should alleviate any potential security concerns. In practice, this should not noticeably change the kinds of data that v3 can serialize when compared to v2, but it should be done in a much safer manner (and without relying on any unsafe built-in serialization infrastructure), and it should be both more memory efficient and higher performance. You can read more about this below.

Description of the issue

Currently we use BinaryFormatter to support serializing arbitrary test case data across process boundaries. This replaces support for IXunitSerializable from v2 (which was added because the lowest common denominator of target frameworks for v2 does not include ISerializable). This serializer is used today to allow arbitrary data to be passed to test cases via theory rows, and allow the end user to run those individual theory rows in Visual Studio's Test Explorer. Without some kind of serialization support, we would end up reverting back to xUnit.net v1 behavior (one Test Explorer item per test method, which runs all the associated data rows without the ability to run individual rows independently of one another).

Binary serialization has been deprecated because of security concerns and will be outright removed from future versions of .NET. Other related serializers (SoapFormatter, LosFormatter, NetDataContractSerializer, ObjectStateFormatter) suffer from the same concerns. A quick analysis of the serialization in v2 shows that it, too, suffers from the arbitrary type deserialization concern.

Generally speaking, we have two alternatives.

1. Remove generalized object serialization

If we were to remove generalized object serialization, that would mean that any test method which contains unsupported objects in a theory row would revert to the v1 behavior in Test Explorer (this is also what happens today in v2 for arbitrary objects which do not implement IXunitSerializable).

What types would be supported in box? Likely the list would look very much like what we support intrinsically in v2. Any data row which contains data outside this supported type list would still be supported, but would lose the ability in Test Explorer to run individual data rows.

2. Add support for a safer serialization alternative

There are some safer alternatives (such as XmlSerializer) which would allow test authors to implement serialization that would be supported, but an important caveat would be that these serialization formats do not support polymorphism.

What does that mean? In practical terms: if the object in your data row is of type Employee, then your test method argument must also be of type Employee: trying to use base types (like Person) or interfaces (like IPerson) would not be supported. During the discovery process, we would have to look for exact type matches between data row element and test method signature, and fall back to the non-serialized behavior when they don't match.

Additionally, there is no good single serialization system to support, since every one of these safer serializers tends to come with its own set of features, limitations, and contracts for how serialization is defined (most tend to use attributes). These feature sets and contracts are not cross-serializer. So it's very likely that we would have to pick just a single (or very small number) of these serializers to support, as we would not be equipped to support an arbitrary number of them.

We could also re-introduce support for something like IXunitSerializable but with the new exact-type-match requirements.

Thoughts

Please provide thoughts on this in this discussion thread. Should we support any kind of arbitrary object serialization? If so, would you prefer something built-in like XmlSerializer (and which one?), or would you prefer us to keep IXunitSerializable but with the new same-type restrictions?

Thanks!

[Tornhoof]

I personally think that the intrinsic types are enough. I can't remember ever looking or needing more than that. So from my POV, your first alternative would be fine.

[jzabroski]

To further this explanation, Rust's SerDe is not vulnerable in general to the type exploitations you see discussed in ysoserial.net, because the implementer of Deserialize has to pass in a Type to deserialize into. The magic trick is in how to build a safe Serialize Implementation through composition. So, for example, the yaml-rust Crate (aka nuget package in C#) is not vulnerable to ysoserial attacks, but there many languages with yaml serialization libraries that are vulnerable.

I think I'm either 100% right or off by a country mile and this will age poorly. :-) The nuance is in how "arbitrary type" is defined and whether my plug-in model for building serializers actually allows an exploit - I dont think it does.

[jzabroski]

Here is an alternative explanation: https://serde.rs/

Serde is a framework for serializing and deserializing Rust data structures efficiently and generically.

The Serde ecosystem consists of data structures that know how to serialize and deserialize themselves along with data formats that know how to serialize and deserialize other things. Serde provides the layer by which these two groups interact with each other, allowing any supported data structure to be serialized and deserialized using any supported data format.

In other words,

data structures that know how to serialize and deserialize themselves

^^ These are your built-in/intrinsic data types

along with data formats that know how to serialize and deserialize other things

^^ these "data formats" are your formatters. the "other things" are complex data types built out of intrinsics.

Serde provides the layer by which these two groups interact with each other, allowing any supported data structure to be serialized and deserialized using any supported data format.

^^ This layer is what you use as a magic trick to allow serializing "any supported data structure" (complex types built out of intrinsics).

[bradwilson]

I haven't investigated that framework, but the problem isn't things that know how to serialize and deserialize themselves, it's putting a generic "what type should I create?" into the serialization stream where the security issue comes from. (By which I mean, not a fixed lookup table, but the ability to arbitrarily create instances of any type in the type system.)

[jzabroski]

Right, and the magic trick is that MyTests.dll is the authoritative reference for both sides of what extends the fixed lookup table.

This is the part I'm not 100% is secure, but I think it is. Whether it is something that works with Visual Studio architecture is another question. But... It's not much different from how a Test class has a reference to ITestOutputHelper. The test defines what object-capability security people call a Powerbox of capabilities the Test class instance has access to.

An attacker would need to somehow replace the dll to confuse the system.

[jzabroski]

Hi @bradwilson ,
I've been discussing this serialization issue with a friend. I think, practically, xunit.net needs a threat model before making architecture changes. Whether to discuss a threat model publicly is up to you, but probably also requires discussion with VS Engineering and JetBrains. I also spent some time studying how JetBrains hooks into your xunit.testrunner, but it's time consuming and I've only made some progress.

[jzabroski]

I prefer ability to run individual theory rows individually. But - is that limitation due to Visual Studio Test Explorer being lame? I use ReSharper so I don't know if this would affect me.

What problem are you solving by removing this?

EDIT: I see the problem.

Binary serialization has been deprecated because of security concerns and will be outright removed from future versions of .NET. Other related serializers (SoapFormatter, LosFormatter, NetDataContractSerializer, ObjectStateFormatter) suffer from the same concerns.

A lot easier to read discussions on Desktop than mobile phone...

[SimonCropp]

this would break a few things for me. But all of those can be worked around. And IMO removing ISerializable support is a good idea to remove the associated complexity

[bradwilson]

I want to say complexity will go down, but I'm not sure that's necessarily true if I re-introduce IXunitSerializable. Some of the complexity will shift to supporting that (effectively the same as the previous support for that, except that now it has stronger type limitations to prevent polymorphic object creation).

[bradwilson]

That is, assuming I go with strategy 2 and not strategy 1.

[markm77]

It is extremely important for me to be able to run theory rows individually. Currently I am using IXunitSerializable and split my theory data into the following two classes which allows for custom compact test runner labelling via ToString():

BankTestData1: https://github.com/finlabsuk/open-banking-connector/blob/02f1b903321d07e59e60d3cca60a95a228902269/test/OpenBanking.Library.Connector.BankTests/BankTests/BankTestData1.cs
BankTestData2: https://github.com/finlabsuk/open-banking-connector/blob/02f1b903321d07e59e60d3cca60a95a228902269/test/OpenBanking.Library.Connector.BankTests/BankTests/BankTestData2.cs

You can see in the above classes I am using custom serialisation for some properties.

My tests do use the same types returned by the TheoryData. For example: https://github.com/finlabsuk/open-banking-connector/blob/02f1b903321d07e59e60d3cca60a95a228902269/test/OpenBanking.Library.Connector.BankTests/BankTests/GenericHostAppTests.cs#L24-L47

Would be very disappointing to lose either ability to run theory rows individually or even ability to customise labelling in test runner by grouping in classes as above....

[jzabroski]

@markm77 Something neat is coming to C# 11 that could be a game changing feature for TheoryData: Generic Attributes. Note, it's in the C# 10.0 proposal folder, but I believe it's been pushed to C# 11 and is in Preview mode now.

[bradwilson]

I've pushed up into dev a removal of ISerializable and re-introduction of IXunitSerializable: 87e058d

The implementation supports:

• Intrinsic types: String, Char, Byte, SByte, Int16, UInt16, Int32, UInt32, Int64, UInt64, Single, Double, Decimal, Boolean, DateTime, DateTimeOffset, Type
• Enum values
• Nullable versions of the value-type intrinsics and enum values
• Custom types implementing IXunitSerializable
• Arrays of supported types (f.e., String[])
• Method parameter lists (aka Object[] that contains serializable values)
• Traits dictionaries (aka Dictionary<string, List<string>>)

Anything not on this list won't be supported; in particular, the generic collection class like List<> are not supported (with the exception of trait dictionaries, which have both hard-coded types as well as hard-coded comparers). I believe in practice, this should allow any test from v2 currently shows up as individually runnable in Visual Studio Test Explorer today should continue to show up as runnable in v3.

Just as important, since types are still embedded (albeit safely now, see discussion below), this will not impact the parameter types you can use; you are still able to use things like object or generics to represent polymorphic theory data.

Based on the discussion here, this implementation does a safe(r) form of type embedding in the serialization: rather than always embedding type names it uses a type index, and we only embed full types names for enums and IXunitSerializable. I consider this to be safe(r) because you can no longer create arbitrary types; besides the intrinsics (which should all be safe), enum values are limited to know enum types (which should all be safe) and IXunitSerializable values are limited to only things which implement IXunitSerializable, which means they're explicitly designed to be serialized and deserialized only in the context of the testing framework.

Hopefully this will address the concerns with the loss of flexibility by rolling back the ISerializable support while also address the arbitrary type creation concerns of the .NET team.

I haven't merged this into main yet, so there is no NuGet-based build, but you wish to review the source and provide feedback, that would be most welcome. The best place to start to understand what the code does is, as usual, the tests; in particular, SerializationHelperTests.cs and XunitSerializationInfoTests.cs (the latter is used for serializing IXunitSerializable objects as well as arrays).

[bradwilson]

Currently beyond the minimum requirements (not supported in .NET Standard 2.0). If it's important enough, it could be done via reflection, perhaps.

[jzabroski]

I guess that's fine, but...

while also address the arbitrary type creation concerns of the .NET team.

I still don't think we know what those concerns were. What is xunit.net's threat model given its an IDE plug-in (xunit.testrunner)?

[bradwilson]

@jzabroski The v3 behavior will be essentially identical to the v2 behavior. Given that essentially all forms of type-embedded serialization built into .NET have been deprecated and may be removed in the future, it's a bad future-looking decision for us to rely on them.

[jzabroski]

OK, I can get on board with that reasoning, but the previous comment made it sound like you had specific advice given to you, and that it would benefit the project to share it.

Overall, thank you for this discussion, as it made me think of some stuff I haven't had to think about in years (object capability security, taming, application threat modeling, etc.)

[bradwilson]

FYI, I merged this into main today.