ALEPH-2018004 - DOS vulnerability is still exploitable

[mirgil]

The fix that was applied for the DOS vulnerability in Newtonsoft.Json 13.0.1 (#2462) does not really solve the problem.
It fixes only one potential way to create an highly nested object that will throw a StackOverflowException (Deserialize a given serialized object).
But what if the system contains nested objects like file system structure or groups that can contain sub-groups?
An attacker can create highly nested object in the system, and then whenever this object will be serialized, it will initiate a fatal StuckOverflowException.

Expected behavior

The recursion depth should be limited by default also for the Serialization functions (like JsonSerializer.Serialize(..)).
Actually, all the recursive functions should be limited by default to prevent potential DOS attacks.

Actual behavior

Recursion depth is not limited by default in the Serialization functions.

Sample code that demopnstrates the issue

//The following code is similar to the one that was used in the original report 
// (https://alephsecurity.com/vulns/aleph-2018004), with some modifications to demonstrate the issue.

public class TreeNode
{
    public TreeNode[] Suns { get; set; }
}

class Program
{
    static void Main()
    {
        const int nDepth = 50000;

        //Create a highly nested Tree
        TreeNode root = new TreeNode();
        TreeNode iNode = root;
        for (int i = 0; i < nDepth; i++)
        {
            iNode.Suns = new TreeNode[] { new TreeNode() };
            iNode = iNode.Suns[0];
        }
        try
        {
            using MemoryStream s1 = new MemoryStream();
            var jsonWriter = new Newtonsoft.Json.JsonTextWriter(new StreamWriter(s1, System.Text.Encoding.UTF8));
            
            var jsonSerializer = new Newtonsoft.Json.JsonSerializer();
            //Following line will throw an un-catchable StackOverflowException
            jsonSerializer.Serialize(jsonWriter, root, typeof(TreeNode));
            
            //This also throws StackOverflowException:
            //var jObj = Newtonsoft.Json.Linq.JObject.FromObject(root);
        }
        catch (Exception ex)
        {
            //The StackOverflowException will not be caught; It will crash the application.
            Console.WriteLine("Exception was caught: " +ex.ToString());
        }
    }
}

[elgonzo]

Scratch my former comment (which i deleted). Makes sense to classify this also as a potential exploit for DOS if the attacker finds ways for the affected app/service to on-board deeply nested data that the app/service is then going to json-serialize down the road...

[JamesNK]

Deserializing in a web api means deserializing what JSON you’re given. Protection is needed in that direction to prevent JSON from impacting the availability of your app.

Serializing outbound JSON is controlled by the app because your app code controls the object being serialized. You should take steps not to give the serializer a bad type.

Throwing a StackoverflowException by itself isn’t a vulnerability. The exploit was that an external attacker could trigger it by sending a web api JSON.

[mirgil]

An infrastructure library that throws StackoverflowException exception (which cannot be caught and terminate the process) is a potential risk to applications that use it.
The object that is being serialized might be controlled by input to the application.
For example, if I have an application that gets an algebraic expression and return a json representation of the expression tree. In this case, the serialized object is controlled by user input.
I agree that the prevalence of such scenarios is low, but they do exist. And applications that contain such functionality are vulnerable.
Note that System.Text.Json library limits the depth of Serialization to 64 by default - it is done to protect against such scenarios.

[JamesNK]

For example, if I have an application that gets an algebraic expression and return a json representation of the expression tree. In this case, the serialized object is controlled by user input.

The bug is the user didn't validate the depth of the input: the expression tree.

DataContractJsonSerializer is the same as Newtonsoft.Json. There is max depth on the reader, no max depth on the writer. If I was going to start from a blank slate then I'd probably have max depth limit both directions because it can give a better error message, but it's not a security vulnerability because code controls what is passed to the serializer and can validate that what it is going to be serialized is valid.

[mirgil]

The bug is the user didn't validate the depth of the input: the expression tree.

I disagree with this approach.
In the Expression tree example, there is nothing wrong with highly nested expression – it is a valid expression. The problem arises when using the Newtonsoft.Json library to serialize it.
You cannot assume that users will be aware of the function's recursive nature or that providing a highly nested object to the function will cause their application to crash.
The reason that it should be treated as a potential DOS vulnerability, is the consequences of such scenario.

DataContractJsonSerializer is the same as Newtonsoft.Json. There is max depth on the reader, no max depth on the writer.

DataContractJsonSerializer Its an old library and it is not part of System.Text.Json.
According to its documentation: “For most scenarios that involve serializing to JSON and deserializing from JSON, we recommend the APIs in the System.Text.Json namespace”