diff --git a/java/change-notes/2021-08-09-flexjson-unsafe-deserialization.md b/java/change-notes/2021-08-09-flexjson-unsafe-deserialization.md new file mode 100644 index 00000000000..0f878ac99c1 --- /dev/null +++ b/java/change-notes/2021-08-09-flexjson-unsafe-deserialization.md @@ -0,0 +1,2 @@ +lgtm,codescanning +* The "Deserialization of user-controlled data" (`java/unsafe-deserialization`) query now recognizes deserialization using the `Flexjson` library. diff --git a/java/ql/lib/semmle/code/java/dataflow/ExternalFlow.qll b/java/ql/lib/semmle/code/java/dataflow/ExternalFlow.qll index 73404a30061..034132c0e2b 100644 --- a/java/ql/lib/semmle/code/java/dataflow/ExternalFlow.qll +++ b/java/ql/lib/semmle/code/java/dataflow/ExternalFlow.qll @@ -81,6 +81,7 @@ private module Frameworks { private import semmle.code.java.frameworks.ApacheHttp private import semmle.code.java.frameworks.apache.Collections private import semmle.code.java.frameworks.apache.Lang + private import semmle.code.java.frameworks.Flexjson private import semmle.code.java.frameworks.guava.Guava private import semmle.code.java.frameworks.jackson.JacksonSerializability private import semmle.code.java.frameworks.JavaxJson diff --git a/java/ql/lib/semmle/code/java/frameworks/Flexjson.qll b/java/ql/lib/semmle/code/java/frameworks/Flexjson.qll new file mode 100644 index 00000000000..43cca2f6a2d --- /dev/null +++ b/java/ql/lib/semmle/code/java/frameworks/Flexjson.qll @@ -0,0 +1,41 @@ +/** + * Provides classes for working with the Flexjson framework. + */ + +import java +private import semmle.code.java.dataflow.ExternalFlow + +/** The class `flexjson.JSONDeserializer`. */ +class FlexjsonDeserializer extends RefType { + FlexjsonDeserializer() { this.hasQualifiedName("flexjson", "JSONDeserializer") } +} + +/** The class `flexjson.ObjectFactory`. */ +class FlexjsonObjectFactory extends RefType { + FlexjsonObjectFactory() { this.hasQualifiedName("flexjson", "ObjectFactory") } +} + +/** The deserialization method `deserialize`. */ +class FlexjsonDeserializeMethod extends Method { + FlexjsonDeserializeMethod() { + this.getDeclaringType().getSourceDeclaration().getASourceSupertype*() instanceof + FlexjsonDeserializer and + this.getName() = "deserialize" and + not this.getAParameter().getType() instanceof FlexjsonObjectFactory // deserialization method with specified class types in object factory is unlikely to be vulnerable + } +} + +/** The method `use` to configure allowed class type. */ +class FlexjsonDeserializerUseMethod extends Method { + FlexjsonDeserializerUseMethod() { + this.getDeclaringType().getSourceDeclaration().getASourceSupertype*() instanceof + FlexjsonDeserializer and + this.hasName("use") + } +} + +private class FluentUseMethodModel extends SummaryModelCsv { + override predicate row(string r) { + r = "flexjson;JSONDeserializer;false;use;;;Argument[-1];ReturnValue;value" + } +} diff --git a/java/ql/lib/semmle/code/java/security/UnsafeDeserializationQuery.qll b/java/ql/lib/semmle/code/java/security/UnsafeDeserializationQuery.qll index a45c4e8aa5d..5f38317acdb 100644 --- a/java/ql/lib/semmle/code/java/security/UnsafeDeserializationQuery.qll +++ b/java/ql/lib/semmle/code/java/security/UnsafeDeserializationQuery.qll @@ -16,6 +16,7 @@ private import semmle.code.java.frameworks.Castor private import semmle.code.java.frameworks.Jackson private import semmle.code.java.frameworks.Jabsorb private import semmle.code.java.frameworks.JoddJson +private import semmle.code.java.frameworks.Flexjson private import semmle.code.java.frameworks.apache.Lang private import semmle.code.java.Reflection @@ -203,6 +204,9 @@ predicate unsafeDeserialization(MethodAccess ma, Expr sink) { // jodd.json.JsonParser may be configured for unrestricted deserialization to user-specified types joddJsonParserConfiguredUnsafely(ma.getQualifier()) ) + or + m instanceof FlexjsonDeserializeMethod and + sink = ma.getArgument(0) ) } @@ -270,9 +274,48 @@ class UnsafeDeserializationConfig extends TaintTracking::Configuration { not ma.getArgument(1).getType().getName() = ["Class", "Class"] and node.asExpr() = ma.getAnArgument() ) + or + exists(MethodAccess ma | + // Sanitize the input to flexjson.JSONDeserializer.deserialize whenever it appears + // to be called with an explicit class argument limiting those types that can + // be instantiated during deserialization, or if the deserializer has already been + // configured to use a specified root class. + ma.getMethod() instanceof FlexjsonDeserializeMethod and + node.asExpr() = ma.getAnArgument() and + ( + ma.getArgument(1).getType() instanceof TypeClass and + not ma.getArgument(1) instanceof NullLiteral and + not ma.getArgument(1).getType().getName() = ["Class", "Class"] + or + isSafeFlexjsonDeserializer(ma.getQualifier()) + ) + ) } } +/** + * Gets a safe usage of the `use` method of Flexjson, which could be: + * use(String, ...) where the path is null or + * use(ObjectFactory, String...) where the string varargs (or array) contains null + */ +MethodAccess getASafeFlexjsonUseCall() { + result.getMethod() instanceof FlexjsonDeserializerUseMethod and + ( + result.getMethod().getParameterType(0) instanceof TypeString and + result.getArgument(0) instanceof NullLiteral + or + result.getMethod().getParameterType(0) instanceof FlexjsonObjectFactory and + exists(NullLiteral e | e = result.getAnArgument()) + ) +} + +/** + * Holds if `e` is a safely configured Flexjson `JSONDeserializer`. + */ +predicate isSafeFlexjsonDeserializer(Expr e) { + DataFlow::localExprFlow(getASafeFlexjsonUseCall().getQualifier(), e) +} + /** Holds if `fromNode` to `toNode` is a dataflow step that resolves a class. */ predicate resolveClassStep(DataFlow::Node fromNode, DataFlow::Node toNode) { exists(ReflectiveClassIdentifierMethodAccess ma | diff --git a/java/ql/src/Security/CWE/CWE-502/UnsafeDeserialization.qhelp b/java/ql/src/Security/CWE/CWE-502/UnsafeDeserialization.qhelp index 0e7cfae08d0..e3c1d8df033 100644 --- a/java/ql/src/Security/CWE/CWE-502/UnsafeDeserialization.qhelp +++ b/java/ql/src/Security/CWE/CWE-502/UnsafeDeserialization.qhelp @@ -15,7 +15,7 @@ may have unforeseen effects, such as the execution of arbitrary code.

There are many different serialization frameworks. This query currently supports Kryo, XmlDecoder, XStream, SnakeYaml, JYaml, JsonIO, YAMLBeans, HessianBurlap, Castor, Burlap, -Jackson, Jabsorb, Jodd JSON and Java IO serialization through +Jackson, Jabsorb, Jodd JSON, Flexjson and Java IO serialization through ObjectInputStream/ObjectOutputStream.

@@ -109,6 +109,10 @@ Jabsorb documentation on deserialization: Jodd JSON documentation on deserialization: JoddJson Parser. +
  • +RCE in Flexjson: +Flexjson deserialization. +
    • diff --git a/java/ql/test/query-tests/security/CWE-502/FlexjsonServlet.java b/java/ql/test/query-tests/security/CWE-502/FlexjsonServlet.java new file mode 100644 index 00000000000..c7e5c1ce587 --- /dev/null +++ b/java/ql/test/query-tests/security/CWE-502/FlexjsonServlet.java @@ -0,0 +1,126 @@ +import java.io.IOException; +import java.io.Reader; + +import javax.servlet.http.HttpServlet; +import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpServletResponse; + +import flexjson.JSONDeserializer; +import flexjson.factories.ExistingObjectFactory; + +import com.example.User; +import com.thirdparty.Person; + +public class FlexjsonServlet extends HttpServlet { + + private static final long serialVersionUID = 1L; + + @Override + // GOOD: a final class type is specified + public void doGet(HttpServletRequest req, HttpServletResponse resp) throws IOException { + JSONDeserializer deserializer = new JSONDeserializer<>(); + User user = deserializer.deserialize(req.getReader(), User.class); + } + + @Override + // GOOD: a non-null class type is specified which is not the generic `Object` type + public void doHead(HttpServletRequest req, HttpServletResponse resp) throws IOException { + JSONDeserializer deserializer = new JSONDeserializer(); + Person person = deserializer.deserialize(req.getReader(), Person.class); + } + + @Override + // BAD: allow class name to be controlled by remote source + public void doPost(HttpServletRequest req, HttpServletResponse resp) throws IOException { + JSONDeserializer deserializer = new JSONDeserializer<>(); + User user = (User) deserializer.deserialize(req.getReader()); // $unsafeDeserialization + + } + + @Override + // BAD: allow class name to be controlled by remote source + public void doTrace(HttpServletRequest req, HttpServletResponse resp) throws IOException { + JSONDeserializer deserializer = new JSONDeserializer<>(); + User user = (User) deserializer.deserialize(req.getReader()); // $unsafeDeserialization + + } + + @Override + // BAD: specify overly generic class type + public void doPut(HttpServletRequest req, HttpServletResponse resp) throws IOException { + JSONDeserializer deserializer = new JSONDeserializer(); + User user = (User) deserializer.deserialize(req.getReader(), Object.class); // $unsafeDeserialization + } + + private Person fromJsonToPerson(String json) { + return new JSONDeserializer().use(null, Person.class).deserialize(json); + } + + // GOOD: Specify the class to deserialize with `use` + public void doPut2(HttpServletRequest req, HttpServletResponse resp) throws IOException { + String json = req.getParameter("json"); + Person person = fromJsonToPerson(json); + } + + // BAD: Specify a concrete class type to `use` with `ObjectFactory` + public void doPut3(HttpServletRequest req, HttpServletResponse resp) throws IOException { + String json = req.getParameter("json"); + Person person = new JSONDeserializer().use(Person.class, new ExistingObjectFactory(new Person())).deserialize(json); // $unsafeDeserialization + } + + // GOOD: Specify a null path to `use` with a concrete class type + public void doPut4(HttpServletRequest req, HttpServletResponse resp) throws IOException { + String json = req.getParameter("json"); + Person person = new JSONDeserializer().use(null, Person.class).deserialize(json); + } + + // BAD: Specify a non-null json path to `use` with a concrete class type + public void doPut5(HttpServletRequest req, HttpServletResponse resp) throws IOException { + String json = req.getParameter("json"); + Person person = new JSONDeserializer().use("abc", Person.class).deserialize(json); // $unsafeDeserialization + } + + // GOOD: Specify a null json path to `use` with `ObjectFactory` + public void doPut6(HttpServletRequest req, HttpServletResponse resp) throws IOException { + String json = req.getParameter("json"); + Person person = new JSONDeserializer().use(new ExistingObjectFactory(new Person()), "abc", null).deserialize(json); + } + + // GOOD: Specify a concrete class type to deserialize with `ObjectFactory` + public void doPut7(HttpServletRequest req, HttpServletResponse resp) throws IOException { + String json = req.getParameter("json"); + Person person = new JSONDeserializer().deserialize(json, new ExistingObjectFactory(new Person())); + } + + // GOOD: Specify the class type to deserialize into + public void doPut8(HttpServletRequest req, HttpServletResponse resp) throws IOException { + String json = req.getParameter("json"); + Person person = new JSONDeserializer().deserializeInto(json, new Person()); + } + + // GOOD: Specify a null json path to `use` with a concrete class type, interwoven with irrelevant use directives + public void doPut9(HttpServletRequest req, HttpServletResponse resp) throws IOException { + String json = req.getParameter("json"); + Person person = new JSONDeserializer().use(Person.class, null).use(null, Person.class).use(String.class, null).deserialize(json); + } + + // GOOD: Specify a null json path to `use` with a concrete class type, interwoven with irrelevant use directives, without using fluent method chaining + public void doPut10(HttpServletRequest req, HttpServletResponse resp) throws IOException { + String json = req.getParameter("json"); + JSONDeserializer deserializer = new JSONDeserializer(); + deserializer.use(Person.class, null); + deserializer.use(null, Person.class); + deserializer.use(String.class, null); + Person person = deserializer.deserialize(json); + } + + // BAD: Specify a non-null json path to `use` with a concrete class type, interwoven with irrelevant use directives, without using fluent method chaining + public void doPut11(HttpServletRequest req, HttpServletResponse resp) throws IOException { + String json = req.getParameter("json"); + JSONDeserializer deserializer = new JSONDeserializer(); + deserializer.use(Person.class, null); + deserializer.use("someKey", Person.class); + deserializer.use(String.class, null); + Person person = deserializer.deserialize(json); // $unsafeDeserialization + } +} diff --git a/java/ql/test/query-tests/security/CWE-502/options b/java/ql/test/query-tests/security/CWE-502/options index 79476eded38..cdd0375684e 100644 --- a/java/ql/test/query-tests/security/CWE-502/options +++ b/java/ql/test/query-tests/security/CWE-502/options @@ -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 +//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 diff --git a/java/ql/test/stubs/flexjson-2.1/flexjson/JSONDeserializer.java b/java/ql/test/stubs/flexjson-2.1/flexjson/JSONDeserializer.java new file mode 100644 index 00000000000..f165b51bd49 --- /dev/null +++ b/java/ql/test/stubs/flexjson-2.1/flexjson/JSONDeserializer.java @@ -0,0 +1,121 @@ +package flexjson; + +import java.io.Reader; + +public class JSONDeserializer { + + public JSONDeserializer() { + } + + /** + * Deserialize the given json formatted input into a Java object. + * + * @param input a json formatted string. + * @return an Java instance deserialized from the json input. + */ + public T deserialize( String input ) { + return null; + } + + /** + * Same as {@link #deserialize(String)}, but uses an instance of + * java.io.Reader as json input. + * + * @param input the stream where the json input is coming from. + * @return an Java instance deserialized from the java.io.Reader's input. + */ + public T deserialize( Reader input ) { + return null; + } + + /** + * Deserialize the given json input, and use the given Class as + * the type of the initial object to deserialize into. This object + * must implement a no-arg constructor. + * + * @param input a json formatted string. + * @param root a Class used to create the initial object. + * @return the object created from the given json input. + */ + public T deserialize( String input, Class root ) { + return null; + } + + /** + * Same as {@link #deserialize(java.io.Reader, Class)}, but uses an instance of + * java.io.Reader as json input. + * + * @param input the stream where the json input is coming from. + * @param root a Class used to create the initial object. + * @return an Java instance deserialized from the java.io.Reader's input. + */ + public T deserialize( Reader input, Class root ) { + return null; + } + + /** + * Deserialize the given json input, and use the given ObjectFactory to + * create the initial object to deserialize into. + * + * @param input a json formatted string. + * @param factory an ObjectFactory used to create the initial object. + * @return the object created from the given json input. + */ + public T deserialize( String input, ObjectFactory factory ) { + return null; + } + + /** + * Same as {@link #deserialize(String, ObjectFactory)}, but uses an instance of + * java.io.Reader as json input. + * + * @param input the stream where the json input is coming from. + * @param factory an ObjectFactory used to create the initial object. + * @return an Java instance deserialized from the java.io.Reader's input. + */ + public T deserialize( Reader input, ObjectFactory factory ) { + return null; + } + + /** + * Deserialize the given input into the existing object target. + * Values in the json input will overwrite values in the + * target object. This means if a value is included in json + * a new object will be created and set into the existing object. + * + * @param input a json formatted string. + * @param target an instance to set values into from the json string. + * @return will return a reference to target. + */ + public T deserializeInto( String input, T target ) { + return null; + } + + /** + * Same as {@link #deserializeInto(String, Object)}, but uses an instance of + * java.io.Reader as json input. + * + * @param input the stream where the json input is coming from. + * @param target an instance to set values into from the json string. + * @return will return a reference to target. + */ + public T deserializeInto( Reader input, T target ) { + return null; + } + + public JSONDeserializer use( String path, Class clazz ) { + return null; + } + + public JSONDeserializer use( Class clazz, ObjectFactory factory ) { + return null; + } + + public JSONDeserializer use( String path, ObjectFactory factory ) { + return null; + } + + public JSONDeserializer use(ObjectFactory factory, String... paths) { + return null; + } +} diff --git a/java/ql/test/stubs/flexjson-2.1/flexjson/ObjectBinder.java b/java/ql/test/stubs/flexjson-2.1/flexjson/ObjectBinder.java new file mode 100644 index 00000000000..efb648971dc --- /dev/null +++ b/java/ql/test/stubs/flexjson-2.1/flexjson/ObjectBinder.java @@ -0,0 +1,40 @@ +package flexjson; + +import java.lang.reflect.Type; + +import java.util.Map; +import java.util.Collection; + +public class ObjectBinder { + + public ObjectBinder() { + } + + public ObjectBinder use(Class clazz, ObjectFactory factory) { + return null; + } + + public Object bind( Object input ) { + return null; + } + + public Object bind( Object source, Object target ) { + return null; + } + + public Object bind( Object input, Type targetType ) { + return null; + } + + public > T bindIntoCollection(Collection value, T target, Type targetType) { + return null; + } + + public Object bindIntoMap(Map input, Map result, Type keyType, Type valueType) { + return null; + } + + public Object bindIntoObject(Map jsonOwner, Object target, Type targetType) { + return null; + } +} diff --git a/java/ql/test/stubs/flexjson-2.1/flexjson/ObjectFactory.java b/java/ql/test/stubs/flexjson-2.1/flexjson/ObjectFactory.java new file mode 100644 index 00000000000..87989d29947 --- /dev/null +++ b/java/ql/test/stubs/flexjson-2.1/flexjson/ObjectFactory.java @@ -0,0 +1,26 @@ +package flexjson; + +import java.lang.reflect.Type; + +/** + * ObjectFactory allows you to instantiate specific types of objects on path or class types. This interface allows + * you to override the default rules. + */ +public interface ObjectFactory { + /** + * This method is called by the deserializer to construct and bind an object. At the end of this method + * the object should be fully constructed. {@link flexjson.ObjectBinder} can be used to bind values into + * the object according to default rules. For simple implementations you won't need to use this, but + * for more complex or generic objects reusing methods like {@link flexjson.ObjectBinder#bind(Object, java.lang.reflect.Type)} + * and {@link flexjson.ObjectBinder#bindIntoCollection(java.util.Collection, java.util.Collection, java.lang.reflect.Type)}. + * + * @param context the object binding context to keep track of where we are in the object graph + * and used for binding into objects. + * @param value This is the value from the json object at the current path. + * @param targetType This is the type pulled from the object introspector. Used for Collections and generic types. + * @param targetClass concrete class pulled from the configuration of the deserializer. + * + * @return the fully bound object. At the end of this method the object should be fully constructed. + */ + public Object instantiate(ObjectBinder context, Object value, Type targetType, Class targetClass); +} diff --git a/java/ql/test/stubs/flexjson-2.1/flexjson/factories/ExistingObjectFactory.java b/java/ql/test/stubs/flexjson-2.1/flexjson/factories/ExistingObjectFactory.java new file mode 100644 index 00000000000..eab94681557 --- /dev/null +++ b/java/ql/test/stubs/flexjson-2.1/flexjson/factories/ExistingObjectFactory.java @@ -0,0 +1,17 @@ +package flexjson.factories; + +import flexjson.ObjectBinder; +import flexjson.ObjectFactory; + +import java.lang.reflect.Type; + +public class ExistingObjectFactory implements ObjectFactory { + + public ExistingObjectFactory(Object source) { + } + + @Override + public Object instantiate(ObjectBinder context, Object value, Type targetType, Class targetClass) { + return null; + } +} \ No newline at end of file