The JSON-parsing vulnerability fixed by Steemd 0.20.9

In my previous bug report on steemd, @crokkon asked if custom JSON was also vulnerable which inspired me to take a closer look.

The JSON parsing code in the FC library used by Steem did have a check for nesting, but it was not adequate:

   /** the purpose of this check is to verify that we will not get a stack overflow in the recursive descent parser */
   void check_string_depth( const string& utf8_str  )
   {
      int32_t open_object = 0;
      int32_t open_array  = 0;
      for( auto c : utf8_str )
      {
         switch( c )
         {
            case '{': open_object++; break;
            case '}': open_object--; break;
            case '[': open_array++; break;
            case ']': open_array--; break;
            default: break;
         }
         FC_ASSERT( open_object < 100 && open_array < 100, "object graph too deep", ("object depth",open_object)("array depth", open_array) );
      }
   }

This count checks the depth of opening braces, but it has both false positives and false negatives, because it neglects to check whether the brace appears within a string.

For example, the following JSON string would be viewed as having a large array depth, even though it does not have any arrays at all:

{ "foo" : "[[[[[[...[[[" }

(Example had to be truncated, see addendum below.)

The other direction is more concerning; we can construct a malfomed JSON-RPC call which exhausts the stack of the caller, causing the process to crash. The following pattern can be repeated up the maximum size of an accepted message (2MB).

{"jsonrpc":"2.0","method": ["]",["]",["]",["]",["]",["]",["]",["]",...

This bug was reported to security@steem.com on January 2nd and fixed in Steemd version 0.20.9 on January 22nd.

Analysis

Stack-exhaustion attacks are not as easy to find with a coverage-based tool like AFL, as memory-exhaustion attacks are. It might be that setting a very low recursion depth or stack size might have allowed AFL to find this problem, but it seems likely that the necessary path through the code is one that's not easy for a purely coverage-based tool to find. I'd be interested in learning what other sort of tools would be better at identifying this sort of weakness.

Bitshares fixed this vulnerability far earlier, in March 2018: https://github.com/bitshares/bitshares-fc/commit/f9802f686007e7efeaa88cba31f647ca96f1ee0c But, the fractured nature of the FC and Graphene ecosystem means that the vulnerability was not communicated to other implementations (or ignored if it was.)

Addendum

An earlier version of this article was rejected with the "object too deep" error, because the example above of a false positive triggered the code check I quoted! So while the false negative had been fixed, the false positive bug remains.

Photo by Etienne Steenkamp on Unsplash