Java: add endpoints for parameters of overridden methods in automodel application mode

Author: Stephan Brandauer

java/ql/automodel/src/AutomodelApplicationModeCharacteristics.qll

@@ -36,12 +36,18 @@ newtype TApplicationModeEndpoint =
       not exists(int i | i < idx and call.getArgument(i).(Argument).isVararg())
     )
   } or
-  TMethodCall(Call call) { not call instanceof ConstructorCall }
+  TMethodCall(Call call) { not call instanceof ConstructorCall } or
+  TOverriddenParameter(Parameter p) {
+    not p.getCallable().callsConstructor(_) and
+    p.getCallable().(Method).overrides(_)
+  }
 
 /**
  * An endpoint is a node that is a candidate for modeling.
  */
 abstract private class ApplicationModeEndpoint extends TApplicationModeEndpoint {
+  abstract Callable getCallable();
+
   abstract Call getCall();
 
   abstract string getMaDInput();
@@ -74,6 +80,8 @@ class ExplicitArgument extends ApplicationModeEndpoint, TExplicitArgument {
 
   ExplicitArgument() { this = TExplicitArgument(call, arg) }
 
+  override Callable getCallable() { result = call.getCallee() }
+
   override Call getCall() { result = call }
 
   private int getArgIndex() { this.asTop() = call.getArgument(result) }
@@ -95,6 +103,8 @@ class InstanceArgument extends ApplicationModeEndpoint, TInstanceArgument {
 
   InstanceArgument() { this = TInstanceArgument(call, arg) }
 
+  override Callable getCallable() { result = call.getCallee() }
+
   override Call getCall() { result = call }
 
   override string getMaDInput() { result = "Argument[this]" }
@@ -124,13 +134,15 @@ class ImplicitVarargsArray extends ApplicationModeEndpoint, TImplicitVarargsArra
 
   ImplicitVarargsArray() { this = TImplicitVarargsArray(call, vararg, idx) }
 
+  override Callable getCallable() { result = call.getCallee() }
+
   override Call getCall() { result = call }
 
   override string getMaDInput() { result = "Argument[" + idx + "]" }
 
   override string getMaDOutput() { none() }
 
-  override Top asTop() { result = this.getCall() }
+  override Top asTop() { result = call }
 
   override DataFlow::Node asNode() { result = vararg }
 
@@ -145,6 +157,8 @@ class MethodCall extends ApplicationModeEndpoint, TMethodCall {
 
   MethodCall() { this = TMethodCall(call) }
 
+  override Callable getCallable() { result = call.getCallee() }
+
   override Call getCall() { result = call }
 
   override string getMaDInput() { result = "Argument[this]" }
@@ -158,6 +172,28 @@ class MethodCall extends ApplicationModeEndpoint, TMethodCall {
   override string toString() { result = call.toString() }
 }
 
+class OverriddenParameter extends ApplicationModeEndpoint, TOverriddenParameter {
+  Parameter p;
+
+  OverriddenParameter() { this = TOverriddenParameter(p) }
+
+  override Callable getCallable() { result = p.getCallable() }
+
+  override Call getCall() { none() }
+
+  private int getArgIndex() { p.getCallable().getParameter(result) = p }
+
+  override string getMaDInput() { none() }
+
+  override string getMaDOutput() { result = "Parameter[" + this.getArgIndex() + "]" }
+
+  override Top asTop() { result = p }
+
+  override DataFlow::Node asNode() { result.(DataFlow::ParameterNode).asParameter() = p }
+
+  override string toString() { result = p.toString() }
+}
+
 /**
  * A candidates implementation.
  *
@@ -208,7 +244,8 @@ module ApplicationCandidatesImpl implements SharedCharacteristics::CandidateSig
   predicate isSource(Endpoint e, string kind, string provenance) {
     exists(string package, string type, string name, string signature, string ext, string output |
       sourceSpec(e, package, type, name, signature, ext, output) and
-      ExternalFlow::sourceModel(package, type, _, name, [signature, ""], ext, output, kind, provenance)
+      ExternalFlow::sourceModel(package, type, _, name, [signature, ""], ext, output, kind,
+        provenance)
     )
   }
 
@@ -230,7 +267,8 @@ module ApplicationCandidatesImpl implements SharedCharacteristics::CandidateSig
   }
 
   additional predicate sourceSpec(
-    Endpoint e, string package, string type, string name, string signature, string ext, string output
+    Endpoint e, string package, string type, string name, string signature, string ext,
+    string output
   ) {
     ApplicationModeGetCallable::getCallable(e).hasQualifiedName(package, type, name) and
     signature = ExternalFlow::paramsString(ApplicationModeGetCallable::getCallable(e)) and
@@ -293,7 +331,7 @@ class ApplicationModeMetadataExtractor extends string {
     string input, string output, string isVarargsArray
   ) {
     exists(Callable callable |
-      e.getCall().getCallee() = callable and
+      e.getCallable() = callable and
       (if exists(e.getMaDInput()) then input = e.getMaDInput() else input = "") and
       (if exists(e.getMaDOutput()) then output = e.getMaDOutput() else output = "") and
       package = callable.getDeclaringType().getPackage().getName() and
@@ -328,8 +366,8 @@ private class UnexploitableIsCharacteristic extends CharacteristicsImpl::NotASin
 
   override predicate appliesToEndpoint(Endpoint e) {
     not ApplicationCandidatesImpl::isSink(e, _, _) and
-    ApplicationModeGetCallable::getCallable(e).getName().matches("is%") and
-    ApplicationModeGetCallable::getCallable(e).getReturnType() instanceof BooleanType
+    e.getCallable().getName().matches("is%") and
+    e.getCallable().getReturnType() instanceof BooleanType
   }
 }
 

java/ql/automodel/src/AutomodelApplicationModeExtractCandidates.ql

@@ -66,11 +66,11 @@ where
   endpoint =
     getSampleForSignature(9, package, type, subtypes, name, signature, input, output,
       isVarargsArray, extensibleType) and
-  // If a node is already a known sink for any of our existing ATM queries and is already modeled as a MaD sink, we
-  // don't include it as a candidate. Otherwise, we might include it as a candidate for query A, but the model will
-  // label it as a sink for one of the sink types of query B, for which it's already a known sink. This would result in
-  // overlap between our detected sinks and the pre-existing modeling. We assume that, if a sink has already been
-  // modeled in a MaD model, then it doesn't belong to any additional sink types, and we don't need to reexamine it.
+  // If a node is already modeled in MaD, we don't include it as a candidate. Otherwise, we might include it as a
+  // candidate for query A, but the model will label it as a sink for one of the sink types of query B, for which it's
+  // already a known sink. This would result in overlap between our detected sinks and the pre-existing modeling. We
+  // assume that, if a sink has already been modeled in a MaD model, then it doesn't belong to any additional sink
+  // types, and we don't need to reexamine it.
   (
     not CharacteristicsImpl::isSink(endpoint, _, _) and alreadyAiModeled = ""
     or

java/ql/automodel/test/AutomodelApplicationModeExtraction/AutomodelApplicationModeExtractCandidates.expected

@@ -9,3 +9,4 @@
 | Test.java:54:4:54:4 | o | command-injection, path-injection, request-forgery, sql-injection\nrelated locations: $@.\nmetadata: $@, $@, $@, $@, $@, $@, $@, $@, $@, $@. | Test.java:52:3:57:3 | walk(...) | CallContext | file://java.nio.file:1:1:1:1 | java.nio.file | package | file://Files:1:1:1:1 | Files | type | file://false:1:1:1:1 | false | subtypes | file://walk:1:1:1:1 | walk | name | file://(Path,FileVisitOption[]):1:1:1:1 | (Path,FileVisitOption[]) | signature | file://Argument[1]:1:1:1:1 | Argument[1] | input | file://:1:1:1:1 |  | output | file://true:1:1:1:1 | true | isVarargsArray | file://:1:1:1:1 |  | alreadyAiModeled | file://sinkModel:1:1:1:1 | sinkModel | extensibleType |
 | Test.java:61:3:61:3 | c | command-injection, path-injection, request-forgery, sql-injection\nrelated locations: $@.\nmetadata: $@, $@, $@, $@, $@, $@, $@, $@, $@, $@. | Test.java:61:3:61:20 | getInputStream(...) | CallContext | file://java.net:1:1:1:1 | java.net | package | file://URLConnection:1:1:1:1 | URLConnection | type | file://true:1:1:1:1 | true | subtypes | file://getInputStream:1:1:1:1 | getInputStream | name | file://():1:1:1:1 | () | signature | file://Argument[this]:1:1:1:1 | Argument[this] | input | file://:1:1:1:1 |  | output | file://false:1:1:1:1 | false | isVarargsArray | file://:1:1:1:1 |  | alreadyAiModeled | file://sinkModel:1:1:1:1 | sinkModel | extensibleType |
 | Test.java:61:3:61:20 | getInputStream(...) | command-injection, path-injection, request-forgery, sql-injection\nrelated locations: $@.\nmetadata: $@, $@, $@, $@, $@, $@, $@, $@, $@, $@. | Test.java:61:3:61:20 | getInputStream(...) | CallContext | file://java.net:1:1:1:1 | java.net | package | file://URLConnection:1:1:1:1 | URLConnection | type | file://true:1:1:1:1 | true | subtypes | file://getInputStream:1:1:1:1 | getInputStream | name | file://():1:1:1:1 | () | signature | file://Argument[this]:1:1:1:1 | Argument[this] | input | file://ReturnValue:1:1:1:1 | ReturnValue | output | file://false:1:1:1:1 | false | isVarargsArray | file://:1:1:1:1 |  | alreadyAiModeled | file://sourceModel:1:1:1:1 | sourceModel | extensibleType |
+| Test.java:66:24:66:31 | o | command-injection, path-injection, request-forgery, sql-injection\nrelated locations: $@.\nmetadata: $@, $@, $@, $@, $@, $@, $@, $@, $@, $@. | Test.java:66:24:66:31 | o | CallContext | file://com.github.codeql.test:1:1:1:1 | com.github.codeql.test | package | file://OverrideTest:1:1:1:1 | OverrideTest | type | file://true:1:1:1:1 | true | subtypes | file://equals:1:1:1:1 | equals | name | file://(Object):1:1:1:1 | (Object) | signature | file://:1:1:1:1 |  | input | file://Parameter[0]:1:1:1:1 | Parameter[0] | output | file://false:1:1:1:1 | false | isVarargsArray | file://:1:1:1:1 |  | alreadyAiModeled | file://sourceModel:1:1:1:1 | sourceModel | extensibleType |

java/ql/automodel/test/AutomodelApplicationModeExtraction/Test.java

@@ -61,3 +61,9 @@ public static void WebSocketExample(URLConnection c) throws Exception {
 		c.getInputStream(); // the call is a source example, c is a sink candidate
 	}
 }
+
+class OverrideTest {
+	public boolean equals(Object o) { // o is a source candidate because it overrides an existing method
+		return false;
+	}
+}