Skip to content

Commit d297ce2

Browse files
aschackmullaschackmull
authored
Merge pull request github#3436 from artem-smotrakov/revocation-checking
Java: Added a query for disabled certificate revocation checking
2 parents b53b905 + f5f30ce commit d297ce2

9 files changed

+262
-0
lines changed

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

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
public void validate(KeyStore cacerts, CertPath certPath) throws Exception {
2+
CertPathValidator validator = CertPathValidator.getInstance("PKIX");
3+
PKIXParameters params = new PKIXParameters(cacerts);
4+
params.setRevocationEnabled(false);
5+
PKIXRevocationChecker checker = (PKIXRevocationChecker) validator.getRevocationChecker();
6+
checker.setOcspResponder(OCSP_RESPONDER_URL);
7+
checker.setOcspResponderCert(OCSP_RESPONDER_CERT);
8+
params.addCertPathChecker(checker);
9+
validator.validate(certPath, params);
10+
}

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

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
public void validate(KeyStore cacerts, CertPath chain) throws Exception {
2+
CertPathValidator validator = CertPathValidator.getInstance("PKIX");
3+
PKIXParameters params = new PKIXParameters(cacerts);
4+
validator.validate(chain, params);
5+
}

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

Lines changed: 63 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,63 @@
1+
<!DOCTYPE qhelp PUBLIC "-//Semmle//qhelp//EN" "qhelp.dtd">
2+
<qhelp>
3+
4+
<overview>
5+
<p>Validating a certificate chain includes multiple steps. One of them is checking whether or not
6+
certificates in the chain have been revoked. A certificate may be revoked due to multiple reasons.
7+
One of the reasons why the certificate authority (CA) may revoke a certificate is that its private key
8+
has been compromised. For example, the private key might have been stolen by an adversary.
9+
In this case, the adversary may be able to impersonate the owner of the private key.
10+
Therefore, trusting a revoked certificate may be dangerous.</p>
11+
12+
<p>The Java Certification Path API provides a revocation checking mechanism
13+
that supports both CRL and OCSP.
14+
Revocation checking happens while building and validating certificate chains.
15+
If at least one of the certificates is revoked, then an exception is thrown.
16+
This mechanism is enabled by default. However, it may be disabled
17+
by passing <code>false</code> to the <code>PKIXParameters.setRevocationEnabled()</code> method.
18+
If an application doesn't set a custom <code>PKIXRevocationChecker</code>
19+
via <code>PKIXParameters.addCertPathChecker()</code>
20+
or <code>PKIXParameters.setCertPathCheckers()</code> methods,
21+
then revocation checking is not going to happen.</p>
22+
23+
</overview>
24+
<recommendation>
25+
26+
<p>An application should not disable the default revocationg checking mechanism
27+
unless it provides a custom revocation checker.</p>
28+
29+
</recommendation>
30+
<example>
31+
32+
<p>The following example turns off revocation checking for validating a certificate chain.
33+
That should be avoided.</p>
34+
35+
<sample src="NoRevocationChecking.java" />
36+
37+
<p>The next example uses the default revocation checking mechanism.</p>
38+
39+
<sample src="DefaultRevocationChecking.java" />
40+
41+
<p>The third example turns off the default revocation mechanism. However, it registers another
42+
revocation checker that uses OCSP to obtain revocation status of certificates.</p>
43+
44+
<sample src="CustomRevocationChecking.java" />
45+
46+
</example>
47+
<references>
48+
49+
<li>
50+
Wikipedia:
51+
<a href="https://en.wikipedia.org/wiki/Public_key_certificate">Public key certificate</a>
52+
</li>
53+
<li>
54+
Java SE Documentation:
55+
<a href="https://docs.oracle.com/javase/8/docs/technotes/guides/security/certpath/CertPathProgGuide.html">Java PKI Programmer's Guide</a>
56+
</li>
57+
<li>
58+
Java SE API Specification:
59+
<a href="https://docs.oracle.com/javase/8/docs/api/java/security/cert/CertPathValidator.html">CertPathValidator</a>
60+
</li>
61+
62+
</references>
63+
</qhelp>

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

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
/**
2+
* @name Disabled ceritificate revocation checking
3+
* @description Using revoked certificates is dangerous.
4+
* Therefore, revocation status of certificates in a chain should be checked.
5+
* @kind path-problem
6+
* @problem.severity error
7+
* @precision high
8+
* @id java/disabled-certificate-revocation-checking
9+
* @tags security
10+
* external/cwe/cwe-299
11+
*/
12+
13+
import java
14+
import RevocationCheckingLib
15+
import DataFlow::PathGraph
16+
17+
from DataFlow::PathNode source, DataFlow::PathNode sink, DisabledRevocationCheckingConfig config
18+
where config.hasFlowPath(source, sink)
19+
select source.getNode(), source, sink, "Revocation checking is disabled $@.", source.getNode(),
20+
"here"

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

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
public void validateUnsafe(KeyStore cacerts, CertPath chain) throws Exception {
2+
CertPathValidator validator = CertPathValidator.getInstance("PKIX");
3+
PKIXParameters params = new PKIXParameters(cacerts);
4+
params.setRevocationEnabled(false);
5+
validator.validate(chain, params);
6+
}

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

Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,60 @@
1+
import java
2+
import semmle.code.java.dataflow.FlowSources
3+
import DataFlow
4+
5+
/**
6+
* A taint-tracking configuration for disabling revocation checking.
7+
*/
8+
class DisabledRevocationCheckingConfig extends TaintTracking::Configuration {
9+
DisabledRevocationCheckingConfig() { this = "DisabledRevocationCheckingConfig" }
10+
11+
override predicate isSource(DataFlow::Node source) {
12+
exists(BooleanLiteral b | b.getBooleanValue() = false | source.asExpr() = b)
13+
}
14+
15+
override predicate isSink(DataFlow::Node sink) { sink instanceof SetRevocationEnabledSink }
16+
}
17+
18+
/**
19+
* A sink that disables revocation checking,
20+
* i.e. calling `PKIXParameters.setRevocationEnabled(false)`
21+
* without setting a custom revocation checker in `PKIXParameters`.
22+
*/
23+
class SetRevocationEnabledSink extends DataFlow::ExprNode {
24+
SetRevocationEnabledSink() {
25+
exists(MethodAccess setRevocationEnabledCall |
26+
setRevocationEnabledCall.getMethod() instanceof SetRevocationEnabledMethod and
27+
setRevocationEnabledCall.getArgument(0) = getExpr() and
28+
not exists(MethodAccess ma, Method m | m = ma.getMethod() |
29+
(m instanceof AddCertPathCheckerMethod or m instanceof SetCertPathCheckersMethod) and
30+
ma.getQualifier().(VarAccess).getVariable() =
31+
setRevocationEnabledCall.getQualifier().(VarAccess).getVariable()
32+
)
33+
)
34+
}
35+
}
36+
37+
class SetRevocationEnabledMethod extends Method {
38+
SetRevocationEnabledMethod() {
39+
getDeclaringType() instanceof PKIXParameters and
40+
hasName("setRevocationEnabled")
41+
}
42+
}
43+
44+
class AddCertPathCheckerMethod extends Method {
45+
AddCertPathCheckerMethod() {
46+
getDeclaringType() instanceof PKIXParameters and
47+
hasName("addCertPathChecker")
48+
}
49+
}
50+
51+
class SetCertPathCheckersMethod extends Method {
52+
SetCertPathCheckersMethod() {
53+
getDeclaringType() instanceof PKIXParameters and
54+
hasName("setCertPathCheckers")
55+
}
56+
}
57+
58+
class PKIXParameters extends RefType {
59+
PKIXParameters() { hasQualifiedName("java.security.cert", "PKIXParameters") }
60+
}

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

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
edges
2+
| DisabledRevocationChecking.java:17:5:17:8 | this <.field> [post update] [flag] : Boolean | DisabledRevocationChecking.java:21:5:21:31 | this <.method> [post update] [flag] : Boolean |
3+
| DisabledRevocationChecking.java:17:12:17:16 | false : Boolean | DisabledRevocationChecking.java:17:5:17:8 | this <.field> [post update] [flag] : Boolean |
4+
| DisabledRevocationChecking.java:21:5:21:31 | this <.method> [post update] [flag] : Boolean | DisabledRevocationChecking.java:22:5:22:31 | this <.method> [flag] : Boolean |
5+
| DisabledRevocationChecking.java:22:5:22:31 | this <.method> [flag] : Boolean | DisabledRevocationChecking.java:25:15:25:22 | parameter this [flag] : Boolean |
6+
| DisabledRevocationChecking.java:25:15:25:22 | parameter this [flag] : Boolean | DisabledRevocationChecking.java:28:33:28:36 | this <.field> [flag] : Boolean |
7+
| DisabledRevocationChecking.java:28:33:28:36 | this <.field> [flag] : Boolean | DisabledRevocationChecking.java:28:33:28:36 | flag |
8+
nodes
9+
| DisabledRevocationChecking.java:17:5:17:8 | this <.field> [post update] [flag] : Boolean | semmle.label | this <.field> [post update] [flag] : Boolean |
10+
| DisabledRevocationChecking.java:17:12:17:16 | false : Boolean | semmle.label | false : Boolean |
11+
| DisabledRevocationChecking.java:21:5:21:31 | this <.method> [post update] [flag] : Boolean | semmle.label | this <.method> [post update] [flag] : Boolean |
12+
| DisabledRevocationChecking.java:22:5:22:31 | this <.method> [flag] : Boolean | semmle.label | this <.method> [flag] : Boolean |
13+
| DisabledRevocationChecking.java:25:15:25:22 | parameter this [flag] : Boolean | semmle.label | parameter this [flag] : Boolean |
14+
| DisabledRevocationChecking.java:28:33:28:36 | flag | semmle.label | flag |
15+
| DisabledRevocationChecking.java:28:33:28:36 | this <.field> [flag] : Boolean | semmle.label | this <.field> [flag] : Boolean |
16+
#select
17+
| 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

Lines changed: 80 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,80 @@
1+
import java.security.KeyStore;
2+
import java.security.cert.CertPath;
3+
import java.security.cert.CertPathValidator;
4+
import java.security.cert.PKIXCertPathChecker;
5+
import java.security.cert.PKIXParameters;
6+
import java.security.cert.PKIXRevocationChecker;
7+
import java.util.ArrayList;
8+
import java.util.Arrays;
9+
import java.util.Collections;
10+
import java.util.List;
11+
12+
public class DisabledRevocationChecking {
13+
14+
private boolean flag = true;
15+
16+
public void disableRevocationChecking() {
17+
flag = false;
18+
}
19+
20+
public void testDisabledRevocationChecking(KeyStore cacerts, CertPath certPath) throws Exception {
21+
disableRevocationChecking();
22+
validate(cacerts, certPath);
23+
}
24+
25+
public void validate(KeyStore cacerts, CertPath certPath) throws Exception {
26+
CertPathValidator validator = CertPathValidator.getInstance("PKIX");
27+
PKIXParameters params = new PKIXParameters(cacerts);
28+
params.setRevocationEnabled(flag);
29+
validator.validate(certPath, params);
30+
}
31+
32+
public void testSettingRevocationCheckerWithCollectionsSingletonList(KeyStore cacerts, CertPath certPath) throws Exception {
33+
CertPathValidator validator = CertPathValidator.getInstance("PKIX");
34+
PKIXParameters params = new PKIXParameters(cacerts);
35+
params.setRevocationEnabled(false);
36+
PKIXRevocationChecker checker = (PKIXRevocationChecker) validator.getRevocationChecker();
37+
params.setCertPathCheckers(Collections.singletonList(checker));
38+
validator.validate(certPath, params);
39+
}
40+
41+
public void testSettingRevocationCheckerWithArraysAsList(KeyStore cacerts, CertPath certPath) throws Exception {
42+
CertPathValidator validator = CertPathValidator.getInstance("PKIX");
43+
PKIXParameters params = new PKIXParameters(cacerts);
44+
params.setRevocationEnabled(false);
45+
PKIXRevocationChecker checker = (PKIXRevocationChecker) validator.getRevocationChecker();
46+
params.setCertPathCheckers(Arrays.asList(checker));
47+
validator.validate(certPath, params);
48+
}
49+
50+
public void testSettingRevocationCheckerWithAddingToArrayList(KeyStore cacerts, CertPath certPath) throws Exception {
51+
CertPathValidator validator = CertPathValidator.getInstance("PKIX");
52+
PKIXParameters params = new PKIXParameters(cacerts);
53+
params.setRevocationEnabled(false);
54+
PKIXRevocationChecker checker = (PKIXRevocationChecker) validator.getRevocationChecker();
55+
List<PKIXCertPathChecker> checkers = new ArrayList<>();
56+
checkers.add(checker);
57+
params.setCertPathCheckers(checkers);
58+
validator.validate(certPath, params);
59+
}
60+
61+
public void testSettingRevocationCheckerWithListOf(KeyStore cacerts, CertPath certPath) throws Exception {
62+
CertPathValidator validator = CertPathValidator.getInstance("PKIX");
63+
PKIXParameters params = new PKIXParameters(cacerts);
64+
params.setRevocationEnabled(false);
65+
PKIXRevocationChecker checker = (PKIXRevocationChecker) validator.getRevocationChecker();
66+
List<PKIXCertPathChecker> checkers = List.of(checker);
67+
params.setCertPathCheckers(checkers);
68+
validator.validate(certPath, params);
69+
}
70+
71+
public void testAddingRevocationChecker(KeyStore cacerts, CertPath certPath) throws Exception {
72+
CertPathValidator validator = CertPathValidator.getInstance("PKIX");
73+
PKIXParameters params = new PKIXParameters(cacerts);
74+
params.setRevocationEnabled(false);
75+
PKIXRevocationChecker checker = (PKIXRevocationChecker) validator.getRevocationChecker();
76+
params.addCertPathChecker(checker);
77+
validator.validate(certPath, params);
78+
}
79+
80+
}

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
experimental/Security/CWE/CWE-299/DisabledRevocationChecking.ql

0 commit comments

Comments
 (0)