Merge pull request #18340 from jbj/diff-informed-getASelectedLocation

jbj · web-flow · commit 773a98a9eb76 · 2025-01-22T14:25:33.000+01:00
Java: make more queries diff-informed with getASelectedLocation
diff --git a/java/ql/lib/semmle/code/java/security/BrokenCryptoAlgorithmQuery.qll b/java/ql/lib/semmle/code/java/security/BrokenCryptoAlgorithmQuery.qll
@@ -31,6 +31,12 @@ module InsecureCryptoConfig implements DataFlow::ConfigSig {
   predicate isSink(DataFlow::Node n) { exists(CryptoAlgoSpec c | n.asExpr() = c.getAlgoSpec()) }
 
   predicate isBarrier(DataFlow::Node node) { node instanceof SimpleTypeSanitizer }
+
+  predicate observeDiffInformedIncrementalMode() { any() }
+
+  Location getASelectedSinkLocation(DataFlow::Node sink) {
+    exists(CryptoAlgoSpec c | sink.asExpr() = c.getAlgoSpec() | result = c.getLocation())
+  }
 }
 
 /**
diff --git a/java/ql/lib/semmle/code/java/security/CommandLineQuery.qll b/java/ql/lib/semmle/code/java/security/CommandLineQuery.qll
@@ -58,6 +58,13 @@ module InputToArgumentToExecFlowConfig implements DataFlow::ConfigSig {
   predicate isAdditionalFlowStep(DataFlow::Node n1, DataFlow::Node n2) {
     any(CommandInjectionAdditionalTaintStep s).step(n1, n2)
   }
+
+  // It's valid to use diff-informed data flow for this configuration because
+  // the location of the selected element in the query is contained inside the
+  // location of the sink. The query, as a predicate, is used negated in
+  // another query, but that's only to prevent overlapping results between two
+  // queries.
+  predicate observeDiffInformedIncrementalMode() { any() }
 }
 
 /**
diff --git a/java/ql/lib/semmle/code/java/security/ImproperIntentVerificationQuery.qll b/java/ql/lib/semmle/code/java/security/ImproperIntentVerificationQuery.qll
@@ -25,6 +25,25 @@ private module VerifiedIntentConfig implements DataFlow::ConfigSig {
       sink.asExpr() = ma.getQualifier()
     )
   }
+
+  predicate observeDiffInformedIncrementalMode() { any() }
+
+  Location getASelectedSourceLocation(DataFlow::Node src) {
+    exists(AndroidReceiverXmlElement rec, OnReceiveMethod orm, SystemActionName sa |
+      src.asParameter() = orm.getIntentParameter() and
+      anySystemReceiver(rec, orm, sa)
+    |
+      result = rec.getLocation()
+      or
+      result = orm.getLocation()
+      or
+      result = sa.getLocation()
+    )
+  }
+
+  // All sinks are set to have no locations because sinks aren't selected in
+  // the query. This effectively means that we're filtering on sources only.
+  Location getASelectedSinkLocation(DataFlow::Node sink) { none() }
 }
 
 private module VerifiedIntentFlow = DataFlow::Global<VerifiedIntentConfig>;
@@ -67,13 +86,20 @@ class SystemActionName extends AndroidActionXmlElement {
   string getSystemActionName() { result = name }
 }
 
-/** Holds if the XML element `rec` declares a receiver `orm` to receive the system action named `sa` that doesn't verify intents it receives. */
-predicate unverifiedSystemReceiver(
-  AndroidReceiverXmlElement rec, UnverifiedOnReceiveMethod orm, SystemActionName sa
+private predicate anySystemReceiver(
+  AndroidReceiverXmlElement rec, OnReceiveMethod orm, SystemActionName sa
 ) {
   exists(Class ormty |
     ormty = orm.getDeclaringType() and
     rec.getComponentName() = ["." + ormty.getName(), ormty.getQualifiedName()] and
     rec.getAnIntentFilterElement().getAnActionElement() = sa
   )
 }
+
+/** Holds if the XML element `rec` declares a receiver `orm` to receive the system action named `sa` that doesn't verify intents it receives. */
+predicate unverifiedSystemReceiver(
+  AndroidReceiverXmlElement rec, UnverifiedOnReceiveMethod orm, SystemActionName sa
+) {
+  // The type of `orm` is different in these two predicates
+  anySystemReceiver(rec, orm, sa)
+}
diff --git a/java/ql/lib/semmle/code/java/security/InsecureTrustManagerQuery.qll b/java/ql/lib/semmle/code/java/security/InsecureTrustManagerQuery.qll
@@ -18,6 +18,17 @@ module InsecureTrustManagerConfig implements DataFlow::ConfigSig {
     node.getType() instanceof Array and
     c instanceof DataFlow::ArrayContent
   }
+
+  predicate observeDiffInformedIncrementalMode() { any() }
+
+  Location getASelectedSourceLocation(DataFlow::Node source) {
+    isSource(source) and
+    (
+      result = source.getLocation()
+      or
+      result = source.asExpr().(ClassInstanceExpr).getConstructedType().getLocation()
+    )
+  }
 }
 
 module InsecureTrustManagerFlow = DataFlow::Global<InsecureTrustManagerConfig>;
diff --git a/java/ql/lib/semmle/code/java/security/RandomQuery.qll b/java/ql/lib/semmle/code/java/security/RandomQuery.qll
@@ -37,6 +37,18 @@ private module PredictableSeedFlowConfig implements DataFlow::ConfigSig {
   predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2) {
     predictableCalcStep(node1.asExpr(), node2.asExpr())
   }
+
+  predicate observeDiffInformedIncrementalMode() { any() }
+
+  Location getASelectedSinkLocation(DataFlow::Node sink) {
+    // This predicate matches `PredictableSeed.ql`, which is the only place
+    // where `PredictableSeedFlowConfig` is used.
+    exists(GetRandomData da, VarRead use |
+      result = da.getLocation() and
+      da.getQualifier() = use and
+      isSeeding(sink.asExpr(), use)
+    )
+  }
 }
 
 private module PredictableSeedFlow = DataFlow::Global<PredictableSeedFlowConfig>;
diff --git a/java/ql/lib/semmle/code/java/security/SqlInjectionQuery.qll b/java/ql/lib/semmle/code/java/security/SqlInjectionQuery.qll
@@ -24,6 +24,8 @@ module QueryInjectionFlowConfig implements DataFlow::ConfigSig {
   predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2) {
     any(AdditionalQueryInjectionTaintStep s).step(node1, node2)
   }
+
+  predicate observeDiffInformedIncrementalMode() { any() }
 }
 
 /** Tracks flow of unvalidated user input that is used in SQL queries. */
diff --git a/java/ql/lib/semmle/code/java/security/TaintedPermissionsCheckQuery.qll b/java/ql/lib/semmle/code/java/security/TaintedPermissionsCheckQuery.qll
@@ -59,6 +59,15 @@ module TaintedPermissionsCheckFlowConfig implements DataFlow::ConfigSig {
   predicate isSink(DataFlow::Node sink) {
     sink.asExpr() = any(PermissionsConstruction p).getInput()
   }
+
+  predicate observeDiffInformedIncrementalMode() { any() }
+
+  Location getASelectedSinkLocation(DataFlow::Node sink) {
+    exists(PermissionsConstruction p |
+      sink.asExpr() = p.getInput() and
+      result = p.getLocation()
+    )
+  }
 }
 
 /** Tracks flow from user input to a permissions check. */
diff --git a/java/ql/lib/semmle/code/java/security/UnsafeHostnameVerificationQuery.qll b/java/ql/lib/semmle/code/java/security/UnsafeHostnameVerificationQuery.qll
@@ -65,6 +65,17 @@ module TrustAllHostnameVerifierConfig implements DataFlow::ConfigSig {
             "|(set)?(accept|trust|ignore|allow)(all|every|any)" +
             "|(use|do|enable)insecure|(set|do|use)?no.*(check|validation|verify|verification)|disable).*$")
   }
+
+  predicate observeDiffInformedIncrementalMode() { any() }
+
+  Location getASelectedSourceLocation(DataFlow::Node source) {
+    isSource(source) and
+    (
+      result = source.getLocation()
+      or
+      result = source.asExpr().(ClassInstanceExpr).getConstructedType().getLocation()
+    )
+  }
 }
 
 /** Data flow to model the flow of a `TrustAllHostnameVerifier` to a `set(Default)HostnameVerifier` call. */
diff --git a/java/ql/lib/semmle/code/java/security/regexp/PolynomialReDoSQuery.qll b/java/ql/lib/semmle/code/java/security/regexp/PolynomialReDoSQuery.qll
@@ -47,6 +47,18 @@ module PolynomialRedosConfig implements DataFlow::ConfigSig {
     node instanceof SimpleTypeSanitizer or
     node.asExpr().(MethodCall).getMethod() instanceof LengthRestrictedMethod
   }
+
+  predicate observeDiffInformedIncrementalMode() { any() }
+
+  Location getASelectedSinkLocation(DataFlow::Node sink) {
+    exists(SuperlinearBackTracking::PolynomialBackTrackingTerm regexp |
+      regexp.getRootTerm() = sink.(PolynomialRedosSink).getRegExp()
+    |
+      result = sink.getLocation()
+      or
+      result = regexp.getLocation()
+    )
+  }
 }
 
 module PolynomialRedosFlow = TaintTracking::Global<PolynomialRedosConfig>;
diff --git a/java/ql/test/query-tests/security/CWE-297/UnsafeHostnameVerification.expected b/java/ql/test/query-tests/security/CWE-297/UnsafeHostnameVerification.expected
@@ -4,6 +4,7 @@
 | UnsafeHostnameVerification.java:47:55:47:71 | ...->... | UnsafeHostnameVerification.java:47:55:47:71 | ...->... | UnsafeHostnameVerification.java:47:55:47:71 | ...->... | The $@ defined by $@ always accepts any certificate, even if the hostname does not match. | UnsafeHostnameVerification.java:47:55:47:71 | ...->... | hostname verifier | UnsafeHostnameVerification.java:47:55:47:71 | new HostnameVerifier(...) { ... } | this type |
 | UnsafeHostnameVerification.java:81:55:81:62 | verifier | UnsafeHostnameVerification.java:66:37:80:9 | new (...) : new HostnameVerifier(...) { ... } | UnsafeHostnameVerification.java:81:55:81:62 | verifier | The $@ defined by $@ always accepts any certificate, even if the hostname does not match. | UnsafeHostnameVerification.java:66:37:80:9 | new (...) : new HostnameVerifier(...) { ... } | hostname verifier | UnsafeHostnameVerification.java:66:41:66:56 | new HostnameVerifier(...) { ... } | this type |
 | UnsafeHostnameVerification.java:94:55:94:62 | verifier | UnsafeHostnameVerification.java:88:37:93:9 | new (...) : new HostnameVerifier(...) { ... } | UnsafeHostnameVerification.java:94:55:94:62 | verifier | The $@ defined by $@ always accepts any certificate, even if the hostname does not match. | UnsafeHostnameVerification.java:88:37:93:9 | new (...) : new HostnameVerifier(...) { ... } | hostname verifier | UnsafeHostnameVerification.java:88:41:88:56 | new HostnameVerifier(...) { ... } | this type |
+| UnsafeHostnameVerification.java:116:55:116:78 | new AlwaysTrueVerifier(...) | UnsafeHostnameVerification.java:116:55:116:78 | new AlwaysTrueVerifier(...) | UnsafeHostnameVerification.java:116:55:116:78 | new AlwaysTrueVerifier(...) | The $@ defined by $@ always accepts any certificate, even if the hostname does not match. | UnsafeHostnameVerification.java:116:55:116:78 | new AlwaysTrueVerifier(...) | hostname verifier | UnsafeHostnameVerification.java:104:26:104:43 | AlwaysTrueVerifier | this type |
 edges
 | UnsafeHostnameVerification.java:66:37:80:9 | new (...) : new HostnameVerifier(...) { ... } | UnsafeHostnameVerification.java:81:55:81:62 | verifier | provenance | Sink:MaD:1 |
 | UnsafeHostnameVerification.java:88:37:93:9 | new (...) : new HostnameVerifier(...) { ... } | UnsafeHostnameVerification.java:94:55:94:62 | verifier | provenance | Sink:MaD:1 |
@@ -23,4 +24,5 @@ nodes
 | UnsafeHostnameVerification.java:94:55:94:62 | verifier | semmle.label | verifier |
 | UnsafeHostnameVerification.java:97:42:97:68 | ALLOW_ALL_HOSTNAME_VERIFIER : new HostnameVerifier(...) { ... } | semmle.label | ALLOW_ALL_HOSTNAME_VERIFIER : new HostnameVerifier(...) { ... } |
 | UnsafeHostnameVerification.java:97:72:102:5 | new (...) : new HostnameVerifier(...) { ... } | semmle.label | new (...) : new HostnameVerifier(...) { ... } |
+| UnsafeHostnameVerification.java:116:55:116:78 | new AlwaysTrueVerifier(...) | semmle.label | new AlwaysTrueVerifier(...) |
 subpaths
diff --git a/java/ql/test/query-tests/security/CWE-297/UnsafeHostnameVerification.java b/java/ql/test/query-tests/security/CWE-297/UnsafeHostnameVerification.java
@@ -100,4 +100,20 @@ public boolean verify(String hostname, SSLSession session) {
             return true; // BAD, always returns true
         }
     };
+
+    private static class AlwaysTrueVerifier implements HostnameVerifier {
+        @Override
+        public boolean verify(String hostname, SSLSession session) {
+            return true; // BAD, always returns true
+        }
+    }
+
+    /**
+     * Same as testTrustAllHostnameOfAnonymousClass, but with a named class.
+     * This is for testing the diff-informed functionality of the query.
+     */
+    public void testTrustAllHostnameOfNamedClass() {
+        HttpsURLConnection.setDefaultHostnameVerifier(new AlwaysTrueVerifier());
+    }
+
 }
diff --git a/shared/dataflow/codeql/dataflow/DataFlow.qll b/shared/dataflow/codeql/dataflow/DataFlow.qll
@@ -442,6 +442,28 @@ module Configs<LocationSig Location, InputSig<Location> Lang> {
      * are used directly in a query result.
      */
     default predicate observeDiffInformedIncrementalMode() { none() }
+
+    /**
+     * Gets a location that will be associated with the given `source` in a
+     * diff-informed query that uses this configuration (see
+     * `observeDiffInformedIncrementalMode`). By default, this is the location
+     * of the source itself, but this predicate should include any locations
+     * that are reported as the primary-location of the query or as an
+     * additional location ("$@" interpolation). For a query that doesn't
+     * report the source at all, this predicate can be `none()`.
+     */
+    default Location getASelectedSourceLocation(Node source) { result = source.getLocation() }
+
+    /**
+     * Gets a location that will be associated with the given `sink` in a
+     * diff-informed query that uses this configuration (see
+     * `observeDiffInformedIncrementalMode`). By default, this is the location
+     * of the sink itself, but this predicate should include any locations
+     * that are reported as the primary-location of the query or as an
+     * additional location ("$@" interpolation). For a query that doesn't
+     * report the sink at all, this predicate can be `none()`.
+     */
+    default Location getASelectedSinkLocation(Node sink) { result = sink.getLocation() }
   }
 
   /** An input configuration for data flow using flow state. */
@@ -569,6 +591,28 @@ module Configs<LocationSig Location, InputSig<Location> Lang> {
      * are used directly in a query result.
      */
     default predicate observeDiffInformedIncrementalMode() { none() }
+
+    /**
+     * Gets a location that will be associated with the given `source` in a
+     * diff-informed query that uses this configuration (see
+     * `observeDiffInformedIncrementalMode`). By default, this is the location
+     * of the source itself, but this predicate should include any locations
+     * that are reported as the primary-location of the query or as an
+     * additional location ("$@" interpolation). For a query that doesn't
+     * report the source at all, this predicate can be `none()`.
+     */
+    default Location getASelectedSourceLocation(Node source) { result = source.getLocation() }
+
+    /**
+     * Gets a location that will be associated with the given `sink` in a
+     * diff-informed query that uses this configuration (see
+     * `observeDiffInformedIncrementalMode`). By default, this is the location
+     * of the sink itself, but this predicate should include any locations
+     * that are reported as the primary-location of the query or as an
+     * additional location ("$@" interpolation). For a query that doesn't
+     * report the sink at all, this predicate can be `none()`.
+     */
+    default Location getASelectedSinkLocation(Node sink) { result = sink.getLocation() }
   }
 }
 
diff --git a/shared/dataflow/codeql/dataflow/internal/DataFlowImpl.qll b/shared/dataflow/codeql/dataflow/internal/DataFlowImpl.qll
@@ -137,6 +137,10 @@ module MakeImpl<LocationSig Location, InputSig<Location> Lang> {
      * are used directly in a query result.
      */
     predicate observeDiffInformedIncrementalMode();
+
+    Location getASelectedSourceLocation(Node source);
+
+    Location getASelectedSinkLocation(Node sink);
   }
 
   /**
@@ -177,7 +181,7 @@ module MakeImpl<LocationSig Location, InputSig<Location> Lang> {
       private predicate isFilteredSource(Node source) {
         Config::isSource(source, _) and
         if Config::observeDiffInformedIncrementalMode()
-        then AlertFiltering::filterByLocation(source.getLocation())
+        then AlertFiltering::filterByLocation(Config::getASelectedSourceLocation(source))
         else any()
       }
 
@@ -188,7 +192,7 @@ module MakeImpl<LocationSig Location, InputSig<Location> Lang> {
           Config::isSink(sink)
         ) and
         if Config::observeDiffInformedIncrementalMode()
-        then AlertFiltering::filterByLocation(sink.getLocation())
+        then AlertFiltering::filterByLocation(Config::getASelectedSinkLocation(sink))
         else any()
       }
 

Original file line number	Diff line number	Diff line change
`@@ -58,6 +58,13 @@ module InputToArgumentToExecFlowConfig implements DataFlow::ConfigSig {`
`58`	`58`	`predicate isAdditionalFlowStep(DataFlow::Node n1, DataFlow::Node n2) {`
`59`	`59`	`any(CommandInjectionAdditionalTaintStep s).step(n1, n2)`
`60`	`60`	`}`
	`61`	`+`
	`62`	`+ // It's valid to use diff-informed data flow for this configuration because`
	`63`	`+ // the location of the selected element in the query is contained inside the`
	`64`	`+ // location of the sink. The query, as a predicate, is used negated in`
	`65`	`+ // another query, but that's only to prevent overlapping results between two`
	`66`	`+ // queries.`
	`67`	`+ predicate observeDiffInformedIncrementalMode() { any() }`
`61`	`68`	`}`
`62`	`69`
`63`	`70`	`/**`
Original file line number	Diff line number	Diff line change
`@@ -24,6 +24,8 @@ module QueryInjectionFlowConfig implements DataFlow::ConfigSig {`
`24`	`24`	`predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2) {`
`25`	`25`	`any(AdditionalQueryInjectionTaintStep s).step(node1, node2)`
`26`	`26`	`}`
	`27`	`+`
	`28`	`+ predicate observeDiffInformedIncrementalMode() { any() }`
`27`	`29`	`}`
`28`	`30`
`29`	`31`	`/** Tracks flow of unvalidated user input that is used in SQL queries. */`