Merge pull request #17236 from michaelnebel/java/viablecallableheuristic

michaelnebel · web-flow · commit 0df0d8a51fd4 · 2024-08-29T10:46:30.000+02:00
Java: Make more finegrained dataflow dispatch viable callable heuristic.
diff --git a/java/ql/lib/change-notes/2024-08-20-dataflow-dispatch.md b/java/ql/lib/change-notes/2024-08-20-dataflow-dispatch.md
@@ -0,0 +1,4 @@
+---
+category: majorAnalysis
+---
+* A generated (Models as Data) summary model is no longer used, if there exists a source code alternative. This primarily affects the analysis, when the analysis includes generated models for the source code being analysed.
diff --git a/java/ql/lib/ext/java.net.model.yml b/java/ql/lib/ext/java.net.model.yml
@@ -48,6 +48,7 @@ extensions:
       - ["java.net", "URI", False, "URI", "(String,String,String)", "", "Argument[1]", "ReturnValue", "taint", "ai-manual"]
       - ["java.net", "URI", False, "URI", "(String)", "", "Argument[0]", "Argument[this]", "taint", "manual"]
       - ["java.net", "URI", False, "create", "", "", "Argument[0]", "ReturnValue", "taint", "manual"]
+      - ["java.net", "URI", False, "getPath", "()", "", "Argument[this]", "ReturnValue", "taint", "df-manual"]
       - ["java.net", "URI", False, "resolve", "(String)", "", "Argument[0]", "ReturnValue", "taint", "ai-manual"]
       - ["java.net", "URI", False, "resolve", "(URI)", "", "Argument[0]", "ReturnValue", "taint", "ai-manual"]
       - ["java.net", "URI", False, "toASCIIString", "", "", "Argument[this]", "ReturnValue", "taint", "manual"]
diff --git a/java/ql/lib/ext/java.nio.model.yml b/java/ql/lib/ext/java.nio.model.yml
@@ -5,7 +5,20 @@ extensions:
     data:
       - ["java.nio", "ByteBuffer", False, "array", "()", "", "Argument[this]", "ReturnValue", "taint", "manual"]
       - ["java.nio", "ByteBuffer", False, "get", "", "", "Argument[this]", "ReturnValue", "taint", "manual"]
-      - ["java.nio", "ByteBuffer", False, "wrap", "(byte[])", "", "Argument[0]", "ReturnValue", "taint", "manual"]
+      - ["java.nio", "ByteBuffer", True, "put", "(ByteBuffer)", "", "Argument[0]", "Argument[this]", "taint", "manual"]
+      - ["java.nio", "ByteBuffer", True, "put", "(ByteBuffer)", "", "Argument[this]", "ReturnValue", "value", "manual"]
+      - ["java.nio", "ByteBuffer", True, "put", "(byte)", "", "Argument[this]", "ReturnValue", "value", "manual"]
+      - ["java.nio", "ByteBuffer", True, "put", "(byte[])", "", "Argument[0]", "Argument[this]", "taint", "manual"]
+      - ["java.nio", "ByteBuffer", True, "put", "(byte[])", "", "Argument[this]", "ReturnValue", "value", "manual"]
+      - ["java.nio", "ByteBuffer", True, "put", "(byte[],int,int)", "", "Argument[0]", "Argument[this]", "taint", "manual"]
+      - ["java.nio", "ByteBuffer", True, "put", "(byte[],int,int)", "", "Argument[this]", "ReturnValue", "value", "manual"]
+      - ["java.nio", "ByteBuffer", True, "wrap", "(byte[])", "", "Argument[0]", "ReturnValue", "taint", "manual"]
+      - ["java.nio", "ByteBuffer", True, "wrap", "(byte[],int,int)", "", "Argument[0]", "ReturnValue", "taint", "manual"]
+      - ["java.nio", "CharBuffer", True, "wrap", "(CharSequence)", "", "Argument[0]", "ReturnValue", "taint", "manual"]
+      - ["java.nio", "CharBuffer", True, "wrap", "(CharSequence,int,int)", "", "Argument[0]", "ReturnValue", "taint", "manual"]
+      - ["java.nio", "CharBuffer", True, "wrap", "(char[])", "", "Argument[0]", "ReturnValue", "taint", "manual"]
+      - ["java.nio", "CharBuffer", True, "wrap", "(char[],int,int)", "", "Argument[0]", "ReturnValue", "taint", "manual"]
+
 
   - addsTo:
       pack: codeql/java-all
diff --git a/java/ql/lib/ext/java.security.cert.model.yml b/java/ql/lib/ext/java.security.cert.model.yml
@@ -4,6 +4,14 @@ extensions:
       extensible: sinkModel
     data:
       - ["java.security.cert", "X509CertSelector", False, "setSubjectPublicKey", "(byte[])", "", "Argument[0]", "credentials-key", "hq-generated"]
+
+  - addsTo:
+      pack: codeql/java-all
+      extensible: summaryModel
+    data:
+      - ["java.security.cert", "X509Certificate", True, "getIssuerX500Principal", "()", "", "Argument[this]", "ReturnValue", "taint", "df-manual"]
+      - ["java.security.cert", "X509Certificate", True, "getSubjectX500Principal", "()", "", "Argument[this]", "ReturnValue", "taint", "df-manual"]
+
   - addsTo:
       pack: codeql/java-all
       extensible: neutralModel
diff --git a/java/ql/lib/ext/java.security.model.yml b/java/ql/lib/ext/java.security.model.yml
@@ -26,6 +26,9 @@ extensions:
       - ["java.security", "CodeSource", False, "getCertificates", "()", "", "Argument[this].SyntheticField[java.security.CodeSource.certificates].ArrayElement", "ReturnValue.ArrayElement", "value", "df-manual"]
       - ["java.security", "CodeSource", False, "getCodeSigners", "()", "", "Argument[this].SyntheticField[java.security.CodeSource.codeSigners].ArrayElement", "ReturnValue.ArrayElement", "value", "df-manual"]
       - ["java.security", "CodeSource", False, "getLocation", "()", "", "Argument[this]", "ReturnValue", "taint", "df-manual"]
+      - ["java.security", "Permission", True, "Permission", "(String)", "", "Argument[0]", "Argument[this]", "taint", "df-manual"]
+      - ["java.security", "Permission", True, "getName", "()", "", "Argument[this]", "ReturnValue", "taint", "df-manual"]
+
   - addsTo:
       pack: codeql/java-all
       extensible: neutralModel
diff --git a/java/ql/lib/ext/java.util.logging.model.yml b/java/ql/lib/ext/java.util.logging.model.yml
@@ -46,8 +46,13 @@ extensions:
       - ["java.util.logging", "Logger", False, "getLogger", "(String)", "", "Argument[0]", "ReturnValue.SyntheticField[java.util.logging.Logger.name]", "value", "manual"]
       - ["java.util.logging", "Logger", False, "getName", "()", "", "Argument[this].SyntheticField[java.util.logging.Logger.name]", "ReturnValue", "value", "manual"]
       - ["java.util.logging", "LogRecord", False, "LogRecord", "", "", "Argument[1]", "Argument[this]", "taint", "manual"]
+      - ["java.util.logging", "LogRecord", True, "getMessage", "()", "", "Argument[this]", "ReturnValue", "taint", "df-manual"]
+      - ["java.util.logging", "LogRecord", True, "getParameters", "()", "", "Argument[this].SyntheticField[java.util.logging.LogRecord.parameters].ArrayElement", "ReturnValue.ArrayElement", "value", "manual"]
+      - ["java.util.logging", "LogRecord", True, "setParameters", "(Object[])", "", "Argument[0].ArrayElement", "Argument[this].SyntheticField[java.util.logging.LogRecord.parameters].ArrayElement", "value", "manual"]
   - addsTo:
       pack: codeql/java-all
       extensible: neutralModel
     data:
+      - ["java.util.logging", "Handler", "getEncoding", "()", "summary", "manual"]
       - ["java.util.logging", "Logger", "isLoggable", "(Level)", "summary", "manual"]
+      - ["java.util.logging", "LogRecord", "getResourceBundle", "()", "summary", "df-manual"]
diff --git a/java/ql/lib/ext/java.util.logging.yml b/java/ql/lib/ext/java.util.logging.yml
diff --git a/java/ql/lib/ext/javax.management.model.yml b/java/ql/lib/ext/javax.management.model.yml
@@ -3,4 +3,7 @@ extensions:
       pack: codeql/java-all
       extensible: summaryModel
     data:
+      - ["javax.management", "Notification", True, "Notification", "(String,Object,long,String)", "", "Argument[3]", "Argument[this]", "taint", "df-manual"]
+      - ["javax.management", "Notification", True, "Notification", "(String,Object,long,long,String)", "", "Argument[4]", "Argument[this]", "taint", "df-manual"]
+      - ["javax.management", "Notification", True, "getMessage", "()", "", "Argument[this]", "ReturnValue", "taint", "df-manual"]
       - ["javax.management", "ObjectName", True, "ObjectName", "(String)", "", "Argument[0]", "Argument[this]", "taint", "ai-manual"]
diff --git a/java/ql/lib/semmle/code/java/Element.qll b/java/ql/lib/semmle/code/java/Element.qll
@@ -36,6 +36,13 @@ class Element extends @element, Top {
    */
   predicate fromSource() { this.getCompilationUnit().isSourceFile() }
 
+  /**
+   * Holds if this element is from source and classified as a stub implementation.
+   * An implementation is considered a stub, if the the path to the
+   * source file contains `/stubs/`.
+   */
+  predicate isStub() { this.fromSource() and this.getFile().getAbsolutePath().matches("%/stubs/%") }
+
   /** Gets the compilation unit that this element belongs to. */
   CompilationUnit getCompilationUnit() { result = this.getFile() }
 
diff --git a/java/ql/lib/semmle/code/java/dataflow/internal/DataFlowDispatch.qll b/java/ql/lib/semmle/code/java/dataflow/internal/DataFlowDispatch.qll
@@ -40,11 +40,32 @@ private module DispatchImpl {
     else any()
   }
 
-  /** Gets a viable implementation of the target of the given `Call`. */
+  /**
+   * Gets a viable implementation of the target of the given `Call`.
+   * The following heuristic is applied for finding the appropriate callable:
+   * In general, dispatch to both any existing model and any viable source dispatch.
+   * However, if the model is generated and the static call target is in the source then
+   * we trust the source more than the model and skip dispatch to the model.
+   * Vice versa, if the model is manual and the source dispatch has a comparatively low
+   * confidence then we only dispatch to the model. Additionally, manual models that
+   * match a source dispatch exactly take precedence over the source.
+   */
   DataFlowCallable viableCallable(DataFlowCall c) {
-    result.asCallable() = sourceDispatch(c.asCall())
-    or
-    result.asSummarizedCallable().getACall() = c.asCall()
+    exists(Call call | call = c.asCall() |
+      result.asCallable() = sourceDispatch(call)
+      or
+      not (
+        // Only use summarized callables with generated summaries in case
+        // the static call target is not in the source code.
+        // Note that if applyGeneratedModel holds it implies that there doesn't
+        // exist a manual model.
+        exists(Callable staticTarget | staticTarget = call.getCallee().getSourceDeclaration() |
+          staticTarget.fromSource() and not staticTarget.isStub()
+        ) and
+        result.asSummarizedCallable().applyGeneratedModel()
+      ) and
+      result.asSummarizedCallable().getACall() = call
+    )
   }
 
   /**
diff --git a/java/ql/test/library-tests/dataflow/external-models/C.java b/java/ql/test/library-tests/dataflow/external-models/C.java
@@ -1,5 +1,7 @@
 package my.qltest;
 
+import my.qltest.external.Library;
+
 public class C {
   void foo() {
     Object arg1 = new Object();
@@ -25,35 +27,82 @@ void foo() {
   }
 
   void fooGenerated() {
+    Object arg = new Object();
+
+    // The (generated) summary is ignored because the source code is available.
+    stepArgResGenerated(arg);
+  }
+
+  // Library functionality is emulated by placing the source code in a "stubs"
+  // folder. This means that a generated summary will be applied, if there
+  // doesn't exist a manual summary or manual summary neutral.
+  void fooLibrary() {
     Object arg1 = new Object();
-    stepArgResGenerated(arg1);
+
+    Library lib = new Library();
+
+    lib.apiStepArgResGenerated(arg1);
 
     Object arg2 = new Object();
-    // The summary for the first parameter is ignored, because it is generated and
-    // because there is hand written summary for the second parameter.
-    stepArgResGeneratedIgnored(arg1, arg2);
 
-    stepArgQualGenerated(arg1);
     // The summary for the first parameter is ignored, because it is generated and
-    // because there is hand written neutral summary model for this callable.
-    stepArgQualGeneratedIgnored(arg1);
+    // because there is a manual summary for the second parameter.
+    lib.apiStepArgResGeneratedIgnored(arg1, arg2);
+
+    lib.apiStepArgQualGenerated(arg1);
+
+    // The summary for the parameter is ignored, because it is generated and
+    // because there is a manual neutral summary model for this callable.
+    lib.apiStepArgQualGeneratedIgnored(arg1);
   }
 
-  Object stepArgRes(Object x) { return null; }
+  void fooPossibleLibraryDispatch(Library lib) {
+    Object arg1 = new Object();
 
-  void stepArgArg(Object in, Object out) { }
+    lib.id(arg1);
+  }
 
-  void stepArgQual(Object x) { }
+  void fooExplicitDispatch() {
+    Object arg1 = new Object();
 
-  Object stepQualRes() { return null; }
+    MyLibrary lib = new MyLibrary();
 
-  void stepQualArg(Object out) { }
+    lib.id(arg1);
+  }
 
-  Object stepArgResGenerated(Object x) { return null; }
+  void fooGeneric(MyGenericLibrary<String> lib) {
+    lib.get();
+  }
+
+  Object stepArgRes(Object x) {
+    return null;
+  }
+
+  void stepArgArg(Object in, Object out) {}
+
+  void stepArgQual(Object x) {}
+
+  Object stepQualRes() {
+    return null;
+  }
+
+  void stepQualArg(Object out) {}
+
+  Object stepArgResGenerated(Object x) {
+    return null;
+  }
 
-  Object stepArgResGeneratedIgnored(Object x, Object y) { return null; }
+  class MyLibrary extends Library {
+    @Override
+    // Bad implementation of the id function.
+    public Object id(Object x) {
+      return null;
+    }
+  }
 
-  Object stepArgQualGenerated(Object x) { return null; }
-  
-  Object stepArgQualGeneratedIgnored(Object x) { return null; }
+  public class MyGenericLibrary<T> {
+    public T get() {
+      return null;
+    }
+  }
 }
diff --git a/java/ql/test/library-tests/dataflow/external-models/steps.expected b/java/ql/test/library-tests/dataflow/external-models/steps.expected
@@ -1,13 +1,14 @@
 invalidModelRow
 #select
-| C.java:6:16:6:19 | arg1 | C.java:6:5:6:20 | stepArgRes(...) |
-| C.java:10:16:10:21 | argIn1 | C.java:10:24:10:30 | argOut1 [post update] |
-| C.java:13:16:13:21 | argIn2 | C.java:13:24:13:30 | argOut2 [post update] |
-| C.java:16:17:16:20 | arg2 | C.java:16:5:16:21 | this <.method> [post update] |
-| C.java:18:22:18:25 | arg3 | C.java:18:5:18:8 | this [post update] |
-| C.java:20:5:20:8 | this | C.java:20:5:20:22 | stepQualRes(...) |
-| C.java:21:5:21:17 | this <.method> | C.java:21:5:21:17 | stepQualRes(...) |
-| C.java:24:5:24:23 | this <.method> | C.java:24:17:24:22 | argOut [post update] |
-| C.java:29:25:29:28 | arg1 | C.java:29:5:29:29 | stepArgResGenerated(...) |
-| C.java:34:38:34:41 | arg2 | C.java:34:5:34:42 | stepArgResGeneratedIgnored(...) |
-| C.java:36:26:36:29 | arg1 | C.java:36:5:36:30 | this <.method> [post update] |
+| C.java:8:16:8:19 | arg1 | C.java:8:5:8:20 | stepArgRes(...) |
+| C.java:12:16:12:21 | argIn1 | C.java:12:24:12:30 | argOut1 [post update] |
+| C.java:15:16:15:21 | argIn2 | C.java:15:24:15:30 | argOut2 [post update] |
+| C.java:18:17:18:20 | arg2 | C.java:18:5:18:21 | this <.method> [post update] |
+| C.java:20:22:20:25 | arg3 | C.java:20:5:20:8 | this [post update] |
+| C.java:22:5:22:8 | this | C.java:22:5:22:22 | stepQualRes(...) |
+| C.java:23:5:23:17 | this <.method> | C.java:23:5:23:17 | stepQualRes(...) |
+| C.java:26:5:26:23 | this <.method> | C.java:26:17:26:22 | argOut [post update] |
+| C.java:44:32:44:35 | arg1 | C.java:44:5:44:36 | apiStepArgResGenerated(...) |
+| C.java:50:45:50:48 | arg2 | C.java:50:5:50:49 | apiStepArgResGeneratedIgnored(...) |
+| C.java:52:33:52:36 | arg1 | C.java:52:5:52:7 | lib [post update] |
+| C.java:62:12:62:15 | arg1 | C.java:62:5:62:16 | id(...) |
diff --git a/java/ql/test/library-tests/dataflow/external-models/steps.ext.yml b/java/ql/test/library-tests/dataflow/external-models/steps.ext.yml
@@ -9,13 +9,16 @@ extensions:
       - ["my.qltest", "C", False, "stepQualRes", "()", "", "Argument[this]", "ReturnValue", "taint", "manual"]
       - ["my.qltest", "C", False, "stepQualArg", "(Object)", "", "Argument[this]", "Argument[0]", "taint", "manual"]
       - ["my.qltest", "C", False, "stepArgResGenerated", "(Object)", "", "Argument[0]", "ReturnValue", "taint", "df-generated"]
-      - ["my.qltest", "C", False, "stepArgResGeneratedIgnored", "(Object,Object)", "", "Argument[0]", "ReturnValue", "taint", "df-generated"]
-      - ["my.qltest", "C", False, "stepArgResGeneratedIgnored", "(Object,Object)", "", "Argument[1]", "ReturnValue", "taint", "manual"]
-      - ["my.qltest", "C", False, "stepArgQualGenerated", "(Object)", "", "Argument[0]", "Argument[this]", "taint", "df-generated"]
-      - ["my.qltest", "C", False, "stepArgQualGeneratedIgnored", "(Object)", "", "Argument[0]", "Argument[this]", "taint", "df-generated"]
+      - ["my.qltest", "C$MyGenericLibrary", True, "get", "()", "", "Argument[this]", "ReturnValue", "taint", "df-generated"]
+      - ["my.qltest.external", "Library", False, "apiStepArgResGenerated", "(Object)", "", "Argument[0]", "ReturnValue", "taint", "df-generated"]
+      - ["my.qltest.external", "Library", False, "apiStepArgResGeneratedIgnored", "(Object,Object)", "", "Argument[0]", "ReturnValue", "taint", "df-generated"]
+      - ["my.qltest.external", "Library", False, "apiStepArgResGeneratedIgnored", "(Object,Object)", "", "Argument[1]", "ReturnValue", "taint", "manual"]
+      - ["my.qltest.external", "Library", False, "apiStepArgQualGenerated", "(Object)", "", "Argument[0]", "Argument[this]", "taint", "df-generated"]
+      - ["my.qltest.external", "Library", False, "apiStepArgQualGeneratedIgnored", "(Object)", "", "Argument[0]", "Argument[this]", "taint", "df-generated"]
+      - ["my.qltest.external", "Library", False, "id", "(Object)", "", "Argument[0]", "ReturnValue", "taint", "df-generated"]
   - addsTo:
       pack: codeql/java-all
       extensible: neutralModel
     data:
-      - ["my.qltest", "C", "stepArgQualGenerated", "(Object)", "summary", "df-generated"]
-      - ["my.qltest", "C", "stepArgQualGeneratedIgnored", "(Object)", "summary", "manual"]
+      - ["my.qltest.external", "Library", "apiStepArgQualGenerated", "(Object)", "summary", "df-generated"]
+      - ["my.qltest.external", "Library", "apiStepArgQualGeneratedIgnored", "(Object)", "summary", "manual"]
diff --git a/java/ql/test/library-tests/dataflow/external-models/stubs/Library.java b/java/ql/test/library-tests/dataflow/external-models/stubs/Library.java
@@ -0,0 +1,23 @@
+package my.qltest.external;
+
+public class Library {
+  public Object apiStepArgResGenerated(Object x) {
+    return null;
+  }
+
+  public Object apiStepArgResGeneratedIgnored(Object x, Object y) {
+    return null;
+  }
+
+  public Object apiStepArgQualGenerated(Object x) {
+    return null;
+  }
+
+  public Object apiStepArgQualGeneratedIgnored(Object x) {
+    return null;
+  }
+
+  public Object id(Object x) {
+    return null;
+  }
+}
diff --git a/java/ql/test/library-tests/frameworks/apache-commons-lang3/flow.expected b/java/ql/test/library-tests/frameworks/apache-commons-lang3/flow.expected
diff --git a/java/ql/test/library-tests/logging/Test.java b/java/ql/test/library-tests/logging/Test.java
diff --git a/java/ql/test/library-tests/logging/test.expected b/java/ql/test/library-tests/logging/test.expected

-Original file line number
+Diff line change
@@ @@ -0,0 +1,4 @@ @@
 +---
 +category: majorAnalysis
 +---
 +* A generated (Models as Data) summary model is no longer used, if there exists a source code alternative. This primarily affects the analysis, when the analysis includes generated models for the source code being analysed.