Merge branch 'main' into atorralba/java/command-injection-mad-sinks

2026-03-04 22:56:47 +01:00 · 2023-06-02 08:57:24 +02:00
parent a276cc3094 84a7b3ca52
commit c3b1ef2cdf
652 changed files with 20081 additions and 5057 deletions
--- a/java/ql/lib/semmle/code/java/Serializability.qll
+++ b/java/ql/lib/semmle/code/java/Serializability.qll
@@ -4,6 +4,7 @@

 import java
 private import frameworks.jackson.JacksonSerializability
+private import frameworks.google.GsonSerializability
 private import frameworks.google.GoogleHttpClientApi

 /**
--- a/java/ql/lib/semmle/code/java/dataflow/ExternalFlow.qll
+++ b/java/ql/lib/semmle/code/java/dataflow/ExternalFlow.qll
@@ -274,11 +274,12 @@ module ModelValidation {
    exists(string kind | sinkModel(_, _, _, _, _, _, _, kind, _) |
      not kind =
        [
-          "open-url", "jndi-injection", "ldap", "sql", "jdbc-url", "logging", "mvel", "xpath",
-          "groovy", "xss", "ognl-injection", "intent-start", "pending-intent-sent", "url-redirect",
-          "create-file", "read-file", "write-file", "set-hostname-verifier", "header-splitting",
-          "information-leak", "xslt", "jexl", "bean-validation", "ssti", "fragment-injection",
-          "command-injection"
+          "request-forgery", "jndi-injection", "ldap-injection", "sql-injection", "log-injection",
+          "mvel-injection", "xpath-injection", "groovy-injection", "html-injection", "js-injection",
+          "ognl-injection", "intent-redirection", "pending-intents", "url-redirection",
+          "path-injection", "file-content-store", "hostname-verification", "response-splitting",
+          "information-leak", "xslt-injection", "jexl-injection", "bean-validation",
+          "template-injection", "fragment-injection", "command-injection"
        ] and
      not kind.matches("regex-use%") and
      not kind.matches("qltest%") and
@@ -286,7 +287,7 @@ module ModelValidation {
    )
    or
    exists(string kind | sourceModel(_, _, _, _, _, _, _, kind, _) |
-      not kind = ["remote", "contentprovider", "android-widget", "android-external-storage-dir"] and
+      not kind = ["remote", "contentprovider", "android-external-storage-dir"] and
      not kind.matches("qltest%") and
      result = "Invalid kind \"" + kind + "\" in source model."
    )
--- a/java/ql/lib/semmle/code/java/dataflow/FlowSources.qll
+++ b/java/ql/lib/semmle/code/java/dataflow/FlowSources.qll
@@ -36,13 +36,6 @@ abstract class RemoteFlowSource extends DataFlow::Node {
  abstract string getSourceType();
 }

-/**
- * A module for importing frameworks that define remote flow sources.
- */
-private module RemoteFlowSources {
-  private import semmle.code.java.frameworks.android.Widget
-}
-
 private class ExternalRemoteFlowSource extends RemoteFlowSource {
  ExternalRemoteFlowSource() { sourceNode(this, "remote") }

--- a/java/ql/lib/semmle/code/java/dataflow/internal/FlowSummaryImpl.qll
+++ b/java/ql/lib/semmle/code/java/dataflow/internal/FlowSummaryImpl.qll
@@ -166,28 +166,21 @@ module Public {
    SummaryComponentStack return(ReturnKind rk) { result = singleton(SummaryComponent::return(rk)) }
  }

-  private predicate noComponentSpecific(SummaryComponent sc) {
-    not exists(getComponentSpecific(sc))
-  }
-
  /** Gets a textual representation of this component used for flow summaries. */
  private string getComponent(SummaryComponent sc) {
    result = getComponentSpecific(sc)
    or
-    noComponentSpecific(sc) and
-    (
-      exists(ArgumentPosition pos |
-        sc = TParameterSummaryComponent(pos) and
-        result = "Parameter[" + getArgumentPosition(pos) + "]"
-      )
-      or
-      exists(ParameterPosition pos |
-        sc = TArgumentSummaryComponent(pos) and
-        result = "Argument[" + getParameterPosition(pos) + "]"
-      )
-      or
-      sc = TReturnSummaryComponent(getReturnValueKind()) and result = "ReturnValue"
+    exists(ArgumentPosition pos |
+      sc = TParameterSummaryComponent(pos) and
+      result = "Parameter[" + getArgumentPosition(pos) + "]"
    )
+    or
+    exists(ParameterPosition pos |
+      sc = TArgumentSummaryComponent(pos) and
+      result = "Argument[" + getParameterPosition(pos) + "]"
+    )
+    or
+    sc = TReturnSummaryComponent(getReturnValueKind()) and result = "ReturnValue"
  }

  /** Gets a textual representation of this stack used for flow summaries. */
--- a/java/ql/lib/semmle/code/java/frameworks/android/Widget.qll
+++ b/java/ql/lib/semmle/code/java/frameworks/android/Widget.qll
@@ -4,12 +4,6 @@ import java
 private import semmle.code.java.dataflow.ExternalFlow
 private import semmle.code.java.dataflow.FlowSources

-private class DefaultAndroidWidgetSources extends RemoteFlowSource {
-  DefaultAndroidWidgetSources() { sourceNode(this, "android-widget") }
-
-  override string getSourceType() { result = "Android widget source" }
-}
-
 private class EditableToStringStep extends AdditionalTaintStep {
  override predicate step(DataFlow::Node n1, DataFlow::Node n2) {
    exists(MethodAccess ma |
--- a/java/ql/lib/semmle/code/java/frameworks/google/GsonSerializability.qll
+++ b/java/ql/lib/semmle/code/java/frameworks/google/GsonSerializability.qll
@@ -0,0 +1,60 @@
+/**
+ * Provides classes and predicates for working with Java Serialization in the context of
+ * the `com.google.gson` JSON processing framework.
+ */
+
+import java
+private import semmle.code.java.Serializability
+private import semmle.code.java.dataflow.DataFlow
+private import semmle.code.java.dataflow.FlowSteps
+
+/**
+ * A method used for deserializing objects using Gson. The first parameter is the object to be
+ * deserialized.
+ */
+private class GsonReadValueMethod extends Method {
+  GsonReadValueMethod() { this.hasQualifiedName("com.google.gson", "Gson", "fromJson") }
+}
+
+/** A type whose values may be deserialized by the Gson JSON framework. */
+abstract class GsonDeserializableType extends Type { }
+
+/** A type whose values are explicitly deserialized in a call to a Gson method. */
+private class ExplicitlyReadGsonDeserializableType extends GsonDeserializableType {
+  ExplicitlyReadGsonDeserializableType() {
+    exists(MethodAccess ma |
+      // A call to a Gson read method...
+      ma.getMethod() instanceof GsonReadValueMethod and
+      // ...where `this` is used in the final argument, indicating that this type will be deserialized.
+      // TODO: find a way to get the type represented by java.lang.reflect.Type and com.google.gson.reflect.TypeToken
+      // fromJson(String json, TypeToken<T> typeOfT)
+      // fromJson(String json, Type typeOfT)
+      usesType(ma.getArgument(1).getType(), this) and
+      not this instanceof TypeClass and
+      not this instanceof TypeObject
+    )
+  }
+}
+
+/** A type used in a `GsonDeserializableField` declaration. */
+private class FieldReferencedGsonDeserializableType extends GsonDeserializableType {
+  FieldReferencedGsonDeserializableType() {
+    exists(GsonDeserializableField f | usesType(f.getType(), this))
+  }
+}
+
+/** A field that may be deserialized using the Gson JSON framework. */
+private class GsonDeserializableField extends DeserializableField {
+  pragma[assume_small_delta]
+  GsonDeserializableField() {
+    exists(GsonDeserializableType superType |
+      superType = this.getDeclaringType().getAnAncestor() and
+      not superType instanceof TypeObject and
+      superType.fromSource()
+    )
+  }
+}
+
+private class GsonInheritTaint extends DataFlow::FieldContent, TaintInheritingContent {
+  GsonInheritTaint() { this.getField() instanceof GsonDeserializableField }
+}
--- a/java/ql/lib/semmle/code/java/security/AndroidIntentRedirection.qll
+++ b/java/ql/lib/semmle/code/java/security/AndroidIntentRedirection.qll
@@ -30,7 +30,7 @@ class IntentRedirectionAdditionalTaintStep extends Unit {

 /** Default sink for Intent redirection vulnerabilities. */
 private class DefaultIntentRedirectionSink extends IntentRedirectionSink {
-  DefaultIntentRedirectionSink() { sinkNode(this, "intent-start") }
+  DefaultIntentRedirectionSink() { sinkNode(this, "intent-redirection") }
 }

 /**
--- a/java/ql/lib/semmle/code/java/security/CleartextStorageAndroidFilesystemQuery.qll
+++ b/java/ql/lib/semmle/code/java/security/CleartextStorageAndroidFilesystemQuery.qll
@@ -20,7 +20,7 @@ private class AndroidFilesystemCleartextStorageSink extends CleartextStorageSink
 /** A call to a method or constructor that may write to files to the local filesystem. */
 class LocalFileOpenCall extends Storable {
  LocalFileOpenCall() {
-    this = any(DataFlow::Node sink | sinkNode(sink, "create-file")).asExpr().(Argument).getCall()
+    this = any(DataFlow::Node sink | sinkNode(sink, "path-injection")).asExpr().(Argument).getCall()
  }

  override Expr getAnInput() {
@@ -40,7 +40,7 @@ class LocalFileOpenCall extends Storable {

 /** Holds if `input` is written into `file`. */
 private predicate filesystemInput(DataFlow::Node file, Argument input) {
-  exists(DataFlow::Node write | sinkNode(write, "write-file") |
+  exists(DataFlow::Node write | sinkNode(write, "file-content-store") |
    input = write.asExpr() or
    isVarargs(input, write)
  ) and
--- a/java/ql/lib/semmle/code/java/security/GroovyInjection.qll
+++ b/java/ql/lib/semmle/code/java/security/GroovyInjection.qll
@@ -21,7 +21,7 @@ class GroovyInjectionAdditionalTaintStep extends Unit {
 }

 private class DefaultGroovyInjectionSink extends GroovyInjectionSink {
-  DefaultGroovyInjectionSink() { sinkNode(this, "groovy") }
+  DefaultGroovyInjectionSink() { sinkNode(this, "groovy-injection") }
 }

 /** A set of additional taint steps to consider when taint tracking Groovy related data flows. */
--- a/java/ql/lib/semmle/code/java/security/HttpsUrls.qll
+++ b/java/ql/lib/semmle/code/java/security/HttpsUrls.qll
@@ -30,7 +30,8 @@ class HttpStringLiteral extends StringLiteral {
 abstract class UrlOpenSink extends DataFlow::Node { }

 private class DefaultUrlOpenSink extends UrlOpenSink {
-  DefaultUrlOpenSink() { sinkNode(this, "open-url") }
+  // request-forgery sinks control the URL of a request
+  DefaultUrlOpenSink() { sinkNode(this, "request-forgery") }
 }

 /**
--- a/java/ql/lib/semmle/code/java/security/ImplicitPendingIntents.qll
+++ b/java/ql/lib/semmle/code/java/security/ImplicitPendingIntents.qll
@@ -54,7 +54,8 @@ private class IntentCreationSource extends ImplicitPendingIntentSource {

 private class SendPendingIntent extends ImplicitPendingIntentSink {
  SendPendingIntent() {
-    sinkNode(this, "intent-start") and
+    // intent redirection sinks are method calls that start Android components
+    sinkNode(this, "intent-redirection") and
    // implicit intents can't be started as services since API 21
    not exists(MethodAccess ma, Method m |
      ma.getMethod() = m and
@@ -63,7 +64,7 @@ private class SendPendingIntent extends ImplicitPendingIntentSink {
      this.asExpr() = ma.getArgument(0)
    )
    or
-    sinkNode(this, "pending-intent-sent")
+    sinkNode(this, "pending-intents")
  }

  override predicate hasState(DataFlow::FlowState state) { state = "MutablePendingIntent" }
--- a/java/ql/lib/semmle/code/java/security/JexlInjectionQuery.qll
+++ b/java/ql/lib/semmle/code/java/security/JexlInjectionQuery.qll
@@ -13,7 +13,7 @@ abstract class JexlEvaluationSink extends DataFlow::ExprNode { }

 /** Default sink for JXEL injection vulnerabilities. */
 private class DefaultJexlEvaluationSink extends JexlEvaluationSink {
-  DefaultJexlEvaluationSink() { sinkNode(this, "jexl") }
+  DefaultJexlEvaluationSink() { sinkNode(this, "jexl-injection") }
 }

 /**
--- a/java/ql/lib/semmle/code/java/security/LdapInjection.qll
+++ b/java/ql/lib/semmle/code/java/security/LdapInjection.qll
@@ -29,7 +29,7 @@ class LdapInjectionAdditionalTaintStep extends Unit {

 /** Default sink for LDAP injection vulnerabilities. */
 private class DefaultLdapInjectionSink extends LdapInjectionSink {
-  DefaultLdapInjectionSink() { sinkNode(this, "ldap") }
+  DefaultLdapInjectionSink() { sinkNode(this, "ldap-injection") }
 }

 /** A sanitizer that clears the taint on (boxed) primitive types. */
--- a/java/ql/lib/semmle/code/java/security/LogInjection.qll
+++ b/java/ql/lib/semmle/code/java/security/LogInjection.qll
@@ -27,7 +27,7 @@ class LogInjectionAdditionalTaintStep extends Unit {
 }

 private class DefaultLogInjectionSink extends LogInjectionSink {
-  DefaultLogInjectionSink() { sinkNode(this, "logging") }
+  DefaultLogInjectionSink() { sinkNode(this, "log-injection") }
 }

 private class DefaultLogInjectionSanitizer extends LogInjectionSanitizer {
--- a/java/ql/lib/semmle/code/java/security/MvelInjection.qll
+++ b/java/ql/lib/semmle/code/java/security/MvelInjection.qll
@@ -25,7 +25,7 @@ class MvelInjectionAdditionalTaintStep extends Unit {

 /** Default sink for MVEL injection vulnerabilities. */
 private class DefaultMvelEvaluationSink extends MvelEvaluationSink {
-  DefaultMvelEvaluationSink() { sinkNode(this, "mvel") }
+  DefaultMvelEvaluationSink() { sinkNode(this, "mvel-injection") }
 }

 /** A default sanitizer that considers numeric and boolean typed data safe for building MVEL expressions */
--- a/java/ql/lib/semmle/code/java/security/QueryInjection.qll
+++ b/java/ql/lib/semmle/code/java/security/QueryInjection.qll
@@ -25,7 +25,7 @@ class AdditionalQueryInjectionTaintStep extends Unit {

 /** A sink for SQL injection vulnerabilities. */
 private class SqlInjectionSink extends QueryInjectionSink {
-  SqlInjectionSink() { sinkNode(this, "sql") }
+  SqlInjectionSink() { sinkNode(this, "sql-injection") }
 }

 /** A sink for Java Persistence Query Language injection vulnerabilities. */
--- a/java/ql/lib/semmle/code/java/security/RequestForgery.qll
+++ b/java/ql/lib/semmle/code/java/security/RequestForgery.qll
@@ -52,12 +52,8 @@ private class TypePropertiesRequestForgeryAdditionalTaintStep extends RequestFor
 /** A data flow sink for server-side request forgery (SSRF) vulnerabilities. */
 abstract class RequestForgerySink extends DataFlow::Node { }

-private class UrlOpenSinkAsRequestForgerySink extends RequestForgerySink {
-  UrlOpenSinkAsRequestForgerySink() { sinkNode(this, "open-url") }
-}
-
-private class JdbcUrlSinkAsRequestForgerySink extends RequestForgerySink {
-  JdbcUrlSinkAsRequestForgerySink() { sinkNode(this, "jdbc-url") }
+private class DefaultRequestForgerySink extends RequestForgerySink {
+  DefaultRequestForgerySink() { sinkNode(this, "request-forgery") }
 }

 /** A sanitizer for request forgery vulnerabilities. */
--- a/java/ql/lib/semmle/code/java/security/ResponseSplitting.qll
+++ b/java/ql/lib/semmle/code/java/security/ResponseSplitting.qll
@@ -11,7 +11,7 @@ private import semmle.code.java.dataflow.ExternalFlow
 abstract class HeaderSplittingSink extends DataFlow::Node { }

 private class DefaultHeaderSplittingSink extends HeaderSplittingSink {
-  DefaultHeaderSplittingSink() { sinkNode(this, "header-splitting") }
+  DefaultHeaderSplittingSink() { sinkNode(this, "response-splitting") }
 }

 /** A source that introduces data considered safe to use by a header splitting source. */
--- a/java/ql/lib/semmle/code/java/security/SensitiveLoggingQuery.qll
+++ b/java/ql/lib/semmle/code/java/security/SensitiveLoggingQuery.qll
@@ -35,7 +35,7 @@ deprecated class SensitiveLoggerConfiguration extends TaintTracking::Configurati

  override predicate isSource(DataFlow::Node source) { source.asExpr() instanceof CredentialExpr }

-  override predicate isSink(DataFlow::Node sink) { sinkNode(sink, "logging") }
+  override predicate isSink(DataFlow::Node sink) { sinkNode(sink, "log-injection") }

  override predicate isSanitizer(DataFlow::Node sanitizer) {
    sanitizer.asExpr() instanceof LiveLiteral or
@@ -52,7 +52,7 @@ deprecated class SensitiveLoggerConfiguration extends TaintTracking::Configurati
 module SensitiveLoggerConfig implements DataFlow::ConfigSig {
  predicate isSource(DataFlow::Node source) { source.asExpr() instanceof CredentialExpr }

-  predicate isSink(DataFlow::Node sink) { sinkNode(sink, "logging") }
+  predicate isSink(DataFlow::Node sink) { sinkNode(sink, "log-injection") }

  predicate isBarrier(DataFlow::Node sanitizer) {
    sanitizer.asExpr() instanceof LiveLiteral or
--- a/java/ql/lib/semmle/code/java/security/TaintedPathQuery.qll
+++ b/java/ql/lib/semmle/code/java/security/TaintedPathQuery.qll
@@ -58,7 +58,7 @@ module TaintedPathConfig implements DataFlow::ConfigSig {
  predicate isSink(DataFlow::Node sink) {
    sink.asExpr() = any(PathCreation p).getAnInput()
    or
-    sinkNode(sink, ["create-file", "read-file"])
+    sinkNode(sink, "path-injection")
  }

  predicate isBarrier(DataFlow::Node sanitizer) {
@@ -85,7 +85,7 @@ module TaintedPathLocalConfig implements DataFlow::ConfigSig {
  predicate isSink(DataFlow::Node sink) {
    sink.asExpr() = any(PathCreation p).getAnInput()
    or
-    sinkNode(sink, "create-file")
+    sinkNode(sink, "path-injection")
  }

  predicate isBarrier(DataFlow::Node sanitizer) {
--- a/java/ql/lib/semmle/code/java/security/TemplateInjection.qll
+++ b/java/ql/lib/semmle/code/java/security/TemplateInjection.qll
@@ -66,7 +66,7 @@ private class DefaultTemplateInjectionSource extends TemplateInjectionSource ins
 { }

 private class DefaultTemplateInjectionSink extends TemplateInjectionSink {
-  DefaultTemplateInjectionSink() { sinkNode(this, "ssti") }
+  DefaultTemplateInjectionSink() { sinkNode(this, "template-injection") }
 }

 private class DefaultTemplateInjectionSanitizer extends TemplateInjectionSanitizer {
--- a/java/ql/lib/semmle/code/java/security/UnsafeHostnameVerificationQuery.qll
+++ b/java/ql/lib/semmle/code/java/security/UnsafeHostnameVerificationQuery.qll
@@ -74,7 +74,7 @@ module TrustAllHostnameVerifierFlow = DataFlow::Global<TrustAllHostnameVerifierC
 * A sink that sets the `HostnameVerifier` on `HttpsURLConnection`.
 */
 private class HostnameVerifierSink extends DataFlow::Node {
-  HostnameVerifierSink() { sinkNode(this, "set-hostname-verifier") }
+  HostnameVerifierSink() { sinkNode(this, "hostname-verification") }
 }

 /**
--- a/java/ql/lib/semmle/code/java/security/UrlRedirect.qll
+++ b/java/ql/lib/semmle/code/java/security/UrlRedirect.qll
@@ -12,7 +12,7 @@ abstract class UrlRedirectSink extends DataFlow::Node { }

 /** A default sink represeting methods susceptible to URL redirection attacks. */
 private class DefaultUrlRedirectSink extends UrlRedirectSink {
-  DefaultUrlRedirectSink() { sinkNode(this, "url-redirect") }
+  DefaultUrlRedirectSink() { sinkNode(this, "url-redirection") }
 }

 /** A Servlet URL redirection sink. */
--- a/java/ql/lib/semmle/code/java/security/XPath.qll
+++ b/java/ql/lib/semmle/code/java/security/XPath.qll
@@ -13,7 +13,7 @@ abstract class XPathInjectionSink extends DataFlow::Node { }
 /** A default sink representing methods susceptible to XPath Injection attacks. */
 private class DefaultXPathInjectionSink extends XPathInjectionSink {
  DefaultXPathInjectionSink() {
-    sinkNode(this, "xpath")
+    sinkNode(this, "xpath-injection")
    or
    exists(ClassInstanceExpr constructor |
      constructor.getConstructedType().getASourceSupertype*().hasQualifiedName("org.dom4j", "XPath")
--- a/java/ql/lib/semmle/code/java/security/XSS.qll
+++ b/java/ql/lib/semmle/code/java/security/XSS.qll
@@ -39,7 +39,7 @@ class XssAdditionalTaintStep extends Unit {
 /** A default sink representing methods susceptible to XSS attacks. */
 private class DefaultXssSink extends XssSink {
  DefaultXssSink() {
-    sinkNode(this, "xss")
+    sinkNode(this, ["html-injection", "js-injection"])
    or
    exists(MethodAccess ma |
      ma.getMethod() instanceof WritingMethod and
--- a/java/ql/lib/semmle/code/java/security/XsltInjection.qll
+++ b/java/ql/lib/semmle/code/java/security/XsltInjection.qll
@@ -12,7 +12,7 @@ abstract class XsltInjectionSink extends DataFlow::Node { }

 /** A default sink representing methods susceptible to XSLT Injection attacks. */
 private class DefaultXsltInjectionSink extends XsltInjectionSink {
-  DefaultXsltInjectionSink() { sinkNode(this, "xslt") }
+  DefaultXsltInjectionSink() { sinkNode(this, "xslt-injection") }
 }

 /**
--- a/java/ql/lib/semmle/code/java/security/ZipSlipQuery.qll
+++ b/java/ql/lib/semmle/code/java/security/ZipSlipQuery.qll
@@ -40,5 +40,5 @@ module ZipSlipFlow = TaintTracking::Global<ZipSlipConfig>;
 * 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") }
+  FileCreationSink() { sinkNode(this, "path-injection") }
 }