Get rid of negative sink types.

Instead of positively implying the negative sink type, negative sink characteristics now negatively imply all sink types (but not source types). This is simpler and sice we will never have a huge number of sink types it doesn't impact performance either.

Changes to test results:

- The call to `createDirectories` at `Test.java:87` is now correctly classified as a source candidate, having previously been erroneously excluded by a negative _sink_ characteristic.
- The call to `compareTo` at `Test.java:48` is now erroneously classified as a source candidate; it should be suppressed by `IsSanitizerCharacteristic`, which is a negative sink characteristic, but should really be a negative source characteristic.
- In framework mode, several endpoints are now erroneously classified as source candidates even though they have neutral models, because `NeutralModelCharacteristic` is currently only a negative sink characteristic and not a negative source characteristic.

Author: Max Schaefer

java/ql/automodel/src/AutomodelApplicationModeCharacteristics.qll

@@ -259,7 +259,9 @@ module ApplicationCandidatesImpl implements SharedCharacteristics::CandidateSig
 
   class EndpointType = AutomodelEndpointTypes::EndpointType;
 
-  class NegativeEndpointType = AutomodelEndpointTypes::NegativeSinkType;
+  class SinkType = AutomodelEndpointTypes::SinkType;
+
+  class SourceType = AutomodelEndpointTypes::SourceType;
 
   class RelatedLocation = Location::Top;
 

java/ql/automodel/src/AutomodelApplicationModeExtractNegativeExamples.ql

@@ -48,18 +48,20 @@ from
 where
   endpoint = getSampleForCharacteristic(characteristic, 100) and
   extensibleType = endpoint.getExtensibleType() and
+  // the node is know not to be an endpoint of any appropriate type
+  forall(EndpointType tp | tp = endpoint.getAPotentialType() |
+    characteristic.hasImplications(tp, false, _)
+  ) and
+  // the lowest confidence across all endpoint types should be at least highConfidence
+  confidence = min(float c | characteristic.hasImplications(endpoint.getAPotentialType(), false, c)) and
   confidence >= SharedCharacteristics::highConfidence() and
-  characteristic.hasImplications(any(NegativeSinkType negative), true, confidence) and
   meta.hasMetadata(endpoint, package, type, subtypes, name, signature, input, output, isVarargsArray) and
-  // It's valid for a node to satisfy the logic for both `isSink` and `isSanitizer`, but in that case it will be
-  // treated by the actual query as a sanitizer, since the final logic is something like
-  // `isSink(n) and not isSanitizer(n)`. We don't want to include such nodes as negative examples in the prompt, because
-  // they're ambiguous and might confuse the model, so we explicitly exclude all known sinks from the negative examples.
-  not exists(EndpointCharacteristic characteristic2, float confidence2, SinkType positiveType |
-    not positiveType instanceof NegativeSinkType and
+  // It's valid for a node to be both a potential source/sanitizer and a sink. We don't want to include such nodes
+  // as negative examples in the prompt, because they're ambiguous and might confuse the model, so we explicitly them here.
+  not exists(EndpointCharacteristic characteristic2, float confidence2, EndpointType type2 |
     characteristic2.appliesToEndpoint(endpoint) and
     confidence2 >= SharedCharacteristics::maximalConfidence() and
-    characteristic2.hasImplications(positiveType, true, confidence2)
+    characteristic2.hasImplications(type2, true, confidence2)
   ) and
   message = characteristic
 select endpoint.asNode(),

java/ql/automodel/src/AutomodelEndpointTypes.qll

@@ -30,11 +30,6 @@ abstract class SinkType extends EndpointType {
   SinkType() { any() }
 }
 
-/** The `Negative` class for non-sinks. */
-class NegativeSinkType extends SinkType {
-  NegativeSinkType() { this = "non-sink" }
-}
-
 /** A sink relevant to the SQL injection query */
 class SqlInjectionSinkType extends SinkType {
   SqlInjectionSinkType() { this = "sql-injection" }

java/ql/automodel/src/AutomodelFrameworkModeCharacteristics.qll

@@ -214,7 +214,9 @@ module FrameworkCandidatesImpl implements SharedCharacteristics::CandidateSig {
 
   class EndpointType = AutomodelEndpointTypes::EndpointType;
 
-  class NegativeEndpointType = AutomodelEndpointTypes::NegativeSinkType;
+  class SinkType = AutomodelEndpointTypes::SinkType;
+
+  class SourceType = AutomodelEndpointTypes::SourceType;
 
   class RelatedLocation = Location::Top;
 

java/ql/automodel/src/AutomodelFrameworkModeExtractNegativeExamples.ql

@@ -21,18 +21,20 @@ from
 where
   endpoint.getExtensibleType() = extensibleType and
   characteristic.appliesToEndpoint(endpoint) and
+  // the node is known not to be an endpoint of any appropriate type
+  forall(EndpointType tp | tp = endpoint.getAPotentialType() |
+    characteristic.hasImplications(tp, false, _)
+  ) and
+  // the lowest confidence across all endpoint types should be at least highConfidence
+  confidence = min(float c | characteristic.hasImplications(endpoint.getAPotentialType(), false, c)) and
   confidence >= SharedCharacteristics::highConfidence() and
-  characteristic.hasImplications(any(NegativeSinkType negative), true, confidence) and
   meta.hasMetadata(endpoint, package, type, subtypes, name, signature, input, output, parameterName) and
-  // It's valid for a node to satisfy the logic for both `isSink` and `isSanitizer`, but in that case it will be
-  // treated by the actual query as a sanitizer, since the final logic is something like
-  // `isSink(n) and not isSanitizer(n)`. We don't want to include such nodes as negative examples in the prompt, because
-  // they're ambiguous and might confuse the model, so we explicitly exclude all known sinks from the negative examples.
-  not exists(EndpointCharacteristic characteristic2, float confidence2, SinkType positiveType |
-    not positiveType instanceof NegativeSinkType and
+  // It's valid for a node to be both a potential source/sanitizer and a sink. We don't want to include such nodes
+  // as negative examples in the prompt, because they're ambiguous and might confuse the model, so we explicitly them here.
+  not exists(EndpointCharacteristic characteristic2, float confidence2, EndpointType type2 |
     characteristic2.appliesToEndpoint(endpoint) and
     confidence2 >= SharedCharacteristics::maximalConfidence() and
-    characteristic2.hasImplications(positiveType, true, confidence2)
+    characteristic2.hasImplications(type2, true, confidence2)
   ) and
   message = characteristic
 select endpoint,

java/ql/automodel/src/AutomodelSharedCharacteristics.qll

@@ -33,14 +33,19 @@ signature module CandidateSig {
   class RelatedLocationType;
 
   /**
-   * A class kind for an endpoint.
+   * An endpoint type considered by this specification.
    */
   class EndpointType extends string;
 
   /**
-   * An EndpointType that denotes the absence of any sink.
+   * A sink endpoint type considered by this specification.
    */
-  class NegativeEndpointType extends EndpointType;
+  class SinkType extends EndpointType;
+
+  /**
+   * A source endpoint type considered by this specification.
+   */
+  class SourceType extends EndpointType;
 
   /**
    * Gets the endpoint as a location.
@@ -105,15 +110,14 @@ module SharedCharacteristics<CandidateSig Candidate> {
   }
 
   /**
-   * Holds if `endpoint` is modeled as `endpointType` (endpoint type must not be negative).
+   * Holds if `endpoint` is modeled as `endpointType`.
    */
   predicate isKnownAs(
     Candidate::Endpoint endpoint, Candidate::EndpointType endpointType,
     EndpointCharacteristic characteristic
   ) {
     // If the list of characteristics includes positive indicators with maximal confidence for this class, then it's a
     // known sink for the class.
-    not endpointType instanceof Candidate::NegativeEndpointType and
     characteristic.appliesToEndpoint(endpoint) and
     characteristic.hasImplications(endpointType, true, maximalConfidence())
   }
@@ -125,7 +129,6 @@ module SharedCharacteristics<CandidateSig Candidate> {
    * A candidate is an endpoint that cannot be excluded from `endpointType` based on its characteristics.
    */
   predicate isCandidate(Candidate::Endpoint endpoint, Candidate::EndpointType endpointType) {
-    not endpointType instanceof Candidate::NegativeEndpointType and
     endpointType = endpoint.getAPotentialType() and
     not exists(getAnExcludingCharacteristic(endpoint, endpointType))
   }
@@ -143,26 +146,16 @@ module SharedCharacteristics<CandidateSig Candidate> {
   }
 
   /**
-   * Gets a characteristics that disbar `endpoint` from being a candidate for `endpointType`.
+   * Gets a characteristics that disbar `endpoint` from being a candidate for `endpointType`
+   * with at least medium confidence.
    */
   EndpointCharacteristic getAnExcludingCharacteristic(
     Candidate::Endpoint endpoint, Candidate::EndpointType endpointType
   ) {
-    // An endpoint is a sink candidate if none of its characteristics give much indication whether or not it is a sink.
-    not endpointType instanceof Candidate::NegativeEndpointType and
     result.appliesToEndpoint(endpoint) and
-    (
-      // Exclude endpoints that have a characteristic that implies they're not sinks for _any_ sink type.
-      exists(float confidence |
-        confidence >= mediumConfidence() and
-        result.hasImplications(any(Candidate::NegativeEndpointType t), true, confidence)
-      )
-      or
-      // Exclude endpoints that have a characteristic that implies they're not sinks for _this particular_ sink type.
-      exists(float confidence |
-        confidence >= mediumConfidence() and
-        result.hasImplications(endpointType, false, confidence)
-      )
+    exists(float confidence |
+      confidence >= mediumConfidence() and
+      result.hasImplications(endpointType, false, confidence)
     )
   }
 
@@ -253,8 +246,8 @@ module SharedCharacteristics<CandidateSig Candidate> {
     override predicate hasImplications(
       Candidate::EndpointType endpointType, boolean isPositiveIndicator, float confidence
     ) {
-      endpointType instanceof Candidate::NegativeEndpointType and
-      isPositiveIndicator = true and
+      endpointType instanceof Candidate::SinkType and
+      isPositiveIndicator = false and
       confidence = highConfidence()
     }
   }
@@ -272,8 +265,8 @@ module SharedCharacteristics<CandidateSig Candidate> {
     override predicate hasImplications(
       Candidate::EndpointType endpointType, boolean isPositiveIndicator, float confidence
     ) {
-      endpointType instanceof Candidate::NegativeEndpointType and
-      isPositiveIndicator = true and
+      endpointType instanceof Candidate::SinkType and
+      isPositiveIndicator = false and
       confidence = mediumConfidence()
     }
   }
@@ -293,8 +286,8 @@ module SharedCharacteristics<CandidateSig Candidate> {
     override predicate hasImplications(
       Candidate::EndpointType endpointType, boolean isPositiveIndicator, float confidence
     ) {
-      endpointType instanceof Candidate::NegativeEndpointType and
-      isPositiveIndicator = true and
+      endpointType instanceof Candidate::SinkType and
+      isPositiveIndicator = false and
       confidence = mediumConfidence()
     }
   }

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

@@ -7,8 +7,10 @@
 | Test.java:36:10:38:3 | newInputStream(...) | Related locations: $@, $@, $@.\nmetadata: $@, $@, $@, $@, $@, $@, $@, $@, $@, $@. | Test.java:36:10:38:3 | newInputStream(...) | CallContext | Test.java:36:10:38:3 | newInputStream(...) | MethodDoc | Test.java:36:10:38:3 | newInputStream(...) | ClassDoc | 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://newInputStream:1:1:1:1 | newInputStream | name | file://(Path,OpenOption[]):1:1:1:1 | (Path,OpenOption[]) | signature | file://:1:1:1:1 |  | 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:37:4:37:11 | openPath | Related locations: $@, $@, $@.\nmetadata: $@, $@, $@, $@, $@, $@, $@, $@, $@, $@. | Test.java:36:10:38:3 | newInputStream(...) | CallContext | Test.java:37:4:37:11 | openPath | MethodDoc | Test.java:37:4:37:11 | openPath | ClassDoc | 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://newInputStream:1:1:1:1 | newInputStream | name | file://(Path,OpenOption[]):1:1:1:1 | (Path,OpenOption[]) | signature | file://Argument[0]:1:1:1:1 | Argument[0] | input | file://:1:1:1:1 |  | output | file://false:1:1:1:1 | false | isVarargsArray | file://ai-manual:1:1:1:1 | ai-manual | alreadyAiModeled | file://sinkModel:1:1:1:1 | sinkModel | extensibleType |
 | Test.java:43:4:43:22 | get(...) | Related locations: $@, $@, $@.\nmetadata: $@, $@, $@, $@, $@, $@, $@, $@, $@, $@. | Test.java:43:4:43:22 | get(...) | CallContext | Test.java:43:4:43:22 | get(...) | MethodDoc | Test.java:43:4:43:22 | get(...) | ClassDoc | file://java.nio.file:1:1:1:1 | java.nio.file | package | file://Paths:1:1:1:1 | Paths | type | file://false:1:1:1:1 | false | subtypes | file://get:1:1:1:1 | get | name | file://(String,String[]):1:1:1:1 | (String,String[]) | signature | file://:1:1:1:1 |  | 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:48:10:50:3 | compareTo(...) | Related locations: $@, $@, $@.\nmetadata: $@, $@, $@, $@, $@, $@, $@, $@, $@, $@. | Test.java:48:10:50:3 | compareTo(...) | CallContext | Test.java:48:10:50:3 | compareTo(...) | MethodDoc | Test.java:48:10:50:3 | 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://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:54:3:59:3 | walk(...) | Related locations: $@, $@, $@.\nmetadata: $@, $@, $@, $@, $@, $@, $@, $@, $@, $@. | Test.java:54:3:59:3 | walk(...) | CallContext | Test.java:54:3:59:3 | walk(...) | MethodDoc | Test.java:54:3:59:3 | walk(...) | ClassDoc | 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://:1:1:1:1 |  | 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:56:4:56:4 | o | Related locations: $@, $@, $@.\nmetadata: $@, $@, $@, $@, $@, $@, $@, $@, $@, $@. | Test.java:54:3:59:3 | walk(...) | CallContext | Test.java:54:3:59:3 | walk(...) | MethodDoc | Test.java:54:3:59:3 | walk(...) | ClassDoc | 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:63:3:63:3 | c | Related locations: $@, $@, $@.\nmetadata: $@, $@, $@, $@, $@, $@, $@, $@, $@, $@. | Test.java:63:3:63:20 | getInputStream(...) | CallContext | Test.java:63:3:63:3 | c | MethodDoc | Test.java:63:3:63:3 | c | ClassDoc | 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:68:30:68:47 | writer | Related locations: $@, $@, $@.\nmetadata: $@, $@, $@, $@, $@, $@, $@, $@, $@, $@. | Test.java:68:30:68:47 | writer | CallContext | Test.java:68:30:68:47 | writer | MethodDoc | Test.java:68:30:68:47 | writer | ClassDoc | file://java.lang:1:1:1:1 | java.lang | package | file://Throwable:1:1:1:1 | Throwable | type | file://true:1:1:1:1 | true | subtypes | file://printStackTrace:1:1:1:1 | printStackTrace | name | file://(PrintWriter):1:1:1:1 | (PrintWriter) | 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 |
 | Test.java:86:3:88:3 | list(...) | Related locations: $@, $@, $@.\nmetadata: $@, $@, $@, $@, $@, $@, $@, $@, $@, $@. | Test.java:86:3:88:3 | list(...) | CallContext | Test.java:86:3:88:3 | list(...) | MethodDoc | Test.java:86:3:88:3 | list(...) | ClassDoc | 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://list:1:1:1:1 | list | name | file://(Path):1:1:1:1 | (Path) | signature | file://:1:1:1:1 |  | 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:87:4:87:29 | createDirectories(...) | Related locations: $@, $@, $@.\nmetadata: $@, $@, $@, $@, $@, $@, $@, $@, $@, $@. | Test.java:87:4:87:29 | createDirectories(...) | CallContext | Test.java:87:4:87:29 | createDirectories(...) | MethodDoc | Test.java:87:4:87:29 | createDirectories(...) | ClassDoc | 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://createDirectories:1:1:1:1 | createDirectories | name | file://(Path,FileAttribute[]):1:1:1:1 | (Path,FileAttribute[]) | signature | file://:1:1:1:1 |  | 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 |

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

@@ -1,3 +1,2 @@
-| Test.java:48:10:50:3 | compareTo(...) | known sanitizer\nrelated locations: $@, $@, $@.\nmetadata: $@, $@, $@, $@, $@, $@, $@, $@, $@. | Test.java:48:10:50:3 | compareTo(...) | CallContext | Test.java:48:10:50:3 | compareTo(...) | MethodDoc | Test.java:48:10:50:3 | 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://ReturnValue:1:1:1:1 | ReturnValue | output | file://false:1:1:1:1 | false | isVarargsArray | file://sourceModel:1:1:1:1 | sourceModel | extensibleType |
 | Test.java:49:4:49:5 | f2 | known non-sink\nrelated locations: $@, $@, $@.\nmetadata: $@, $@, $@, $@, $@, $@, $@, $@, $@. | Test.java:48:10:50:3 | compareTo(...) | CallContext | Test.java:49:4:49:5 | f2 | MethodDoc | Test.java:49:4:49:5 | f2 | 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[0]:1:1:1:1 | Argument[0] | input | file://:1:1:1:1 |  | output | file://false:1:1:1:1 | false | isVarargsArray | file://sinkModel:1:1:1:1 | sinkModel | extensibleType |
 | Test.java:55:4:55:4 | p | taint step\nrelated locations: $@, $@, $@.\nmetadata: $@, $@, $@, $@, $@, $@, $@, $@, $@. | Test.java:54:3:59:3 | walk(...) | CallContext | Test.java:55:4:55:4 | p | MethodDoc | Test.java:55:4:55:4 | p | ClassDoc | 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[0]:1:1:1:1 | Argument[0] | input | file://:1:1:1:1 |  | output | file://false:1:1:1:1 | false | isVarargsArray | file://sinkModel:1:1:1:1 | sinkModel | extensibleType |