Optimize vulnerable call detection and summarization

Applied performance optimizations to vulnerable call detection by materializing intermediate predicates, separating static target and external reference matching, and using direct string comparisons. Updated summarization logic to handle both external and static target calls. Improved efficiency and scalability for analyzing CIL and JVM binaries.

Author: gfs

binary/ql/src/VulnerableCalls/VulnerableCalls.qll

@@ -1,6 +1,14 @@
 /**
  * Provides predicates for finding calls that transitively make a call to a
- * known-vulnerable method in CIL (C# IL) binaries.
+ * known-vulnerable method in CIL (C# IL) and JVM binaries.
+ *
+ * Performance optimizations applied:
+ * - Materialized intermediate predicates with pragma[nomagic] to prevent
+ *   expensive cross-product computations
+ * - Direct string matching on getExternalName() instead of SSA traversal
+ *   where possible
+ * - Separated external reference matching from static target matching
+ *   for better join ordering
  */
 
 private import binary
@@ -16,19 +24,98 @@ extensible predicate vulnerableCallModel(
   string namespace, string className, string methodName, string id
 );
 
+/*
+ * ============================================================================
+ * Materialized helper predicates for performance
+ * ============================================================================
+ */
+
+/**
+ * Materialized: builds the fully qualified name string for each vulnerable method
+ * in the model. This enables efficient string comparison.
+ */
+pragma[nomagic]
+private predicate vulnerableMethodFqn(string fqn, string id) {
+  exists(string namespace, string className, string methodName |
+    vulnerableCallModel(namespace, className, methodName, id) and
+    fqn = namespace + "." + className + "." + methodName
+  )
+}
+
+/**
+ * Materialized: gets ExternalRefInstructions that directly reference vulnerable methods.
+ * Uses direct string comparison against getExternalName() to avoid expensive
+ * hasFullyQualifiedName() regex parsing.
+ */
+pragma[nomagic]
+private predicate isVulnerableExternalRef(ExternalRefInstruction eri, string id) {
+  exists(string fqn |
+    vulnerableMethodFqn(fqn, id) and
+    eri.getExternalName() = fqn
+  )
+}
+
+/**
+ * Materialized: gets Functions that are marked as vulnerable in the model.
+ * Used for matching calls with static targets.
+ */
+pragma[nomagic]
+private predicate isVulnerableFunction(Function f, string id) {
+  exists(string namespace, string className, string methodName |
+    vulnerableCallModel(namespace, className, methodName, id) and
+    f.hasFullyQualifiedName(namespace, className, methodName)
+  )
+}
+
+/**
+ * Materialized: maps ExternalRefInstructions to their enclosing functions.
+ * This avoids repeated getEnclosingFunction() lookups in the recursive predicate.
+ */
+pragma[nomagic]
+private predicate externalRefInFunction(ExternalRefInstruction eri, Function f) {
+  f = eri.getEnclosingFunction()
+}
+
+/**
+ * Materialized: maps CallInstructions to their enclosing functions.
+ */
+pragma[nomagic]
+private predicate callInFunction(CallInstruction call, Function f) {
+  f = call.getEnclosingFunction()
+}
+
+/**
+ * Materialized: maps ExternalRefInstructions to their external names.
+ */
+pragma[nomagic]
+private predicate externalRefName(ExternalRefInstruction eri, string name) {
+  name = eri.getExternalName()
+}
+
+/*
+ * ============================================================================
+ * Direct vulnerable call detection
+ * ============================================================================
+ */
+
 /**
  * A method call that has been marked as vulnerable by a model.
+ *
+ * This class matches calls where either:
+ * 1. The static target is a vulnerable function (internal calls)
+ * 2. The external reference points to a vulnerable method (external calls)
  */
 class VulnerableMethodCall extends CallInstruction {
   string vulnerabilityId;
 
   VulnerableMethodCall() {
-    exists(string namespace, string className, string methodName |
-      vulnerableCallModel(namespace, className, methodName, vulnerabilityId) and
-      this.getTargetOperand()
-          .getAnyDef()
-          .(ExternalRefInstruction)
-          .hasFullyQualifiedName(namespace, className, methodName)
+    // Match via static target (more efficient, no SSA traversal)
+    isVulnerableFunction(this.getStaticTarget(), vulnerabilityId)
+    or
+    // Match via external reference for external calls
+    exists(ExternalRefInstruction eri |
+      isVulnerableExternalRef(eri, vulnerabilityId) and
+      this.getTargetOperand().getAnyDef() = eri
     )
   }
 
@@ -44,21 +131,29 @@ VulnerableMethodCall getAVulnerableCallFromModel(string id) { result.getVulnerab
 /**
  * Gets a method that directly contains a vulnerable call.
  */
+pragma[nomagic]
 Function getADirectlyVulnerableMethod(string id) {
   result = getAVulnerableCallFromModel(id).getEnclosingFunction()
 }
 
+/*
+ * ============================================================================
+ * C# Iterator/Async State Machine handling
+ * ============================================================================
+ */
+
 /**
  * Holds if `stub` is an iterator/async stub method and `stateMachine` is its
  * corresponding state machine implementation (the MoveNext method).
- * 
+ *
  * Iterator/async methods in C# are compiled to:
  * 1. A stub method that creates a state machine object
  * 2. A nested class with a MoveNext method containing the actual implementation
- * 
- * The pattern is: method `Foo` in class `Bar` creates `Bar.<Foo>d__N` 
+ *
+ * The pattern is: method `Foo` in class `Bar` creates `Bar.<Foo>d__N`
  * and the impl is in `Bar.<Foo>d__N.MoveNext`
  */
+pragma[nomagic]
 private predicate isStateMachineImplementation(Function stub, Function stateMachine) {
   exists(string stubName, Type stubType, string stateMachineTypeName |
     stubName = stub.getName() and
@@ -67,7 +162,7 @@ private predicate isStateMachineImplementation(Function stub, Function stateMach
     // The state machine type is nested in the same type as the stub
     // and named <MethodName>d__N
     stateMachineTypeName = stateMachine.getDeclaringType().getName() and
-    stateMachineTypeName.matches("<" + stubName + ">d__%" ) and
+    stateMachineTypeName.matches("<" + stubName + ">d__%") and
     // The state machine's declaring type's namespace should be the stub's type full name
     stateMachine.getDeclaringType().getNamespace() = stubType.getFullName()
   )
@@ -80,32 +175,81 @@ Function getStateMachineImplementation(Function stub) {
   isStateMachineImplementation(stub, result)
 }
 
+/*
+ * ============================================================================
+ * Optimized call graph edges for transitive closure
+ * ============================================================================
+ */
+
+/**
+ * Materialized: holds if function `caller` contains a call to function `callee`
+ * via a static target (direct call, no SSA traversal needed).
+ */
+pragma[nomagic]
+private predicate callsViaStaticTarget(Function caller, Function callee) {
+  exists(CallInstruction call |
+    callInFunction(call, caller) and
+    callee = call.getStaticTarget()
+  )
+}
+
+/**
+ * Materialized: holds if function `caller` contains a call to a function
+ * with fully qualified name `calleeFqn` via an external reference.
+ */
+pragma[nomagic]
+private predicate callsViaExternalRef(Function caller, string calleeFqn) {
+  exists(CallInstruction call, ExternalRefInstruction eri |
+    callInFunction(call, caller) and
+    call.getTargetOperand().getAnyDef() = eri and
+    externalRefName(eri, calleeFqn)
+  )
+}
+
+/**
+ * Materialized: maps functions to their fully qualified names for join efficiency.
+ */
+pragma[nomagic]
+private predicate functionFqn(Function f, string fqn) {
+  fqn = f.getFullyQualifiedName()
+}
+
+/*
+ * ============================================================================
+ * Transitive vulnerable method detection
+ * ============================================================================
+ */
+
 /**
  * Gets a method that transitively calls a vulnerable method.
  * This computes the transitive closure of the call graph.
- * 
+ *
  * Also handles iterator/async methods by linking stub methods to their
  * state machine implementations.
+ *
+ * Performance notes:
+ * - Uses materialized helper predicates to avoid repeated expensive operations
+ * - Separates static target calls from external reference calls for better join ordering
+ * - The recursion is bounded by the call graph depth
  */
 Function getAVulnerableMethod(string id) {
-  // Direct call to vulnerable method
+  // Base case: direct call to vulnerable method
   result = getADirectlyVulnerableMethod(id)
   or
-  // Transitive: method calls another method that is vulnerable (via ExternalRef for external calls)
-  exists(CallInstruction call, Function callee |
-    call.getEnclosingFunction() = result and
+  // Transitive case 1: method calls a vulnerable method via static target
+  exists(Function callee |
     callee = getAVulnerableMethod(id) and
-    call.getTargetOperand().getAnyDef().(ExternalRefInstruction).getFullyQualifiedName() =
-      callee.getFullyQualifiedName()
+    callsViaStaticTarget(result, callee)
   )
   or
-  // Transitive: method calls another method that is vulnerable (via static target for direct calls)
-  exists(CallInstruction call |
-    call.getEnclosingFunction() = result and
-    call.getStaticTarget() = getAVulnerableMethod(id)
+  // Transitive case 2: method calls a vulnerable method via external reference
+  exists(Function callee, string calleeFqn |
+    callee = getAVulnerableMethod(id) and
+    functionFqn(callee, calleeFqn) and
+    callsViaExternalRef(result, calleeFqn)
   )
   or
-  // Iterator/async: if a state machine's MoveNext is vulnerable, 
+  // Iterator/async: if a state machine's MoveNext is vulnerable,
   // the stub method that creates it is also vulnerable
   exists(Function stateMachine |
     stateMachine = getAVulnerableMethod(id) and
@@ -116,6 +260,7 @@ Function getAVulnerableMethod(string id) {
 /**
  * Gets a public method that transitively calls a vulnerable method.
  */
+pragma[nomagic]
 Function getAPublicVulnerableMethod(string id) {
   result = getAVulnerableMethod(id) and
   result.isPublic()

binary/ql/src/VulnerableCalls/VulnerableCallsSummarize.ql

@@ -30,6 +30,7 @@ query predicate publicVulnerableCallModel(
 
 /**
  * Lists the direct vulnerable call sites with their enclosing method context.
+ * Handles both external reference calls and static target calls.
  */
 query predicate vulnerableCallLocations(
   VulnerableMethodCall call,
@@ -40,6 +41,14 @@ query predicate vulnerableCallLocations(
   string id
 ) {
   call.getVulnerabilityId() = id and
-  call.getEnclosingFunction().hasFullyQualifiedName(callerNamespace, callerClassName, callerMethodName) and
-  targetFqn = call.getTargetOperand().getAnyDef().(ExternalRefInstruction).getFullyQualifiedName()
+  call.getEnclosingFunction()
+      .hasFullyQualifiedName(callerNamespace, callerClassName, callerMethodName) and
+  (
+    // External call via ExternalRefInstruction
+    targetFqn = call.getTargetOperand().getAnyDef().(ExternalRefInstruction).getFullyQualifiedName()
+    or
+    // Internal call via static target
+    not exists(call.getTargetOperand().getAnyDef().(ExternalRefInstruction)) and
+    targetFqn = call.getStaticTarget().getFullyQualifiedName()
+  )
 }