Use of Externally-Controlled Input to Select Classes or Code ('Unsafe Reflection')

java/ql/src/experimental/Security/CWE/CWE-470/UnsafeReflection.java

@@ -0,0 +1,63 @@
+import java.util.HashSet;
+import javax.servlet.http.HttpServletRequest;
+import org.springframework.stereotype.Controller;
+import org.springframework.web.bind.annotation.GetMapping;
+import org.springframework.web.bind.annotation.ResponseBody;
+
+@Controller
+public class UnsafeReflection {
+
+    @GetMapping(value = "uf1")
+    public void bad1(HttpServletRequest request) {
+        String className = request.getParameter("className");
+        try {
+            Class clazz = Class.forName(className); //bad
+        } catch (ClassNotFoundException e) {
+            e.printStackTrace();
+        }
+    }
+
+    @GetMapping(value = "uf2")
+    public void bad2(HttpServletRequest request) {
+        String className = request.getParameter("className");
+        try {
+            ClassLoader classLoader = ClassLoader.getSystemClassLoader();
+            Class clazz = classLoader.loadClass(className); //bad
+        } catch (ClassNotFoundException e) {
+            e.printStackTrace();
+        }
+    }
+
+    @GetMapping(value = "uf3")
+    public void good1(HttpServletRequest request) throws Exception {
+        HashSet<String> hashSet = new HashSet<>();
+        hashSet.add("com.example.test1");
+        hashSet.add("com.example.test2");
+        String className = request.getParameter("className");
+        if (hashSet.contains(className)){ //good
+            throw new Exception("Class not valid: "  + className);
+        }
+        ClassLoader classLoader = ClassLoader.getSystemClassLoader();
+        Class clazz = classLoader.loadClass(className);
+    }
+
+    @GetMapping(value = "uf4")
+    public void good2(HttpServletRequest request) throws Exception {
+        String className = request.getParameter("className");
+        if (!"com.example.test1".equals(className)){ //good
+            throw new Exception("Class not valid: "  + className);
+        }
+        ClassLoader classLoader = ClassLoader.getSystemClassLoader();
+        Class clazz = classLoader.loadClass(className);
+    }
+
+    @GetMapping(value = "uf5")
+    public void good3(HttpServletRequest request) throws Exception {
+        String className = request.getParameter("className");
+        if (!className.equals("com.example.test1")){ //good
+            throw new Exception("Class not valid: "  + className);
+        }
+        ClassLoader classLoader = ClassLoader.getSystemClassLoader();
+        Class clazz = classLoader.loadClass(className);
+    }
+}

java/ql/src/experimental/Security/CWE/CWE-470/UnsafeReflection.qhelp

@@ -0,0 +1,43 @@
+<!DOCTYPE qhelp PUBLIC "-//Semmle//qhelp//EN" "qhelp.dtd">
+<qhelp>
+
+<overview>
+<p>
+Dynamically loaded classes could contain malicious code executed by a static class initializer. 
+I.E. you wouldn't even have to instantiate or explicitly invoke methods on such classes to be 
+vulnerable to an attack.
+</p>
+</overview>
+
+<recommendation>
+<p>
+Create a whitelist of allowed reflection classes and strictly verify the input content to ensure that 
+users can only execute classes or codes that are running.
+</p>
+</recommendation>
+
+<example>
+<p>
+The following examples show good examples and bad examples respectively. When using <code>Class.forName(...)</code> 
+or <code>ClassLoader.loadClass(...)</code> and not verifying user input, it is easy to cause security risks, for example: 
+<code>bad1</code>/<code>bad2</code>. When the user input is verified by <code>contains</code> or <code>equals</code> and then 
+the reflection operation is performed, the system security can be well controlled, for example: 
+<code >good1</code>/<code>good2</code>/<code>good3</code>.
+</p>
+<sample src="UnsafeReflection.java" />
+
+</example>
+
+<references>
+
+<li>
+Unsafe use of Reflection | OWASP:
+<a href="https://owasp.org/www-community/vulnerabilities/Unsafe_use_of_Reflection">Unsafe use of Reflection</a>.
+</li>
+<li>
+Java owasp: Classes should not be loaded dynamically:
+<a href="https://rules.sonarsource.com/java/tag/owasp/RSPEC-2658">Classes should not be loaded dynamically</a>.
+</li>
+</references>
+
+</qhelp>

java/ql/src/experimental/Security/CWE/CWE-470/UnsafeReflection.ql

@@ -0,0 +1,49 @@
+/**
+ * @name Use of Externally-Controlled Input to Select Classes or Code ('Unsafe Reflection')
+ * @description Use external input with reflection function to select the class or code to
+ *              be used, which brings serious security risks.
+ * @kind path-problem
+ * @problem.severity error
+ * @precision high
+ * @id java/unsafe-reflection
+ * @tags security
+ *       external/cwe/cwe-470
+ */
+
+import java
+import UnsafeReflectionLib
+import semmle.code.java.dataflow.FlowSources
+import DataFlow::PathGraph
+
+class ContainsSanitizer extends DataFlow::BarrierGuard {
+  ContainsSanitizer() { this.(MethodAccess).getMethod().hasName("contains") }
+
+  override predicate checks(Expr e, boolean branch) {
+    e = this.(MethodAccess).getArgument(0) and branch = false
+  }
+}
+
+class EqualsSanitizer extends DataFlow::BarrierGuard {
+  EqualsSanitizer() { this.(MethodAccess).getMethod().hasName("equals") }
+
+  override predicate checks(Expr e, boolean branch) {
+    (e = this.(MethodAccess).getArgument(0) or e = this.(MethodAccess).getQualifier()) and
+    branch = true
+  }
+}
+
+class UnsafeReflectionConfig extends TaintTracking::Configuration {
+  UnsafeReflectionConfig() { this = "UnsafeReflectionConfig" }
+
+  override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource }
+
+  override predicate isSink(DataFlow::Node sink) { sink instanceof UnsafeReflectionSink }
+
+  override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) {
+    guard instanceof ContainsSanitizer or guard instanceof EqualsSanitizer
+  }
+}
+
+from DataFlow::PathNode source, DataFlow::PathNode sink, UnsafeReflectionConfig conf
+where conf.hasFlowPath(source, sink)
+select sink.getNode(), source, sink, "Unsafe reflection of $@.", source.getNode(), "user input"

java/ql/src/experimental/Security/CWE/CWE-470/UnsafeReflectionLib.qll

@@ -0,0 +1,13 @@
+import java
+import semmle.code.java.Reflection
+import semmle.code.java.dataflow.DataFlow
+
+/**
+ * Unsafe reflection sink,
+ * e.g `Class.forName(...)` or `ClassLoader.loadClass(...)`.
+ */
+class UnsafeReflectionSink extends DataFlow::ExprNode {
+  UnsafeReflectionSink() {
+    exists(ReflectiveClassIdentifierMethodAccess rcima | rcima.getArgument(0) = this.getExpr())
+  }
+}

java/ql/test/experimental/query-tests/security/CWE-470/UnsafeReflection.expected

@@ -0,0 +1,11 @@
+edges
+| UnsafeReflection.java:12:28:12:60 | getParameter(...) : String | UnsafeReflection.java:14:41:14:49 | className |
+| UnsafeReflection.java:22:28:22:60 | getParameter(...) : String | UnsafeReflection.java:25:49:25:57 | className |
+nodes
+| UnsafeReflection.java:12:28:12:60 | getParameter(...) : String | semmle.label | getParameter(...) : String |
+| UnsafeReflection.java:14:41:14:49 | className | semmle.label | className |
+| UnsafeReflection.java:22:28:22:60 | getParameter(...) : String | semmle.label | getParameter(...) : String |
+| UnsafeReflection.java:25:49:25:57 | className | semmle.label | className |
+#select
+| UnsafeReflection.java:14:41:14:49 | className | UnsafeReflection.java:12:28:12:60 | getParameter(...) : String | UnsafeReflection.java:14:41:14:49 | className | Unsafe reflection of $@. | UnsafeReflection.java:12:28:12:60 | getParameter(...) | user input |
+| UnsafeReflection.java:25:49:25:57 | className | UnsafeReflection.java:22:28:22:60 | getParameter(...) : String | UnsafeReflection.java:25:49:25:57 | className | Unsafe reflection of $@. | UnsafeReflection.java:22:28:22:60 | getParameter(...) | user input |

java/ql/test/experimental/query-tests/security/CWE-470/UnsafeReflection.java

@@ -0,0 +1,63 @@
+import java.util.HashSet;
+import javax.servlet.http.HttpServletRequest;
+import org.springframework.stereotype.Controller;
+import org.springframework.web.bind.annotation.GetMapping;
+import org.springframework.web.bind.annotation.ResponseBody;
+
+@Controller
+public class UnsafeReflection {
+
+    @GetMapping(value = "uf1")
+    public void bad1(HttpServletRequest request) {
+        String className = request.getParameter("className");
+        try {
+            Class clazz = Class.forName(className); //bad
+        } catch (ClassNotFoundException e) {
+            e.printStackTrace();
+        }
+    }
+
+    @GetMapping(value = "uf2")
+    public void bad2(HttpServletRequest request) {
+        String className = request.getParameter("className");
+        try {
+            ClassLoader classLoader = ClassLoader.getSystemClassLoader();
+            Class clazz = classLoader.loadClass(className); //bad
+        } catch (ClassNotFoundException e) {
+            e.printStackTrace();
+        }
+    }
+
+    @GetMapping(value = "uf3")
+    public void good1(HttpServletRequest request) throws Exception {
+        HashSet<String> hashSet = new HashSet<>();
+        hashSet.add("com.example.test1");
+        hashSet.add("com.example.test2");
+        String className = request.getParameter("className");
+        if (hashSet.contains(className)){ //good
+            throw new Exception("Class not valid: "  + className);
+        }
+        ClassLoader classLoader = ClassLoader.getSystemClassLoader();
+        Class clazz = classLoader.loadClass(className);
+    }
+
+    @GetMapping(value = "uf4")
+    public void good2(HttpServletRequest request) throws Exception {
+        String className = request.getParameter("className");
+        if (!"com.example.test1".equals(className)){ //good
+            throw new Exception("Class not valid: "  + className);
+        }
+        ClassLoader classLoader = ClassLoader.getSystemClassLoader();
+        Class clazz = classLoader.loadClass(className);
+    }
+
+    @GetMapping(value = "uf5")
+    public void good3(HttpServletRequest request) throws Exception {
+        String className = request.getParameter("className");
+        if (!className.equals("com.example.test1")){ //good
+            throw new Exception("Class not valid: "  + className);
+        }
+        ClassLoader classLoader = ClassLoader.getSystemClassLoader();
+        Class clazz = classLoader.loadClass(className);
+    }
+}

java/ql/test/experimental/query-tests/security/CWE-470/UnsafeReflection.qlref

@@ -0,0 +1 @@
+experimental/Security/CWE/CWE-470/UnsafeReflection.ql

java/ql/test/experimental/query-tests/security/CWE-470/options

@@ -0,0 +1 @@
+//semmle-extractor-options: --javac-args -cp ${testdir}/../../../../stubs/servlet-api-2.4:${testdir}/../../../../stubs/spring-context-5.3.2/:${testdir}/../../../../stubs/spring-web-5.3.2/:${testdir}/../../../../stubs/spring-core-5.3.2/

java/ql/test/stubs/spring-context-5.3.2/org/springframework/stereotype/Controller.java

@@ -0,0 +1,14 @@
+package org.springframework.stereotype;
+
+import java.lang.annotation.Documented;
+import java.lang.annotation.ElementType;
+import java.lang.annotation.Retention;
+import java.lang.annotation.RetentionPolicy;
+import java.lang.annotation.Target;
+
+@Target({ElementType.TYPE})
+@Retention(RetentionPolicy.RUNTIME)
+@Documented
+public @interface Controller {
+    String value() default "";
+}

java/ql/test/stubs/spring-core-5.3.2/org/springframework/core/annotation/AliasFor.java

@@ -0,0 +1,21 @@
+package org.springframework.core.annotation;
+
+import java.lang.annotation.Annotation;
+import java.lang.annotation.Documented;
+import java.lang.annotation.ElementType;
+import java.lang.annotation.Retention;
+import java.lang.annotation.RetentionPolicy;
+import java.lang.annotation.Target;
+
+@Retention(RetentionPolicy.RUNTIME)
+@Target({ElementType.METHOD})
+@Documented
+public @interface AliasFor {
+    @AliasFor("attribute")
+    String value() default "";
+
+    @AliasFor("value")
+    String attribute() default "";
+ 
+    Class<? extends Annotation> annotation() default Annotation.class;
+}

java/ql/test/stubs/spring-core-5.3.2/org/springframework/core/io/InputStreamSource.java

@@ -0,0 +1,8 @@
+package org.springframework.core.io;
+
+import java.io.IOException;
+import java.io.InputStream;
+
+public interface InputStreamSource {
+    InputStream getInputStream() throws IOException;
+}

java/ql/test/stubs/spring-core-5.3.2/org/springframework/core/io/Resource.java

@@ -0,0 +1,46 @@
+package org.springframework.core.io;
+
+import java.io.File;
+import java.io.IOException;
+import java.net.URI;
+import java.net.URL;
+import java.nio.channels.Channels;
+import java.nio.channels.ReadableByteChannel;
+import org.springframework.lang.Nullable;
+
+public interface Resource extends InputStreamSource {
+    boolean exists();
+
+    default boolean isReadable() {
+        return this.exists();
+    }
+
+    default boolean isOpen() {
+        return false;
+    }
+
+    default boolean isFile() {
+        return false;
+    }
+
+    URL getURL() throws IOException;
+
+    URI getURI() throws IOException;
+
+    File getFile() throws IOException;
+
+    default ReadableByteChannel readableChannel() throws IOException {
+        return null;
+    }
+
+    long contentLength() throws IOException;
+
+    long lastModified() throws IOException;
+
+    Resource createRelative(String var1) throws IOException;
+
+    @Nullable
+    String getFilename();
+
+    String getDescription();
+}

java/ql/test/stubs/spring-core-5.3.2/org/springframework/lang/Nullable.java

@@ -0,0 +1,13 @@
+package org.springframework.lang;
+
+import java.lang.annotation.Documented;
+import java.lang.annotation.ElementType;
+import java.lang.annotation.Retention;
+import java.lang.annotation.RetentionPolicy;
+import java.lang.annotation.Target;
+
+@Target({ElementType.METHOD, ElementType.PARAMETER, ElementType.FIELD})
+@Retention(RetentionPolicy.RUNTIME)
+@Documented
+public @interface Nullable {
+}

java/ql/test/stubs/spring-core-5.3.2/org/springframework/util/FileCopyUtils.java

@@ -0,0 +1,53 @@
+package org.springframework.util;
+
+import java.io.ByteArrayInputStream;
+import java.io.ByteArrayOutputStream;
+import java.io.Closeable;
+import java.io.File;
+import java.io.IOException;
+import java.io.InputStream;
+import java.io.OutputStream;
+import java.io.Reader;
+import java.io.StringWriter;
+import java.io.Writer;
+import java.nio.file.Files;
+import org.springframework.lang.Nullable;
+
+public abstract class FileCopyUtils {
+    public static final int BUFFER_SIZE = 4096;
+
+    public FileCopyUtils() {
+    }
+
+    public static int copy(File in, File out) throws IOException {
+        return 1;
+    }
+
+    public static void copy(byte[] in, File out) throws IOException {}
+
+    public static byte[] copyToByteArray(File in) throws IOException {
+        return null;
+    }
+
+    public static int copy(InputStream in, OutputStream out) throws IOException {
+        return 1;
+    }
+
+    public static void copy(byte[] in, OutputStream out) throws IOException {}
+
+    public static byte[] copyToByteArray(@Nullable InputStream in) throws IOException {
+        return null;    
+    }
+
+    public static int copy(Reader in, Writer out) throws IOException {
+        return 1;
+    }
+
+    public static void copy(String in, Writer out) throws IOException {}
+
+    public static String copyToString(@Nullable Reader in) throws IOException {
+        return null;
+    }
+
+    private static void close(Closeable closeable) {}
+}

java/ql/test/stubs/spring-core-5.3.2/org/springframework/util/StringUtils.java

@@ -0,0 +1,8 @@
+package org.springframework.util;
+
+public abstract class StringUtils {
+
+    public static boolean isEmpty(Object str) {
+        return str == null || "".equals(str);
+    }
+}

java/ql/test/stubs/spring-web-5.3.2/org/springframework/web/bind/annotation/GetMapping.java

@@ -0,0 +1,51 @@
+package org.springframework.web.bind.annotation;
+
+import java.lang.annotation.Documented;
+import java.lang.annotation.ElementType;
+import java.lang.annotation.Retention;
+import java.lang.annotation.RetentionPolicy;
+import java.lang.annotation.Target;
+import org.springframework.core.annotation.AliasFor;
+
+@Target({ElementType.METHOD})
+@Retention(RetentionPolicy.RUNTIME)
+@Documented
+@RequestMapping(
+    method = {RequestMethod.GET}
+)
+public @interface GetMapping {
+    @AliasFor(
+        annotation = RequestMapping.class
+    )
+    String name() default "";
+
+    @AliasFor(
+        annotation = RequestMapping.class
+    )
+    String[] value() default {};
+
+    @AliasFor(
+        annotation = RequestMapping.class
+    )
+    String[] path() default {};
+
+    @AliasFor(
+        annotation = RequestMapping.class
+    )
+    String[] params() default {};
+
+    @AliasFor(
+        annotation = RequestMapping.class
+    )
+    String[] headers() default {};
+
+    @AliasFor(
+        annotation = RequestMapping.class
+    )
+    String[] consumes() default {};
+
+    @AliasFor(
+        annotation = RequestMapping.class
+    )
+    String[] produces() default {};
+}

java/ql/test/stubs/spring-web-5.3.2/org/springframework/web/bind/annotation/Mapping.java

@@ -0,0 +1,4 @@
+package org.springframework.web.bind.annotation;
+
+public @interface Mapping {
+}

java/ql/test/stubs/spring-web-5.3.2/org/springframework/web/bind/annotation/RequestMapping.java

@@ -0,0 +1,32 @@
+package org.springframework.web.bind.annotation;
+
+import java.lang.annotation.Documented;
+import java.lang.annotation.ElementType;
+import java.lang.annotation.Retention;
+import java.lang.annotation.RetentionPolicy;
+import java.lang.annotation.Target;
+import org.springframework.core.annotation.AliasFor;
+
+@Target({ElementType.TYPE, ElementType.METHOD})
+@Retention(RetentionPolicy.RUNTIME)
+@Documented
+@Mapping
+public @interface RequestMapping {
+    String name() default "";
+
+    @AliasFor("path")
+    String[] value() default {};
+    
+    @AliasFor("value")
+    String[] path() default {};
+
+    RequestMethod[] method() default {};
+
+    String[] params() default {};
+
+    String[] headers() default {};
+
+    String[] consumes() default {};
+
+    String[] produces() default {};
+}

java/ql/test/stubs/spring-web-5.3.2/org/springframework/web/bind/annotation/RequestMethod.java

@@ -0,0 +1,15 @@
+package org.springframework.web.bind.annotation;
+
+public enum RequestMethod {
+    GET,
+    HEAD,
+    POST,
+    PUT,
+    PATCH,
+    DELETE,
+    OPTIONS,
+    TRACE;
+
+    private RequestMethod() {
+    }
+}

java/ql/test/stubs/spring-web-5.3.2/org/springframework/web/bind/annotation/RequestParam.java

@@ -0,0 +1,23 @@
+package org.springframework.web.bind.annotation;
+
+import java.lang.annotation.Documented;
+import java.lang.annotation.ElementType;
+import java.lang.annotation.Retention;
+import java.lang.annotation.RetentionPolicy;
+import java.lang.annotation.Target;
+import org.springframework.core.annotation.AliasFor;
+
+@Target({ElementType.PARAMETER})
+@Retention(RetentionPolicy.RUNTIME)
+@Documented
+public @interface RequestParam {
+    @AliasFor("name")
+    String value() default "";
+
+    @AliasFor("value")
+    String name() default "";
+
+    boolean required() default true;
+
+    String defaultValue() default "\n\t\t\n\t\t\n\ue000\ue001\ue002\n\t\t\t\t\n";
+}

java/ql/test/stubs/spring-web-5.3.2/org/springframework/web/bind/annotation/ResponseBody.java

@@ -0,0 +1,13 @@
+package org.springframework.web.bind.annotation;
+
+import java.lang.annotation.Documented;
+import java.lang.annotation.ElementType;
+import java.lang.annotation.Retention;
+import java.lang.annotation.RetentionPolicy;
+import java.lang.annotation.Target;
+
+@Target({ElementType.TYPE, ElementType.METHOD})
+@Retention(RetentionPolicy.RUNTIME)
+@Documented
+public @interface ResponseBody {
+}

java/ql/test/stubs/spring-web-5.3.2/org/springframework/web/multipart/MultipartFile.java

@@ -0,0 +1,38 @@
+package org.springframework.web.multipart;
+
+import java.io.File;
+import java.io.IOException;
+import java.io.InputStream;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import org.springframework.core.io.InputStreamSource;
+import org.springframework.core.io.Resource;
+import org.springframework.lang.Nullable;
+import org.springframework.util.FileCopyUtils;
+
+public interface MultipartFile extends InputStreamSource {
+    String getName();
+
+    @Nullable
+    String getOriginalFilename();
+
+    @Nullable
+    String getContentType();
+
+    boolean isEmpty();
+
+    long getSize();
+
+    byte[] getBytes() throws IOException;
+
+    InputStream getInputStream() throws IOException;
+
+    default Resource getResource() {
+        return null;
+    }
+
+    void transferTo(File var1) throws IOException, IllegalStateException;
+
+    default void transferTo(Path dest) throws IOException, IllegalStateException {
+    }
+}