Remove redundant code

`isOtherModeledArgument` and `isArgumentToBuiltinFunction` contained the old logic for selecting negative endpoints for training.

These can now be deleted, and replaced by a single base class that collects all EndpointCharacteristics that are currently used to indicate negative training samples: `OtherModeledArgumentCharacteristic`.

This in turn lets us delete code from `StandardEndpointFilters` that effectively said that endpoints that are high-confidence non-sinks shouldn't be scored at inference time, either.

Author: tiferet

javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/CoreKnowledge.qll

@@ -44,7 +44,6 @@ private import semmle.javascript.security.dataflow.HttpToFileAccessCustomization
 private import semmle.javascript.security.dataflow.BrokenCryptoAlgorithmCustomizations
 private import semmle.javascript.security.dataflow.LoopBoundInjectionCustomizations
 private import semmle.javascript.security.dataflow.CleartextStorageCustomizations
-import FilteringReasons
 
 /**
  * Holds if the node `n` is a known sink in a modeled library, or a sibling-argument of such a sink.
@@ -110,116 +109,3 @@ predicate isKnownStepSrc(DataFlow::Node n) {
   DataFlow::SharedFlowStep::step(n, _) or
   DataFlow::SharedFlowStep::step(n, _, _, _)
 }
-
-/**
- * Holds if `n` is an argument to a function of a builtin object.
- */
-private predicate isArgumentToBuiltinFunction(DataFlow::Node n, FilteringReason reason) {
-  exists(DataFlow::SourceNode builtin, DataFlow::SourceNode receiver, DataFlow::InvokeNode invk |
-    (
-      builtin instanceof DataFlow::ArrayCreationNode and
-      reason instanceof ArgumentToArrayReason
-      or
-      builtin =
-        DataFlow::globalVarRef([
-            "Map", "Set", "WeakMap", "WeakSet", "Number", "Object", "String", "Array", "Error",
-            "Math", "Boolean"
-          ]) and
-      reason instanceof ArgumentToBuiltinGlobalVarRefReason
-    )
-  |
-    receiver = [builtin.getAnInvocation(), builtin] and
-    invk = [receiver, receiver.getAPropertyRead()].getAnInvocation() and
-    invk.getAnArgument() = n
-  )
-  or
-  exists(Expr primitive, MethodCallExpr c |
-    primitive instanceof ConstantString or
-    primitive instanceof NumberLiteral or
-    primitive instanceof BooleanLiteral
-  |
-    c.calls(primitive, _) and
-    c.getAnArgument() = n.asExpr() and
-    reason instanceof ConstantReceiverReason
-  )
-  or
-  exists(DataFlow::CallNode call |
-    call.getAnArgument() = n and
-    call.getCalleeName() =
-      [
-        "indexOf", "hasOwnProperty", "substring", "isDecimal", "decode", "encode", "keys", "shift",
-        "values", "forEach", "toString", "slice", "splice", "push", "isArray", "sort"
-      ] and
-    reason instanceof BuiltinCallNameReason
-  )
-}
-
-predicate isOtherModeledArgument(DataFlow::Node n, FilteringReason reason) {
-  isArgumentToBuiltinFunction(n, reason)
-  or
-  any(LodashUnderscore::Member m).getACall().getAnArgument() = n and
-  reason instanceof LodashUnderscoreArgumentReason
-  or
-  any(JQuery::MethodCall m).getAnArgument() = n and
-  reason instanceof JQueryArgumentReason
-  or
-  exists(ClientRequest r |
-    r.getAnArgument() = n or n = r.getUrl() or n = r.getHost() or n = r.getADataNode()
-  ) and
-  reason instanceof ClientRequestReason
-  or
-  exists(PromiseDefinition p |
-    n = [p.getResolveParameter(), p.getRejectParameter()].getACall().getAnArgument()
-  ) and
-  reason instanceof PromiseDefinitionReason
-  or
-  n instanceof CryptographicKey and reason instanceof CryptographicKeyReason
-  or
-  any(CryptographicOperation op).getInput() = n and
-  reason instanceof CryptographicOperationFlowReason
-  or
-  exists(DataFlow::CallNode call | n = call.getAnArgument() |
-    call.getCalleeName() = getAStandardLoggerMethodName() and
-    reason instanceof LoggerMethodReason
-    or
-    call.getCalleeName() = ["setTimeout", "clearTimeout"] and
-    reason instanceof TimeoutReason
-    or
-    call.getReceiver() = DataFlow::globalVarRef(["localStorage", "sessionStorage"]) and
-    reason instanceof ReceiverStorageReason
-    or
-    call instanceof StringOps::StartsWith and reason instanceof StringStartsWithReason
-    or
-    call instanceof StringOps::EndsWith and reason instanceof StringEndsWithReason
-    or
-    call instanceof StringOps::RegExpTest and reason instanceof StringRegExpTestReason
-    or
-    call instanceof EventRegistration and reason instanceof EventRegistrationReason
-    or
-    call instanceof EventDispatch and reason instanceof EventDispatchReason
-    or
-    call = any(MembershipCandidate c).getTest() and
-    reason instanceof MembershipCandidateTestReason
-    or
-    call instanceof FileSystemAccess and reason instanceof FileSystemAccessReason
-    or
-    // TODO database accesses are less well defined than database query sinks, so this may cover unmodeled sinks on existing database models
-    [
-      call, call.getAMethodCall()
-    /* command pattern where the query is built, and then exec'ed later */ ] instanceof
-      DatabaseAccess and
-    reason instanceof DatabaseAccessReason
-    or
-    call = DOM::domValueRef() and reason instanceof DomReason
-    or
-    call.getCalleeName() = "next" and
-    exists(DataFlow::FunctionNode f | call = f.getLastParameter().getACall()) and
-    reason instanceof NextFunctionCallReason
-    or
-    call = DataFlow::globalVarRef("dojo").getAPropertyRead("require").getACall() and
-    reason instanceof DojoRequireReason
-  )
-  or
-  (exists(Base64::Decode d | n = d.getInput()) or exists(Base64::Encode d | n = d.getInput())) and
-  reason instanceof Base64ManipulationReason
-}

javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/EndpointCharacteristics.qll

@@ -143,11 +143,19 @@ private class NosqlInjectionSinkCharacteristic extends EndpointCharacteristic {
  * negative samples for training.
  */
 
+/**
+ * A characteristic that is an indicator of not being a sink of any type, because it's a modeled argument.
+ */
+abstract class OtherModeledArgumentCharacteristic extends EndpointCharacteristic {
+  bindingset[this]
+  OtherModeledArgumentCharacteristic() { any() }
+}
+
 /**
  * A characteristic that is an indicator of not being a sink of any type, because it's an argument to a function of a
  * builtin object.
  */
-abstract private class ArgumentToBuiltinFunctionCharacteristic extends EndpointCharacteristic {
+abstract private class ArgumentToBuiltinFunctionCharacteristic extends OtherModeledArgumentCharacteristic {
   bindingset[this]
   ArgumentToBuiltinFunctionCharacteristic() { any() }
 }
@@ -187,23 +195,26 @@ abstract class LikelyNotASinkCharacteristic extends EndpointCharacteristic {
   }
 }
 
-private class LodashUnderscore extends NotASinkCharacteristic {
-  LodashUnderscore() { this = "LodashUnderscoreArgument" }
+private class LodashUnderscoreCharacteristic extends NotASinkCharacteristic,
+  OtherModeledArgumentCharacteristic {
+  LodashUnderscoreCharacteristic() { this = "LodashUnderscoreArgument" }
 
   override predicate getEndpoints(DataFlow::Node n) {
     any(LodashUnderscore::Member m).getACall().getAnArgument() = n
   }
 }
 
-private class JQueryArgumentCharacteristic extends NotASinkCharacteristic {
+private class JQueryArgumentCharacteristic extends NotASinkCharacteristic,
+  OtherModeledArgumentCharacteristic {
   JQueryArgumentCharacteristic() { this = "JQueryArgument" }
 
   override predicate getEndpoints(DataFlow::Node n) {
     any(JQuery::MethodCall m).getAnArgument() = n
   }
 }
 
-private class ClientRequestCharacteristic extends NotASinkCharacteristic {
+private class ClientRequestCharacteristic extends NotASinkCharacteristic,
+  OtherModeledArgumentCharacteristic {
   ClientRequestCharacteristic() { this = "ClientRequest" }
 
   override predicate getEndpoints(DataFlow::Node n) {
@@ -213,7 +224,8 @@ private class ClientRequestCharacteristic extends NotASinkCharacteristic {
   }
 }
 
-private class PromiseDefinitionCharacteristic extends NotASinkCharacteristic {
+private class PromiseDefinitionCharacteristic extends NotASinkCharacteristic,
+  OtherModeledArgumentCharacteristic {
   PromiseDefinitionCharacteristic() { this = "PromiseDefinition" }
 
   override predicate getEndpoints(DataFlow::Node n) {
@@ -223,21 +235,24 @@ private class PromiseDefinitionCharacteristic extends NotASinkCharacteristic {
   }
 }
 
-private class CryptographicKeyCharacteristic extends NotASinkCharacteristic {
+private class CryptographicKeyCharacteristic extends NotASinkCharacteristic,
+  OtherModeledArgumentCharacteristic {
   CryptographicKeyCharacteristic() { this = "CryptographicKey" }
 
   override predicate getEndpoints(DataFlow::Node n) { n instanceof CryptographicKey }
 }
 
-private class CryptographicOperationFlowCharacteristic extends NotASinkCharacteristic {
+private class CryptographicOperationFlowCharacteristic extends NotASinkCharacteristic,
+  OtherModeledArgumentCharacteristic {
   CryptographicOperationFlowCharacteristic() { this = "CryptographicOperationFlow" }
 
   override predicate getEndpoints(DataFlow::Node n) {
     any(CryptographicOperation op).getInput() = n
   }
 }
 
-private class LoggerMethodCharacteristic extends NotASinkCharacteristic {
+private class LoggerMethodCharacteristic extends NotASinkCharacteristic,
+  OtherModeledArgumentCharacteristic {
   LoggerMethodCharacteristic() { this = "LoggerMethod" }
 
   override predicate getEndpoints(DataFlow::Node n) {
@@ -247,7 +262,8 @@ private class LoggerMethodCharacteristic extends NotASinkCharacteristic {
   }
 }
 
-private class TimeoutCharacteristic extends NotASinkCharacteristic {
+private class TimeoutCharacteristic extends NotASinkCharacteristic,
+  OtherModeledArgumentCharacteristic {
   TimeoutCharacteristic() { this = "Timeout" }
 
   override predicate getEndpoints(DataFlow::Node n) {
@@ -257,7 +273,8 @@ private class TimeoutCharacteristic extends NotASinkCharacteristic {
   }
 }
 
-private class ReceiverStorageCharacteristic extends NotASinkCharacteristic {
+private class ReceiverStorageCharacteristic extends NotASinkCharacteristic,
+  OtherModeledArgumentCharacteristic {
   ReceiverStorageCharacteristic() { this = "ReceiverStorage" }
 
   override predicate getEndpoints(DataFlow::Node n) {
@@ -267,7 +284,8 @@ private class ReceiverStorageCharacteristic extends NotASinkCharacteristic {
   }
 }
 
-private class StringStartsWithCharacteristic extends NotASinkCharacteristic {
+private class StringStartsWithCharacteristic extends NotASinkCharacteristic,
+  OtherModeledArgumentCharacteristic {
   StringStartsWithCharacteristic() { this = "StringStartsWith" }
 
   override predicate getEndpoints(DataFlow::Node n) {
@@ -277,15 +295,17 @@ private class StringStartsWithCharacteristic extends NotASinkCharacteristic {
   }
 }
 
-private class StringEndsWithCharacteristic extends NotASinkCharacteristic {
+private class StringEndsWithCharacteristic extends NotASinkCharacteristic,
+  OtherModeledArgumentCharacteristic {
   StringEndsWithCharacteristic() { this = "StringEndsWith" }
 
   override predicate getEndpoints(DataFlow::Node n) {
     exists(DataFlow::CallNode call | n = call.getAnArgument() | call instanceof StringOps::EndsWith)
   }
 }
 
-private class StringRegExpTestCharacteristic extends NotASinkCharacteristic {
+private class StringRegExpTestCharacteristic extends NotASinkCharacteristic,
+  OtherModeledArgumentCharacteristic {
   StringRegExpTestCharacteristic() { this = "StringRegExpTest" }
 
   override predicate getEndpoints(DataFlow::Node n) {
@@ -295,23 +315,26 @@ private class StringRegExpTestCharacteristic extends NotASinkCharacteristic {
   }
 }
 
-private class EventRegistrationCharacteristic extends NotASinkCharacteristic {
+private class EventRegistrationCharacteristic extends NotASinkCharacteristic,
+  OtherModeledArgumentCharacteristic {
   EventRegistrationCharacteristic() { this = "EventRegistration" }
 
   override predicate getEndpoints(DataFlow::Node n) {
     exists(DataFlow::CallNode call | n = call.getAnArgument() | call instanceof EventRegistration)
   }
 }
 
-private class EventDispatchCharacteristic extends NotASinkCharacteristic {
+private class EventDispatchCharacteristic extends NotASinkCharacteristic,
+  OtherModeledArgumentCharacteristic {
   EventDispatchCharacteristic() { this = "EventDispatch" }
 
   override predicate getEndpoints(DataFlow::Node n) {
     exists(DataFlow::CallNode call | n = call.getAnArgument() | call instanceof EventDispatch)
   }
 }
 
-private class MembershipCandidateTestCharacteristic extends NotASinkCharacteristic {
+private class MembershipCandidateTestCharacteristic extends NotASinkCharacteristic,
+  OtherModeledArgumentCharacteristic {
   MembershipCandidateTestCharacteristic() { this = "MembershipCandidateTest" }
 
   override predicate getEndpoints(DataFlow::Node n) {
@@ -321,15 +344,17 @@ private class MembershipCandidateTestCharacteristic extends NotASinkCharacterist
   }
 }
 
-private class FileSystemAccessCharacteristic extends NotASinkCharacteristic {
+private class FileSystemAccessCharacteristic extends NotASinkCharacteristic,
+  OtherModeledArgumentCharacteristic {
   FileSystemAccessCharacteristic() { this = "FileSystemAccess" }
 
   override predicate getEndpoints(DataFlow::Node n) {
     exists(DataFlow::CallNode call | n = call.getAnArgument() | call instanceof FileSystemAccess)
   }
 }
 
-private class DatabaseAccessCharacteristic extends NotASinkCharacteristic {
+private class DatabaseAccessCharacteristic extends NotASinkCharacteristic,
+  OtherModeledArgumentCharacteristic {
   DatabaseAccessCharacteristic() { this = "DatabaseAccess" }
 
   override predicate getEndpoints(DataFlow::Node n) {
@@ -344,15 +369,16 @@ private class DatabaseAccessCharacteristic extends NotASinkCharacteristic {
   }
 }
 
-private class DomCharacteristic extends NotASinkCharacteristic {
+private class DomCharacteristic extends NotASinkCharacteristic, OtherModeledArgumentCharacteristic {
   DomCharacteristic() { this = "DOM" }
 
   override predicate getEndpoints(DataFlow::Node n) {
     exists(DataFlow::CallNode call | n = call.getAnArgument() | call = DOM::domValueRef())
   }
 }
 
-private class NextFunctionCallCharacteristic extends NotASinkCharacteristic {
+private class NextFunctionCallCharacteristic extends NotASinkCharacteristic,
+  OtherModeledArgumentCharacteristic {
   NextFunctionCallCharacteristic() { this = "NextFunctionCall" }
 
   override predicate getEndpoints(DataFlow::Node n) {
@@ -363,7 +389,8 @@ private class NextFunctionCallCharacteristic extends NotASinkCharacteristic {
   }
 }
 
-private class DojoRequireCharacteristic extends NotASinkCharacteristic {
+private class DojoRequireCharacteristic extends NotASinkCharacteristic,
+  OtherModeledArgumentCharacteristic {
   DojoRequireCharacteristic() { this = "DojoRequire" }
 
   override predicate getEndpoints(DataFlow::Node n) {
@@ -373,7 +400,8 @@ private class DojoRequireCharacteristic extends NotASinkCharacteristic {
   }
 }
 
-private class Base64ManipulationCharacteristic extends NotASinkCharacteristic {
+private class Base64ManipulationCharacteristic extends NotASinkCharacteristic,
+  OtherModeledArgumentCharacteristic {
   Base64ManipulationCharacteristic() { this = "Base64Manipulation" }
 
   override predicate getEndpoints(DataFlow::Node n) {
@@ -475,7 +503,7 @@ abstract private class StandardEndpointFilterCharacteristic extends EndpointFilt
   }
 }
 
-private class IsArgumentToModeledFunctionCharacteristic extends StandardEndpointFilterCharacteristic {
+class IsArgumentToModeledFunctionCharacteristic extends StandardEndpointFilterCharacteristic {
   IsArgumentToModeledFunctionCharacteristic() { this = "argument to modeled function" }
 
   override predicate getEndpoints(DataFlow::Node n) {
@@ -487,7 +515,9 @@ private class IsArgumentToModeledFunctionCharacteristic extends StandardEndpoint
         or
         CoreKnowledge::isKnownStepSrc(known)
         or
-        CoreKnowledge::isOtherModeledArgument(known, _)
+        exists(OtherModeledArgumentCharacteristic characteristic |
+          characteristic.getEndpoints(known)
+        )
       )
     )
   }