Merge branch 'main' into tutorial/library-pack

2026-04-30 03:05:15 +02:00 · 2023-01-03 14:08:37 -08:00
parent d2ee8c08c0 c5138674a4
commit 9988c19a42
406 changed files with 26663 additions and 5387 deletions
--- a/java/ql/lib/change-notes/2022-12-15-empty-multiline-comments.md
+++ b/java/ql/lib/change-notes/2022-12-15-empty-multiline-comments.md
@@ -0,0 +1,4 @@
+---
+category: fix
+---
+* We now correctly handle empty block comments, like `/**/`. Previously these could be mistaken for Javadoc comments and led to attribution of Javadoc tags to the wrong declaration.
--- a/java/ql/lib/change-notes/2022-12-16-add-top-jdk-models.md
+++ b/java/ql/lib/change-notes/2022-12-16-add-top-jdk-models.md
@@ -0,0 +1,4 @@
+---
+category: minorAnalysis
+---
+* Added more dataflow models for frequently-used JDK APIs.
--- a/java/ql/lib/ext/java.lang.model.yml
+++ b/java/ql/lib/ext/java.lang.model.yml
@@ -37,12 +37,17 @@ extensions:
      - ["java.lang", "CharSequence", True, "charAt", "", "", "Argument[-1]", "ReturnValue", "taint", "manual"]
      - ["java.lang", "CharSequence", True, "subSequence", "", "", "Argument[-1]", "ReturnValue", "taint", "manual"]
      - ["java.lang", "CharSequence", True, "toString", "", "", "Argument[-1]", "ReturnValue", "taint", "manual"]
+      - ["java.lang", "IllegalArgumentException", False, "IllegalArgumentException", "(String)", "", "Argument[0]", "Argument[-1].SyntheticField[java.lang.Throwable.message]", "value", "manual"]
+      - ["java.lang", "IllegalStateException", False, "IllegalStateException", "(String)", "", "Argument[0]", "Argument[-1].SyntheticField[java.lang.Throwable.message]", "value", "manual"]
+      - ["java.lang", "Integer", False, "parseInt", "(String)", "", "Argument[0]", "ReturnValue", "taint", "manual"]
      - ["java.lang", "Iterable", True, "forEach", "(Consumer)", "", "Argument[-1].Element", "Argument[0].Parameter[0]", "value", "manual"]
      - ["java.lang", "Iterable", True, "iterator", "()", "", "Argument[-1].Element", "ReturnValue.Element", "value", "manual"]
      - ["java.lang", "Iterable", True, "spliterator", "()", "", "Argument[-1].Element", "ReturnValue.Element", "value", "manual"]
      - ["java.lang", "Object", True, "clone", "", "", "Argument[-1].Element", "ReturnValue.Element", "value", "manual"]
      - ["java.lang", "Object", True, "clone", "", "", "Argument[-1].MapKey", "ReturnValue.MapKey", "value", "manual"]
      - ["java.lang", "Object", True, "clone", "", "", "Argument[-1].MapValue", "ReturnValue.MapValue", "value", "manual"]
+      - ["java.lang", "RuntimeException", False, "RuntimeException", "(String)", "", "Argument[0]", "Argument[-1].SyntheticField[java.lang.Throwable.message]", "value", "manual"]
+      - ["java.lang", "RuntimeException", False, "RuntimeException", "(Throwable)", "", "Argument[0]", "Argument[-1].SyntheticField[java.lang.Throwable.cause]", "value", "manual"]
      - ["java.lang", "String", False, "String", "", "", "Argument[0]", "Argument[-1]", "taint", "manual"]
      - ["java.lang", "String", False, "concat", "(String)", "", "Argument[-1]", "ReturnValue", "taint", "manual"]
      - ["java.lang", "String", False, "concat", "(String)", "", "Argument[0]", "ReturnValue", "taint", "manual"]
@@ -82,7 +87,34 @@ extensions:
      - ["java.lang", "String", False, "valueOf", "(char)", "", "Argument[0]", "ReturnValue", "taint", "manual"]
      - ["java.lang", "String", False, "valueOf", "(char[])", "", "Argument[0]", "ReturnValue", "taint", "manual"]
      - ["java.lang", "String", False, "valueOf", "(char[],int,int)", "", "Argument[0]", "ReturnValue", "taint", "manual"]
+      - ["java.lang", "String", False, "valueOf", "(int)", "", "Argument[0]", "ReturnValue", "taint", "manual"]
      - ["java.lang", "StringBuffer", True, "StringBuffer", "(CharSequence)", "", "Argument[0]", "Argument[-1]", "taint", "manual"]
      - ["java.lang", "StringBuffer", True, "StringBuffer", "(String)", "", "Argument[0]", "Argument[-1]", "taint", "manual"]
      - ["java.lang", "StringBuilder", True, "StringBuilder", "", "", "Argument[0]", "Argument[-1]", "taint", "manual"]
      - ["java.lang", "System", False, "arraycopy", "", "", "Argument[0]", "Argument[2]", "taint", "manual"]
+      - ["java.lang", "Throwable", False, "Throwable", "(Throwable)", "", "Argument[0]", "Argument[-1].SyntheticField[java.lang.Throwable.cause]", "value", "manual"]
+      - ["java.lang", "Throwable", False, "getCause", "()", "", "Argument[-1].SyntheticField[java.lang.Throwable.cause]", "ReturnValue", "value", "manual"]
+      - ["java.lang", "Throwable", False, "getMessage", "()", "", "Argument[-1].SyntheticField[java.lang.Throwable.message]", "ReturnValue", "value", "manual"]
+
+  - addsTo:
+      pack: codeql/java-all
+      extensible: neutralModel
+    data:
+      - ["java.lang", "Class", "getName", "()", "manual"]
+      - ["java.lang", "Class", "getSimpleName", "()", "manual"]
+      - ["java.lang", "Enum", "Enum", "(String,int)", "manual"]
+      - ["java.lang", "Enum", "equals", "(Object)", "manual"]
+      - ["java.lang", "Enum", "name", "()", "manual"]
+      - ["java.lang", "Enum", "toString", "()", "manual"]
+      - ["java.lang", "Object", "equals", "(Object)", "manual"]
+      - ["java.lang", "Object", "getClass", "()", "manual"]
+      - ["java.lang", "Object", "hashCode", "()", "manual"]
+      - ["java.lang", "Object", "toString", "()", "manual"]
+      - ["java.lang", "String", "contains", "(CharSequence)", "manual"]
+      - ["java.lang", "String", "equals", "(Object)", "manual"]
+      - ["java.lang", "String", "equalsIgnoreCase", "(String)", "manual"]
+      - ["java.lang", "String", "hashCode", "()", "manual"]
+      - ["java.lang", "String", "isEmpty", "()", "manual"]
+      - ["java.lang", "String", "length", "()", "manual"]
+      - ["java.lang", "String", "startsWith", "(String)", "manual"]
+      - ["java.lang", "System", "currentTimeMillis", "()", "manual"]
--- a/java/ql/lib/ext/java.math.model.yml
+++ b/java/ql/lib/ext/java.math.model.yml
@@ -0,0 +1,6 @@
+extensions:
+  - addsTo:
+      pack: codeql/java-all
+      extensible: summaryModel
+    data:
+      - ["java.math", "BigDecimal", False, "BigDecimal", "(String)", "", "Argument[0]", "Argument[-1]", "taint", "manual"]
--- a/java/ql/lib/ext/java.sql.model.yml
+++ b/java/ql/lib/ext/java.sql.model.yml
@@ -14,3 +14,9 @@ extensions:
      - ["java.sql", "Statement", True, "executeLargeUpdate", "", "", "Argument[0]", "sql", "manual"]
      - ["java.sql", "Statement", True, "executeQuery", "", "", "Argument[0]", "sql", "manual"]
      - ["java.sql", "Statement", True, "executeUpdate", "", "", "Argument[0]", "sql", "manual"]
+
+  - addsTo:
+      pack: codeql/java-all
+      extensible: summaryModel
+    data:
+      - ["java.sql", "ResultSet", True, "getString", "(String)", "", "Argument[-1]", "ReturnValue", "taint", "manual"]
--- a/java/ql/lib/ext/java.util.model.yml
+++ b/java/ql/lib/ext/java.util.model.yml
@@ -355,3 +355,26 @@ extensions:
      - ["java.util", "Vector", True, "setElementAt", "(Object,int)", "", "Argument[0]", "Argument[-1].Element", "value", "manual"]
      - ["java.util", "WeakHashMap", False, "WeakHashMap", "(Map)", "", "Argument[0].MapKey", "Argument[-1].MapKey", "value", "manual"]
      - ["java.util", "WeakHashMap", False, "WeakHashMap", "(Map)", "", "Argument[0].MapValue", "Argument[-1].MapValue", "value", "manual"]
+
+  - addsTo:
+      pack: codeql/java-all
+      extensible: neutralModel
+    data:
+      - ["java.util", "Collections", "emptyList", "()", "manual"]
+      - ["java.util", "Collection", "size", "()", "manual"]
+      - ["java.util", "Iterator", "hasNext", "()", "manual"]
+      - ["java.util", "List", "contains", "(Object)", "manual"]
+      - ["java.util", "List", "isEmpty", "()", "manual"]
+      - ["java.util", "List", "size", "()", "manual"]
+      - ["java.util", "Map", "containsKey", "(Object)", "manual"]
+      - ["java.util", "Map", "isEmpty", "()", "manual"]
+      - ["java.util", "Map", "size", "()", "manual"]
+      - ["java.util", "Objects", "equals", "(Object,Object)", "manual"]
+      - ["java.util", "Objects", "hash", "(Object[])", "manual"]
+      - ["java.util", "Optional", "empty", "()", "manual"]
+      - ["java.util", "Optional", "isPresent", "()", "manual"]
+      - ["java.util", "Set", "contains", "(Object)", "manual"]
+      - ["java.util", "Set", "isEmpty", "()", "manual"]
+      - ["java.util", "Set", "size", "()", "manual"]
+      - ["java.util", "UUID", "randomUUID", "()", "manual"]
+      - ["java.util", "UUID", "toString", "()", "manual"]
--- a/java/ql/lib/ext/java.util.stream.model.yml
+++ b/java/ql/lib/ext/java.util.stream.model.yml
@@ -87,3 +87,9 @@ extensions:
      - ["java.util.stream", "Stream", True, "takeWhile", "(Predicate)", "", "Argument[-1].Element", "ReturnValue.Element", "value", "manual"]
      - ["java.util.stream", "Stream", True, "toArray", "", "", "Argument[-1].Element", "ReturnValue.ArrayElement", "value", "manual"]
      - ["java.util.stream", "Stream", True, "toList", "()", "", "Argument[-1].Element", "ReturnValue.Element", "value", "manual"]
+
+  - addsTo:
+      pack: codeql/java-all
+      extensible: neutralModel
+    data:
+      - ["java.util.stream", "Collectors", "toList", "()", "manual"]
--- a/java/ql/lib/semmle/code/java/Javadoc.qll
+++ b/java/ql/lib/semmle/code/java/Javadoc.qll
@@ -33,7 +33,11 @@ class Javadoc extends JavadocParent, @javadoc {
  string getAuthor() { result = this.getATag("@author").getChild(0).toString() }

  override string toString() {
-    result = this.toStringPrefix() + this.getChild(0) + this.toStringPostfix()
+    exists(string childStr |
+      if exists(this.getChild(0)) then childStr = this.getChild(0).toString() else childStr = ""
+    |
+      result = this.toStringPrefix() + childStr + this.toStringPostfix()
+    )
  }

  private string toStringPrefix() {
@@ -48,7 +52,7 @@ class Javadoc extends JavadocParent, @javadoc {
    if isEolComment(this)
    then result = ""
    else (
-      if strictcount(this.getAChild()) = 1 then result = " */" else result = " ... */"
+      if strictcount(this.getAChild()) > 1 then result = " ... */" else result = " */"
    )
  }

--- a/java/ql/lib/semmle/code/java/dataflow/internal/FlowSummaryImpl.qll
+++ b/java/ql/lib/semmle/code/java/dataflow/internal/FlowSummaryImpl.qll
@@ -260,6 +260,12 @@ module Public {
     *  Holds if the neutral is auto generated.
     */
    predicate isAutoGenerated() { neutralElement(this, true) }
+
+    /**
+     * Holds if the neutral has the given provenance where `true` is
+     * `generated` and `false` is `manual`.
+     */
+    predicate hasProvenance(boolean generated) { neutralElement(this, generated) }
  }
 }

--- a/java/ql/lib/semmle/code/java/security/AndroidCertificatePinningQuery.qll
+++ b/java/ql/lib/semmle/code/java/security/AndroidCertificatePinningQuery.qll
@@ -0,0 +1,140 @@
+/** Definitions for the Android Missing Certificate Pinning query. */
+
+import java
+import semmle.code.xml.AndroidManifest
+import semmle.code.java.dataflow.TaintTracking
+import semmle.code.java.frameworks.Networking
+import semmle.code.java.security.Encryption
+import semmle.code.java.security.HttpsUrls
+
+/** An Android Network Security Configuration XML file. */
+class AndroidNetworkSecurityConfigFile extends XmlFile {
+  AndroidNetworkSecurityConfigFile() {
+    exists(AndroidApplicationXmlElement app, AndroidXmlAttribute confAttr, string confName |
+      confAttr.getElement() = app and
+      confAttr.getValue() = "@xml/" + confName and
+      this.getRelativePath().matches("%res/xml/" + confName + ".xml") and
+      this.getARootElement().getName() = "network-security-config"
+    )
+  }
+}
+
+/** Holds if this database is of an Android application. */
+predicate isAndroid() { exists(AndroidManifestXmlFile m) }
+
+/** Holds if the given domain name is trusted by the Network Security Configuration XML file. */
+private predicate trustedDomainViaXml(string domainName) {
+  exists(
+    AndroidNetworkSecurityConfigFile confFile, XmlElement domConf, XmlElement domain,
+    XmlElement trust
+  |
+    domConf.getFile() = confFile and
+    domConf.getName() = "domain-config" and
+    domain.getParent() = domConf and
+    domain.getName() = "domain" and
+    domain.getACharactersSet().getCharacters() = domainName and
+    trust.getParent() = domConf and
+    trust.getName() = ["trust-anchors", "pin-set"]
+  )
+}
+
+/** Holds if the given domain name is trusted by an OkHttp `CertificatePinner`. */
+private predicate trustedDomainViaOkHttp(string domainName) {
+  exists(CompileTimeConstantExpr domainExpr, MethodAccess certPinnerAdd |
+    domainExpr.getStringValue().replaceAll("*.", "") = domainName and // strip wildcard patterns like *.example.com
+    certPinnerAdd.getMethod().hasQualifiedName("okhttp3", "CertificatePinner$Builder", "add") and
+    DataFlow::localExprFlow(domainExpr, certPinnerAdd.getArgument(0))
+  )
+}
+
+/** Holds if the given domain name is trusted by some certificate pinning implementation. */
+predicate trustedDomain(string domainName) {
+  trustedDomainViaXml(domainName)
+  or
+  trustedDomainViaOkHttp(domainName)
+}
+
+/**
+ * Holds if `setSocketFactory` is a call to `HttpsURLConnection.setSSLSocketFactory` or `HttpsURLConnection.setDefaultSSLSocketFactory`
+ * that uses a socket factory derived from a `TrustManager`.
+ * `default` is true if the default SSL socket factory for all URLs is being set.
+ */
+private predicate trustedSocketFactory(MethodAccess setSocketFactory, boolean default) {
+  exists(MethodAccess getSocketFactory, MethodAccess initSslContext |
+    exists(Method m | setSocketFactory.getMethod() = m |
+      default = true and m instanceof SetDefaultConnectionFactoryMethod
+      or
+      default = false and m instanceof SetConnectionFactoryMethod
+    ) and
+    initSslContext.getMethod().getDeclaringType() instanceof SslContext and
+    initSslContext.getMethod().hasName("init") and
+    getSocketFactory.getMethod().getASourceOverriddenMethod*() instanceof GetSocketFactory and
+    not initSslContext.getArgument(1) instanceof NullLiteral and
+    DataFlow::localExprFlow(initSslContext.getQualifier(), getSocketFactory.getQualifier()) and
+    DataFlow::localExprFlow(getSocketFactory, setSocketFactory.getArgument(0))
+  )
+}
+
+/**
+ * Holds if the given expression is an qualifier to a `URL.openConnection` or `URL.openStream` call
+ * that is trusted due to its SSL socket factory being set.
+ */
+private predicate trustedUrlConnection(Expr url) {
+  exists(MethodAccess openCon |
+    openCon.getMethod().getASourceOverriddenMethod*() instanceof UrlOpenConnectionMethod and
+    url = openCon.getQualifier() and
+    exists(MethodAccess setSocketFactory |
+      trustedSocketFactory(setSocketFactory, false) and
+      TaintTracking::localExprTaint(openCon, setSocketFactory.getQualifier())
+    )
+  )
+  or
+  trustedSocketFactory(_, true) and
+  exists(MethodAccess open, Method m |
+    m instanceof UrlOpenConnectionMethod or m instanceof UrlOpenStreamMethod
+  |
+    open.getMethod().getASourceOverriddenMethod*() = m and
+    url = open.getQualifier()
+  )
+}
+
+private class MissingPinningSink extends DataFlow::Node {
+  MissingPinningSink() {
+    this instanceof UrlOpenSink and
+    not trustedUrlConnection(this.asExpr())
+  }
+}
+
+/** Configuration for finding uses of non trusted URLs. */
+private class UntrustedUrlConfig extends TaintTracking::Configuration {
+  UntrustedUrlConfig() { this = "UntrustedUrlConfig" }
+
+  override predicate isSource(DataFlow::Node node) {
+    trustedDomain(_) and
+    exists(string lit | lit = node.asExpr().(CompileTimeConstantExpr).getStringValue() |
+      lit.matches("%://%") and // it's a URL
+      not exists(string dom | trustedDomain(dom) and lit.matches("%" + dom + "%"))
+    )
+  }
+
+  override predicate isSink(DataFlow::Node node) { node instanceof MissingPinningSink }
+}
+
+/** Holds if `node` is a network communication call for which certificate pinning is not implemented. */
+predicate missingPinning(DataFlow::Node node, string domain) {
+  isAndroid() and
+  node instanceof MissingPinningSink and
+  (
+    not trustedDomain(_) and domain = ""
+    or
+    exists(UntrustedUrlConfig conf, DataFlow::Node src |
+      conf.hasFlow(src, node) and
+      domain = getDomain(src.asExpr())
+    )
+  )
+}
+
+/** Gets the domain name from the given string literal */
+private string getDomain(CompileTimeConstantExpr expr) {
+  result = expr.getStringValue().regexpCapture("(https?://)?([^/]*)(/.*)?", 2)
+}
--- a/java/ql/lib/semmle/code/java/security/Encryption.qll
+++ b/java/ql/lib/semmle/code/java/security/Encryption.qll
@@ -143,6 +143,7 @@ class CreateSslEngineMethod extends Method {
  }
 }

+/** The `setConnectionFactory` method of the class `javax.net.ssl.HttpsURLConnection`. */
 class SetConnectionFactoryMethod extends Method {
  SetConnectionFactoryMethod() {
    this.hasName("setSSLSocketFactory") and
@@ -150,6 +151,14 @@ class SetConnectionFactoryMethod extends Method {
  }
 }

+/** The `setDefaultConnectionFactory` method of the class `javax.net.ssl.HttpsURLConnection`. */
+class SetDefaultConnectionFactoryMethod extends Method {
+  SetDefaultConnectionFactoryMethod() {
+    this.hasName("setDefaultSSLSocketFactory") and
+    this.getDeclaringType().getAnAncestor() instanceof HttpsUrlConnection
+  }
+}
+
 class SetHostnameVerifierMethod extends Method {
  SetHostnameVerifierMethod() {
    this.hasName("setHostnameVerifier") and