Merge pull request #12632 from egregius313/egregius313/java/android/refactor-android-query-libraries

Java: Refactor Android `Query.qll` libraries to new dataflow api

java/ql/lib/change-notes/2023-03-22-deprecate-webviewdubuggingenabledquery.md

@@ -0,0 +1,4 @@
+---
+category: deprecated
+---
+* The `WebViewDubuggingQuery` library has been renamed to `WebViewDebuggingQuery` to fix the typo in the file name. `WebViewDubuggingQuery` is now deprecated. 

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

@@ -106,10 +106,8 @@ private class MissingPinningSink extends DataFlow::Node {
 }
 
 /** Configuration for finding uses of non trusted URLs. */
-private class UntrustedUrlConfig extends TaintTracking::Configuration {
-  UntrustedUrlConfig() { this = "UntrustedUrlConfig" }
-
-  override predicate isSource(DataFlow::Node node) {
+private module UntrustedUrlConfig implements DataFlow::ConfigSig {
+  predicate isSource(DataFlow::Node node) {
     trustedDomain(_) and
     exists(string lit | lit = node.asExpr().(CompileTimeConstantExpr).getStringValue() |
       lit.matches("%://%") and // it's a URL
@@ -117,9 +115,11 @@ private class UntrustedUrlConfig extends TaintTracking::Configuration {
     )
   }
 
-  override predicate isSink(DataFlow::Node node) { node instanceof MissingPinningSink }
+  predicate isSink(DataFlow::Node node) { node instanceof MissingPinningSink }
 }
 
+private module UntrustedUrlFlow = TaintTracking::Global<UntrustedUrlConfig>;
+
 /** Holds if `node` is a network communication call for which certificate pinning is not implemented. */
 predicate missingPinning(DataFlow::Node node, string domain) {
   isAndroid() and
@@ -127,8 +127,8 @@ predicate missingPinning(DataFlow::Node node, string domain) {
   (
     not trustedDomain(_) and domain = ""
     or
-    exists(UntrustedUrlConfig conf, DataFlow::Node src |
-      conf.hasFlow(src, node) and
+    exists(DataFlow::Node src |
+      UntrustedUrlFlow::flow(src, node) and
       domain = getDomain(src.asExpr())
     )
   )

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

@@ -8,9 +8,11 @@ import semmle.code.java.dataflow.TaintTracking3
 import semmle.code.java.security.AndroidIntentRedirection
 
 /**
+ * DEPRECATED: Use `IntentRedirectionFlow` instead.
+ *
  * A taint tracking configuration for tainted Intents being used to start Android components.
  */
-class IntentRedirectionConfiguration extends TaintTracking::Configuration {
+deprecated class IntentRedirectionConfiguration extends TaintTracking::Configuration {
   IntentRedirectionConfiguration() { this = "IntentRedirectionConfiguration" }
 
   override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource }
@@ -26,31 +28,45 @@ class IntentRedirectionConfiguration extends TaintTracking::Configuration {
   }
 }
 
+/** A taint tracking configuration for tainted Intents being used to start Android components. */
+module IntentRedirectionConfig implements DataFlow::ConfigSig {
+  predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource }
+
+  predicate isSink(DataFlow::Node sink) { sink instanceof IntentRedirectionSink }
+
+  predicate isBarrier(DataFlow::Node sanitizer) { sanitizer instanceof IntentRedirectionSanitizer }
+
+  predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2) {
+    any(IntentRedirectionAdditionalTaintStep c).step(node1, node2)
+  }
+}
+
+/** Tracks the flow of tainted Intents being used to start Android components. */
+module IntentRedirectionFlow = TaintTracking::Global<IntentRedirectionConfig>;
+
 /**
  * A sanitizer for sinks that receive the original incoming Intent,
  * since its component cannot be arbitrarily set.
  */
 private class OriginalIntentSanitizer extends IntentRedirectionSanitizer {
-  OriginalIntentSanitizer() { any(SameIntentBeingRelaunchedConfiguration c).hasFlowTo(this) }
+  OriginalIntentSanitizer() { SameIntentBeingRelaunchedFlow::flowTo(this) }
 }
 
 /**
  * Data flow configuration used to discard incoming Intents
  * flowing directly to sinks that start Android components.
  */
-private class SameIntentBeingRelaunchedConfiguration extends DataFlow2::Configuration {
-  SameIntentBeingRelaunchedConfiguration() { this = "SameIntentBeingRelaunchedConfiguration" }
+private module SameIntentBeingRelaunchedConfig implements DataFlow::ConfigSig {
+  predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource }
 
-  override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource }
+  predicate isSink(DataFlow::Node sink) { sink instanceof IntentRedirectionSink }
 
-  override predicate isSink(DataFlow::Node sink) { sink instanceof IntentRedirectionSink }
-
-  override predicate isBarrier(DataFlow::Node barrier) {
+  predicate isBarrier(DataFlow::Node barrier) {
     // Don't discard the Intent if its original component is tainted
     barrier instanceof IntentWithTaintedComponent
   }
 
-  override predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2) {
+  predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2) {
     // Intents being built with the copy constructor from the original Intent are discarded too
     exists(ClassInstanceExpr cie |
       cie.getConstructedType() instanceof TypeIntent and
@@ -61,12 +77,14 @@ private class SameIntentBeingRelaunchedConfiguration extends DataFlow2::Configur
   }
 }
 
+private module SameIntentBeingRelaunchedFlow = DataFlow::Global<SameIntentBeingRelaunchedConfig>;
+
 /** An `Intent` with a tainted component. */
 private class IntentWithTaintedComponent extends DataFlow::Node {
   IntentWithTaintedComponent() {
-    exists(IntentSetComponent setExpr, TaintedIntentComponentConf conf |
+    exists(IntentSetComponent setExpr |
       setExpr.getQualifier() = this.asExpr() and
-      conf.hasFlowTo(DataFlow::exprNode(setExpr.getSink()))
+      TaintedIntentComponentFlow::flowTo(DataFlow::exprNode(setExpr.getSink()))
     )
   }
 }
@@ -74,16 +92,16 @@ private class IntentWithTaintedComponent extends DataFlow::Node {
 /**
  * A taint tracking configuration for tainted data flowing to an `Intent`'s component.
  */
-private class TaintedIntentComponentConf extends TaintTracking3::Configuration {
-  TaintedIntentComponentConf() { this = "TaintedIntentComponentConf" }
+private module TaintedIntentComponentConfig implements DataFlow::ConfigSig {
+  predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource }
 
-  override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource }
-
-  override predicate isSink(DataFlow::Node sink) {
+  predicate isSink(DataFlow::Node sink) {
     any(IntentSetComponent setComponent).getSink() = sink.asExpr()
   }
 }
 
+private module TaintedIntentComponentFlow = TaintTracking::Global<TaintedIntentComponentConfig>;
+
 /** A call to a method that changes the component of an `Intent`. */
 private class IntentSetComponent extends MethodAccess {
   int sinkArg;

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

@@ -14,14 +14,12 @@ private class OnReceiveMethod extends Method {
 }
 
 /** A configuration to detect whether the `action` of an `Intent` is checked. */
-private class VerifiedIntentConfig extends DataFlow::Configuration {
-  VerifiedIntentConfig() { this = "VerifiedIntentConfig" }
-
-  override predicate isSource(DataFlow::Node src) {
+private module VerifiedIntentConfig implements DataFlow::ConfigSig {
+  predicate isSource(DataFlow::Node src) {
     src.asParameter() = any(OnReceiveMethod orm).getIntentParameter()
   }
 
-  override predicate isSink(DataFlow::Node sink) {
+  predicate isSink(DataFlow::Node sink) {
     exists(MethodAccess ma |
       ma.getMethod().hasQualifiedName("android.content", "Intent", "getAction") and
       sink.asExpr() = ma.getQualifier()
@@ -29,10 +27,12 @@ private class VerifiedIntentConfig extends DataFlow::Configuration {
   }
 }
 
+private module VerifiedIntentFlow = DataFlow::Global<VerifiedIntentConfig>;
+
 /** An `onReceive` method that doesn't verify the action of the intent it receives. */
 private class UnverifiedOnReceiveMethod extends OnReceiveMethod {
   UnverifiedOnReceiveMethod() {
-    not any(VerifiedIntentConfig c).hasFlow(DataFlow::parameterNode(this.getIntentParameter()), _)
+    not VerifiedIntentFlow::flow(DataFlow::parameterNode(this.getIntentParameter()), _)
   }
 }
 

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

@@ -91,16 +91,14 @@ private predicate inputTypeFieldNotCached(Field f) {
 }
 
 /** Configuration that finds uses of `setInputType` for non cached fields. */
-private class GoodInputTypeConf extends DataFlow::Configuration {
-  GoodInputTypeConf() { this = "GoodInputTypeConf" }
-
-  override predicate isSource(DataFlow::Node node) {
+private module GoodInputTypeConfig implements DataFlow::ConfigSig {
+  predicate isSource(DataFlow::Node node) {
     inputTypeFieldNotCached(node.asExpr().(FieldAccess).getField())
   }
 
-  override predicate isSink(DataFlow::Node node) { node.asExpr() = setInputTypeForId(_) }
+  predicate isSink(DataFlow::Node node) { node.asExpr() = setInputTypeForId(_) }
 
-  override predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2) {
+  predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2) {
     exists(OrBitwiseExpr bitOr |
       node1.asExpr() = bitOr.getAChildExpr() and
       node2.asExpr() = bitOr
@@ -108,6 +106,8 @@ private class GoodInputTypeConf extends DataFlow::Configuration {
   }
 }
 
+private module GoodInputTypeFlow = DataFlow::Global<GoodInputTypeConfig>;
+
 /** Gets a regex indicating that an input field may contain sensitive data. */
 private string getInputSensitiveInfoRegex() {
   result =
@@ -130,8 +130,8 @@ AndroidEditableXmlElement getASensitiveCachedInput() {
   result.getId().regexpMatch(getInputSensitiveInfoRegex()) and
   (
     not inputTypeNotCached(result.getInputType()) and
-    not exists(GoodInputTypeConf conf, DataFlow::Node sink |
-      conf.hasFlowTo(sink) and
+    not exists(DataFlow::Node sink |
+      GoodInputTypeFlow::flowTo(sink) and
       sink.asExpr() = setInputTypeForId(result.getId())
     )
   )

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

@@ -7,9 +7,11 @@ import semmle.code.java.security.RequestForgery
 import semmle.code.java.security.UnsafeAndroidAccess
 
 /**
+ * DEPRECATED: Use `FetchUntrustedResourceFlow` instead.
+ *
  * A taint configuration tracking flow from untrusted inputs to a resource fetching call.
  */
-class FetchUntrustedResourceConfiguration extends TaintTracking::Configuration {
+deprecated class FetchUntrustedResourceConfiguration extends TaintTracking::Configuration {
   FetchUntrustedResourceConfiguration() { this = "FetchUntrustedResourceConfiguration" }
 
   override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource }
@@ -20,3 +22,19 @@ class FetchUntrustedResourceConfiguration extends TaintTracking::Configuration {
     sanitizer instanceof RequestForgerySanitizer
   }
 }
+
+/**
+ * A taint configuration tracking flow from untrusted inputs to a resource fetching call.
+ */
+module FetchUntrustedResourceConfig implements DataFlow::ConfigSig {
+  predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource }
+
+  predicate isSink(DataFlow::Node sink) { sink instanceof UrlResourceSink }
+
+  predicate isBarrier(DataFlow::Node sanitizer) { sanitizer instanceof RequestForgerySanitizer }
+}
+
+/**
+ * Detects taint flow from untrusted inputs to a resource fetching call.
+ */
+module FetchUntrustedResourceFlow = TaintTracking::Global<FetchUntrustedResourceConfig>;

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

@@ -0,0 +1,70 @@
+/** Definitions for the Android Webview Debugging Enabled query */
+
+import java
+import semmle.code.java.dataflow.DataFlow
+import semmle.code.java.controlflow.Guards
+import semmle.code.java.security.SecurityTests
+
+/** Holds if `ex` looks like a check that this is a debug build. */
+private predicate isDebugCheck(Expr ex) {
+  exists(Expr subex, string debug |
+    debug.toLowerCase().matches(["%debug%", "%test%"]) and
+    subex.getParent*() = ex
+  |
+    subex.(VarAccess).getVariable().getName() = debug
+    or
+    subex.(MethodAccess).getMethod().hasName("getProperty") and
+    subex.(MethodAccess).getAnArgument().(CompileTimeConstantExpr).getStringValue() = debug
+  )
+}
+
+/**
+ * DEPRECATED: Use `WebviewDebugEnabledFlow` instead.
+ *
+ * A configuration to find instances of `setWebContentDebuggingEnabled` called with `true` values.
+ */
+deprecated class WebviewDebugEnabledConfig extends DataFlow::Configuration {
+  WebviewDebugEnabledConfig() { this = "WebviewDebugEnabledConfig" }
+
+  override predicate isSource(DataFlow::Node node) {
+    node.asExpr().(BooleanLiteral).getBooleanValue() = true
+  }
+
+  override predicate isSink(DataFlow::Node node) {
+    exists(MethodAccess ma |
+      ma.getMethod().hasQualifiedName("android.webkit", "WebView", "setWebContentsDebuggingEnabled") and
+      node.asExpr() = ma.getArgument(0)
+    )
+  }
+
+  override predicate isBarrier(DataFlow::Node node) {
+    exists(Guard debug | isDebugCheck(debug) and debug.controls(node.asExpr().getBasicBlock(), _))
+    or
+    node.getEnclosingCallable().getDeclaringType() instanceof NonSecurityTestClass
+  }
+}
+
+/** A configuration to find instances of `setWebContentDebuggingEnabled` called with `true` values. */
+module WebviewDebugEnabledConfig implements DataFlow::ConfigSig {
+  predicate isSource(DataFlow::Node node) {
+    node.asExpr().(BooleanLiteral).getBooleanValue() = true
+  }
+
+  predicate isSink(DataFlow::Node node) {
+    exists(MethodAccess ma |
+      ma.getMethod().hasQualifiedName("android.webkit", "WebView", "setWebContentsDebuggingEnabled") and
+      node.asExpr() = ma.getArgument(0)
+    )
+  }
+
+  predicate isBarrier(DataFlow::Node node) {
+    exists(Guard debug | isDebugCheck(debug) and debug.controls(node.asExpr().getBasicBlock(), _))
+    or
+    node.getEnclosingCallable().getDeclaringType() instanceof NonSecurityTestClass
+  }
+}
+
+/**
+ * Tracks instances of `setWebContentDebuggingEnabled` with `true` values.
+ */
+module WebviewDebugEnabledFlow = DataFlow::Global<WebviewDebugEnabledConfig>;

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

@@ -1,41 +1,11 @@
-/** Definitions for the Android Webview Debugging Enabled query */
+/**
+ * DEPRECATED: Use `semmle.code.java.security.WebviewDebuggingEnabledQuery` instead.
+ *
+ * Definitions for the Android Webview Debugging Enabled query
+ */
 
 import java
-import semmle.code.java.dataflow.DataFlow
-import semmle.code.java.controlflow.Guards
-import semmle.code.java.security.SecurityTests
+private import semmle.code.java.security.WebviewDebuggingEnabledQuery as WebviewDebuggingEnabledQuery
 
-/** Holds if `ex` looks like a check that this is a debug build. */
-private predicate isDebugCheck(Expr ex) {
-  exists(Expr subex, string debug |
-    debug.toLowerCase().matches(["%debug%", "%test%"]) and
-    subex.getParent*() = ex
-  |
-    subex.(VarAccess).getVariable().getName() = debug
-    or
-    subex.(MethodAccess).getMethod().hasName("getProperty") and
-    subex.(MethodAccess).getAnArgument().(CompileTimeConstantExpr).getStringValue() = debug
-  )
-}
-
-/** A configuration to find instances of `setWebContentDebuggingEnabled` called with `true` values. */
-class WebviewDebugEnabledConfig extends DataFlow::Configuration {
-  WebviewDebugEnabledConfig() { this = "WebviewDebugEnabledConfig" }
-
-  override predicate isSource(DataFlow::Node node) {
-    node.asExpr().(BooleanLiteral).getBooleanValue() = true
-  }
-
-  override predicate isSink(DataFlow::Node node) {
-    exists(MethodAccess ma |
-      ma.getMethod().hasQualifiedName("android.webkit", "WebView", "setWebContentsDebuggingEnabled") and
-      node.asExpr() = ma.getArgument(0)
-    )
-  }
-
-  override predicate isBarrier(DataFlow::Node node) {
-    exists(Guard debug | isDebugCheck(debug) and debug.controls(node.asExpr().getBasicBlock(), _))
-    or
-    node.getEnclosingCallable().getDeclaringType() instanceof NonSecurityTestClass
-  }
-}
+deprecated class WebviewDebugEnabledConfig =
+  WebviewDebuggingEnabledQuery::WebviewDebugEnabledConfig;

java/ql/src/Security/CWE/CWE-489/WebviewDebuggingEnabled.ql

@@ -11,9 +11,9 @@
  */
 
 import java
-import semmle.code.java.security.WebviewDubuggingEnabledQuery
-import DataFlow::PathGraph
+import semmle.code.java.security.WebviewDebuggingEnabledQuery
+import WebviewDebugEnabledFlow::PathGraph
 
-from WebviewDebugEnabledConfig conf, DataFlow::PathNode source, DataFlow::PathNode sink
-where conf.hasFlowPath(source, sink)
+from WebviewDebugEnabledFlow::PathNode source, WebviewDebugEnabledFlow::PathNode sink
+where WebviewDebugEnabledFlow::flowPath(source, sink)
 select sink, source, sink, "Webview debugging is enabled."

java/ql/src/Security/CWE/CWE-749/UnsafeAndroidAccess.ql

@@ -14,9 +14,9 @@
 
 import java
 import semmle.code.java.security.UnsafeAndroidAccessQuery
-import DataFlow::PathGraph
+import FetchUntrustedResourceFlow::PathGraph
 
-from DataFlow::PathNode source, DataFlow::PathNode sink, FetchUntrustedResourceConfiguration conf
-where conf.hasFlowPath(source, sink)
+from FetchUntrustedResourceFlow::PathNode source, FetchUntrustedResourceFlow::PathNode sink
+where FetchUntrustedResourceFlow::flowPath(source, sink)
 select sink.getNode(), source, sink, "Unsafe resource fetching in Android WebView due to $@.",
   source.getNode(), sink.getNode().(UrlResourceSink).getSinkType()

java/ql/src/Security/CWE/CWE-940/AndroidIntentRedirection.ql

@@ -15,10 +15,10 @@
 
 import java
 import semmle.code.java.security.AndroidIntentRedirectionQuery
-import DataFlow::PathGraph
+import IntentRedirectionFlow::PathGraph
 
-from DataFlow::PathNode source, DataFlow::PathNode sink, IntentRedirectionConfiguration conf
-where conf.hasFlowPath(source, sink)
+from IntentRedirectionFlow::PathNode source, IntentRedirectionFlow::PathNode sink
+where IntentRedirectionFlow::flowPath(source, sink)
 select sink.getNode(), source, sink,
   "Arbitrary Android activities or services can be started from a $@.", source.getNode(),
   "user-provided value"

java/ql/test/query-tests/security/CWE-489/webview-debugging/WebviewDebuggingEnabled.ql

@@ -1,15 +1,11 @@
 import java
 import TestUtilities.InlineFlowTest
-import semmle.code.java.security.WebviewDubuggingEnabledQuery
-
-class EnableLegacy extends EnableLegacyConfiguration {
-  EnableLegacy() { exists(this) }
-}
+import semmle.code.java.security.WebviewDebuggingEnabledQuery
 
 class HasFlowTest extends InlineFlowTest {
-  override DataFlow::Configuration getTaintFlowConfig() { none() }
+  override predicate hasTaintFlow(DataFlow::Node src, DataFlow::Node sink) { none() }
 
-  override DataFlow::Configuration getValueFlowConfig() {
-    result = any(WebviewDebugEnabledConfig c)
+  override predicate hasValueFlow(DataFlow::Node src, DataFlow::Node sink) {
+    WebviewDebugEnabledFlow::flow(src, sink)
   }
 }

java/ql/test/query-tests/security/CWE-749/UnsafeAndroidAccessTest.ql

@@ -9,7 +9,7 @@ class UnsafeAndroidAccessTest extends InlineExpectationsTest {
 
   override predicate hasActualResult(Location location, string element, string tag, string value) {
     tag = "hasUnsafeAndroidAccess" and
-    exists(DataFlow::Node sink, FetchUntrustedResourceConfiguration conf | conf.hasFlowTo(sink) |
+    exists(DataFlow::Node sink | FetchUntrustedResourceFlow::flowTo(sink) |
       sink.getLocation() = location and
       element = sink.toString() and
       value = ""

java/ql/test/query-tests/security/CWE-940/AndroidIntentRedirectionTest.ql

@@ -9,7 +9,7 @@ class HasAndroidIntentRedirectionTest extends InlineExpectationsTest {
 
   override predicate hasActualResult(Location location, string element, string tag, string value) {
     tag = "hasAndroidIntentRedirection" and
-    exists(DataFlow::Node sink, IntentRedirectionConfiguration conf | conf.hasFlowTo(sink) |
+    exists(DataFlow::Node sink | IntentRedirectionFlow::flowTo(sink) |
       sink.getLocation() = location and
       element = sink.toString() and
       value = ""