Skip to content

Commit 0b6c991

Browse files
smowtonsmowton
committed
Unsafe deserialization: add support for Jodd JSON library
1 parent 6471092 commit 0b6c991

File tree

12 files changed

+1294
-2
lines changed

12 files changed

+1294
-2
lines changed

‎java/change-notes/2021-08-05-jodd-unsafe-deserialization.md

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
lgtm,codescanning
2+
* The "Deserialization of user-controlled data" (`java/unsafe-deserialization`) query now recognizes deserialization using the `Jodd JSON` library.

‎java/ql/src/Security/CWE/CWE-502/UnsafeDeserialization.qhelp

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@ may have unforeseen effects, such as the execution of arbitrary code.
1515
<p>
1616
There are many different serialization frameworks. This query currently
1717
supports Kryo, XmlDecoder, XStream, SnakeYaml, JYaml, JsonIO, YAMLBeans, HessianBurlap, Castor, Burlap,
18-
Jackson, Jabsorb and Java IO serialization through
18+
Jackson, Jabsorb, Jodd JSON and Java IO serialization through
1919
<code>ObjectInputStream</code>/<code>ObjectOutputStream</code>.
2020
</p>
2121
</overview>
@@ -105,6 +105,10 @@ Blog posts by the developer of Jackson libraries:
105105
Jabsorb documentation on deserialization:
106106
<a href="https://github.com/Servoy/jabsorb/blob/master/src/org/jabsorb/">Jabsorb JSON Serializer</a>.
107107
</li>
108+
<li>
109+
Jodd JSON documentation on deserialization:
110+
<a href="https://json.jodd.org/parser">JoddJson Parser</a>.
111+
</li>
108112
</references>
109113

110114
</qhelp>

‎java/ql/src/semmle/code/java/dataflow/ExternalFlow.qll

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -85,6 +85,7 @@ private module Frameworks {
8585
private import semmle.code.java.frameworks.jackson.JacksonSerializability
8686
private import semmle.code.java.frameworks.JavaxJson
8787
private import semmle.code.java.frameworks.JaxWS
88+
private import semmle.code.java.frameworks.JoddJson
8889
private import semmle.code.java.frameworks.JsonJava
8990
private import semmle.code.java.frameworks.Optional
9091
private import semmle.code.java.frameworks.spring.SpringCache

‎java/ql/src/semmle/code/java/frameworks/JoddJson.qll

Lines changed: 68 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,68 @@
1+
/**
2+
* Provides classes and predicates for working with the Jodd JSON framework.
3+
*/
4+
5+
import java
6+
private import semmle.code.java.dataflow.ExternalFlow
7+
8+
/** The class `jodd.json.Parser`. */
9+
class JoddJsonParser extends RefType {
10+
JoddJsonParser() { this.hasQualifiedName("jodd.json", "JsonParser") }
11+
}
12+
13+
/** A `JsonParser.parse*` deserialization method. */
14+
class JoddJsonParseMethod extends Method {
15+
JoddJsonParseMethod() {
16+
this.getDeclaringType() instanceof JoddJsonParser and
17+
this.getName().matches("parse%")
18+
}
19+
}
20+
21+
/** The `JsonParser.setClassMetadataName` method. */
22+
class SetClassMetadataNameMethod extends Method {
23+
SetClassMetadataNameMethod() {
24+
this.getDeclaringType() instanceof JoddJsonParser and
25+
this.hasName("setClassMetadataName")
26+
}
27+
}
28+
29+
/** The `JsonParser.withClassMetadata` method. */
30+
class WithClassMetadataMethod extends Method {
31+
WithClassMetadataMethod() {
32+
this.getDeclaringType() instanceof JoddJsonParser and
33+
this.hasName("withClassMetadata")
34+
}
35+
}
36+
37+
/** The `JsonParser.allowClass` method. */
38+
class AllowClassMethod extends Method {
39+
AllowClassMethod() {
40+
this.getDeclaringType() instanceof JoddJsonParser and
41+
this.hasName("allowClass")
42+
}
43+
}
44+
45+
/**
46+
* A partial model of jodd.json.JsonParser noting fluent methods.
47+
*
48+
* This means that DataFlow::localFlow and similar methods are aware
49+
* that the result of (e.g.) JsonParser.allowClass is an alias of the
50+
* qualifier.
51+
*/
52+
private class JsonParserFluentMethods extends SummaryModelCsv {
53+
override predicate row(string s) {
54+
s =
55+
[
56+
"jodd.json;JsonParser;false;allowAllClasses;;;Argument[-1];ReturnValue;value",
57+
"jodd.json;JsonParser;false;allowClass;;;Argument[-1];ReturnValue;value",
58+
"jodd.json;JsonParser;false;lazy;;;Argument[-1];ReturnValue;value",
59+
"jodd.json;JsonParser;false;looseMode;;;Argument[-1];ReturnValue;value",
60+
"jodd.json;JsonParser;false;map;;;Argument[-1];ReturnValue;value",
61+
"jodd.json;JsonParser;false;setClassMetadataName;;;Argument[-1];ReturnValue;value",
62+
"jodd.json;JsonParser;false;strictTypes;;;Argument[-1];ReturnValue;value",
63+
"jodd.json;JsonParser;false;useAltPaths;;;Argument[-1];ReturnValue;value",
64+
"jodd.json;JsonParser;false;withClassMetadata;;;Argument[-1];ReturnValue;value",
65+
"jodd.json;JsonParser;false;withValueConverter;;;Argument[-1];ReturnValue;value"
66+
]
67+
}
68+
}

‎java/ql/src/semmle/code/java/security/UnsafeDeserializationQuery.qll

Lines changed: 106 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ private import semmle.code.java.frameworks.HessianBurlap
1515
private import semmle.code.java.frameworks.Castor
1616
private import semmle.code.java.frameworks.Jackson
1717
private import semmle.code.java.frameworks.Jabsorb
18+
private import semmle.code.java.frameworks.JoddJson
1819
private import semmle.code.java.frameworks.apache.Lang
1920
private import semmle.code.java.Reflection
2021

@@ -192,6 +193,16 @@ predicate unsafeDeserialization(MethodAccess ma, Expr sink) {
192193
or
193194
m instanceof JabsorbFromJsonMethod and
194195
sink = ma.getArgument(0)
196+
or
197+
m instanceof JoddJsonParseMethod and
198+
sink = ma.getArgument(0) and
199+
(
200+
// User controls the target type for deserialization
201+
any(UnsafeTypeConfig c).hasFlowToExpr(ma.getArgument(1))
202+
or
203+
// jodd.json.JsonParser may be configured for unrestricted deserialization to user-specified types
204+
joddJsonParserConfiguredUnsafely(ma.getQualifier())
205+
)
195206
)
196207
}
197208

@@ -248,6 +259,17 @@ class UnsafeDeserializationConfig extends TaintTracking::Configuration {
248259
ma.getArgument(0) = node.asExpr() and
249260
exists(SafeJsonIoConfig sji | sji.hasFlowToExpr(ma.getArgument(1)))
250261
)
262+
or
263+
exists(MethodAccess ma |
264+
// Sanitize the input to jodd.json.JsonParser.parse et al whenever it appears
265+
// to be called with an explicit class argument limiting those types that can
266+
// be instantiated during deserialization.
267+
ma.getMethod() instanceof JoddJsonParseMethod and
268+
ma.getArgument(1).getType() instanceof TypeClass and
269+
not ma.getArgument(1) instanceof NullLiteral and
270+
not ma.getArgument(1).getType().getName() = ["Class<Object>", "Class<?>"] and
271+
node.asExpr() = ma.getAnArgument()
272+
)
251273
}
252274
}
253275

@@ -295,6 +317,8 @@ class UnsafeTypeConfig extends TaintTracking2::Configuration {
295317
ma.getMethod() instanceof ObjectMapperReadMethod
296318
or
297319
ma.getMethod() instanceof JabsorbUnmarshallMethod
320+
or
321+
ma.getMethod() instanceof JoddJsonParseMethod
298322
) and
299323
// Note `JacksonTypeDescriptorType` includes plain old `java.lang.Class`
300324
arg.getType() instanceof JacksonTypeDescriptorType and
@@ -356,3 +380,85 @@ class SafeObjectMapperConfig extends DataFlow2::Configuration {
356380
)
357381
}
358382
}
383+
384+
/**
385+
* A method that configures Jodd's JsonParser, either enabling dangerous deserialization to
386+
* arbitrary Java types or restricting the types that can be instantiated.
387+
*/
388+
private class JoddJsonParserConfigurationMethodQualifier extends DataFlow::ExprNode {
389+
JoddJsonParserConfigurationMethodQualifier() {
390+
exists(MethodAccess ma, Method m | ma.getQualifier() = this.asExpr() and m = ma.getMethod() |
391+
m instanceof WithClassMetadataMethod
392+
or
393+
m instanceof SetClassMetadataNameMethod
394+
or
395+
m instanceof AllowClassMethod
396+
)
397+
}
398+
}
399+
400+
/**
401+
* Configuration tracking flow from methods that configure `jodd.json.JsonParser`'s class
402+
* instantiation feature to a `.parse` call on the same parser.
403+
*/
404+
private class JoddJsonParserConfigurationMethodConfig extends DataFlow2::Configuration {
405+
JoddJsonParserConfigurationMethodConfig() {
406+
this = "UnsafeDeserialization::JoddJsonParserConfigurationMethodConfig"
407+
}
408+
409+
override predicate isSource(DataFlow::Node src) {
410+
src instanceof JoddJsonParserConfigurationMethodQualifier
411+
}
412+
413+
override predicate isSink(DataFlow::Node sink) {
414+
exists(MethodAccess ma |
415+
ma.getMethod() instanceof JoddJsonParseMethod and
416+
sink.asExpr() = ma.getQualifier() // The class type argument
417+
)
418+
}
419+
}
420+
421+
/**
422+
* Gets the qualifier to a method call that configures a `jodd.json.JsonParser` instance unsafely.
423+
*
424+
* Such a parser may instantiate an arbtirary type when deserializing untrusted data.
425+
*/
426+
private DataFlow::Node getAnUnsafelyConfiguredParser() {
427+
exists(MethodAccess ma | result.asExpr() = ma.getQualifier() |
428+
ma.getMethod() instanceof WithClassMetadataMethod and
429+
ma.getArgument(0).(CompileTimeConstantExpr).getBooleanValue() = true
430+
or
431+
ma.getMethod() instanceof SetClassMetadataNameMethod and
432+
not ma.getArgument(0) instanceof NullLiteral
433+
)
434+
}
435+
436+
/**
437+
* Gets the qualifier to a method call that configures a `jodd.json.JsonParser` instance safely.
438+
*
439+
* Such a parser will not instantiate an arbtirary type when deserializing untrusted data.
440+
*/
441+
private DataFlow::Node getASafelyConfiguredParser() {
442+
exists(MethodAccess ma | result.asExpr() = ma.getQualifier() |
443+
ma.getMethod() instanceof WithClassMetadataMethod and
444+
ma.getArgument(0).(CompileTimeConstantExpr).getBooleanValue() = false
445+
or
446+
ma.getMethod() instanceof SetClassMetadataNameMethod and
447+
ma.getArgument(0) instanceof NullLiteral
448+
or
449+
ma.getMethod() instanceof AllowClassMethod
450+
)
451+
}
452+
453+
/**
454+
* Holds if `parseMethodQualifierExpr` is a `jodd.json.JsonParser` instance that is configured unsafely
455+
* and which never appears to be configured safely.
456+
*/
457+
private predicate joddJsonParserConfiguredUnsafely(Expr parserExpr) {
458+
exists(DataFlow::Node parser, JoddJsonParserConfigurationMethodConfig config |
459+
parser.asExpr() = parserExpr
460+
|
461+
config.hasFlow(getAnUnsafelyConfiguredParser(), parser) and
462+
not config.hasFlow(getASafelyConfiguredParser(), parser)
463+
)
464+
}

‎java/ql/test/query-tests/security/CWE-502/JoddJsonServlet.java

Lines changed: 102 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,102 @@
1+
import java.io.IOException;
2+
import java.io.Reader;
3+
4+
import javax.servlet.http.HttpServlet;
5+
import javax.servlet.http.HttpServletRequest;
6+
import javax.servlet.http.HttpServletResponse;
7+
8+
import jodd.json.JsonParser;
9+
10+
import com.example.User;
11+
import com.thirdparty.Person;
12+
13+
public class JoddJsonServlet extends HttpServlet {
14+
15+
private static final long serialVersionUID = 1L;
16+
17+
@Override
18+
// GOOD: class type specified (despite a dangerous configuration)
19+
public void doGet(HttpServletRequest req, HttpServletResponse resp) throws IOException {
20+
String json = req.getParameter("json");
21+
String clazz = req.getParameter("class");
22+
23+
JsonParser parser = new JsonParser();
24+
parser.setClassMetadataName("class");
25+
Person person = parser.parse(json, Person.class);
26+
}
27+
28+
@Override
29+
// BAD: dangerously configured parser with no class restriction passed to `parse`,
30+
// using a few different possible call sequences.
31+
public void doHead(HttpServletRequest req, HttpServletResponse resp) throws IOException {
32+
String json = req.getParameter("json");
33+
String clazz = req.getParameter("class");
34+
int callOrder;
35+
try {
36+
callOrder = Integer.parseInt(req.getParameter("callOrder"));
37+
}
38+
catch(NumberFormatException e) {
39+
throw new RuntimeException(e);
40+
}
41+
42+
JsonParser parser = new JsonParser();
43+
if(callOrder == 0) {
44+
parser.setClassMetadataName("class");
45+
User obj = parser.parse(json, null); // $unsafeDeserialization
46+
} else if(callOrder == 1) {
47+
parser.setClassMetadataName("class").parse(json, null); // $unsafeDeserialization
48+
} else if(callOrder == 2) {
49+
parser.setClassMetadataName("class").lazy(true).parse(json, null); // $unsafeDeserialization
50+
} else if(callOrder == 3) {
51+
parser.withClassMetadata(true).lazy(true).parse(json, null); // $unsafeDeserialization
52+
}
53+
}
54+
55+
@Override
56+
// BAD: allow class name to be controlled by remote source
57+
public void doPost(HttpServletRequest req, HttpServletResponse resp) throws IOException {
58+
String json = req.getParameter("json");
59+
String clazz = req.getParameter("class");
60+
61+
try {
62+
JsonParser parser = new JsonParser();
63+
Object obj = parser.parse(json, Class.forName(clazz)); // $unsafeDeserialization
64+
} catch (ClassNotFoundException cne) {
65+
throw new IOException(cne.getMessage());
66+
}
67+
}
68+
69+
@Override
70+
// GOOD: dangerously configured parser is ameliorated by setting a list of allowed classes, using various call orders,
71+
// or by explicitly disabling the class metadata option.
72+
public void doPut(HttpServletRequest req, HttpServletResponse resp) throws IOException {
73+
String json = req.getParameter("json");
74+
String clazz = req.getParameter("class");
75+
int callOrder;
76+
try {
77+
callOrder = Integer.parseInt(req.getParameter("callOrder"));
78+
}
79+
catch(NumberFormatException e) {
80+
throw new RuntimeException(e);
81+
}
82+
83+
JsonParser parser = new JsonParser();
84+
if(callOrder == 0) {
85+
parser.setClassMetadataName("class");
86+
parser.allowClass("example.Class");
87+
User obj = parser.parse(json, null);
88+
} else if(callOrder == 1) {
89+
parser.allowClass("example.Class");
90+
parser.setClassMetadataName("class");
91+
User obj = parser.parse(json, null);
92+
} else if(callOrder == 2) {
93+
parser.setClassMetadataName("class").allowClass("example.Class").parse(json, null);
94+
} else if(callOrder == 3) {
95+
parser.allowClass("example.Class").setClassMetadataName("class").parse(json, null);
96+
} else if(callOrder == 4) {
97+
parser.setClassMetadataName("class").withClassMetadata(false).parse(json, null);
98+
} else if(callOrder == 5) {
99+
parser.withClassMetadata(true).setClassMetadataName(null).parse(json, null);
100+
}
101+
}
102+
}

‎java/ql/test/query-tests/security/CWE-502/options

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1 +1 @@
1-
//semmle-extractor-options: --javac-args -cp ${testdir}/../../../stubs/snakeyaml-1.21:${testdir}/../../../stubs/xstream-1.4.10:${testdir}/../../../stubs/kryo-4.0.2:${testdir}/../../../stubs/jsr311-api-1.1.1:${testdir}/../../../stubs/fastjson-1.2.74:${testdir}/../../../stubs/springframework-5.3.8:${testdir}/../../../stubs/servlet-api-2.4:${testdir}/../../../stubs/jyaml-1.3:${testdir}/../../../stubs/json-io-4.10.0:${testdir}/../../../stubs/yamlbeans-1.09:${testdir}/../../../stubs/hessian-4.0.38:${testdir}/../../../stubs/castor-1.4.1:${testdir}/../../../stubs/jackson-databind-2.12:${testdir}/../../../stubs/jackson-core-2.12:${testdir}/../../../stubs/jabsorb-1.3.2:${testdir}/../../../stubs/json-java-20210307
1+
//semmle-extractor-options: --javac-args -cp ${testdir}/../../../stubs/snakeyaml-1.21:${testdir}/../../../stubs/xstream-1.4.10:${testdir}/../../../stubs/kryo-4.0.2:${testdir}/../../../stubs/jsr311-api-1.1.1:${testdir}/../../../stubs/fastjson-1.2.74:${testdir}/../../../stubs/springframework-5.3.8:${testdir}/../../../stubs/servlet-api-2.4:${testdir}/../../../stubs/jyaml-1.3:${testdir}/../../../stubs/json-io-4.10.0:${testdir}/../../../stubs/yamlbeans-1.09:${testdir}/../../../stubs/hessian-4.0.38:${testdir}/../../../stubs/castor-1.4.1:${testdir}/../../../stubs/jackson-databind-2.12:${testdir}/../../../stubs/jackson-core-2.12:${testdir}/../../../stubs/jabsorb-1.3.2:${testdir}/../../../stubs/json-java-20210307:${testdir}/../../../stubs/joddjson-6.0.3

0 commit comments

Comments
 (0)