0

I was reading about the vulnerability of deserializing types with Json.Net using a setting different from TypeNameHandling.None. The Json.Net docs recommend implementing a custom SerializationBinder. A simple example of a custom binder that checks types against a list of known types is given here.

While this solution certainly works, the set of known types is not fixed in my scenario, since the application has to support extensions, which might define their own data classes. One solution would be to extend the known type list during the registration of an extension, however, I had a second approach in mind, that I'd like to verify:

I want to define a common interface for trusted types:

(suggested by dbc: A custom attribute could be used instead of a marker interface.)

public interface ITrustedType { }

Then, any types that need to be (de-)serialized by the application or an extension, can implement this interface:

public class Car : ITrustedType
{
   public IList<Passenger> Passengers { get; set; }
   // ...
}

public class Passenger : ITrustedType
{
   // ...
}

The custom binder does not check against a list of types, but instead, it will make sure that each (de-)serialized type is implementing the trusted type interface, applying the default behavior in case it does or failing if it doesn't.

In the case of an enumerable type, it would make sure that the generic type parameters are trusted types.

This is a simple implementation with which I tested so far:

public class TrustedTypesBinder : DefaultSerializationBinder
{
    static bool IsCollectionType(Type type)
    {
        return type.GetInterfaces().Any(s => s.Namespace == "System.Collections.Generic" && s.Name.StartsWith("IEnumerable`"));
    }

    static public bool IsTrustedType(Type type)
    {
        if (IsCollectionType(type))
        {
            return type.GenericTypeArguments.All(t => IsTrustedType(t));
        }
        return typeof(ITrustedType).IsAssignableFrom(type);
    }

    public override Type BindToType(string assemblyName, string typeName)
    {
        Type type = base.BindToType(assemblyName, typeName);
        if (!IsTrustedType(type))
            throw new JsonSerializationException(typeName + " is not a trusted type.");
        return type;
    }

    public override void BindToName(Type serializedType, out string assemblyName, out string typeName)
    {
        if (!IsTrustedType(serializedType))
            throw new JsonSerializationException(serializedType.FullName + " is not a trusted type.");
        base.BindToName(serializedType, out assemblyName, out typeName);
    }
}

I suspect this solution to be safe since no type from outside the app or extension assembly will implement this interface, so no attack gadgets can be constructed from a malicious JSON input.

Am I missing any attack vectors? Do you know a way in which this could be compromised?

Edit: I am aware, that it is possible for anyone to implement this ITrustedType interface and give it some arbitrary behavior. As far as I know, this should not be relevant in the context of (de-)serialization at runtime. The app itself, as well as the extensions, are coming from trusted sources, so of course, implemented types are controlled and it will not be possible to add any new types into the published application binaries for an attacker.

This question is specifically about instantiating types that are present at runtime and could be used for malicious purposes.

Edit 2: Changed ICollection to IEnumerable, based on Ben Voigts suggestion.

loe
  • 83
  • 7
  • Why are you looking for `ICollection` and not `IEnumerable`? Doesn't that open the barn door wide open? Are "attack gadgets" guaranteed never to implement an interface under the attacker's control? Merely implementing `ICollection` doesn't guarantee that the class is a Microsoft-provided collection or even that its behavior is consistent with a collection. – Ben Voigt Jun 10 '21 at 22:38
  • BTW a custom type can be named `System.Collections.Generic.ICollectionImposter`... a namespace is not secured in any way. If you mean to test for the real system-provided `ICollection` then use `typeof(System.Collections.Generic.ICollection)).GetGenericTypeDefinition()` – Ben Voigt Jun 10 '21 at 22:39
  • 1
    Also note `class Generic : ICollection`... you are testing `T` when you meant to be checking `U`. – Ben Voigt Jun 10 '21 at 22:43
  • @BenVoigt I understand what you mean. Of course, it is possible to introduce any behavior by implementing a new type. I am however only concerned about the security of the app at runtime *after it has been built* when loading data files, containing serial JSON. The app core, as well as the extensions, can be assumed to come from a trusted source and will not include any other "imposter" implementations. Adding an imposter type would therefore require introducing new malicious types into the built and signed binaries. The data files however could be shared across users and manipulated. – loe Jun 10 '21 at 22:49
  • @BenVoigt Can you please explain why you would suggest IEnumerable over ICollection? – loe Jun 10 '21 at 22:51
  • 1
    Microsoft's [Framework Design Guidelines: Interface Design](https://learn.microsoft.com/en-us/dotnet/standard/design-guidelines/interface?redirectedfrom=MSDN) recommend ***AVOID using marker interfaces (interfaces with no members).** If you need to mark a class as having a specific characteristic (marker), in general, use a custom attribute rather than an interface.* So you might consider using some attribute instead of an interface. – dbc Jun 11 '21 at 01:32
  • @dbc Thank you for this information, a custom attribute seems more appropriate. I think that this also does not alter the original question about the security implications, since it means the binder will still just check the type for some kind of marker. – loe Jun 11 '21 at 09:23
  • 1
    `IEnumerable` includes fixed-length collections, unlike `ICollection`. For example arrays. – Ben Voigt Jun 11 '21 at 15:04
  • 1
    In general though, your problem arises from a plugin defining class `DangerousCollection : ICollection>` which it intended for its own use, not for JSON. Your current tests will only check if `T` is marked safe, not if `Foo` is marked safe. And then deserialization will happily create a whole lot of `Foo` instances. – Ben Voigt Jun 11 '21 at 15:09
  • @BenVoigt thanks a lot, I did not consider that before. – loe Jun 12 '21 at 08:36

1 Answers1

1

When you encounter a type that isn't marked, it is not sufficient to check its generic type arguments, you need to check the type of every public property and every parameter of a public constructor, because these are the serialization footprint.

For example, you really do not want to allow deserialization of a System.Data.TypedTableBase<T> even if T is safe, because it has public properties that allow configuring database access.

Ben Voigt
  • 277,958
  • 43
  • 419
  • 720
  • Ok, thank you, I think that I now understand your point, from this answer and the other comment you left. Just to recapitulate, there are two problems: (1) Dangerous enumerable types like `System.Data.TypedTableBase` can be present, which are used by a plugin in a legitimate way but could be misused by an attacker. (2) The inheritance of the marker can lead to plugins (unknowingly) introducing dangerous types like `DangerousCollection : ICollection>` Now how could this be mitigated? Would it be sufficient to whitelist only specific "safe" enumerable types? – loe Jun 12 '21 at 08:43
  • 1
    @Joe: You could whitelist collection types, or you could check the type of the public properties and parameters of public constructors. In the `DangerousCollection` example, the property walk would find `Foo`. – Ben Voigt Jun 14 '21 at 15:32