Skip to content

Commit 69549e9

Browse files
smowtonsmowton
committed
Add unsafe-deserialization support for Jabsorb
This is partly extracted from github#5954
1 parent fe654dc commit 69549e9

File tree

15 files changed

+887
-31
lines changed

15 files changed

+887
-31
lines changed

‎java/change-notes/2021-08-04-jabsorb-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 `Jabsorb` library.

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

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,8 @@ 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 and Java IO serialization through <code>ObjectInputStream</code>/<code>ObjectOutputStream</code>.
18+
Jackson, Jabsorb and Java IO serialization through
19+
<code>ObjectInputStream</code>/<code>ObjectOutputStream</code>.
1920
</p>
2021
</overview>
2122

@@ -100,6 +101,10 @@ Blog posts by the developer of Jackson libraries:
100101
<a href="https://cowtowncoder.medium.com/on-jackson-cves-dont-panic-here-is-what-you-need-to-know-54cd0d6e8062">On Jackson CVEs: Don’t Panic — Here is what you need to know</a>
101102
<a href="https://cowtowncoder.medium.com/jackson-2-10-safe-default-typing-2d018f0ce2ba">Jackson 2.10: Safe Default Typing</a>
102103
</li>
104+
<li>
105+
Jabsorb documentation on deserialization:
106+
<a href="https://github.com/Servoy/jabsorb/blob/master/src/org/jabsorb/">Jabsorb JSON Serializer</a>.
107+
</li>
103108
</references>
104109

105110
</qhelp>

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

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,26 @@
1+
/**
2+
* Provides classes for working with the Jabsorb JSON-RPC ORB framework.
3+
*/
4+
5+
import java
6+
7+
/** The class `org.jabsorb.JSONSerializer`. */
8+
class JabsorbSerializer extends RefType {
9+
JabsorbSerializer() { this.hasQualifiedName("org.jabsorb", "JSONSerializer") }
10+
}
11+
12+
/** The deserialization method `unmarshall`. */
13+
class JabsorbUnmarshallMethod extends Method {
14+
JabsorbUnmarshallMethod() {
15+
this.getDeclaringType().getASupertype*() instanceof JabsorbSerializer and
16+
this.getName() = "unmarshall"
17+
}
18+
}
19+
20+
/** The deserialization method `fromJSON`. */
21+
class JabsorbFromJsonMethod extends Method {
22+
JabsorbFromJsonMethod() {
23+
this.getDeclaringType().getASupertype*() instanceof JabsorbSerializer and
24+
this.getName() = "fromJSON"
25+
}
26+
}

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

Lines changed: 0 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -77,14 +77,6 @@ class SetPolymorphicTypeValidatorSource extends DataFlow::ExprNode {
7777
}
7878
}
7979

80-
/** Holds if `fromNode` to `toNode` is a dataflow step that resolves a class. */
81-
predicate resolveClassStep(DataFlow::Node fromNode, DataFlow::Node toNode) {
82-
exists(ReflectiveClassIdentifierMethodAccess ma |
83-
ma.getArgument(0) = fromNode.asExpr() and
84-
ma = toNode.asExpr()
85-
)
86-
}
87-
8880
/**
8981
* Holds if `fromNode` to `toNode` is a dataflow step that creates a Jackson parser.
9082
*
@@ -153,22 +145,3 @@ predicate hasArgumentWithUnsafeJacksonAnnotation(MethodAccess call) {
153145
hasJsonTypeInfoAnnotation(argType.(ParameterizedType).getATypeArgument())
154146
)
155147
}
156-
157-
/**
158-
* Holds if `fromNode` to `toNode` is a dataflow step that looks like resolving a class.
159-
* A method probably resolves a class if it takes a string, returns a type descriptor,
160-
* and its name contains "resolve", "load", etc.
161-
*
162-
* Any method call that satisfies the rule above is assumed to propagate taint from its string arguments,
163-
* so methods that accept user-controlled data but sanitize it or use it for some
164-
* completely different purpose before returning a type descriptor could result in false positives.
165-
*/
166-
predicate looksLikeResolveClassStep(DataFlow::Node fromNode, DataFlow::Node toNode) {
167-
exists(MethodAccess ma, Method m, Expr arg | m = ma.getMethod() and arg = ma.getAnArgument() |
168-
m.getReturnType() instanceof JacksonTypeDescriptorType and
169-
m.getName().toLowerCase().regexpMatch("(.*)(resolve|load|class|type)(.*)") and
170-
arg.getType() instanceof TypeString and
171-
arg = fromNode.asExpr() and
172-
ma = toNode.asExpr()
173-
)
174-
}

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

Lines changed: 42 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ private import semmle.code.java.frameworks.YamlBeans
1414
private import semmle.code.java.frameworks.HessianBurlap
1515
private import semmle.code.java.frameworks.Castor
1616
private import semmle.code.java.frameworks.Jackson
17+
private import semmle.code.java.frameworks.Jabsorb
1718
private import semmle.code.java.frameworks.apache.Lang
1819
private import semmle.code.java.Reflection
1920

@@ -184,6 +185,13 @@ predicate unsafeDeserialization(MethodAccess ma, Expr sink) {
184185
hasArgumentWithUnsafeJacksonAnnotation(ma)
185186
) and
186187
not exists(SafeObjectMapperConfig config | config.hasFlowToExpr(ma.getQualifier()))
188+
or
189+
m instanceof JabsorbUnmarshallMethod and
190+
sink = ma.getArgument(2) and
191+
any(UnsafeTypeConfig config).hasFlowToExpr(ma.getArgument(1))
192+
or
193+
m instanceof JabsorbFromJsonMethod and
194+
sink = ma.getArgument(0)
187195
)
188196
}
189197

@@ -243,9 +251,36 @@ class UnsafeDeserializationConfig extends TaintTracking::Configuration {
243251
}
244252
}
245253

254+
/** Holds if `fromNode` to `toNode` is a dataflow step that resolves a class. */
255+
predicate resolveClassStep(DataFlow::Node fromNode, DataFlow::Node toNode) {
256+
exists(ReflectiveClassIdentifierMethodAccess ma |
257+
ma.getArgument(0) = fromNode.asExpr() and
258+
ma = toNode.asExpr()
259+
)
260+
}
261+
262+
/**
263+
* Holds if `fromNode` to `toNode` is a dataflow step that looks like resolving a class.
264+
* A method probably resolves a class if it takes a string, returns a type descriptor,
265+
* and its name contains "resolve", "load", etc.
266+
*
267+
* Any method call that satisfies the rule above is assumed to propagate taint from its string arguments,
268+
* so methods that accept user-controlled data but sanitize it or use it for some
269+
* completely different purpose before returning a type descriptor could result in false positives.
270+
*/
271+
predicate looksLikeResolveClassStep(DataFlow::Node fromNode, DataFlow::Node toNode) {
272+
exists(MethodAccess ma, Method m, Expr arg | m = ma.getMethod() and arg = ma.getAnArgument() |
273+
m.getReturnType() instanceof JacksonTypeDescriptorType and
274+
m.getName().toLowerCase().regexpMatch("(?i).*(resolve|load|class|type).*") and
275+
arg.getType() instanceof TypeString and
276+
arg = fromNode.asExpr() and
277+
ma = toNode.asExpr()
278+
)
279+
}
280+
246281
/**
247282
* Tracks flow from a remote source to a type descriptor (e.g. a `java.lang.Class` instance)
248-
* passed to a Jackson deserialization method.
283+
* passed to a deserialization method.
249284
*
250285
* If this is user-controlled, arbitrary code could be executed while instantiating the user-specified type.
251286
*/
@@ -256,7 +291,12 @@ class UnsafeTypeConfig extends TaintTracking2::Configuration {
256291

257292
override predicate isSink(DataFlow::Node sink) {
258293
exists(MethodAccess ma, int i, Expr arg | i > 0 and ma.getArgument(i) = arg |
259-
ma.getMethod() instanceof ObjectMapperReadMethod and
294+
(
295+
ma.getMethod() instanceof ObjectMapperReadMethod
296+
or
297+
ma.getMethod() instanceof JabsorbUnmarshallMethod
298+
) and
299+
// Note `JacksonTypeDescriptorType` includes plain old `java.lang.Class`
260300
arg.getType() instanceof JacksonTypeDescriptorType and
261301
arg = sink.asExpr()
262302
)

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

Lines changed: 121 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,121 @@
1+
import java.io.IOException;
2+
3+
import javax.servlet.http.HttpServlet;
4+
import javax.servlet.http.HttpServletRequest;
5+
import javax.servlet.http.HttpServletResponse;
6+
7+
import org.json.JSONObject;
8+
import org.jabsorb.JSONSerializer;
9+
import org.jabsorb.serializer.SerializerState;
10+
import org.jabsorb.serializer.ObjectMatch;
11+
12+
import com.example.User;
13+
import com.thirdparty.Person;
14+
15+
public class JabsorbServlet extends HttpServlet {
16+
17+
private static final long serialVersionUID = 1L;
18+
19+
@Override
20+
// GOOD: final class type specified
21+
public void doGet(HttpServletRequest req, HttpServletResponse resp) throws IOException {
22+
String json = req.getParameter("json");
23+
String clazz = req.getParameter("class");
24+
25+
try {
26+
Object jsonObject = new JSONObject(json);
27+
28+
JSONSerializer serializer = new JSONSerializer();
29+
serializer.registerDefaultSerializers();
30+
31+
serializer.setMarshallClassHints(true);
32+
serializer.setMarshallNullAttributes(true);
33+
34+
SerializerState state = new SerializerState();
35+
User user = (User) serializer.unmarshall(state, User.class, jsonObject);
36+
} catch (Exception e) {
37+
throw new IOException(e.getMessage());
38+
}
39+
}
40+
41+
// GOOD: concrete class type specified even if it has vulnerable subclasses
42+
public void doHead(HttpServletRequest req, HttpServletResponse resp) throws IOException {
43+
String json = req.getParameter("json");
44+
String clazz = req.getParameter("class");
45+
46+
try {
47+
Object jsonObject = new JSONObject(json);
48+
49+
JSONSerializer serializer = new JSONSerializer();
50+
serializer.registerDefaultSerializers();
51+
52+
serializer.setMarshallClassHints(true);
53+
serializer.setMarshallNullAttributes(true);
54+
55+
SerializerState state = new SerializerState();
56+
Person person = (Person) serializer.unmarshall(state, Person.class, jsonObject);
57+
} catch (Exception e) {
58+
throw new IOException(e.getMessage());
59+
}
60+
}
61+
62+
@Override
63+
// GOOD: try unmarshall but doesn't actually marshall the object
64+
public void doPost(HttpServletRequest req, HttpServletResponse resp) throws IOException {
65+
String json = req.getParameter("json");
66+
String clazz = req.getParameter("class");
67+
68+
try {
69+
Object jsonObject = new JSONObject(json);
70+
71+
JSONSerializer serializer = new JSONSerializer();
72+
serializer.registerDefaultSerializers();
73+
74+
serializer.setMarshallClassHints(true);
75+
serializer.setMarshallNullAttributes(true);
76+
77+
SerializerState state = new SerializerState();
78+
ObjectMatch objMatch = serializer.tryUnmarshall(state, Class.forName(clazz), jsonObject);
79+
User obj = new User();
80+
boolean result = objMatch.equals(obj);
81+
} catch (Exception e) {
82+
throw new IOException(e.getMessage());
83+
}
84+
}
85+
86+
@Override
87+
// BAD: allow class name to be controlled by remote source
88+
public void doPut(HttpServletRequest req, HttpServletResponse resp) throws IOException {
89+
String json = req.getParameter("json");
90+
String clazz = req.getParameter("class");
91+
92+
try {
93+
Object jsonObject = new JSONObject(json);
94+
95+
JSONSerializer serializer = new JSONSerializer();
96+
serializer.registerDefaultSerializers();
97+
98+
serializer.setMarshallClassHints(true);
99+
serializer.setMarshallNullAttributes(true);
100+
101+
SerializerState state = new SerializerState();
102+
User user = (User) serializer.unmarshall(state, Class.forName(clazz), jsonObject); // $unsafeDeserialization
103+
} catch (Exception e) {
104+
throw new IOException(e.getMessage());
105+
}
106+
}
107+
108+
// BAD: allow explicit class type controlled by remote source in the format of "json={\"javaClass\":\"com.thirdparty.Attacker\", ...}"
109+
public void doPut2(HttpServletRequest req, HttpServletResponse resp) throws IOException {
110+
String json = req.getParameter("json");
111+
112+
try {
113+
JSONSerializer serializer = new JSONSerializer();
114+
serializer.registerDefaultSerializers();
115+
116+
User user = (User) serializer.fromJSON(json); // $unsafeDeserialization
117+
} catch (Exception e) {
118+
throw new IOException(e.getMessage());
119+
}
120+
}
121+
}

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

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,29 @@
1+
package com.thirdparty;
2+
3+
public class Person {
4+
private int snum;
5+
private String name;
6+
7+
public Person() {
8+
}
9+
10+
public int getSnum() {
11+
return snum;
12+
}
13+
14+
public void setSnum(int snum) {
15+
this.snum = snum;
16+
}
17+
18+
public String getName() {
19+
return name;
20+
}
21+
22+
public void setName(String name) {
23+
this.name = name;
24+
}
25+
26+
public String toString() {
27+
return "Person[ name = "+name+", snum: "+snum+ "]";
28+
}
29+
}

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

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,29 @@
1+
package com.example;
2+
3+
public final class User {
4+
private String uid;
5+
private String name;
6+
7+
public User() {
8+
}
9+
10+
public String getUid() {
11+
return uid;
12+
}
13+
14+
public void setUid(String uid) {
15+
this.uid = uid;
16+
}
17+
18+
public String getName() {
19+
return name;
20+
}
21+
22+
public void setName(String name) {
23+
this.name = name;
24+
}
25+
26+
public String toString() {
27+
return "User[ name = "+name+", uid: "+uid+ "]";
28+
}
29+
}

‎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
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

0 commit comments

Comments
 (0)