is Newtonsoft TypeNameHandling.all with a basic namespace check safe?

user_7239875 0
Viewed 645

On our API we need to take in json, deserialize it to an interface, set a field, and ship it off. To achieve this, on both ends I'm setting the jsonConvert to use TypeNameHandling.All. The endpoint in question is supposed to be fairly locked down, but there's always a chance of someone gaining access and setting $type to a system class with a dangerous constructor or garbage collection method.

My question is would clarifying the namespace of the type before attempting to deserialize it be sufficiently safe? or would there still be a risk of having something like a sub-object with a dangerous class type in the json? If there is still a risk or an exploit I've missed, what other steps can I do to mitigate the danger?

Our company name is at the start of every namespace we use, so in the code below we just check that the type set in the json starts with our company name. The {} at the start is just so the compiler knows it doesn't need to keep the JObject in memory after the check.

{ //check the type is valid
    var securityType = JsonConvert.DeserializeObject<JObject>(request.requestJson);
    JToken type;
    if (securityType.TryGetValue("$type", out type))
    {
        if (!type.ToString().ToLower().StartsWith("foo")) { //'foo' is our company name, all our namespaces start with foo
            await logError($"Possible security violation, client tried  to instantiate {type}", clientId: ClientId);
            throw new Exception($"Request type {type} not supported, please use an IFoo");
        }
    }
    else
    {
        throw new Exception("set a type...");
    }
}
IFoo requestObject = JsonConvert.DeserializeObject<IFoo>(request.requestJson, new JsonSerializerSettings()
{
    TypeNameHandling = TypeNameHandling.All
});
json-deserialization json.net interface security c#
1 Answers

The risk with TypeNameHandling is that an attacker may trick the receiver into constructing an attack gadget - an instance of a type that when constructed, populated or disposed effects an attack on the receiving system. For an overview see

If you are going to protect against such attacks by requiring all deserialized types to be in your own company's .Net namespace, be aware that, when serializing with TypeNameHandling.All, "$type" information will appear throughout the JSON token hierarchy, for all arrays and objects (including for .Net types such as List<T>). As such you must needs apply your "$type" check everywhere type information might occur. The easiest way to do this is with a custom serialization binder such as the following:

public class MySerializationBinder : DefaultSerializationBinder
{
    const string MyNamespace = "foo"; //'foo' is our company name, all our namespaces start with foo

    public override Type BindToType(string assemblyName, string typeName)
    {
        if (!typeName.StartsWith(MyNamespace, StringComparison.OrdinalIgnoreCase))
            throw new JsonSerializationException($"Request type {typeName} not supported, please use an IFoo");
        var type = base.BindToType(assemblyName, typeName);
        return type;
    }
}

Which can be used as follows:

var settings = new JsonSerializerSettings
{
    SerializationBinder = new MySerializationBinder(),
    TypeNameHandling = TypeNameHandling.All,
};

This has the added advantage of being more performant than your solution since pre-loading into a JObject is no longer required.

However, having done so, you may encounter the following issues:

  1. Even if the root object is always from your company's namespace, the "$type" properties for nested values may not necessarily be in your companies namespace. Specifically, type information for harmless generic system collections such as List<T> and Dictionary<TKey, Value> as well as arrays will be included. You may need to enhance BindToType() to whitelist such types.

    Serializing with TypeNameHandling.Objects or TypeNameHandling.Auto can ameliorate the need to whitelist such harmless system types, as type information for such system types is less likely to get included during serialization as compared to TypeNameHandling.All.

    To further simplify the type checking as well as to reduce your attack surface overall, you might consider only allowing type information on the root object. To do that, see json.net - how to add property $type ONLY on root object. SuppressItemTypeNameContractResolver from the accepted answer can be used on the receiving side as well as the sending side, to ignore type information on non-root objects.

    Alternatively, you could serialize and deserialize with TypeNameHandling.None globally and wrap your root object in a container marked with [JsonProperty(TypeNameHandling = TypeNameHandling.Auto)] like so:

    public class Root<TBase>
{
    [JsonProperty(TypeNameHandling = TypeNameHandling.Auto)]
    public TBase Data { get; set; }
}

    Since your root objects all seem to implement some interface IFoo you would serialize and deserialize a Root<IFoo> which would restrict the space of possible attack gadgets to classes implementing IFoo -- a much smaller attack surface.

    Demo fiddle here.

  2. When deserializing generics, both the outer generic and the inner generic parameter types may need to be sanitized recursively. For instance, if your namespace contains a Generic<T> then checking that the typeName begins with your company's namespace will not protect against an attack via a Generic<SomeAttackGadget>.

  3. Even if you only allow types from your own namespace, it's hard to say that's enough to be sufficiently safe, because we don't know whether any of the classes in your own namespace might be repurposed as attack gadgets.

user_37441820
Related