Java: Added a query for disabled certificate revocation checking

- Added experimental/Security/CWE/CWE-299/DisabledRevocationChecking.ql
  The query looks for PKIXParameters.setRevocationEnabled(false) calls.
- Added RevocationCheckingLib.qll
- Added a qhelp file with examples
- Added tests in java/ql/test/experimental/Security/CWE/CWE-299

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

@@ -0,0 +1,10 @@
+public void validate(KeyStore cacerts, CertPath certPath) throws Exception {
+    CertPathValidator validator = CertPathValidator.getInstance("PKIX");
+    PKIXParameters params = new PKIXParameters(cacerts);
+    params.setRevocationEnabled(false);
+    PKIXRevocationChecker checker = (PKIXRevocationChecker) validator.getRevocationChecker();
+    checker.setOcspResponder(OCSP_RESPONDER_URL);
+    checker.setOcspResponderCert(OCSP_RESPONDER_CERT);
+    params.addCertPathChecker(checker);
+    validator.validate(certPath, params);
+}

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

@@ -0,0 +1,5 @@
+public void validate(KeyStore cacerts, CertPath chain) throws Exception {
+    CertPathValidator validator = CertPathValidator.getInstance("PKIX");
+    PKIXParameters params = new PKIXParameters(cacerts);
+    validator.validate(chain, params);
+}

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

@@ -0,0 +1,63 @@
+<!DOCTYPE qhelp PUBLIC "-//Semmle//qhelp//EN" "qhelp.dtd">
+<qhelp>
+
+<overview>
+<p>Validating a certificate chain includes multiple steps. One of them is checking whether or not
+certificates in the chain have been revoked. A certificate may be revoked due to multiple reasons. 
+One of the reasons why the certificate authority (CA) may revoke a certificate is that its private key
+has been compromised. For example, the private key might have been stolen by an adversary.
+In this case, the adversary may be able to impersonate the owner of the private key.
+Therefore, trusting a revoked certificate may be dangerous.</p>
+
+<p>The Java Certification Path API provides a revocation checking mechanism 
+that supports both CRL and OCSP.
+Revocation checking happens while building and validating certificate chains.
+If at least one of the certificates is revoked, then an exception is thrown.
+This mechanism is enabled by default. However, it may be disabled
+by passing <code>false</code> to the <code>PKIXParameters.setRevocationEnabled()</code> method.
+If an application doesn't set a custom <code>PKIXRevocationChecker</code> 
+via <code>PKIXParameters.addCertPathChecker()</code> 
+or <code>PKIXParameters.setCertPathCheckers()</code> methods,
+then revocation checking is not going to happen.</p>
+
+</overview>
+<recommendation>
+
+<p>An application should not disable the default revocationg checking mechanism
+unless it provides a custom revocation checker.</p>
+
+</recommendation>
+<example>
+
+<p>The following example turns off revocation checking for validating a certificate chain.
+That should be avoided.</p>
+
+<sample src="NoRevocationChecking.java" />
+
+<p>The next example uses the default revocation checking mechanism.</p>
+
+<sample src="DefaultRevocationChecking.java" />
+
+<p>The third example turns off the default revocation mechanism. However, it registers another 
+revocation checker that uses OCSP to obtain revocation status of certificates.</p>
+
+<sample src="CustomRevocationChecking.java" />
+
+</example>
+<references>
+
+<li>
+  Wikipedia:
+  <a href="https://en.wikipedia.org/wiki/Public_key_certificate">Public key certificate</a>
+</li>
+<li>
+  Java SE Documentation:
+  <a href="https://docs.oracle.com/javase/8/docs/technotes/guides/security/certpath/CertPathProgGuide.html">Java PKI Programmer's Guide</a>
+</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>
+</li>
+
+</references>
+</qhelp>

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

@@ -0,0 +1,20 @@
+/**
+ * @name Disabled ceritificate revocation checking
+ * @description Using revoked certificates is dangerous.
+ *              Therefore, revocation status of ceritifcates in a chain should be checked.
+ * @kind path-problem
+ * @problem.severity error
+ * @precision high
+ * @id java/disabled-certificate-revocation-checking
+ * @tags security
+ *       external/cwe/cwe-299
+ */
+
+import java
+import RevocationCheckingLib
+import DataFlow::PathGraph
+
+from DataFlow::PathNode source, DataFlow::PathNode sink, DisabledRevocationCheckingConfig config
+where config.hasFlowPath(source, sink)
+select source.getNode(), source, sink, "Revocation checking is disabled $@.", source.getNode(),
+  "here"

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

@@ -0,0 +1,6 @@
+public void validateUnsafe(KeyStore cacerts, CertPath chain) throws Exception {
+    CertPathValidator validator = CertPathValidator.getInstance("PKIX");
+    PKIXParameters params = new PKIXParameters(cacerts);
+    params.setRevocationEnabled(false);
+    validator.validate(chain, params);
+}

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

@@ -0,0 +1,181 @@
+import java
+import semmle.code.java.dataflow.FlowSources
+import semmle.code.java.dataflow.TaintTracking2
+import DataFlow
+
+/**
+ * A taint-tracking configuration for disabling revocation checking.
+ */
+class DisabledRevocationCheckingConfig extends TaintTracking::Configuration {
+  DisabledRevocationCheckingConfig() { this = "DisabledRevocationCheckingConfig" }
+
+  override predicate isSource(DataFlow::Node source) {
+    exists(BooleanLiteral b | b.getBooleanValue() = false | source.asExpr() = b)
+  }
+
+  override predicate isSink(DataFlow::Node sink) { sink instanceof SetRevocationEnabledSink }
+}
+
+/**
+ * A sink that disables revocation checking,
+ * i.e. calling `PKIXParameters.setRevocationEnabled(false)`
+ * without setting a custom revocation checker in `PKIXParameters`.
+ */
+class SetRevocationEnabledSink extends DataFlow::ExprNode {
+  SetRevocationEnabledSink() {
+    exists(MethodAccess setRevocationEnabledCall |
+      setRevocationEnabledCall.getMethod() instanceof SetRevocationEnabledMethod and
+      setRevocationEnabledCall.getArgument(0) = getExpr() and
+      not exists(
+        SettingRevocationCheckerConfig config, DataFlow2::PathNode source, DataFlow2::PathNode sink
+      |
+        config.hasFlowPath(source, sink) and
+        sink.getNode().(SettingRevocationCheckerSink).getVariable() =
+          setRevocationEnabledCall.getQualifier().(VarAccess).getVariable()
+      )
+    )
+  }
+}
+
+/**
+ * A dataflow config for tracking a custom revocation checker.
+ */
+class SettingRevocationCheckerConfig extends DataFlow2::Configuration {
+  SettingRevocationCheckerConfig() {
+    this = "DisabledRevocationChecking::SettingRevocationCheckerConfig"
+  }
+
+  override predicate isSource(DataFlow::Node source) {
+    source instanceof GetRevocationCheckerSource
+  }
+
+  override predicate isSink(DataFlow::Node sink) { sink instanceof SettingRevocationCheckerSink }
+
+  override predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2) {
+    createSingletonListStep(node1, node2) or
+    convertArrayToListStep(node1, node2) or
+    addToListStep(node1, node2)
+  }
+
+  override int fieldFlowBranchLimit() { result = 0 }
+}
+
+/**
+ * A source that creates a custom revocation checker,
+ * i.e. `CertPathValidator.getRevocationChecker()`.
+ */
+class GetRevocationCheckerSource extends DataFlow::ExprNode {
+  GetRevocationCheckerSource() {
+    exists(MethodAccess ma | ma.getMethod() instanceof GetRevocationCheckerMethod |
+      ma = asExpr() or ma.getQualifier() = asExpr()
+    )
+  }
+}
+
+/**
+ * A sink that sets a custom revocation checker in `PKIXParameters`,
+ * i.e. `PKIXParameters.addCertPathChecker()` or `PKIXParameters.setCertPathCheckers()`.
+ */
+class SettingRevocationCheckerSink extends DataFlow::ExprNode {
+  MethodAccess ma;
+
+  SettingRevocationCheckerSink() {
+    (
+      ma.getMethod() instanceof AddCertPathCheckerMethod or
+      ma.getMethod() instanceof SetCertPathCheckersMethod
+    ) and
+    ma.getArgument(0) = asExpr()
+  }
+
+  Variable getVariable() { result = ma.getQualifier().(VarAccess).getVariable() }
+}
+
+/**
+ * Holds if `node1` to `node2` is a dataflow step that creates a singleton list,
+ * i.e. `Collections.singletonList(element)`.
+ */
+predicate createSingletonListStep(DataFlow::Node node1, DataFlow::Node node2) {
+  exists(StaticMethodAccess ma, Method m | m = ma.getMethod() |
+    m.getDeclaringType() instanceof Collections and
+    m.hasName("singletonList") and
+    ma.getArgument(0) = node1.asExpr() and
+    (ma = node2.asExpr() or ma.getQualifier() = node2.asExpr())
+  )
+}
+
+/**
+ * Holds if `node1` to `node2` is a dataflow step that converts an array to a list,class
+ * 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())
+  )
+}
+
+/**
+ * Holds if `node1` to `node2` is a dataflow step that adds an element to a list,
+ * i.e. `list.add(element)` or `list.addAll(elements)`.
+ */
+predicate addToListStep(DataFlow::Node node1, DataFlow::Node node2) {
+  exists(MethodAccess ma, Method m | m = ma.getMethod() |
+    m.getDeclaringType() instanceof List and
+    (
+      m.hasName("add") or
+      m.hasName("addAll")
+    ) and
+    ma.getArgument(0) = node1.asExpr() and
+    (ma = node2.asExpr() or ma.getQualifier() = node2.asExpr())
+  )
+}
+
+class SetRevocationEnabledMethod extends Method {
+  SetRevocationEnabledMethod() {
+    getDeclaringType() instanceof PKIXParameters and
+    hasName("setRevocationEnabled")
+  }
+}
+
+class GetRevocationCheckerMethod extends Method {
+  GetRevocationCheckerMethod() {
+    getDeclaringType() instanceof CertPathValidator and
+    hasName("getRevocationChecker")
+  }
+}
+
+class AddCertPathCheckerMethod extends Method {
+  AddCertPathCheckerMethod() {
+    getDeclaringType() instanceof PKIXParameters and
+    hasName("addCertPathChecker")
+  }
+}
+
+class SetCertPathCheckersMethod extends Method {
+  SetCertPathCheckersMethod() {
+    getDeclaringType() instanceof PKIXParameters and
+    hasName("setCertPathCheckers")
+  }
+}
+
+class PKIXParameters extends RefType {
+  PKIXParameters() { hasQualifiedName("java.security.cert", "PKIXParameters") }
+}
+
+class CertPathValidator extends RefType {
+  CertPathValidator() { hasQualifiedName("java.security.cert", "CertPathValidator") }
+}
+
+class Collections extends RefType {
+  Collections() { hasQualifiedName("java.util", "Collections") }
+}
+
+class Arrays extends RefType {
+  Arrays() { hasQualifiedName("java.util", "Arrays") }
+}
+
+class List extends ParameterizedInterface {
+  List() { getGenericType().hasQualifiedName("java.util", "List") }
+}

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

@@ -0,0 +1,17 @@
+edges
+| DisabledRevocationChecking.java:17:5:17:8 | this <.field> [post update] [flag] : Boolean | DisabledRevocationChecking.java:21:5:21:31 | this <.method> [post update] [flag] : Boolean |
+| DisabledRevocationChecking.java:17:12:17:16 | false : Boolean | DisabledRevocationChecking.java:17:5:17:8 | this <.field> [post update] [flag] : Boolean |
+| DisabledRevocationChecking.java:21:5:21:31 | this <.method> [post update] [flag] : Boolean | DisabledRevocationChecking.java:22:5:22:31 | this <.method> [flag] : Boolean |
+| DisabledRevocationChecking.java:22:5:22:31 | this <.method> [flag] : Boolean | DisabledRevocationChecking.java:25:15:25:22 | parameter this [flag] : Boolean |
+| DisabledRevocationChecking.java:25:15:25:22 | parameter this [flag] : Boolean | DisabledRevocationChecking.java:28:33:28:36 | this <.field> [flag] : Boolean |
+| DisabledRevocationChecking.java:28:33:28:36 | this <.field> [flag] : Boolean | DisabledRevocationChecking.java:28:33:28:36 | flag |
+nodes
+| DisabledRevocationChecking.java:17:5:17:8 | this <.field> [post update] [flag] : Boolean | semmle.label | this <.field> [post update] [flag] : Boolean |
+| DisabledRevocationChecking.java:17:12:17:16 | false : Boolean | semmle.label | false : Boolean |
+| DisabledRevocationChecking.java:21:5:21:31 | this <.method> [post update] [flag] : Boolean | semmle.label | this <.method> [post update] [flag] : Boolean |
+| DisabledRevocationChecking.java:22:5:22:31 | this <.method> [flag] : Boolean | semmle.label | this <.method> [flag] : Boolean |
+| DisabledRevocationChecking.java:25:15:25:22 | parameter this [flag] : Boolean | semmle.label | parameter this [flag] : Boolean |
+| DisabledRevocationChecking.java:28:33:28:36 | flag | semmle.label | flag |
+| DisabledRevocationChecking.java:28:33:28:36 | this <.field> [flag] : Boolean | semmle.label | this <.field> [flag] : Boolean |
+#select
+| DisabledRevocationChecking.java:17:12:17:16 | false | DisabledRevocationChecking.java:17:12:17:16 | false : Boolean | DisabledRevocationChecking.java:28:33:28:36 | flag | Revocation checking is disabled $@. | DisabledRevocationChecking.java:17:12:17:16 | false | here |

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

@@ -0,0 +1,70 @@
+import java.security.KeyStore;
+import java.security.cert.CertPath;
+import java.security.cert.CertPathValidator;
+import java.security.cert.PKIXCertPathChecker;
+import java.security.cert.PKIXParameters;
+import java.security.cert.PKIXRevocationChecker;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.List;
+
+public class DisabledRevocationChecking {
+
+  private boolean flag = true;
+
+  public void disableRevocationChecking() {
+    flag = false;
+  }
+
+  public void testDisabledRevocationChecking(KeyStore cacerts, CertPath certPath) throws Exception {
+    disableRevocationChecking();
+    validate(cacerts, certPath);
+  }
+
+  public void validate(KeyStore cacerts, CertPath certPath) throws Exception {
+    CertPathValidator validator = CertPathValidator.getInstance("PKIX");
+    PKIXParameters params = new PKIXParameters(cacerts);
+    params.setRevocationEnabled(flag);
+    validator.validate(certPath, params);
+  }
+
+  public void testSettingRevocationCheckerWithCollectionsSingletonList(KeyStore cacerts, CertPath certPath) throws Exception {
+    CertPathValidator validator = CertPathValidator.getInstance("PKIX");
+    PKIXParameters params = new PKIXParameters(cacerts);
+    params.setRevocationEnabled(false);
+    PKIXRevocationChecker checker = (PKIXRevocationChecker) validator.getRevocationChecker();
+    params.setCertPathCheckers(Collections.singletonList(checker));
+    validator.validate(certPath, params);
+  }
+
+  public void testSettingRevocationCheckerWithArraysAsList(KeyStore cacerts, CertPath certPath) throws Exception {
+    CertPathValidator validator = CertPathValidator.getInstance("PKIX");
+    PKIXParameters params = new PKIXParameters(cacerts);
+    params.setRevocationEnabled(false);
+    PKIXRevocationChecker checker = (PKIXRevocationChecker) validator.getRevocationChecker();
+    params.setCertPathCheckers(Arrays.asList(checker));
+    validator.validate(certPath, params);
+  }
+
+  public void testSettingRevocationCheckerWithList(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 = new ArrayList<>();
+    checkers.add(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);
+    params.setRevocationEnabled(false);
+    PKIXRevocationChecker checker = (PKIXRevocationChecker) validator.getRevocationChecker();
+    params.addCertPathChecker(checker);
+    validator.validate(certPath, params);
+  }
+
+}

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

@@ -0,0 +1 @@
+experimental/Security/CWE/CWE-299/DisabledRevocationChecking.ql