From a2fa03e4f5d2f1d2599d95092a1bcbe568b93fac Mon Sep 17 00:00:00 2001 From: Artem Smotrakov Date: Mon, 22 Jun 2020 18:16:07 +0300 Subject: [PATCH] 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 --- .../CWE-299/DisabledRevocationChecking.qhelp | 2 +- .../CWE/CWE-299/DisabledRevocationChecking.ql | 2 +- .../CWE/CWE-299/RevocationCheckingLib.qll | 29 +++++++++++++++---- .../CWE-299/DisabledRevocationChecking.java | 12 +++++++- 4 files changed, 36 insertions(+), 9 deletions(-) diff --git a/java/ql/src/experimental/Security/CWE/CWE-299/DisabledRevocationChecking.qhelp b/java/ql/src/experimental/Security/CWE/CWE-299/DisabledRevocationChecking.qhelp index 2ec91f04f9c..c76b56b531a 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-299/DisabledRevocationChecking.qhelp +++ b/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.
  • Java SE API Specification: - CertPathValidator + CertPathValidator
    • diff --git a/java/ql/src/experimental/Security/CWE/CWE-299/DisabledRevocationChecking.ql b/java/ql/src/experimental/Security/CWE/CWE-299/DisabledRevocationChecking.ql index d3ec455c6dd..c38cc39b126 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-299/DisabledRevocationChecking.ql +++ b/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 diff --git a/java/ql/src/experimental/Security/CWE/CWE-299/RevocationCheckingLib.qll b/java/ql/src/experimental/Security/CWE/CWE-299/RevocationCheckingLib.qll index 178a00e320d..5fe8c7f91dd 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-299/RevocationCheckingLib.qll +++ b/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,12 +100,12 @@ 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) { @@ -112,7 +113,7 @@ predicate convertArrayToListStep(DataFlow::Node node1, DataFlow::Node node2) { 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") + } } diff --git a/java/ql/test/experimental/Security/CWE/CWE-299/DisabledRevocationChecking.java b/java/ql/test/experimental/Security/CWE/CWE-299/DisabledRevocationChecking.java index 29a72edd60d..41b470b62d0 100644 --- a/java/ql/test/experimental/Security/CWE/CWE-299/DisabledRevocationChecking.java +++ b/java/ql/test/experimental/Security/CWE/CWE-299/DisabledRevocationChecking.java @@ -47,7 +47,7 @@ public class DisabledRevocationChecking { 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 class DisabledRevocationChecking { 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 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);