Java: Deprecate BarrierGuard class.

aschackmull · aschackmull · commit 33deff9baef4 · 2022-06-16T11:25:28.000+02:00
diff --git a/java/ql/lib/semmle/code/java/dataflow/internal/DataFlowImpl.qll b/java/ql/lib/semmle/code/java/dataflow/internal/DataFlowImpl.qll
@@ -90,14 +90,20 @@ abstract class Configuration extends string {
   /** Holds if data flow out of `node` is prohibited. */
   predicate isBarrierOut(Node node) { none() }
 
-  /** Holds if data flow through nodes guarded by `guard` is prohibited. */
-  predicate isBarrierGuard(BarrierGuard guard) { none() }
+  /**
+   * DEPRECATED: Use `isBarrier` and `BarrierGuard` module instead.
+   *
+   * Holds if data flow through nodes guarded by `guard` is prohibited.
+   */
+  deprecated predicate isBarrierGuard(BarrierGuard guard) { none() }
 
   /**
+   * DEPRECATED: Use `isBarrier` and `BarrierGuard` module instead.
+   *
    * Holds if data flow through nodes guarded by `guard` is prohibited when
    * the flow state is `state`
    */
-  predicate isBarrierGuard(BarrierGuard guard, FlowState state) { none() }
+  deprecated predicate isBarrierGuard(BarrierGuard guard, FlowState state) { none() }
 
   /**
    * Holds if data may flow from `node1` to `node2` in addition to the normal data-flow steps.
@@ -335,6 +341,29 @@ private predicate outBarrier(NodeEx node, Configuration config) {
   )
 }
 
+/** A bridge class to access the deprecated `isBarrierGuard`. */
+private class BarrierGuardGuardedNodeBridge extends Unit {
+  abstract predicate guardedNode(Node n, Configuration config);
+
+  abstract predicate guardedNode(Node n, FlowState state, Configuration config);
+}
+
+private class BarrierGuardGuardedNode extends BarrierGuardGuardedNodeBridge {
+  deprecated override predicate guardedNode(Node n, Configuration config) {
+    exists(BarrierGuard g |
+      config.isBarrierGuard(g) and
+      n = g.getAGuardedNode()
+    )
+  }
+
+  deprecated override predicate guardedNode(Node n, FlowState state, Configuration config) {
+    exists(BarrierGuard g |
+      config.isBarrierGuard(g, state) and
+      n = g.getAGuardedNode()
+    )
+  }
+}
+
 pragma[nomagic]
 private predicate fullBarrier(NodeEx node, Configuration config) {
   exists(Node n | node.asNode() = n |
@@ -348,10 +377,7 @@ private predicate fullBarrier(NodeEx node, Configuration config) {
     not config.isSink(n) and
     not config.isSink(n, _)
     or
-    exists(BarrierGuard g |
-      config.isBarrierGuard(g) and
-      n = g.getAGuardedNode()
-    )
+    any(BarrierGuardGuardedNodeBridge b).guardedNode(n, config)
   )
 }
 
@@ -360,10 +386,7 @@ private predicate stateBarrier(NodeEx node, FlowState state, Configuration confi
   exists(Node n | node.asNode() = n |
     config.isBarrier(n, state)
     or
-    exists(BarrierGuard g |
-      config.isBarrierGuard(g, state) and
-      n = g.getAGuardedNode()
-    )
+    any(BarrierGuardGuardedNodeBridge b).guardedNode(n, state, config)
   )
 }
 
diff --git a/java/ql/lib/semmle/code/java/dataflow/internal/DataFlowUtil.qll b/java/ql/lib/semmle/code/java/dataflow/internal/DataFlowUtil.qll
@@ -332,6 +332,8 @@ module BarrierGuard<guardChecksSig/3 guardChecks> {
 }
 
 /**
+ * DEPRECATED: Use `BarrierGuard` module instead.
+ *
  * A guard that validates some expression.
  *
  * To use this in a configuration, extend the class and provide a
@@ -340,7 +342,7 @@ module BarrierGuard<guardChecksSig/3 guardChecks> {
  *
  * It is important that all extending classes in scope are disjoint.
  */
-class BarrierGuard extends Guard {
+deprecated class BarrierGuard extends Guard {
   /** Holds if this guard validates `e` upon evaluating to `branch`. */
   abstract predicate checks(Expr e, boolean branch);
 
diff --git a/java/ql/lib/semmle/code/java/dataflow/internal/TaintTrackingUtil.qll b/java/ql/lib/semmle/code/java/dataflow/internal/TaintTrackingUtil.qll
@@ -112,12 +112,6 @@ private module Cached {
   }
 }
 
-/**
- * Holds if `guard` should be a sanitizer guard in all global taint flow configurations
- * but not in local taint.
- */
-predicate defaultTaintSanitizerGuard(DataFlow::BarrierGuard guard) { none() }
-
 import Cached
 
 private RefType getElementType(RefType container) {
diff --git a/java/ql/lib/semmle/code/java/dataflow/internal/tainttracking1/TaintTrackingImpl.qll b/java/ql/lib/semmle/code/java/dataflow/internal/tainttracking1/TaintTrackingImpl.qll
@@ -116,20 +116,30 @@ abstract class Configuration extends DataFlow::Configuration {
 
   final override predicate isBarrierOut(DataFlow::Node node) { this.isSanitizerOut(node) }
 
-  /** Holds if taint propagation through nodes guarded by `guard` is prohibited. */
-  predicate isSanitizerGuard(DataFlow::BarrierGuard guard) { none() }
+  /**
+   * DEPRECATED: Use `isSanitizer` and `BarrierGuard` module instead.
+   *
+   * Holds if taint propagation through nodes guarded by `guard` is prohibited.
+   */
+  deprecated predicate isSanitizerGuard(DataFlow::BarrierGuard guard) { none() }
 
-  final override predicate isBarrierGuard(DataFlow::BarrierGuard guard) {
-    this.isSanitizerGuard(guard) or defaultTaintSanitizerGuard(guard)
+  deprecated final override predicate isBarrierGuard(DataFlow::BarrierGuard guard) {
+    this.isSanitizerGuard(guard)
   }
 
   /**
+   * DEPRECATED: Use `isSanitizer` and `BarrierGuard` module instead.
+   *
    * Holds if taint propagation through nodes guarded by `guard` is prohibited
    * when the flow state is `state`.
    */
-  predicate isSanitizerGuard(DataFlow::BarrierGuard guard, DataFlow::FlowState state) { none() }
+  deprecated predicate isSanitizerGuard(DataFlow::BarrierGuard guard, DataFlow::FlowState state) {
+    none()
+  }
 
-  final override predicate isBarrierGuard(DataFlow::BarrierGuard guard, DataFlow::FlowState state) {
+  deprecated final override predicate isBarrierGuard(
+    DataFlow::BarrierGuard guard, DataFlow::FlowState state
+  ) {
     this.isSanitizerGuard(guard, state)
   }
 
diff --git a/java/ql/lib/semmle/code/java/dataflow/internal/tainttracking2/TaintTrackingImpl.qll b/java/ql/lib/semmle/code/java/dataflow/internal/tainttracking2/TaintTrackingImpl.qll
@@ -116,20 +116,30 @@ abstract class Configuration extends DataFlow::Configuration {
 
   final override predicate isBarrierOut(DataFlow::Node node) { this.isSanitizerOut(node) }
 
-  /** Holds if taint propagation through nodes guarded by `guard` is prohibited. */
-  predicate isSanitizerGuard(DataFlow::BarrierGuard guard) { none() }
+  /**
+   * DEPRECATED: Use `isSanitizer` and `BarrierGuard` module instead.
+   *
+   * Holds if taint propagation through nodes guarded by `guard` is prohibited.
+   */
+  deprecated predicate isSanitizerGuard(DataFlow::BarrierGuard guard) { none() }
 
-  final override predicate isBarrierGuard(DataFlow::BarrierGuard guard) {
-    this.isSanitizerGuard(guard) or defaultTaintSanitizerGuard(guard)
+  deprecated final override predicate isBarrierGuard(DataFlow::BarrierGuard guard) {
+    this.isSanitizerGuard(guard)
   }
 
   /**
+   * DEPRECATED: Use `isSanitizer` and `BarrierGuard` module instead.
+   *
    * Holds if taint propagation through nodes guarded by `guard` is prohibited
    * when the flow state is `state`.
    */
-  predicate isSanitizerGuard(DataFlow::BarrierGuard guard, DataFlow::FlowState state) { none() }
+  deprecated predicate isSanitizerGuard(DataFlow::BarrierGuard guard, DataFlow::FlowState state) {
+    none()
+  }
 
-  final override predicate isBarrierGuard(DataFlow::BarrierGuard guard, DataFlow::FlowState state) {
+  deprecated final override predicate isBarrierGuard(
+    DataFlow::BarrierGuard guard, DataFlow::FlowState state
+  ) {
     this.isSanitizerGuard(guard, state)
   }
 
diff --git a/java/ql/lib/semmle/code/java/dataflow/internal/tainttracking3/TaintTrackingImpl.qll b/java/ql/lib/semmle/code/java/dataflow/internal/tainttracking3/TaintTrackingImpl.qll
@@ -116,20 +116,30 @@ abstract class Configuration extends DataFlow::Configuration {
 
   final override predicate isBarrierOut(DataFlow::Node node) { this.isSanitizerOut(node) }
 
-  /** Holds if taint propagation through nodes guarded by `guard` is prohibited. */
-  predicate isSanitizerGuard(DataFlow::BarrierGuard guard) { none() }
+  /**
+   * DEPRECATED: Use `isSanitizer` and `BarrierGuard` module instead.
+   *
+   * Holds if taint propagation through nodes guarded by `guard` is prohibited.
+   */
+  deprecated predicate isSanitizerGuard(DataFlow::BarrierGuard guard) { none() }
 
-  final override predicate isBarrierGuard(DataFlow::BarrierGuard guard) {
-    this.isSanitizerGuard(guard) or defaultTaintSanitizerGuard(guard)
+  deprecated final override predicate isBarrierGuard(DataFlow::BarrierGuard guard) {
+    this.isSanitizerGuard(guard)
   }
 
   /**
+   * DEPRECATED: Use `isSanitizer` and `BarrierGuard` module instead.
+   *
    * Holds if taint propagation through nodes guarded by `guard` is prohibited
    * when the flow state is `state`.
    */
-  predicate isSanitizerGuard(DataFlow::BarrierGuard guard, DataFlow::FlowState state) { none() }
+  deprecated predicate isSanitizerGuard(DataFlow::BarrierGuard guard, DataFlow::FlowState state) {
+    none()
+  }
 
-  final override predicate isBarrierGuard(DataFlow::BarrierGuard guard, DataFlow::FlowState state) {
+  deprecated final override predicate isBarrierGuard(
+    DataFlow::BarrierGuard guard, DataFlow::FlowState state
+  ) {
     this.isSanitizerGuard(guard, state)
   }
 
diff --git a/java/ql/lib/semmle/code/java/security/IntentUriPermissionManipulation.qll b/java/ql/lib/semmle/code/java/security/IntentUriPermissionManipulation.qll
@@ -24,12 +24,14 @@ abstract class IntentUriPermissionManipulationSink extends DataFlow::Node { }
 abstract class IntentUriPermissionManipulationSanitizer extends DataFlow::Node { }
 
 /**
+ * DEPRECATED: Use `IntentUriPermissionManipulationSanitizer` instead.
+ *
  * A guard that makes sure that an Intent is safe to be returned to another Activity.
  *
  * Usually, this is done by checking that the Intent's data URI and/or its flags contain
  * expected values.
  */
-abstract class IntentUriPermissionManipulationGuard extends DataFlow::BarrierGuard { }
+abstract deprecated class IntentUriPermissionManipulationGuard extends DataFlow::BarrierGuard { }
 
 /**
  * An additional taint step for flows related to Intent URI permission manipulation
@@ -95,10 +97,10 @@ private class IntentFlagsOrDataChangedSanitizer extends IntentUriPermissionManip
  *      intent.getFlags() & Intent.FLAG_GRANT_WRITE_URI_PERMISSION != 0) {}
  * ```
  */
-private class IntentFlagsOrDataCheckedGuard extends IntentUriPermissionManipulationGuard {
-  IntentFlagsOrDataCheckedGuard() { intentFlagsOrDataChecked(this, _, _) }
-
-  override predicate checks(Expr e, boolean branch) { intentFlagsOrDataChecked(this, e, branch) }
+private class IntentFlagsOrDataCheckedSanitizer extends IntentUriPermissionManipulationSanitizer {
+  IntentFlagsOrDataCheckedSanitizer() {
+    this = DataFlow::BarrierGuard<intentFlagsOrDataChecked/3>::getABarrierNode()
+  }
 }
 
 /**
diff --git a/java/ql/lib/semmle/code/java/security/IntentUriPermissionManipulationQuery.qll b/java/ql/lib/semmle/code/java/security/IntentUriPermissionManipulationQuery.qll
@@ -24,7 +24,7 @@ class IntentUriPermissionManipulationConf extends TaintTracking::Configuration {
     barrier instanceof IntentUriPermissionManipulationSanitizer
   }
 
-  override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) {
+  deprecated override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) {
     guard instanceof IntentUriPermissionManipulationGuard
   }
 
diff --git a/java/ql/src/experimental/Security/CWE/CWE-073/FilePathInjection.ql b/java/ql/src/experimental/Security/CWE/CWE-073/FilePathInjection.ql
@@ -30,10 +30,8 @@ class InjectFilePathConfig extends TaintTracking::Configuration {
 
   override predicate isSanitizer(DataFlow::Node node) {
     exists(Type t | t = node.getType() | t instanceof BoxedType or t instanceof PrimitiveType)
-  }
-
-  override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) {
-    guard instanceof PathTraversalBarrierGuard
+    or
+    node instanceof PathTraversalSanitizer
   }
 }
 
diff --git a/java/ql/src/experimental/Security/CWE/CWE-200/InsecureWebResourceResponse.ql b/java/ql/src/experimental/Security/CWE/CWE-200/InsecureWebResourceResponse.ql
@@ -24,9 +24,7 @@ class InsecureWebResourceResponseConfig extends TaintTracking::Configuration {
 
   override predicate isSink(DataFlow::Node sink) { sink instanceof WebResourceResponseSink }
 
-  override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) {
-    guard instanceof PathTraversalBarrierGuard
-  }
+  override predicate isSanitizer(DataFlow::Node node) { node instanceof PathTraversalSanitizer }
 }
 
 from DataFlow::PathNode source, DataFlow::PathNode sink, InsecureWebResourceResponseConfig conf
diff --git a/java/ql/src/experimental/Security/CWE/CWE-200/SensitiveAndroidFileLeak.ql b/java/ql/src/experimental/Security/CWE/CWE-200/SensitiveAndroidFileLeak.ql
@@ -15,17 +15,13 @@ import AndroidFileIntentSink
 import AndroidFileIntentSource
 import DataFlow::PathGraph
 
-private class StartsWithSanitizer extends DataFlow::BarrierGuard {
-  StartsWithSanitizer() { this.(MethodAccess).getMethod().hasName("startsWith") }
-
-  override predicate checks(Expr e, boolean branch) {
-    e =
-      [
-        this.(MethodAccess).getQualifier(),
-        this.(MethodAccess).getQualifier().(MethodAccess).getQualifier()
-      ] and
+private predicate startsWithSanitizer(Guard g, Expr e, boolean branch) {
+  exists(MethodAccess ma |
+    g = ma and
+    ma.getMethod().hasName("startsWith") and
+    e = [ma.getQualifier(), ma.getQualifier().(MethodAccess).getQualifier()] and
     branch = false
-  }
+  )
 }
 
 class AndroidFileLeakConfig extends TaintTracking::Configuration {
@@ -75,8 +71,8 @@ class AndroidFileLeakConfig extends TaintTracking::Configuration {
     )
   }
 
-  override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) {
-    guard instanceof StartsWithSanitizer
+  override predicate isSanitizer(DataFlow::Node node) {
+    node = DataFlow::BarrierGuard<startsWithSanitizer/3>::getABarrierNode()
   }
 }
 
diff --git a/java/ql/src/experimental/Security/CWE/CWE-400/LocalThreadResourceAbuse.ql b/java/ql/src/experimental/Security/CWE/CWE-400/LocalThreadResourceAbuse.ql
@@ -59,10 +59,8 @@ class ThreadResourceAbuse extends TaintTracking::Configuration {
       ma.getMethod().hasQualifiedName("java.lang", "Math", "min") and
       node.asExpr() = ma.getAnArgument()
     )
-  }
-
-  override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) {
-    guard instanceof LessThanSanitizer // if (sleepTime > 0 && sleepTime < 5000) { ... }
+    or
+    node instanceof LessThanSanitizer // if (sleepTime > 0 && sleepTime < 5000) { ... }
   }
 }
 
diff --git a/java/ql/src/experimental/Security/CWE/CWE-400/ThreadResourceAbuse.ql b/java/ql/src/experimental/Security/CWE/CWE-400/ThreadResourceAbuse.ql
@@ -33,10 +33,8 @@ class ThreadResourceAbuse extends TaintTracking::Configuration {
       ma.getMethod().hasQualifiedName("java.lang", "Math", "min") and
       node.asExpr() = ma.getAnArgument()
     )
-  }
-
-  override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) {
-    guard instanceof LessThanSanitizer // if (sleepTime > 0 && sleepTime < 5000) { ... }
+    or
+    node instanceof LessThanSanitizer // if (sleepTime > 0 && sleepTime < 5000) { ... }
   }
 }
 
diff --git a/java/ql/src/experimental/Security/CWE/CWE-400/ThreadResourceAbuse.qll b/java/ql/src/experimental/Security/CWE/CWE-400/ThreadResourceAbuse.qll
@@ -4,6 +4,7 @@ import java
 import semmle.code.java.dataflow.DataFlow
 private import semmle.code.java.dataflow.ExternalFlow
 import semmle.code.java.dataflow.FlowSteps
+import semmle.code.java.controlflow.Guards
 
 /** `java.lang.Math` data model for value comparison in the new CSV format. */
 private class MathCompDataModel extends SummaryModelCsv {
@@ -32,15 +33,17 @@ class PauseThreadSink extends DataFlow::Node {
   PauseThreadSink() { sinkNode(this, "thread-pause") }
 }
 
+private predicate lessThanGuard(Guard g, Expr e, boolean branch) {
+  e = g.(ComparisonExpr).getLesserOperand() and
+  branch = true
+  or
+  e = g.(ComparisonExpr).getGreaterOperand() and
+  branch = false
+}
+
 /** A sanitizer for lessThan check. */
-class LessThanSanitizer extends DataFlow::BarrierGuard instanceof ComparisonExpr {
-  override predicate checks(Expr e, boolean branch) {
-    e = super.getLesserOperand() and
-    branch = true
-    or
-    e = super.getGreaterOperand() and
-    branch = false
-  }
+class LessThanSanitizer extends DataFlow::Node {
+  LessThanSanitizer() { this = DataFlow::BarrierGuard<lessThanGuard/3>::getABarrierNode() }
 }
 
 /** Value step from the constructor call of a `Runnable` to the instance parameter (this) of `run`. */
diff --git a/java/ql/src/experimental/Security/CWE/CWE-470/UnsafeReflection.ql b/java/ql/src/experimental/Security/CWE/CWE-470/UnsafeReflection.ql
diff --git a/java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.ql b/java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.ql
diff --git a/java/ql/src/experimental/Security/CWE/CWE-601/SpringUrlRedirect.ql b/java/ql/src/experimental/Security/CWE/CWE-601/SpringUrlRedirect.ql
diff --git a/java/ql/src/experimental/semmle/code/java/PathSanitizer.qll b/java/ql/src/experimental/semmle/code/java/PathSanitizer.qll

Original file line number	Diff line number	Diff line change
`@@ -332,6 +332,8 @@ module BarrierGuard<guardChecksSig/3 guardChecks> {`
`332`	`332`	`}`
`333`	`333`
`334`	`334`	`/**`
	`335`	+ * DEPRECATED: Use `BarrierGuard` module instead.
	`336`	`+ *`
`335`	`337`	`* A guard that validates some expression.`
`336`	`338`	`*`
`337`	`339`	`* To use this in a configuration, extend the class and provide a`
`@@ -340,7 +342,7 @@ module BarrierGuard<guardChecksSig/3 guardChecks> {`
`340`	`342`	`*`
`341`	`343`	`* It is important that all extending classes in scope are disjoint.`
`342`	`344`	`*/`
`343`		`-class BarrierGuard extends Guard {`
	`345`	`+deprecated class BarrierGuard extends Guard {`
`344`	`346`	/** Holds if this guard validates `e` upon evaluating to `branch`. */
`345`	`347`	`abstract predicate checks(Expr e, boolean branch);`
`346`	`348`
Original file line number	Diff line number	Diff line change
`@@ -112,12 +112,6 @@ private module Cached {`
`112`	`112`	`}`
`113`	`113`	`}`
`114`	`114`
`115`		`-/**`
`116`		- * Holds if `guard` should be a sanitizer guard in all global taint flow configurations
`117`		`- * but not in local taint.`
`118`		`- */`
`119`		`-predicate defaultTaintSanitizerGuard(DataFlow::BarrierGuard guard) { none() }`
`120`		`-`
`121`	`115`	`import Cached`
`122`	`116`
`123`	`117`	`private RefType getElementType(RefType container) {`
Original file line number	Diff line number	Diff line change
`@@ -24,7 +24,7 @@ class IntentUriPermissionManipulationConf extends TaintTracking::Configuration {`
`24`	`24`	`barrier instanceof IntentUriPermissionManipulationSanitizer`
`25`	`25`	`}`
`26`	`26`
`27`		`- override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) {`
	`27`	`+ deprecated override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) {`
`28`	`28`	`guard instanceof IntentUriPermissionManipulationGuard`
`29`	`29`	`}`
`30`	`30`
Original file line number	Diff line number	Diff line change
`@@ -30,10 +30,8 @@ class InjectFilePathConfig extends TaintTracking::Configuration {`
`30`	`30`
`31`	`31`	`override predicate isSanitizer(DataFlow::Node node) {`
`32`	`32`	`exists(Type t \| t = node.getType() \| t instanceof BoxedType or t instanceof PrimitiveType)`
`33`		`- }`
`34`		`-`
`35`		`- override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) {`
`36`		`- guard instanceof PathTraversalBarrierGuard`
	`33`	`+ or`
	`34`	`+ node instanceof PathTraversalSanitizer`
`37`	`35`	`}`
`38`	`36`	`}`
`39`	`37`
Original file line number	Diff line number	Diff line change
`@@ -24,9 +24,7 @@ class InsecureWebResourceResponseConfig extends TaintTracking::Configuration {`
`24`	`24`
`25`	`25`	`override predicate isSink(DataFlow::Node sink) { sink instanceof WebResourceResponseSink }`
`26`	`26`
`27`		`- override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) {`
`28`		`- guard instanceof PathTraversalBarrierGuard`
`29`		`- }`
	`27`	`+ override predicate isSanitizer(DataFlow::Node node) { node instanceof PathTraversalSanitizer }`
`30`	`28`	`}`
`31`	`29`
`32`	`30`	`from DataFlow::PathNode source, DataFlow::PathNode sink, InsecureWebResourceResponseConfig conf`