Java: Remove/deprecate FlowStateString-based extension points.

java/ql/lib/semmle/code/java/security/ImplicitPendingIntents.qll

@@ -6,16 +6,44 @@ private import semmle.code.java.dataflow.TaintTracking
 private import semmle.code.java.frameworks.android.Intent
 private import semmle.code.java.frameworks.android.PendingIntent
 
+private newtype TPendingIntentState =
+  TMutablePendingIntent() or
+  TNoState()
+
+/** A flow state for an implicit `PendingIntent` flow. */
+class PendingIntentState extends TPendingIntentState {
+  /** Gets a textual representation of this element. */
+  abstract string toString();
+}
+
+/** A flow state indicating that a mutable `PendingIntent` has been created. */
+class MutablePendingIntent extends PendingIntentState, TMutablePendingIntent {
+  override string toString() { result = "MutablePendingIntent" }
+}
+
+/** The initial flow state for an implicit `PendingIntent` flow. */
+class NoState extends PendingIntentState, TNoState {
+  override string toString() { result = "NoState" }
+}
+
 /** A source for an implicit `PendingIntent` flow. */
 abstract class ImplicitPendingIntentSource extends DataFlow::Node {
-  /** Holds if this source has the specified `state`. */
-  predicate hasState(DataFlow::FlowState state) { state = "" }
+  /**
+   * DEPRECATED: Open-ended flow state is not intended to be part of the extension points.
+   *
+   * Holds if this source has the specified `state`.
+   */
+  deprecated predicate hasState(DataFlow::FlowState state) { state = "" }
 }
 
 /** A sink that sends an implicit and mutable `PendingIntent` to a third party. */
 abstract class ImplicitPendingIntentSink extends DataFlow::Node {
-  /** Holds if this sink has the specified `state`. */
-  predicate hasState(DataFlow::FlowState state) { state = "" }
+  /**
+   * DEPRECATED: Open-ended flow state is not intended to be part of the extension points.
+   *
+   * Holds if this sink has the specified `state`.
+   */
+  deprecated predicate hasState(DataFlow::FlowState state) { state = "" }
 }
 
 /**
@@ -32,11 +60,19 @@ class ImplicitPendingIntentAdditionalTaintStep extends Unit {
   predicate step(DataFlow::Node node1, DataFlow::Node node2) { none() }
 
   /**
+   * Holds if the step from `node1` to `node2` creates a mutable `PendingIntent`.
+   */
+  predicate mutablePendingIntentCreation(DataFlow::Node node1, DataFlow::Node node2) { none() }
+
+  /**
+   * DEPRECATED: Open-ended flow state is not intended to be part of the extension points.
+   * Use `mutablePendingIntentCreation` instead.
+   *
    * Holds if the step from `node1` to `node2` should be considered a taint
    * step for flows related to the use of implicit `PendingIntent`s. This step is only applicable
    * in `state1` and updates the flow state to `state2`.
    */
-  predicate step(
+  deprecated predicate step(
     DataFlow::Node node1, DataFlow::FlowState state1, DataFlow::Node node2,
     DataFlow::FlowState state2
   ) {
@@ -66,17 +102,10 @@ private class SendPendingIntent extends ImplicitPendingIntentSink {
     or
     sinkNode(this, "pending-intents")
   }
-
-  override predicate hasState(DataFlow::FlowState state) { state = "MutablePendingIntent" }
 }
 
 private class MutablePendingIntentFlowStep extends ImplicitPendingIntentAdditionalTaintStep {
-  override predicate step(
-    DataFlow::Node node1, DataFlow::FlowState state1, DataFlow::Node node2,
-    DataFlow::FlowState state2
-  ) {
-    state1 = "" and
-    state2 = "MutablePendingIntent" and
+  override predicate mutablePendingIntentCreation(DataFlow::Node node1, DataFlow::Node node2) {
     exists(PendingIntentCreation pic, Argument flagArg |
       node1.asExpr() = pic.getIntentArg() and
       node2.asExpr() = pic and

java/ql/lib/semmle/code/java/security/ImplicitPendingIntentsQuery.qll

@@ -60,14 +60,14 @@ deprecated class ImplicitPendingIntentStartConf extends TaintTracking::Configura
  * being wrapped in another implicit `Intent` that gets started.
  */
 module ImplicitPendingIntentStartConfig implements DataFlow::StateConfigSig {
-  class FlowState = DataFlow::FlowState;
+  class FlowState = PendingIntentState;
 
   predicate isSource(DataFlow::Node source, FlowState state) {
-    source.(ImplicitPendingIntentSource).hasState(state)
+    source instanceof ImplicitPendingIntentSource and state instanceof NoState
   }
 
   predicate isSink(DataFlow::Node sink, FlowState state) {
-    sink.(ImplicitPendingIntentSink).hasState(state)
+    sink instanceof ImplicitPendingIntentSink and state instanceof MutablePendingIntent
   }
 
   predicate isBarrier(DataFlow::Node sanitizer) { sanitizer instanceof ExplicitIntentSanitizer }
@@ -79,7 +79,9 @@ module ImplicitPendingIntentStartConfig implements DataFlow::StateConfigSig {
   predicate isAdditionalFlowStep(
     DataFlow::Node node1, FlowState state1, DataFlow::Node node2, FlowState state2
   ) {
-    any(ImplicitPendingIntentAdditionalTaintStep c).step(node1, state1, node2, state2)
+    any(ImplicitPendingIntentAdditionalTaintStep c).mutablePendingIntentCreation(node1, node2) and
+    state1 instanceof NoState and
+    state2 instanceof MutablePendingIntent
   }
 
   predicate allowImplicitRead(DataFlow::Node node, DataFlow::ContentSet c) {

java/ql/lib/semmle/code/java/security/InsufficientKeySize.qll

@@ -3,32 +3,76 @@
 private import semmle.code.java.security.Encryption
 private import semmle.code.java.dataflow.DataFlow
 private import semmle.code.java.security.internal.EncryptionKeySizes
+import codeql.util.Either
+
+/** A minimum recommended key size for some algorithm. */
+abstract class MinimumKeySize extends int {
+  bindingset[this]
+  MinimumKeySize() { any() }
+
+  /** Gets a textual representation of this element. */
+  string toString() { result = super.toString() }
+}
+
+/**
+ * A class of algorithms for which a key size smaller than the recommended key
+ * size might be embedded in the algorithm name.
+ */
+abstract class AlgorithmKind extends string {
+  bindingset[this]
+  AlgorithmKind() { any() }
+
+  /** Gets a textual representation of this element. */
+  string toString() { result = super.toString() }
+}
+
+/**
+ * A key size that is greater than the tracked value and equal to the minimum
+ * recommended key size for some algorithm, or a kind of algorithm for which the
+ * tracked string indicates a too small key size.
+ */
+final class KeySizeState = Either<MinimumKeySize, AlgorithmKind>::Either;
 
 /** A source for an insufficient key size. */
 abstract class InsufficientKeySizeSource extends DataFlow::Node {
   /** Holds if this source has the specified `state`. */
-  predicate hasState(DataFlow::FlowState state) { state instanceof DataFlow::FlowStateEmpty }
+  abstract predicate hasState(KeySizeState state);
 }
 
 /** A sink for an insufficient key size. */
 abstract class InsufficientKeySizeSink extends DataFlow::Node {
-  /** Holds if this sink has the specified `state`. */
-  predicate hasState(DataFlow::FlowState state) { state instanceof DataFlow::FlowStateEmpty }
+  /** Holds if this sink accepts the specified `state`. */
+  final predicate hasState(KeySizeState state) {
+    state.asLeft() <= this.minimumKeySize() or this.algorithmKind(state.asRight())
+  }
+
+  /** Gets the minimum recommended key size. */
+  abstract int minimumKeySize();
+
+  /**
+   * Holds if this sink recommends a keysize that is greater than the value in a
+   * source with the given algorithm kind.
+   */
+  predicate algorithmKind(AlgorithmKind kind) { none() }
+}
+
+/** A source for an insufficient key size used in some algorithm. */
+private class IntegerLiteralSource extends InsufficientKeySizeSource {
+  private int value;
+
+  IntegerLiteralSource() { this.asExpr().(IntegerLiteral).getIntValue() = value }
+
+  override predicate hasState(KeySizeState state) {
+    state.asLeft() = min(MinimumKeySize m | value < m)
+  }
 }
 
 /** Provides models for asymmetric cryptography. */
 private module Asymmetric {
   /** Provides models for non-elliptic-curve asymmetric cryptography. */
   private module NonEllipticCurve {
-    /** A source for an insufficient key size used in RSA, DSA, and DH algorithms. */
-    private class Source extends InsufficientKeySizeSource {
-      string algoName;
-
-      Source() { this.asExpr().(IntegerLiteral).getIntValue() < getMinKeySize(algoName) }
-
-      override predicate hasState(DataFlow::FlowState state) {
-        state = getMinKeySize(algoName).toString()
-      }
+    private class NonEllipticCurveKeySize extends MinimumKeySize {
+      NonEllipticCurveKeySize() { this = getMinKeySize(_) }
     }
 
     /** A sink for an insufficient key size used in RSA, DSA, and DH algorithms. */
@@ -46,9 +90,7 @@ private module Asymmetric {
         exists(Spec spec | this.asExpr() = spec.getKeySizeArg() and algoName = spec.getAlgoName())
       }
 
-      override predicate hasState(DataFlow::FlowState state) {
-        state = getMinKeySize(algoName).toString()
-      }
+      override int minimumKeySize() { result = getMinKeySize(algoName) }
     }
 
     /** Returns the minimum recommended key size for RSA, DSA, and DH algorithms. */
@@ -88,16 +130,24 @@ private module Asymmetric {
 
   /** Provides models for elliptic-curve asymmetric cryptography. */
   private module EllipticCurve {
+    private class EllipticCurveKeySize extends MinimumKeySize {
+      EllipticCurveKeySize() { this = getMinKeySize() }
+    }
+
+    private class EllipticCurveKind extends AlgorithmKind {
+      EllipticCurveKind() { this = "EC" }
+    }
+
     /** A source for an insufficient key size used in elliptic curve (EC) algorithms. */
     private class Source extends InsufficientKeySizeSource {
       Source() {
-        this.asExpr().(IntegerLiteral).getIntValue() < getMinKeySize()
-        or
         // the below is needed for cases when the key size is embedded in the curve name
         getKeySize(this.asExpr().(StringLiteral).getValue()) < getMinKeySize()
       }
 
-      override predicate hasState(DataFlow::FlowState state) { state = getMinKeySize().toString() }
+      override predicate hasState(KeySizeState state) {
+        state.asRight() instanceof EllipticCurveKind
+      }
     }
 
     /** A sink for an insufficient key size used in elliptic curve (EC) algorithms. */
@@ -112,7 +162,9 @@ private module Asymmetric {
         exists(Spec s | this.asExpr() = s.getKeySizeArg())
       }
 
-      override predicate hasState(DataFlow::FlowState state) { state = getMinKeySize().toString() }
+      override int minimumKeySize() { result = getMinKeySize() }
+
+      override predicate algorithmKind(AlgorithmKind kind) { kind instanceof EllipticCurveKind }
     }
 
     /** Returns the minimum recommended key size for elliptic curve (EC) algorithms. */
@@ -176,11 +228,8 @@ private module Asymmetric {
 
 /** Provides models for symmetric cryptography. */
 private module Symmetric {
-  /** A source for an insufficient key size used in AES algorithms. */
-  private class Source extends InsufficientKeySizeSource {
-    Source() { this.asExpr().(IntegerLiteral).getIntValue() < getMinKeySize() }
-
-    override predicate hasState(DataFlow::FlowState state) { state = getMinKeySize().toString() }
+  private class SymmetricKeySize extends MinimumKeySize {
+    SymmetricKeySize() { this = getMinKeySize() }
   }
 
   /** A sink for an insufficient key size used in AES algorithms. */
@@ -193,7 +242,7 @@ private module Symmetric {
       )
     }
 
-    override predicate hasState(DataFlow::FlowState state) { state = getMinKeySize().toString() }
+    override int minimumKeySize() { result = getMinKeySize() }
   }
 
   /** Returns the minimum recommended key size for AES algorithms. */

java/ql/lib/semmle/code/java/security/InsufficientKeySizeQuery.qll

@@ -12,11 +12,11 @@ deprecated class KeySizeConfiguration extends DataFlow::Configuration {
   KeySizeConfiguration() { this = "KeySizeConfiguration" }
 
   override predicate isSource(DataFlow::Node source, DataFlow::FlowState state) {
-    source.(InsufficientKeySizeSource).hasState(state)
+    exists(KeySizeState s | source.(InsufficientKeySizeSource).hasState(s) and state = s.toString())
   }
 
   override predicate isSink(DataFlow::Node sink, DataFlow::FlowState state) {
-    sink.(InsufficientKeySizeSink).hasState(state)
+    exists(KeySizeState s | sink.(InsufficientKeySizeSink).hasState(s) and state = s.toString())
   }
 }
 
@@ -24,24 +24,15 @@ deprecated class KeySizeConfiguration extends DataFlow::Configuration {
  * A data flow configuration for tracking key sizes used in cryptographic algorithms.
  */
 module KeySizeConfig implements DataFlow::StateConfigSig {
-  class FlowState = DataFlow::FlowState;
+  class FlowState = KeySizeState;
 
-  predicate isSource(DataFlow::Node source, DataFlow::FlowState state) {
+  predicate isSource(DataFlow::Node source, KeySizeState state) {
     source.(InsufficientKeySizeSource).hasState(state)
   }
 
-  predicate isSink(DataFlow::Node sink, DataFlow::FlowState state) {
+  predicate isSink(DataFlow::Node sink, KeySizeState state) {
     sink.(InsufficientKeySizeSink).hasState(state)
   }
-
-  predicate isBarrier(DataFlow::Node node, DataFlow::FlowState state) { none() }
-
-  predicate isAdditionalFlowStep(
-    DataFlow::Node node1, DataFlow::FlowState state1, DataFlow::Node node2,
-    DataFlow::FlowState state2
-  ) {
-    none()
-  }
 }
 
 /** Tracks key sizes used in cryptographic algorithms. */

java/ql/lib/semmle/code/java/security/TemplateInjection.qll

@@ -9,16 +9,28 @@ private import semmle.code.java.dataflow.TaintTracking
  * A source for server-side template injection (SST) vulnerabilities.
  */
 abstract class TemplateInjectionSource extends DataFlow::Node {
-  /** Holds if this source has the specified `state`. */
-  predicate hasState(DataFlow::FlowState state) { state instanceof DataFlow::FlowStateEmpty }
+  /**
+   * DEPRECATED: Open-ended flow state is not intended to be part of the extension points.
+   *
+   * Holds if this source has the specified `state`.
+   */
+  deprecated predicate hasState(DataFlow::FlowState state) {
+    state instanceof DataFlow::FlowStateEmpty
+  }
 }
 
 /**
  * A sink for server-side template injection (SST) vulnerabilities.
  */
 abstract class TemplateInjectionSink extends DataFlow::Node {
-  /** Holds if this sink has the specified `state`. */
-  predicate hasState(DataFlow::FlowState state) { state instanceof DataFlow::FlowStateEmpty }
+  /**
+   * DEPRECATED: Open-ended flow state is not intended to be part of the extension points.
+   *
+   * Holds if this sink has the specified `state`.
+   */
+  deprecated predicate hasState(DataFlow::FlowState state) {
+    state instanceof DataFlow::FlowStateEmpty
+  }
 }
 
 /**
@@ -35,11 +47,13 @@ class TemplateInjectionAdditionalTaintStep extends Unit {
   predicate isAdditionalTaintStep(DataFlow::Node node1, DataFlow::Node node2) { none() }
 
   /**
+   * DEPRECATED: Open-ended flow state is not intended to be part of the extension points.
+   *
    * Holds if the step from `node1` to `node2` should be considered a taint
    * step for flows related toserver-side template injection (SST) vulnerabilities.
    * This step is only applicable in `state1` and updates the flow state to `state2`.
    */
-  predicate isAdditionalTaintStep(
+  deprecated predicate isAdditionalTaintStep(
     DataFlow::Node node1, DataFlow::FlowState state1, DataFlow::Node node2,
     DataFlow::FlowState state2
   ) {
@@ -53,13 +67,19 @@ class TemplateInjectionAdditionalTaintStep extends Unit {
 abstract class TemplateInjectionSanitizer extends DataFlow::Node { }
 
 /**
+ * DEPRECATED: Open-ended flow state is not intended to be part of the extension points.
+ *
  * A sanitizer for server-side template injection (SST) vulnerabilities.
  * This sanitizer is only applicable when `TemplateInjectionSanitizerWithState::hasState`
  * holds for the flow state.
  */
-abstract class TemplateInjectionSanitizerWithState extends DataFlow::Node {
-  /** Holds if this sanitizer has the specified `state`. */
-  abstract predicate hasState(DataFlow::FlowState state);
+abstract deprecated class TemplateInjectionSanitizerWithState extends DataFlow::Node {
+  /**
+   * DEPRECATED: Open-ended flow state is not intended to be part of the extension points.
+   *
+   * Holds if this sanitizer has the specified `state`.
+   */
+  abstract deprecated predicate hasState(DataFlow::FlowState state);
 }
 
 private class DefaultTemplateInjectionSource extends TemplateInjectionSource instanceof ThreatModelFlowSource

java/ql/lib/semmle/code/java/security/TemplateInjectionQuery.qll

@@ -42,33 +42,17 @@ deprecated class TemplateInjectionFlowConfig extends TaintTracking::Configuratio
 }
 
 /** A taint tracking configuration to reason about server-side template injection (SST) vulnerabilities */
-module TemplateInjectionFlowConfig implements DataFlow::StateConfigSig {
-  class FlowState = DataFlow::FlowState;
+module TemplateInjectionFlowConfig implements DataFlow::ConfigSig {
+  predicate isSource(DataFlow::Node source) { source instanceof TemplateInjectionSource }
 
-  predicate isSource(DataFlow::Node source, FlowState state) {
-    source.(TemplateInjectionSource).hasState(state)
-  }
-
-  predicate isSink(DataFlow::Node sink, FlowState state) {
-    sink.(TemplateInjectionSink).hasState(state)
-  }
+  predicate isSink(DataFlow::Node sink) { sink instanceof TemplateInjectionSink }
 
   predicate isBarrier(DataFlow::Node sanitizer) { sanitizer instanceof TemplateInjectionSanitizer }
 
-  predicate isBarrier(DataFlow::Node sanitizer, FlowState state) {
-    sanitizer.(TemplateInjectionSanitizerWithState).hasState(state)
-  }
-
   predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2) {
     any(TemplateInjectionAdditionalTaintStep a).isAdditionalTaintStep(node1, node2)
   }
-
-  predicate isAdditionalFlowStep(
-    DataFlow::Node node1, FlowState state1, DataFlow::Node node2, FlowState state2
-  ) {
-    any(TemplateInjectionAdditionalTaintStep a).isAdditionalTaintStep(node1, state1, node2, state2)
-  }
 }
 
 /** Tracks server-side template injection (SST) vulnerabilities */
-module TemplateInjectionFlow = TaintTracking::GlobalWithState<TemplateInjectionFlowConfig>;
+module TemplateInjectionFlow = TaintTracking::Global<TemplateInjectionFlowConfig>;

java/ql/src/change-notes/2023-12-14-flowstatestring-deprecated.md

@@ -0,0 +1,4 @@
+---
+category: deprecated
+---
+* The three queries `java/insufficient-key-size`, `java/server-side-template-injection`, and `java/android/implicit-pendingintents` had accidentally general extension points allowing arbitrary string-based flow state. This has been fixed and the old extension points have been deprecated where possible, and otherwise updated.