Make Unexploitable*Characteristic more precise.

Author: Max Schaefer

java/ql/automodel/src/AutomodelApplicationModeCharacteristics.qll

@@ -416,41 +416,56 @@ class ApplicationModeMetadataExtractor extends string {
  */
 
 /**
- * A negative characteristic that indicates that an is-style boolean method is unexploitable even if it is a sink.
+ * A negative characteristic that indicates that parameters of an is-style boolean method should not be considered sinks,
+ * and its return value should not be considered a source.
  *
  * A sink is highly unlikely to be exploitable if its callable's name starts with `is` and the callable has a boolean return
  * type (e.g. `isDirectory`). These kinds of calls normally do only checks, and appear before the proper call that does
  * the dangerous/interesting thing, so we want the latter to be modeled as the sink.
  *
  * TODO: this might filter too much, it's possible that methods with more than one parameter contain interesting sinks
  */
-private class UnexploitableIsCharacteristic extends CharacteristicsImpl::NotASinkCharacteristic {
+private class UnexploitableIsCharacteristic extends CharacteristicsImpl::NeitherSourceNorSinkCharacteristic
+{
   UnexploitableIsCharacteristic() { this = "unexploitable (is-style boolean method)" }
 
   override predicate appliesToEndpoint(Endpoint e) {
-    not ApplicationCandidatesImpl::isSink(e, _, _) and
     e.getCallable().getName().matches("is%") and
-    e.getCallable().getReturnType() instanceof BooleanType
+    e.getCallable().getReturnType() instanceof BooleanType and
+    (
+      e.getExtensibleType() = "sinkModel" and
+      not ApplicationCandidatesImpl::isSink(e, _, _)
+      or
+      e.getExtensibleType() = "sourceModel" and
+      not ApplicationCandidatesImpl::isSource(e, _, _) and
+      e.getMaDOutput() = "ReturnValue"
+    )
   }
 }
 
 /**
- * A negative characteristic that indicates that an existence-checking boolean method is unexploitable even if it is a
- * sink.
+ * A negative characteristic that indicates that parameters of an existence-checking boolean method should not be
+ * considered sinks, and its return value should not be considered a source.
  *
  * A sink is highly unlikely to be exploitable if its callable's name is `exists` or `notExists` and the callable has a
  * boolean return type. These kinds of calls normally do only checks, and appear before the proper call that does the
  * dangerous/interesting thing, so we want the latter to be modeled as the sink.
  */
-private class UnexploitableExistsCharacteristic extends CharacteristicsImpl::NotASinkCharacteristic {
+private class UnexploitableExistsCharacteristic extends CharacteristicsImpl::NeitherSourceNorSinkCharacteristic {
   UnexploitableExistsCharacteristic() { this = "unexploitable (existence-checking boolean method)" }
 
   override predicate appliesToEndpoint(Endpoint e) {
-    not ApplicationCandidatesImpl::isSink(e, _, _) and
     exists(Callable callable |
-      callable = ApplicationModeGetCallable::getCallable(e) and
+      callable = e.getCallable() and
       callable.getName().toLowerCase() = ["exists", "notexists"] and
       callable.getReturnType() instanceof BooleanType
+    |
+      e.getExtensibleType() = "sinkModel" and
+      not ApplicationCandidatesImpl::isSink(e, _, _)
+      or
+      e.getExtensibleType() = "sourceModel" and
+      not ApplicationCandidatesImpl::isSource(e, _, _) and
+      e.getMaDOutput() = "ReturnValue"
     )
   }
 }

java/ql/automodel/src/AutomodelFrameworkModeCharacteristics.qll

@@ -324,41 +324,55 @@ class FrameworkModeMetadataExtractor extends string {
  */
 
 /**
- * A negative characteristic that indicates that an is-style boolean method is unexploitable even if it is a sink.
+ * A negative characteristic that indicates that parameters of an is-style boolean method should not be considered sinks,
+ * and its return value should not be considered a source.
  *
  * A sink is highly unlikely to be exploitable if its callable's name starts with `is` and the callable has a boolean return
  * type (e.g. `isDirectory`). These kinds of calls normally do only checks, and appear before the proper call that does
  * the dangerous/interesting thing, so we want the latter to be modeled as the sink.
  *
  * TODO: this might filter too much, it's possible that methods with more than one parameter contain interesting sinks
  */
-private class UnexploitableIsCharacteristic extends CharacteristicsImpl::NotASinkCharacteristic {
+private class UnexploitableIsCharacteristic extends CharacteristicsImpl::NeitherSourceNorSinkCharacteristic {
   UnexploitableIsCharacteristic() { this = "unexploitable (is-style boolean method)" }
 
   override predicate appliesToEndpoint(Endpoint e) {
-    not FrameworkCandidatesImpl::isSink(e, _, _) and
     e.getEnclosingCallable().getName().matches("is%") and
-    e.getEnclosingCallable().getReturnType() instanceof BooleanType
+    e.getEnclosingCallable().getReturnType() instanceof BooleanType and
+    (
+      e.getExtensibleType() = "sinkModel" and
+      not FrameworkCandidatesImpl::isSink(e, _, _)
+      or
+      e.getExtensibleType() = "sourceModel" and
+      not FrameworkCandidatesImpl::isSource(e, _, _) and
+      e.getMaDOutput() = "ReturnValue"
+    )
   }
 }
 
 /**
- * A negative characteristic that indicates that an existence-checking boolean method is unexploitable even if it is a
- * sink.
+ * A negative characteristic that indicates that parameters of an existence-checking boolean method should not be
+ * considered sinks, and its return value should not be considered a source.
  *
  * A sink is highly unlikely to be exploitable if its callable's name is `exists` or `notExists` and the callable has a
  * boolean return type. These kinds of calls normally do only checks, and appear before the proper call that does the
  * dangerous/interesting thing, so we want the latter to be modeled as the sink.
  */
-private class UnexploitableExistsCharacteristic extends CharacteristicsImpl::NotASinkCharacteristic {
+private class UnexploitableExistsCharacteristic extends CharacteristicsImpl::NeitherSourceNorSinkCharacteristic {
   UnexploitableExistsCharacteristic() { this = "unexploitable (existence-checking boolean method)" }
 
   override predicate appliesToEndpoint(Endpoint e) {
-    not FrameworkCandidatesImpl::isSink(e, _, _) and
     exists(Callable callable |
       callable = e.getEnclosingCallable() and
       callable.getName().toLowerCase() = ["exists", "notexists"] and
       callable.getReturnType() instanceof BooleanType
+    |
+      e.getExtensibleType() = "sourceModel" and
+      not FrameworkCandidatesImpl::isSink(e, _, _)
+      or
+      e.getExtensibleType() = "sourceModel" and
+      not FrameworkCandidatesImpl::isSource(e, _, _) and
+      e.getMaDOutput() = "ReturnValue"
     )
   }
 }

java/ql/automodel/src/AutomodelSharedCharacteristics.qll

@@ -269,6 +269,22 @@ module SharedCharacteristics<CandidateSig Candidate> {
     }
   }
 
+  /**
+   * A high-confidence characteristic that indicates that an endpoint is neither a source nor a sink of any type.
+   */
+  abstract class NeitherSourceNorSinkCharacteristic extends NotASinkCharacteristic, NotASourceCharacteristic {
+    bindingset[this]
+    NeitherSourceNorSinkCharacteristic() { any() }
+
+    final override predicate hasImplications(
+      Candidate::EndpointType endpointType, boolean isPositiveIndicator, float confidence
+    ) {
+      NotASinkCharacteristic.super.hasImplications(endpointType, isPositiveIndicator, confidence) or
+      NotASourceCharacteristic.super
+          .hasImplications(endpointType, isPositiveIndicator, confidence)
+    }
+  }
+
   /**
    * A medium-confidence characteristic that indicates that an endpoint is unlikely to be a sink of any type. These
    * endpoints can be excluded from scoring at inference time, both to save time and to avoid false positives. They should
@@ -357,20 +373,10 @@ module SharedCharacteristics<CandidateSig Candidate> {
     /**
      * A negative characteristic that indicates that an endpoint was manually modeled as a neutral model.
      */
-    private class NeutralModelCharacteristic extends NotASinkCharacteristic,
-      NotASourceCharacteristic
+    private class NeutralModelCharacteristic extends NeitherSourceNorSinkCharacteristic
     {
       NeutralModelCharacteristic() { this = "known non-endpoint" }
 
-      // this is a negative characteristic for both sinks and sources
-      override predicate hasImplications(
-        Candidate::EndpointType endpointType, boolean isPositiveIndicator, float confidence
-      ) {
-        NotASinkCharacteristic.super.hasImplications(endpointType, isPositiveIndicator, confidence) or
-        NotASourceCharacteristic.super
-            .hasImplications(endpointType, isPositiveIndicator, confidence)
-      }
-
       override predicate appliesToEndpoint(Candidate::Endpoint e) { Candidate::isNeutral(e) }
     }
 

java/ql/automodel/test/AutomodelFrameworkModeExtraction/AutomodelFrameworkModeExtractCandidates.expected

@@ -9,6 +9,8 @@
 | com/github/codeql/test/PublicClass.java:13:33:13:42 | arg | Related locations: $@, $@.\nmetadata: $@, $@, $@, $@, $@, $@, $@, $@, $@, $@. | com/github/codeql/test/PublicClass.java:13:33:13:42 | arg | MethodDoc | com/github/codeql/test/PublicClass.java:13:33:13:42 | arg | ClassDoc | file://com.github.codeql.test:1:1:1:1 | com.github.codeql.test | package | file://PublicClass:1:1:1:1 | PublicClass | type | file://true:1:1:1:1 | true | subtypes | file://nonPublicStuff:1:1:1:1 | nonPublicStuff | name | file://(String):1:1:1:1 | (String) | signature | file://Argument[0]:1:1:1:1 | Argument[0] | input | file://:1:1:1:1 |  | output | file://arg:1:1:1:1 | arg | parameterName | file://:1:1:1:1 |  | alreadyAiModeled | file://sinkModel:1:1:1:1 | sinkModel | extensibleType |
 | com/github/codeql/test/PublicClass.java:22:10:22:20 | PublicClass | Related locations: $@, $@.\nmetadata: $@, $@, $@, $@, $@, $@, $@, $@, $@, $@. | com/github/codeql/test/PublicClass.java:22:10:22:20 | PublicClass | MethodDoc | com/github/codeql/test/PublicClass.java:22:10:22:20 | PublicClass | ClassDoc | file://com.github.codeql.test:1:1:1:1 | com.github.codeql.test | package | file://PublicClass:1:1:1:1 | PublicClass | type | file://true:1:1:1:1 | true | subtypes | file://PublicClass:1:1:1:1 | PublicClass | name | file://(Object):1:1:1:1 | (Object) | signature | file://:1:1:1:1 |  | input | file://ReturnValue:1:1:1:1 | ReturnValue | output | file://:1:1:1:1 |  | parameterName | file://:1:1:1:1 |  | alreadyAiModeled | file://sourceModel:1:1:1:1 | sourceModel | extensibleType |
 | com/github/codeql/test/PublicClass.java:22:22:22:33 | input | Related locations: $@, $@.\nmetadata: $@, $@, $@, $@, $@, $@, $@, $@, $@, $@. | com/github/codeql/test/PublicClass.java:22:22:22:33 | input | MethodDoc | com/github/codeql/test/PublicClass.java:22:22:22:33 | input | ClassDoc | file://com.github.codeql.test:1:1:1:1 | com.github.codeql.test | package | file://PublicClass:1:1:1:1 | PublicClass | type | file://true:1:1:1:1 | true | subtypes | file://PublicClass:1:1:1:1 | PublicClass | name | file://(Object):1:1:1:1 | (Object) | signature | file://Argument[0]:1:1:1:1 | Argument[0] | input | file://:1:1:1:1 |  | output | file://input:1:1:1:1 | input | parameterName | file://:1:1:1:1 |  | alreadyAiModeled | file://sinkModel:1:1:1:1 | sinkModel | extensibleType |
+| com/github/codeql/test/PublicClass.java:26:18:26:26 | isIgnored | Related locations: $@, $@.\nmetadata: $@, $@, $@, $@, $@, $@, $@, $@, $@, $@. | com/github/codeql/test/PublicClass.java:26:18:26:26 | isIgnored | MethodDoc | com/github/codeql/test/PublicClass.java:26:18:26:26 | isIgnored | ClassDoc | file://com.github.codeql.test:1:1:1:1 | com.github.codeql.test | package | file://PublicClass:1:1:1:1 | PublicClass | type | file://true:1:1:1:1 | true | subtypes | file://isIgnored:1:1:1:1 | isIgnored | name | file://(Object):1:1:1:1 | (Object) | signature | file://:1:1:1:1 |  | input | file://Parameter[this]:1:1:1:1 | Parameter[this] | output | file://this:1:1:1:1 | this | parameterName | file://:1:1:1:1 |  | alreadyAiModeled | file://sourceModel:1:1:1:1 | sourceModel | extensibleType |
+| com/github/codeql/test/PublicClass.java:26:28:26:39 | input | Related locations: $@, $@.\nmetadata: $@, $@, $@, $@, $@, $@, $@, $@, $@, $@. | com/github/codeql/test/PublicClass.java:26:28:26:39 | input | MethodDoc | com/github/codeql/test/PublicClass.java:26:28:26:39 | input | ClassDoc | file://com.github.codeql.test:1:1:1:1 | com.github.codeql.test | package | file://PublicClass:1:1:1:1 | PublicClass | type | file://true:1:1:1:1 | true | subtypes | file://isIgnored:1:1:1:1 | isIgnored | 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://input:1:1:1:1 | input | parameterName | file://:1:1:1:1 |  | alreadyAiModeled | file://sourceModel:1:1:1:1 | sourceModel | extensibleType |
 | com/github/codeql/test/PublicInterface.java:4:16:4:20 | stuff | Related locations: $@, $@.\nmetadata: $@, $@, $@, $@, $@, $@, $@, $@, $@, $@. | com/github/codeql/test/PublicInterface.java:4:16:4:20 | stuff | MethodDoc | com/github/codeql/test/PublicInterface.java:4:16:4:20 | stuff | ClassDoc | file://com.github.codeql.test:1:1:1:1 | com.github.codeql.test | package | file://PublicInterface:1:1:1:1 | PublicInterface | type | file://true:1:1:1:1 | true | subtypes | file://stuff:1:1:1:1 | stuff | name | file://(String):1:1:1:1 | (String) | signature | file://:1:1:1:1 |  | input | file://Parameter[this]:1:1:1:1 | Parameter[this] | output | file://this:1:1:1:1 | this | parameterName | file://:1:1:1:1 |  | alreadyAiModeled | file://sourceModel:1:1:1:1 | sourceModel | extensibleType |
 | com/github/codeql/test/PublicInterface.java:4:16:4:20 | stuff | Related locations: $@, $@.\nmetadata: $@, $@, $@, $@, $@, $@, $@, $@, $@, $@. | com/github/codeql/test/PublicInterface.java:4:16:4:20 | stuff | MethodDoc | com/github/codeql/test/PublicInterface.java:4:16:4:20 | stuff | ClassDoc | file://com.github.codeql.test:1:1:1:1 | com.github.codeql.test | package | file://PublicInterface:1:1:1:1 | PublicInterface | type | file://true:1:1:1:1 | true | subtypes | file://stuff:1:1:1:1 | stuff | name | file://(String):1:1:1:1 | (String) | signature | file://Argument[this]:1:1:1:1 | Argument[this] | input | file://:1:1:1:1 |  | output | file://this:1:1:1:1 | this | parameterName | file://:1:1:1:1 |  | alreadyAiModeled | file://sinkModel:1:1:1:1 | sinkModel | extensibleType |
 | com/github/codeql/test/PublicInterface.java:4:22:4:31 | arg | Related locations: $@, $@.\nmetadata: $@, $@, $@, $@, $@, $@, $@, $@, $@, $@. | com/github/codeql/test/PublicInterface.java:4:22:4:31 | arg | MethodDoc | com/github/codeql/test/PublicInterface.java:4:22:4:31 | arg | ClassDoc | file://com.github.codeql.test:1:1:1:1 | com.github.codeql.test | package | file://PublicInterface:1:1:1:1 | PublicInterface | type | file://true:1:1:1:1 | true | subtypes | file://stuff:1:1:1:1 | stuff | name | file://(String):1:1:1:1 | (String) | signature | file://:1:1:1:1 |  | input | file://Parameter[0]:1:1:1:1 | Parameter[0] | output | file://arg:1:1:1:1 | arg | parameterName | file://:1:1:1:1 |  | alreadyAiModeled | file://sourceModel:1:1:1:1 | sourceModel | extensibleType |

java/ql/automodel/test/AutomodelFrameworkModeExtraction/AutomodelFrameworkModeExtractNegativeExamples.expected

@@ -1,3 +1,6 @@
+| com/github/codeql/test/PublicClass.java:26:18:26:26 | isIgnored | unexploitable (is-style boolean method)\nrelated locations: $@, $@.\nmetadata: $@, $@, $@, $@, $@, $@, $@, $@, $@. | com/github/codeql/test/PublicClass.java:26:18:26:26 | isIgnored | MethodDoc | com/github/codeql/test/PublicClass.java:26:18:26:26 | isIgnored | ClassDoc | file://com.github.codeql.test:1:1:1:1 | com.github.codeql.test | package | file://PublicClass:1:1:1:1 | PublicClass | type | file://true:1:1:1:1 | true | subtypes | file://isIgnored:1:1:1:1 | isIgnored | name | file://(Object):1:1:1:1 | (Object) | signature | file://:1:1:1:1 |  | input | file://ReturnValue:1:1:1:1 | ReturnValue | output | file://:1:1:1:1 |  | parameterName | file://sourceModel:1:1:1:1 | sourceModel | extensibleType |
+| com/github/codeql/test/PublicClass.java:26:18:26:26 | isIgnored | unexploitable (is-style boolean method)\nrelated locations: $@, $@.\nmetadata: $@, $@, $@, $@, $@, $@, $@, $@, $@. | com/github/codeql/test/PublicClass.java:26:18:26:26 | isIgnored | MethodDoc | com/github/codeql/test/PublicClass.java:26:18:26:26 | isIgnored | ClassDoc | file://com.github.codeql.test:1:1:1:1 | com.github.codeql.test | package | file://PublicClass:1:1:1:1 | PublicClass | type | file://true:1:1:1:1 | true | subtypes | file://isIgnored:1:1:1:1 | isIgnored | name | file://(Object):1:1:1:1 | (Object) | signature | file://Argument[this]:1:1:1:1 | Argument[this] | input | file://:1:1:1:1 |  | output | file://this:1:1:1:1 | this | parameterName | file://sinkModel:1:1:1:1 | sinkModel | extensibleType |
+| com/github/codeql/test/PublicClass.java:26:28:26:39 | input | unexploitable (is-style boolean method)\nrelated locations: $@, $@.\nmetadata: $@, $@, $@, $@, $@, $@, $@, $@, $@. | com/github/codeql/test/PublicClass.java:26:28:26:39 | input | MethodDoc | com/github/codeql/test/PublicClass.java:26:28:26:39 | input | ClassDoc | file://com.github.codeql.test:1:1:1:1 | com.github.codeql.test | package | file://PublicClass:1:1:1:1 | PublicClass | type | file://true:1:1:1:1 | true | subtypes | file://isIgnored:1:1:1:1 | isIgnored | name | file://(Object):1:1:1:1 | (Object) | signature | file://Argument[0]:1:1:1:1 | Argument[0] | input | file://:1:1:1:1 |  | output | file://input:1:1:1:1 | input | parameterName | file://sinkModel:1:1:1:1 | sinkModel | extensibleType |
 | java/io/File.java:4:16:4:24 | compareTo | known non-endpoint\nrelated locations: $@, $@.\nmetadata: $@, $@, $@, $@, $@, $@, $@, $@, $@. | java/io/File.java:4:16:4:24 | compareTo | MethodDoc | java/io/File.java:4:16:4:24 | compareTo | ClassDoc | file://java.io:1:1:1:1 | java.io | package | file://File:1:1:1:1 | File | type | file://true:1:1:1:1 | true | subtypes | file://compareTo:1:1:1:1 | compareTo | name | file://(File):1:1:1:1 | (File) | signature | file://:1:1:1:1 |  | input | file://Parameter[this]:1:1:1:1 | Parameter[this] | output | file://this:1:1:1:1 | this | parameterName | file://sourceModel:1:1:1:1 | sourceModel | extensibleType |
 | java/io/File.java:4:16:4:24 | compareTo | known non-endpoint\nrelated locations: $@, $@.\nmetadata: $@, $@, $@, $@, $@, $@, $@, $@, $@. | java/io/File.java:4:16:4:24 | compareTo | MethodDoc | java/io/File.java:4:16:4:24 | compareTo | ClassDoc | file://java.io:1:1:1:1 | java.io | package | file://File:1:1:1:1 | File | type | file://true:1:1:1:1 | true | subtypes | file://compareTo:1:1:1:1 | compareTo | name | file://(File):1:1:1:1 | (File) | signature | file://Argument[this]:1:1:1:1 | Argument[this] | input | file://:1:1:1:1 |  | output | file://this:1:1:1:1 | this | parameterName | file://sinkModel:1:1:1:1 | sinkModel | extensibleType |
 | java/io/File.java:5:9:5:21 | pathname | known non-endpoint\nrelated locations: $@, $@.\nmetadata: $@, $@, $@, $@, $@, $@, $@, $@, $@. | java/io/File.java:5:9:5:21 | pathname | MethodDoc | java/io/File.java:5:9:5:21 | pathname | ClassDoc | file://java.io:1:1:1:1 | java.io | package | file://File:1:1:1:1 | File | type | file://true:1:1:1:1 | true | subtypes | file://compareTo:1:1:1:1 | compareTo | name | file://(File):1:1:1:1 | (File) | signature | file://:1:1:1:1 |  | input | file://Parameter[0]:1:1:1:1 | Parameter[0] | output | file://pathname:1:1:1:1 | pathname | parameterName | file://sourceModel:1:1:1:1 | sourceModel | extensibleType |

java/ql/automodel/test/AutomodelFrameworkModeExtraction/com/github/codeql/test/PublicClass.java

@@ -5,7 +5,7 @@ public void stuff(String arg) { // `arg` is a sink candidate, `this` is a candid
     System.out.println(arg);
   }
 
-  public static void staticStuff(String arg) { // `arg` is a candidate, `this` is not a candidate (static method), `arg` is not a source candidate (static methods can not be overloaded)
+  public static void staticStuff(String arg) { // `arg` is a sink candidate, but not a source candidate (not overrideabe); `this` is not a candidate (static method)
     System.out.println(arg);
   }
 
@@ -22,4 +22,8 @@ void packagePrivateStuff(String arg) {
   public PublicClass(Object input) {
     // the `this` qualifier is not a candidate
   }
+
+  public Boolean isIgnored(Object input) { // `input` is a source candidate, but not a sink candidate (is-style method); `this` is not a candidate
+    return false;
+  }
 }