Java: CWE-502 Unsafe JSON deserialization with Gson, Flexjson, Jabsorb and JoddJson

[luchua-bc]

Deserialization of untrusted data with JSON frameworks could introduce critical security vulnerabilities such as Remote Code Execution (RCE).

This query checks four popular JSON frameworks - Gson, Flexjson, JoddJson, and Jabsorb and detects the following vulnerabilities:

• Gson
  • both data and type of deserialized object are controlled by attackers
• JoddJson
  • both data and type of deserialized object are controlled by attackers
  • whitelisted class types that are overly generic or have vulnerable subclasses
• Flexjson
  • both data and type of deserialized object are controlled by attackers
  • any configured root class type that is not final or is overly generic
• Jabsorb
  • both data and type of deserialized object are controlled by attackers

Some known CVEs and issues related to these JSON implementations are:

• Liferay Portal JSON Web Service RCE 6.2 by Code White
• PayPal mobile app by Oversecured

Please consider to merge the PR. Thanks.

[smowton]

I've started evaluating this as-is, but for future reference, could the new sanititzer be implemented with Configuration::isSanitizer(DataFlow::Node) instead of by not exists side-conditions in the predicate unsafeDeserialization?

[luchua-bc]

Thanks @smowton for reviewing this PR. I'll make the change of moving not exists to isSanitizer together with other changes to be made for your suggestions.

[luchua-bc]

@smowton I've made the change. Please review and let me know if more changes are needed. Thanks.

[smowton]

@luchua-bc please rebase this now that the other unsafe-deserialization PRs have landed

[luchua-bc]

@smowton I've done the rebase. Please review. Thanks.

[smowton]

File "ql/java/ql/test/stubs/joddjson-6.0.3/jodd/json/JsonParser.java" contains a non-ASCII character at the location marked with | in:

ch opens an actual security hole. Such classes are known as |

File "ql/java/ql/src/semmle/code/java/frameworks/google/Gson.qll" contains a non-ASCII character at the location marked with | in:

Gson and
    this.hasName("fromJson")
  }
}

/** The `toJson|

ASCII check failed!
FAILED: ascii-check.py

These will probably be non-breaking spaces or similar

[luchua-bc]

@smowton I've fixed those two issues - one caused by the Windows style double quotes in the stub source file, and the other one was a Unicode character in Gson.qll.

Please run the ASCII check again. Thanks.

[smowton]

Also this needs a rebase

[luchua-bc]

I've made the changes on the rebased branch so it seems that another rebase is not required. Please advise if so. Thanks.

[smowton]

@luchua-bc there are still conflicts with main that prevent directly merging this branch. Is there another one somewhere? What is the rebased branch you speak of?

[luchua-bc]

I rebased my branch java/unsafe-gson-flexjson-joddjson-deserialization onto the main. Sorry that there are still some conflicts. I will try to rebase again.

[smowton]

A few questions:

1. I still wonder what is important about the Class argument to deserialize being final? If you saw a version passed a non-final class would you think it was dangerous, and why?
2. Annoyingly the use methods aren't well documented; do you know if you'd expect x.use(A.class); x.deserialize(...) to be safe, or it the returned deserializer distinct from the qualifier?
3. What about the other overloads of use(...), do you expect them to be safe? Would it be sensible to consider any use(...) sanitizing?
4. What about the deserialize overloads that take an ObjectFactory, are those likely to be safe?
5. Finally because deserializeInto takes an actual object, do you expect that to constrain the deserialized type also and therefore be safe?

[luchua-bc]

What I did is on my branch:

git checkout java/unsafe-gson-flexjson-joddjson-deserialization
git fetch https://github.com/github/codeql.git main
git rebase FETCH_HEAD
<resolve conflicted files and add my changes>
git add *
git push origin java/unsafe-gson-flexjson-joddjson-deserialization -f

Please let me know if my procedure is not correct. Thanks.

[smowton]

You need a git rebase --continue after that git add to note the conflicts are resolved.

[luchua-bc]

Hi @smowton,

Please find my comments below:

A few questions:

1. I still wonder what is important about the Class argument to deserialize being final? If you saw a version passed a non-final class would you think it was dangerous, and why?

If a class is non-final and one of its subclasses is vulnerable, attacker can feed a malicious payload with an object of the vulnerable subclass.

2. Annoyingly the use methods aren't well documented; do you know if you'd expect x.use(A.class); x.deserialize(...) to be safe, or it the returned deserializer distinct from the qualifier?

If x.use(A.class); x.deserialize(...) is used, only an object of the specified class A can be taken (not objects of subclasses).

3. What about the other overloads of use(...), do you expect them to be safe? Would it be sensible to consider any use(...) sanitizing?

I will test all the other overloads and verify. I have assumed using use(...) will make the class object more specific therefore not vulnerable. Not using use(...) definitely means developers don't have the extra security control.

4. What about the deserialize overloads that take an ObjectFactory, are those likely to be safe?

I will test and verify.

5. Finally because deserializeInto takes an actual object, do you expect that to constrain the deserialized type also and therefore be safe?

As this method is not widely used, I have tested it yet. I will test and verify then report back.

[luchua-bc]

Hi @smowton,

For questions 3-5:

3. What about the other overloads of use(...), do you expect them to be safe? Would it be sensible to consider any use(...) sanitizing?

I've verified that the other overloads of use(...) help to put a constraint on the class type to deserialize into as shown in the newly added test case below in FlexjsonServlet.java:

    // GOOD: Specify the concrete class type to deserialize with `ObjectFactory`
    public void doPut3(HttpServletRequest req, HttpServletResponse resp) throws IOException {
        String json = req.getParameter("json");
        Person person = new JSONDeserializer<Person>().use(Person.class, new ExistingObjectFactory(new Person())).deserialize(json);
    }

4. What about the deserialize overloads that take an ObjectFactory, are those likely to be safe?

deserialize overloads that take an ObjectFactory are safe so I've modified the query to accommodate this. A new test case has been added:

    // GOOD: Specify the concrete class type to deserialize with `ObjectFactory`
    public void doPut4(HttpServletRequest req, HttpServletResponse resp) throws IOException {
        String json = req.getParameter("json");
        Person person = new JSONDeserializer<Person>().deserialize(json, new ExistingObjectFactory(new Person()));
    }

5. Finally because deserializeInto takes an actual object, do you expect that to constrain the deserialized type also and therefore be safe?

deserializeInto takes an actual object, which invokes the deserialize overloads that take an ObjectFactory behind the scene. Therefore it's safe as well. I've modified the query and added the following test case:

    // GOOD: Specify the class type to deserialize into
    public void doPut5(HttpServletRequest req, HttpServletResponse resp) throws IOException {
        String json = req.getParameter("json");
        Person person = new JSONDeserializer<Person>().deserializeInto(json, new Person());
    }

And I've rebased the code. Please review.

Thanks,
@luchua-bc

[smowton]

That's not quite what I meant regarding Gson, but at this point it's faster to just write what I mean rather than try to explain further.

[smowton]

Some other improvements, but happy to make these myself. Would be glad to hear your take re: the false-positive vs. false-negative risk and non-final classes.

[smowton]

Worth noting that

JSONDeserializer<Person> jsd = new flexjson.JSONDeserializer<Person>();
jsd.use(null, Person.class);
jsd.deserialize(json);

is also safe. Without taking on all the cost of a subsidiary Configuration, it might be best to achieve this by modelling all the use(...) methods as having value-flow from qualifier to return value, then using localFlow(any(MethodAccess ma | ma.getMethod() instanceof DeserializerUseMethod).getQualifier(), ...)

This would be best achieved by something like adding a lib file that is imported from ExternalFlow.qll that contains:

class FlexJsonModels extends SummaryModelCsv {
  override predicate row(string row) {
    row =
      [
        "flexjson;JSONDeserializer;false;use;;;Argument[-1];ReturnValue;value"
      ]
  }
}

This says that the use methods are fluent (i.e., they return this), and enables localFlow to do the rest of the work for us.

[luchua-bc]

Great. This looks like a nice solution and I will learn a new technique from your final version. Thanks.

[luchua-bc]

@smowton - for this one, will you make the change or do you want me to make the change following your code snippet? Please advise. Thanks.

[smowton]

I'll pick it up from here

[luchua-bc]

Thanks @smowton:-)

[Marcono1234]

Regarding Flexjson, it appears only usage of use(String, ...) where the path is null or use(ObjectFactory, String...) where the string varargs (or array) contains null is safe. In all other cases (even empty String as Path) it applies only to sub elements of the root JSON value. For example, the following has no effect and calls the malicious constructor:

new JSONDeserializer<Safe>()
    // Only applies to JSON path 'abc'
    .use("abc", Safe.class)
    // Only affects *how* type is deserialized, but not *as which* type JSON is deserialized 
    .use(Safe.class, new ExistingObjectFactory(new Safe()))
    .deserialize(json);

Additionally, if I see that correctly even if you explicitly specify a type, the application might still be vulnerable if that type has fields or setter methods with type Object (or similar), but that might be out of scope for this pull request.

And not really relevant for this query, but Flexjson initializes arbitrary classes in case no thread context class loader is set (there might be one by default), see https://sourceforge.net/p/flexjson/code/162/tree/trunk/src/main/java/flexjson/ObjectBinder.java#l245. That code does not appear to be in the latest commits (maybe it was just moved somewhere else), but they have not been released anyway.

[luchua-bc]

@Marcono1234 Thanks for reviewing the PR and I've made requested changes. Please review.

[luchua-bc]

@smowton and @Marcono1234 - with the most recent change, do we still need to revamp the query using FlexJsonModels?

[smowton]

Jabsorb support factored with existing Jackson support and split off as #6419

[luchua-bc]

Thanks @smowton.

[smowton]

Part 2: Jodd JSON: #6434

[smowton]

Part 3: #6456

Part 4: #6463

That last one implements Gson support and takes quite a different approach, introducing a single step from an Intent to the Parcel parameter of a createFromParcel method whenever the result of getParcelableExtra flows to something of the corresponding type. I also changed the approach to look for concurrent user control of the text and type, similar to our existing analysis of Flexjson.

[luchua-bc]

Wow! That's a lot of changes. Thanks @smowton a lot for your help. For future PRs, I will submit one PR per library/query.

[smowton]

That's useful, especially when targeting non-experimental which is always going to require extra effort on our part.

[smowton]

Last bits of this are now merged

[luchua-bc]

Thanks @smowton a lot for all your help with this complex and comprehensive PR, which is literally 4 PRs:-).