Merge pull request #3436 from artem-smotrakov/revocation-checking

Java: Added a query for disabled certificate revocation checking

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/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 certificates 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,60 @@
+import java
+import semmle.code.java.dataflow.FlowSources
+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(MethodAccess ma, Method m | m = ma.getMethod() |
+        (m instanceof AddCertPathCheckerMethod or m instanceof SetCertPathCheckersMethod) and
+        ma.getQualifier().(VarAccess).getVariable() =
+          setRevocationEnabledCall.getQualifier().(VarAccess).getVariable()
+      )
+    )
+  }
+}
+
+class SetRevocationEnabledMethod extends Method {
+  SetRevocationEnabledMethod() {
+    getDeclaringType() instanceof PKIXParameters and
+    hasName("setRevocationEnabled")
+  }
+}
+
+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") }
+}

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,80 @@
+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 testSettingRevocationCheckerWithAddingToArrayList(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 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);
+    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