Merge pull request #5557 from tamasvajk/feature/java-sinks-csv

tamasvajk · web-flow · commit 4cc88662e2b0 · 2021-04-27T15:58:09.000+02:00
Java: convert sinks to CSV
diff --git a/java/ql/src/Security/CWE/CWE-022/ZipSlip.ql b/java/ql/src/Security/CWE/CWE-022/ZipSlip.ql
@@ -17,6 +17,7 @@ import semmle.code.java.dataflow.SSA
 import semmle.code.java.dataflow.TaintTracking
 import DataFlow
 import PathGraph
+private import semmle.code.java.dataflow.ExternalFlow
 
 /**
  * A method that returns the name of an archive entry.
@@ -33,34 +34,6 @@ class ArchiveEntryNameMethod extends Method {
   }
 }
 
-/**
- * An expression that will be treated as the destination of a write.
- */
-class WrittenFileName extends Expr {
-  WrittenFileName() {
-    // Constructors that write to their first argument.
-    exists(ConstructorCall ctr | this = ctr.getArgument(0) |
-      exists(Class c | ctr.getConstructor() = c.getAConstructor() |
-        c.hasQualifiedName("java.io", "FileOutputStream") or
-        c.hasQualifiedName("java.io", "RandomAccessFile") or
-        c.hasQualifiedName("java.io", "FileWriter")
-      )
-    )
-    or
-    // Methods that write to their n'th argument
-    exists(MethodAccess call, int n | this = call.getArgument(n) |
-      call.getMethod().getDeclaringType().hasQualifiedName("java.nio.file", "Files") and
-      (
-        call.getMethod().getName().regexpMatch("new.*Reader|newOutputStream|create.*") and n = 0
-        or
-        call.getMethod().hasName("copy") and n = 1
-        or
-        call.getMethod().hasName("move") and n = 1
-      )
-    )
-  }
-}
-
 /**
  * Holds if `n1` to `n2` is a dataflow step that converts between `String`,
  * `File`, and `Path`.
@@ -151,7 +124,7 @@ class ZipSlipConfiguration extends TaintTracking::Configuration {
     source.asExpr().(MethodAccess).getMethod() instanceof ArchiveEntryNameMethod
   }
 
-  override predicate isSink(Node sink) { sink.asExpr() instanceof WrittenFileName }
+  override predicate isSink(Node sink) { sink instanceof FileCreationSink }
 
   override predicate isAdditionalTaintStep(Node n1, Node n2) {
     filePathStep(n1, n2) or fileTaintStep(n1, n2)
@@ -173,6 +146,13 @@ class ZipSlipConfiguration extends TaintTracking::Configuration {
   }
 }
 
+/**
+ * A sink that represents a file creation, such as a file write, copy or move operation.
+ */
+private class FileCreationSink extends DataFlow::Node {
+  FileCreationSink() { sinkNode(this, "create-file") }
+}
+
 from PathNode source, PathNode sink
 where any(ZipSlipConfiguration c).hasFlowPath(source, sink)
 select source.getNode(), source, sink,
diff --git a/java/ql/src/Security/CWE/CWE-094/InsecureBeanValidation.ql b/java/ql/src/Security/CWE/CWE-094/InsecureBeanValidation.ql
@@ -13,6 +13,7 @@ import java
 import semmle.code.java.dataflow.TaintTracking
 import semmle.code.java.dataflow.FlowSources
 import DataFlow::PathGraph
+private import semmle.code.java.dataflow.ExternalFlow
 
 /**
  * A message interpolator Type that perform Expression Language (EL) evaluations
@@ -50,19 +51,6 @@ class SetMessageInterpolatorCall extends MethodAccess {
   predicate isSafe() { not this.getAnArgument().getType() instanceof ELMessageInterpolatorType }
 }
 
-/**
- * A method named `buildConstraintViolationWithTemplate` declared on a subtype
- * of `javax.validation.ConstraintValidatorContext`.
- */
-class BuildConstraintViolationWithTemplateMethod extends Method {
-  BuildConstraintViolationWithTemplateMethod() {
-    this.getDeclaringType()
-        .getASupertype*()
-        .hasQualifiedName("javax.validation", "ConstraintValidatorContext") and
-    this.hasName("buildConstraintViolationWithTemplate")
-  }
-}
-
 /**
  * Taint tracking BeanValidationConfiguration describing the flow of data from user input
  * to the argument of a method that builds constraint error messages.
@@ -72,12 +60,15 @@ class BeanValidationConfig extends TaintTracking::Configuration {
 
   override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource }
 
-  override predicate isSink(DataFlow::Node sink) {
-    exists(MethodAccess ma |
-      ma.getMethod() instanceof BuildConstraintViolationWithTemplateMethod and
-      sink.asExpr() = ma.getArgument(0)
-    )
-  }
+  override predicate isSink(DataFlow::Node sink) { sink instanceof BeanValidationSink }
+}
+
+/**
+ * A bean validation sink, such as method `buildConstraintViolationWithTemplate`
+ * declared on a subtype of `javax.validation.ConstraintValidatorContext`.
+ */
+private class BeanValidationSink extends DataFlow::Node {
+  BeanValidationSink() { sinkNode(this, "bean-validation") }
 }
 
 from BeanValidationConfig cfg, DataFlow::PathNode source, DataFlow::PathNode sink
diff --git a/java/ql/src/Security/CWE/CWE-297/UnsafeHostnameVerification.ql b/java/ql/src/Security/CWE/CWE-297/UnsafeHostnameVerification.ql
@@ -15,6 +15,7 @@ import semmle.code.java.dataflow.DataFlow
 import semmle.code.java.dataflow.FlowSources
 import semmle.code.java.security.Encryption
 import DataFlow::PathGraph
+private import semmle.code.java.dataflow.ExternalFlow
 
 /**
  * Holds if `m` always returns `true` ignoring any exceptional flow.
@@ -49,14 +50,7 @@ class TrustAllHostnameVerifierConfiguration extends DataFlow::Configuration {
     source.asExpr().(ClassInstanceExpr).getConstructedType() instanceof TrustAllHostnameVerifier
   }
 
-  override predicate isSink(DataFlow::Node sink) {
-    exists(MethodAccess ma, Method m |
-      (m instanceof SetDefaultHostnameVerifierMethod or m instanceof SetHostnameVerifierMethod) and
-      ma.getMethod() = m
-    |
-      ma.getArgument(0) = sink.asExpr()
-    )
-  }
+  override predicate isSink(DataFlow::Node sink) { sink instanceof HostnameVerifierSink }
 
   override predicate isBarrier(DataFlow::Node barrier) {
     // ignore nodes that are in functions that intentionally disable hostname verification
@@ -84,6 +78,13 @@ class TrustAllHostnameVerifierConfiguration extends DataFlow::Configuration {
   }
 }
 
+/**
+ * A sink that sets the `HostnameVerifier` on `HttpsURLConnection`.
+ */
+private class HostnameVerifierSink extends DataFlow::Node {
+  HostnameVerifierSink() { sinkNode(this, "set-hostname-verifier") }
+}
+
 bindingset[result]
 private string getAFlagName() {
   result
diff --git a/java/ql/src/Security/CWE/CWE-319/HttpsUrls.ql b/java/ql/src/Security/CWE/CWE-319/HttpsUrls.ql
@@ -13,9 +13,10 @@ import java
 import semmle.code.java.dataflow.TaintTracking
 import semmle.code.java.frameworks.Networking
 import DataFlow::PathGraph
+private import semmle.code.java.dataflow.ExternalFlow
 
-class HTTPString extends StringLiteral {
-  HTTPString() {
+class HttpString extends StringLiteral {
+  HttpString() {
     // Avoid matching "https" here.
     exists(string s | this.getRepresentedString() = s |
       (
@@ -30,26 +31,12 @@ class HTTPString extends StringLiteral {
   }
 }
 
-class URLOpenMethod extends Method {
-  URLOpenMethod() {
-    this.getDeclaringType().getQualifiedName() = "java.net.URL" and
-    (
-      this.getName() = "openConnection" or
-      this.getName() = "openStream"
-    )
-  }
-}
+class HttpStringToUrlOpenMethodFlowConfig extends TaintTracking::Configuration {
+  HttpStringToUrlOpenMethodFlowConfig() { this = "HttpsUrls::HttpStringToUrlOpenMethodFlowConfig" }
 
-class HTTPStringToURLOpenMethodFlowConfig extends TaintTracking::Configuration {
-  HTTPStringToURLOpenMethodFlowConfig() { this = "HttpsUrls::HTTPStringToURLOpenMethodFlowConfig" }
+  override predicate isSource(DataFlow::Node src) { src.asExpr() instanceof HttpString }
 
-  override predicate isSource(DataFlow::Node src) { src.asExpr() instanceof HTTPString }
-
-  override predicate isSink(DataFlow::Node sink) {
-    exists(MethodAccess m |
-      sink.asExpr() = m.getQualifier() and m.getMethod() instanceof URLOpenMethod
-    )
-  }
+  override predicate isSink(DataFlow::Node sink) { sink instanceof UrlOpenSink }
 
   override predicate isAdditionalTaintStep(DataFlow::Node node1, DataFlow::Node node2) {
     exists(UrlConstructorCall u |
@@ -63,10 +50,17 @@ class HTTPStringToURLOpenMethodFlowConfig extends TaintTracking::Configuration {
   }
 }
 
-from DataFlow::PathNode source, DataFlow::PathNode sink, MethodAccess m, HTTPString s
+/**
+ * A sink that represents a URL opening method call, such as a call to `java.net.URL.openConnection()`.
+ */
+private class UrlOpenSink extends DataFlow::Node {
+  UrlOpenSink() { sinkNode(this, "open-url") }
+}
+
+from DataFlow::PathNode source, DataFlow::PathNode sink, MethodAccess m, HttpString s
 where
   source.getNode().asExpr() = s and
   sink.getNode().asExpr() = m.getQualifier() and
-  any(HTTPStringToURLOpenMethodFlowConfig c).hasFlowPath(source, sink)
+  any(HttpStringToUrlOpenMethodFlowConfig c).hasFlowPath(source, sink)
 select m, source, sink, "URL may have been constructed with HTTP protocol, using $@.", s,
   "this source"
diff --git a/java/ql/src/semmle/code/java/dataflow/ExternalFlow.qll b/java/ql/src/semmle/code/java/dataflow/ExternalFlow.qll
@@ -77,6 +77,8 @@ private module Frameworks {
   private import semmle.code.java.frameworks.ApacheHttp
   private import semmle.code.java.frameworks.apache.Lang
   private import semmle.code.java.frameworks.guava.Guava
+  private import semmle.code.java.security.ResponseSplitting
+  private import semmle.code.java.security.XSS
   private import semmle.code.java.security.LdapInjection
 }
 
@@ -186,7 +188,33 @@ private predicate sourceModelCsv(string row) {
     ]
 }
 
-private predicate sinkModelCsv(string row) { none() }
+private predicate sinkModelCsv(string row) {
+  row =
+    [
+      // Open URL
+      "java.net;URL;false;openConnection;;;Argument[-1];open-url",
+      "java.net;URL;false;openStream;;;Argument[-1];open-url",
+      // Create file
+      "java.io;FileOutputStream;false;FileOutputStream;;;Argument[0];create-file",
+      "java.io;RandomAccessFile;false;RandomAccessFile;;;Argument[0];create-file",
+      "java.io;FileWriter;false;FileWriter;;;Argument[0];create-file",
+      "java.nio.file;Files;false;move;;;Argument[1];create-file",
+      "java.nio.file;Files;false;copy;;;Argument[1];create-file",
+      "java.nio.file;Files;false;newOutputStream;;;Argument[0];create-file",
+      "java.nio.file;Files;false;newBufferedReader;;;Argument[0];create-file",
+      "java.nio.file;Files;false;createDirectory;;;Argument[0];create-file",
+      "java.nio.file;Files;false;createFile;;;Argument[0];create-file",
+      "java.nio.file;Files;false;createLink;;;Argument[0];create-file",
+      "java.nio.file;Files;false;createSymbolicLink;;;Argument[0];create-file",
+      "java.nio.file;Files;false;createTempDirectory;;;Argument[0];create-file",
+      "java.nio.file;Files;false;createTempFile;;;Argument[0];create-file",
+      // Bean validation
+      "javax.validation;ConstraintValidatorContext;true;buildConstraintViolationWithTemplate;;;Argument[0];bean-validation",
+      // Set hostname
+      "javax.net.ssl;HttpsURLConnection;true;setDefaultHostnameVerifier;;;Argument[0];set-hostname-verifier",
+      "javax.net.ssl;HttpsURLConnection;true;setHostnameVerifier;;;Argument[0];set-hostname-verifier"
+    ]
+}
 
 private predicate summaryModelCsv(string row) {
   row =
diff --git a/java/ql/src/semmle/code/java/security/ResponseSplitting.qll b/java/ql/src/semmle/code/java/security/ResponseSplitting.qll
@@ -5,41 +5,32 @@ import semmle.code.java.dataflow.DataFlow
 import semmle.code.java.dataflow.FlowSources
 import semmle.code.java.frameworks.Servlets
 import semmle.code.java.frameworks.JaxWS
+private import semmle.code.java.dataflow.ExternalFlow
 
 /** A sink that is vulnerable to an HTTP header splitting attack. */
 abstract class HeaderSplittingSink extends DataFlow::Node { }
 
-/** A source that introduces data considered safe to use by a header splitting source. */
-abstract class SafeHeaderSplittingSource extends DataFlow::Node {
-  SafeHeaderSplittingSource() { this instanceof RemoteFlowSource }
+private class DefaultHeaderSplittingSink extends HeaderSplittingSink {
+  DefaultHeaderSplittingSink() { sinkNode(this, "header-splitting") }
 }
 
-/** A sink that identifies a Java Servlet or JaxWs method that is vulnerable to an HTTP header splitting attack. */
-private class ServletHeaderSplittingSink extends HeaderSplittingSink {
-  ServletHeaderSplittingSink() {
-    exists(ResponseAddCookieMethod m, MethodAccess ma |
-      ma.getMethod() = m and
-      this.asExpr() = ma.getArgument(0)
-    )
-    or
-    exists(ResponseAddHeaderMethod m, MethodAccess ma |
-      ma.getMethod() = m and
-      this.asExpr() = ma.getAnArgument()
-    )
-    or
-    exists(ResponseSetHeaderMethod m, MethodAccess ma |
-      ma.getMethod() = m and
-      this.asExpr() = ma.getAnArgument()
-    )
-    or
-    exists(JaxRsResponseBuilder builder, Method m |
-      m = builder.getAMethod() and m.getName() = "header"
-    |
-      this.asExpr() = m.getAReference().getArgument(1)
-    )
+private class HeaderSplittingSinkModel extends SinkModelCsv {
+  override predicate row(string row) {
+    row =
+      [
+        "javax.servlet.http;HttpServletResponse;false;addCookie;;;Argument[0];header-splitting",
+        "javax.servlet.http;HttpServletResponse;false;addHeader;;;Argument[0..1];header-splitting",
+        "javax.servlet.http;HttpServletResponse;false;setHeader;;;Argument[0..1];header-splitting",
+        "javax.ws.rs.core;ResponseBuilder;false;header;;;Argument[1];header-splitting"
+      ]
   }
 }
 
+/** A source that introduces data considered safe to use by a header splitting source. */
+abstract class SafeHeaderSplittingSource extends DataFlow::Node {
+  SafeHeaderSplittingSource() { this instanceof RemoteFlowSource }
+}
+
 /** A default source that introduces data considered safe to use by a header splitting source. */
 private class DefaultSafeHeaderSplittingSource extends SafeHeaderSplittingSource {
   DefaultSafeHeaderSplittingSource() {
diff --git a/java/ql/src/semmle/code/java/security/XSS.qll b/java/ql/src/semmle/code/java/security/XSS.qll
@@ -29,33 +29,30 @@ class XssAdditionalTaintStep extends Unit {
   abstract predicate step(DataFlow::Node node1, DataFlow::Node node2);
 }
 
+/** CSV sink models representing methods susceptible to XSS attacks. */
+private class DefaultXssSinkModel extends SinkModelCsv {
+  override predicate row(string row) {
+    row =
+      [
+        "javax.servlet.http;HttpServletResponse;false;sendError;(int,String);;Argument[1];xss",
+        "android.webkit;WebView;false;loadData;;;Argument[0];xss",
+        "android.webkit;WebView;false;loadUrl;;;Argument[0];xss",
+        "android.webkit;WebView;false;loadDataWithBaseURL;;;Argument[1];xss"
+      ]
+  }
+}
+
 /** A default sink representing methods susceptible to XSS attacks. */
 private class DefaultXssSink extends XssSink {
   DefaultXssSink() {
     sinkNode(this, "xss")
     or
-    exists(HttpServletResponseSendErrorMethod m, MethodAccess ma |
-      ma.getMethod() = m and
-      this.asExpr() = ma.getArgument(1)
-    )
-    or
     exists(ServletWriterSourceToWritingMethodFlowConfig writer, MethodAccess ma |
       ma.getMethod() instanceof WritingMethod and
       writer.hasFlowToExpr(ma.getQualifier()) and
       this.asExpr() = ma.getArgument(_)
     )
     or
-    exists(Method m |
-      m.getDeclaringType() instanceof TypeWebView and
-      (
-        m.getAReference().getArgument(0) = this.asExpr() and m.getName() = "loadData"
-        or
-        m.getAReference().getArgument(0) = this.asExpr() and m.getName() = "loadUrl"
-        or
-        m.getAReference().getArgument(1) = this.asExpr() and m.getName() = "loadDataWithBaseURL"
-      )
-    )
-    or
     exists(SpringRequestMappingMethod requestMappingMethod, ReturnStmt rs |
       requestMappingMethod = rs.getEnclosingCallable() and
       this.asExpr() = rs.getResult() and
