Skip to content

Commit 26eafcb

Browse files
authored
Merge pull request #6456 from smowton/smowton/admin/flexjson-unsafe-deserialization
Java: add unsafe-deserialization support for Flexjson
2 parents 8b7fad8 + d83ed33 commit 26eafcb

File tree

11 files changed

+423
-2
lines changed

11 files changed

+423
-2
lines changed
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 `Flexjson` library.

java/ql/lib/semmle/code/java/dataflow/ExternalFlow.qll

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -81,6 +81,7 @@ private module Frameworks {
8181
private import semmle.code.java.frameworks.ApacheHttp
8282
private import semmle.code.java.frameworks.apache.Collections
8383
private import semmle.code.java.frameworks.apache.Lang
84+
private import semmle.code.java.frameworks.Flexjson
8485
private import semmle.code.java.frameworks.guava.Guava
8586
private import semmle.code.java.frameworks.jackson.JacksonSerializability
8687
private import semmle.code.java.frameworks.JavaxJson
Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,41 @@
1+
/**
2+
* Provides classes for working with the Flexjson framework.
3+
*/
4+
5+
import java
6+
private import semmle.code.java.dataflow.ExternalFlow
7+
8+
/** The class `flexjson.JSONDeserializer`. */
9+
class FlexjsonDeserializer extends RefType {
10+
FlexjsonDeserializer() { this.hasQualifiedName("flexjson", "JSONDeserializer") }
11+
}
12+
13+
/** The class `flexjson.ObjectFactory`. */
14+
class FlexjsonObjectFactory extends RefType {
15+
FlexjsonObjectFactory() { this.hasQualifiedName("flexjson", "ObjectFactory") }
16+
}
17+
18+
/** The deserialization method `deserialize`. */
19+
class FlexjsonDeserializeMethod extends Method {
20+
FlexjsonDeserializeMethod() {
21+
this.getDeclaringType().getSourceDeclaration().getASourceSupertype*() instanceof
22+
FlexjsonDeserializer and
23+
this.getName() = "deserialize" and
24+
not this.getAParameter().getType() instanceof FlexjsonObjectFactory // deserialization method with specified class types in object factory is unlikely to be vulnerable
25+
}
26+
}
27+
28+
/** The method `use` to configure allowed class type. */
29+
class FlexjsonDeserializerUseMethod extends Method {
30+
FlexjsonDeserializerUseMethod() {
31+
this.getDeclaringType().getSourceDeclaration().getASourceSupertype*() instanceof
32+
FlexjsonDeserializer and
33+
this.hasName("use")
34+
}
35+
}
36+
37+
private class FluentUseMethodModel extends SummaryModelCsv {
38+
override predicate row(string r) {
39+
r = "flexjson;JSONDeserializer;true;use;;;Argument[-1];ReturnValue;value"
40+
}
41+
}

java/ql/lib/semmle/code/java/security/UnsafeDeserializationQuery.qll

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ private import semmle.code.java.frameworks.Castor
1616
private import semmle.code.java.frameworks.Jackson
1717
private import semmle.code.java.frameworks.Jabsorb
1818
private import semmle.code.java.frameworks.JoddJson
19+
private import semmle.code.java.frameworks.Flexjson
1920
private import semmle.code.java.frameworks.apache.Lang
2021
private import semmle.code.java.Reflection
2122

@@ -203,6 +204,9 @@ predicate unsafeDeserialization(MethodAccess ma, Expr sink) {
203204
// jodd.json.JsonParser may be configured for unrestricted deserialization to user-specified types
204205
joddJsonParserConfiguredUnsafely(ma.getQualifier())
205206
)
207+
or
208+
m instanceof FlexjsonDeserializeMethod and
209+
sink = ma.getArgument(0)
206210
)
207211
}
208212

@@ -270,9 +274,48 @@ class UnsafeDeserializationConfig extends TaintTracking::Configuration {
270274
not ma.getArgument(1).getType().getName() = ["Class<Object>", "Class<?>"] and
271275
node.asExpr() = ma.getAnArgument()
272276
)
277+
or
278+
exists(MethodAccess ma |
279+
// Sanitize the input to flexjson.JSONDeserializer.deserialize whenever it appears
280+
// to be called with an explicit class argument limiting those types that can
281+
// be instantiated during deserialization, or if the deserializer has already been
282+
// configured to use a specified root class.
283+
ma.getMethod() instanceof FlexjsonDeserializeMethod and
284+
node.asExpr() = ma.getAnArgument() and
285+
(
286+
ma.getArgument(1).getType() instanceof TypeClass and
287+
not ma.getArgument(1) instanceof NullLiteral and
288+
not ma.getArgument(1).getType().getName() = ["Class<Object>", "Class<?>"]
289+
or
290+
isSafeFlexjsonDeserializer(ma.getQualifier())
291+
)
292+
)
273293
}
274294
}
275295

296+
/**
297+
* Gets a safe usage of the `use` method of Flexjson, which could be:
298+
* use(String, ...) where the path is null or
299+
* use(ObjectFactory, String...) where the string varargs (or array) contains null
300+
*/
301+
MethodAccess getASafeFlexjsonUseCall() {
302+
result.getMethod() instanceof FlexjsonDeserializerUseMethod and
303+
(
304+
result.getMethod().getParameterType(0) instanceof TypeString and
305+
result.getArgument(0) instanceof NullLiteral
306+
or
307+
result.getMethod().getParameterType(0) instanceof FlexjsonObjectFactory and
308+
exists(NullLiteral e | e = result.getAnArgument())
309+
)
310+
}
311+
312+
/**
313+
* Holds if `e` is a safely configured Flexjson `JSONDeserializer`.
314+
*/
315+
predicate isSafeFlexjsonDeserializer(Expr e) {
316+
DataFlow::localExprFlow(getASafeFlexjsonUseCall().getQualifier(), e)
317+
}
318+
276319
/** Holds if `fromNode` to `toNode` is a dataflow step that resolves a class. */
277320
predicate resolveClassStep(DataFlow::Node fromNode, DataFlow::Node toNode) {
278321
exists(ReflectiveClassIdentifierMethodAccess ma |

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, Jodd JSON and Java IO serialization through
18+
Jackson, Jabsorb, Jodd JSON, Flexjson and Java IO serialization through
1919
<code>ObjectInputStream</code>/<code>ObjectOutputStream</code>.
2020
</p>
2121
</overview>
@@ -109,6 +109,10 @@ Jabsorb documentation on deserialization:
109109
Jodd JSON documentation on deserialization:
110110
<a href="https://json.jodd.org/parser">JoddJson Parser</a>.
111111
</li>
112+
<li>
113+
RCE in Flexjson:
114+
<a href="https://codewhitesec.blogspot.com/2020/03/liferay-portal-json-vulns.html">Flexjson deserialization</a>.
115+
</li>
112116
</references>
113117

114118
</qhelp>
Lines changed: 126 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,126 @@
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 flexjson.JSONDeserializer;
9+
import flexjson.factories.ExistingObjectFactory;
10+
11+
import com.example.User;
12+
import com.thirdparty.Person;
13+
14+
public class FlexjsonServlet extends HttpServlet {
15+
16+
private static final long serialVersionUID = 1L;
17+
18+
@Override
19+
// GOOD: a final class type is specified
20+
public void doGet(HttpServletRequest req, HttpServletResponse resp) throws IOException {
21+
JSONDeserializer<User> deserializer = new JSONDeserializer<>();
22+
User user = deserializer.deserialize(req.getReader(), User.class);
23+
}
24+
25+
@Override
26+
// GOOD: a non-null class type is specified which is not the generic `Object` type
27+
public void doHead(HttpServletRequest req, HttpServletResponse resp) throws IOException {
28+
JSONDeserializer<Person> deserializer = new JSONDeserializer<Person>();
29+
Person person = deserializer.deserialize(req.getReader(), Person.class);
30+
}
31+
32+
@Override
33+
// BAD: allow class name to be controlled by remote source
34+
public void doPost(HttpServletRequest req, HttpServletResponse resp) throws IOException {
35+
JSONDeserializer<User> deserializer = new JSONDeserializer<>();
36+
User user = (User) deserializer.deserialize(req.getReader()); // $unsafeDeserialization
37+
38+
}
39+
40+
@Override
41+
// BAD: allow class name to be controlled by remote source
42+
public void doTrace(HttpServletRequest req, HttpServletResponse resp) throws IOException {
43+
JSONDeserializer deserializer = new JSONDeserializer<>();
44+
User user = (User) deserializer.deserialize(req.getReader()); // $unsafeDeserialization
45+
46+
}
47+
48+
@Override
49+
// BAD: specify overly generic class type
50+
public void doPut(HttpServletRequest req, HttpServletResponse resp) throws IOException {
51+
JSONDeserializer deserializer = new JSONDeserializer();
52+
User user = (User) deserializer.deserialize(req.getReader(), Object.class); // $unsafeDeserialization
53+
}
54+
55+
private Person fromJsonToPerson(String json) {
56+
return new JSONDeserializer<Person>().use(null, Person.class).deserialize(json);
57+
}
58+
59+
// GOOD: Specify the class to deserialize with `use`
60+
public void doPut2(HttpServletRequest req, HttpServletResponse resp) throws IOException {
61+
String json = req.getParameter("json");
62+
Person person = fromJsonToPerson(json);
63+
}
64+
65+
// BAD: Specify a concrete class type to `use` with `ObjectFactory`
66+
public void doPut3(HttpServletRequest req, HttpServletResponse resp) throws IOException {
67+
String json = req.getParameter("json");
68+
Person person = new JSONDeserializer<Person>().use(Person.class, new ExistingObjectFactory(new Person())).deserialize(json); // $unsafeDeserialization
69+
}
70+
71+
// GOOD: Specify a null path to `use` with a concrete class type
72+
public void doPut4(HttpServletRequest req, HttpServletResponse resp) throws IOException {
73+
String json = req.getParameter("json");
74+
Person person = new JSONDeserializer<Person>().use(null, Person.class).deserialize(json);
75+
}
76+
77+
// BAD: Specify a non-null json path to `use` with a concrete class type
78+
public void doPut5(HttpServletRequest req, HttpServletResponse resp) throws IOException {
79+
String json = req.getParameter("json");
80+
Person person = new JSONDeserializer<Person>().use("abc", Person.class).deserialize(json); // $unsafeDeserialization
81+
}
82+
83+
// GOOD: Specify a null json path to `use` with `ObjectFactory`
84+
public void doPut6(HttpServletRequest req, HttpServletResponse resp) throws IOException {
85+
String json = req.getParameter("json");
86+
Person person = new JSONDeserializer<Person>().use(new ExistingObjectFactory(new Person()), "abc", null).deserialize(json);
87+
}
88+
89+
// GOOD: Specify a concrete class type to deserialize with `ObjectFactory`
90+
public void doPut7(HttpServletRequest req, HttpServletResponse resp) throws IOException {
91+
String json = req.getParameter("json");
92+
Person person = new JSONDeserializer<Person>().deserialize(json, new ExistingObjectFactory(new Person()));
93+
}
94+
95+
// GOOD: Specify the class type to deserialize into
96+
public void doPut8(HttpServletRequest req, HttpServletResponse resp) throws IOException {
97+
String json = req.getParameter("json");
98+
Person person = new JSONDeserializer<Person>().deserializeInto(json, new Person());
99+
}
100+
101+
// GOOD: Specify a null json path to `use` with a concrete class type, interwoven with irrelevant use directives
102+
public void doPut9(HttpServletRequest req, HttpServletResponse resp) throws IOException {
103+
String json = req.getParameter("json");
104+
Person person = new JSONDeserializer<Person>().use(Person.class, null).use(null, Person.class).use(String.class, null).deserialize(json);
105+
}
106+
107+
// GOOD: Specify a null json path to `use` with a concrete class type, interwoven with irrelevant use directives, without using fluent method chaining
108+
public void doPut10(HttpServletRequest req, HttpServletResponse resp) throws IOException {
109+
String json = req.getParameter("json");
110+
JSONDeserializer<Person> deserializer = new JSONDeserializer<Person>();
111+
deserializer.use(Person.class, null);
112+
deserializer.use(null, Person.class);
113+
deserializer.use(String.class, null);
114+
Person person = deserializer.deserialize(json);
115+
}
116+
117+
// BAD: Specify a non-null json path to `use` with a concrete class type, interwoven with irrelevant use directives, without using fluent method chaining
118+
public void doPut11(HttpServletRequest req, HttpServletResponse resp) throws IOException {
119+
String json = req.getParameter("json");
120+
JSONDeserializer<Person> deserializer = new JSONDeserializer<Person>();
121+
deserializer.use(Person.class, null);
122+
deserializer.use("someKey", Person.class);
123+
deserializer.use(String.class, null);
124+
Person person = deserializer.deserialize(json); // $unsafeDeserialization
125+
}
126+
}
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:${testdir}/../../../stubs/joddjson-6.0.3
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:${testdir}/../../../stubs/flexjson-2.1

java/ql/test/stubs/flexjson-2.1/flexjson/JSONDeserializer.java

Lines changed: 121 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

0 commit comments

Comments
 (0)