Java: Improved the query for disabled certificate revocation checking

- Added a taint propagation step for List.of() methods
- Added a testcase with one of the List.of() method
- Simplified conditions
- Fixed typos

Author: artem-smotrakov

java/ql/src/experimental/Security/CWE/CWE-299/DisabledRevocationChecking.qhelp

@@ -56,7 +56,7 @@ revocation checker that uses OCSP to obtain revocation status of certificates.</
 </li>
 <li>
   Java SE API Specification:
-  <a href="https://docs.oracle.com/javase/8/docs/api/index.html?java/security/cert/CertPathValidator.html">CertPathValidator</a>
+  <a href="https://docs.oracle.com/javase/8/docs/api/java/security/cert/CertPathValidator.html">CertPathValidator</a>
 </li>
 
 </references>

java/ql/src/experimental/Security/CWE/CWE-299/DisabledRevocationChecking.ql

@@ -1,7 +1,7 @@
 /**
  * @name Disabled ceritificate revocation checking
  * @description Using revoked certificates is dangerous.
- *              Therefore, revocation status of ceritifcates in a chain should be checked.
+ *              Therefore, revocation status of certificates in a chain should be checked.
  * @kind path-problem
  * @problem.severity error
  * @precision high

java/ql/src/experimental/Security/CWE/CWE-299/RevocationCheckingLib.qll

@@ -53,6 +53,7 @@ class SettingRevocationCheckerConfig extends DataFlow2::Configuration {
 
   override predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2) {
     createSingletonListStep(node1, node2) or
+    createListOfElementsStep(node1, node2) or
     convertArrayToListStep(node1, node2) or
     addToListStep(node1, node2)
   }
@@ -99,20 +100,20 @@ predicate createSingletonListStep(DataFlow::Node node1, DataFlow::Node node2) {
     m.getDeclaringType() instanceof Collections and
     m.hasName("singletonList") and
     ma.getArgument(0) = node1.asExpr() and
-    (ma = node2.asExpr() or ma.getQualifier() = node2.asExpr())
+    ma = node2.asExpr()
   )
 }
 
 /**
- * Holds if `node1` to `node2` is a dataflow step that converts an array to a list,class
+ * Holds if `node1` to `node2` is a dataflow step that converts an array to a list
  * i.e. `Arrays.asList(element)`.
  */
 predicate convertArrayToListStep(DataFlow::Node node1, DataFlow::Node node2) {
   exists(StaticMethodAccess ma, Method m | m = ma.getMethod() |
     m.getDeclaringType() instanceof Arrays and
     m.hasName("asList") and
     ma.getArgument(0) = node1.asExpr() and
-    (ma = node2.asExpr() or ma.getQualifier() = node2.asExpr())
+    ma = node2.asExpr()
   )
 }
 
@@ -128,7 +129,20 @@ predicate addToListStep(DataFlow::Node node1, DataFlow::Node node2) {
       m.hasName("addAll")
     ) and
     ma.getArgument(0) = node1.asExpr() and
-    (ma = node2.asExpr() or ma.getQualifier() = node2.asExpr())
+    ma.getQualifier() = node2.asExpr()
+  )
+}
+
+/**
+ * Holds if `node1` to `node2` is a dataflow step that creates a list,
+ * i.e. `List.of(element)`.
+ */
+predicate createListOfElementsStep(DataFlow::Node node1, DataFlow::Node node2) {
+  exists(StaticMethodAccess ma, Method m | m = ma.getMethod() |
+    m.getDeclaringType() instanceof List and
+    m.hasName("of") and
+    ma.getAnArgument() = node1.asExpr() and
+    ma = node2.asExpr()
   )
 }
 
@@ -176,6 +190,9 @@ class Arrays extends RefType {
   Arrays() { hasQualifiedName("java.util", "Arrays") }
 }
 
-class List extends ParameterizedInterface {
-  List() { getGenericType().hasQualifiedName("java.util", "List") }
+class List extends RefType {
+  List() {
+    this.hasQualifiedName("java.util", "List<>") or
+    this.(ParameterizedInterface).getGenericType().hasQualifiedName("java.util", "List")
+  }
 }

java/ql/test/experimental/Security/CWE/CWE-299/DisabledRevocationChecking.java

@@ -47,7 +47,7 @@ public void testSettingRevocationCheckerWithArraysAsList(KeyStore cacerts, CertP
     validator.validate(certPath, params);
   }
 
-  public void testSettingRevocationCheckerWithList(KeyStore cacerts, CertPath certPath) throws Exception {
+  public void testSettingRevocationCheckerWithAddingToArrayList(KeyStore cacerts, CertPath certPath) throws Exception {
     CertPathValidator validator = CertPathValidator.getInstance("PKIX");
     PKIXParameters params = new PKIXParameters(cacerts);
     params.setRevocationEnabled(false);
@@ -58,6 +58,16 @@ public void testSettingRevocationCheckerWithList(KeyStore cacerts, CertPath cert
     validator.validate(certPath, params);
   }
 
+  public void testSettingRevocationCheckerWithListOf(KeyStore cacerts, CertPath certPath) throws Exception {
+    CertPathValidator validator = CertPathValidator.getInstance("PKIX");
+    PKIXParameters params = new PKIXParameters(cacerts);
+    params.setRevocationEnabled(false);
+    PKIXRevocationChecker checker = (PKIXRevocationChecker) validator.getRevocationChecker();
+    List<PKIXCertPathChecker> checkers = List.of(checker);
+    params.setCertPathCheckers(checkers);
+    validator.validate(certPath, params);
+  }
+
   public void testAddingRevocationChecker(KeyStore cacerts, CertPath certPath) throws Exception {
     CertPathValidator validator = CertPathValidator.getInstance("PKIX");
     PKIXParameters params = new PKIXParameters(cacerts);