From 657e1e62ca07932f7ae50d06ee3350af9ed0d5f4 Mon Sep 17 00:00:00 2001 From: Jami Cogswell Date: Thu, 29 Sep 2022 15:25:08 -0400 Subject: [PATCH] start refactoring query logic into lib file --- .../semmle/code/java/security/Encryption.qll | 4 +- .../security/InsufficientKeySizeQuery.qll | 145 ++++++++++++++++++ .../CWE/CWE-326/InsufficientKeySize.ql | 136 +--------------- .../CWE-326/InsufficientKeySize.qlref | 2 +- 4 files changed, 151 insertions(+), 136 deletions(-) create mode 100644 java/ql/lib/semmle/code/java/security/InsufficientKeySizeQuery.qll diff --git a/java/ql/lib/semmle/code/java/security/Encryption.qll b/java/ql/lib/semmle/code/java/security/Encryption.qll index 39e3f2d2110..9659fb92843 100644 --- a/java/ql/lib/semmle/code/java/security/Encryption.qll +++ b/java/ql/lib/semmle/code/java/security/Encryption.qll @@ -249,7 +249,9 @@ string getASecureAlgorithmName() { result = [ "RSA", "SHA256", "SHA512", "CCM", "GCM", "AES(?![^a-zA-Z](ECB|CBC/PKCS[57]Padding))", - "Blowfish", "ECIES" + "Blowfish", "ECIES" // ! Blowfish not actually secure based on https://rules.sonarsource.com/java/type/Vulnerability/RSPEC-4426 ?? + // ! hmm, other sources imply that it is secure... + // ! also no DH here, etc.? ] } diff --git a/java/ql/lib/semmle/code/java/security/InsufficientKeySizeQuery.qll b/java/ql/lib/semmle/code/java/security/InsufficientKeySizeQuery.qll new file mode 100644 index 00000000000..5e883eca216 --- /dev/null +++ b/java/ql/lib/semmle/code/java/security/InsufficientKeySizeQuery.qll @@ -0,0 +1,145 @@ +import semmle.code.java.security.Encryption +import semmle.code.java.dataflow.TaintTracking + +/** The Java class `java.security.spec.ECGenParameterSpec`. */ +class ECGenParameterSpec extends RefType { + ECGenParameterSpec() { this.hasQualifiedName("java.security.spec", "ECGenParameterSpec") } +} + +/** The `init` method declared in `javax.crypto.KeyGenerator`. */ +class KeyGeneratorInitMethod extends Method { + KeyGeneratorInitMethod() { + this.getDeclaringType() instanceof KeyGenerator and + this.hasName("init") + } +} + +/** The `initialize` method declared in `java.security.KeyPairGenerator`. */ +class KeyPairGeneratorInitMethod extends Method { + KeyPairGeneratorInitMethod() { + this.getDeclaringType() instanceof KeyPairGenerator and + this.hasName("initialize") + } +} + +/** Returns the key size in the EC algorithm string */ +bindingset[algorithm] +int getECKeySize(string algorithm) { + algorithm.matches("sec%") and // specification such as "secp256r1" + result = algorithm.regexpCapture("sec[p|t](\\d+)[a-zA-Z].*", 1).toInt() + or + algorithm.matches("X9.62%") and //specification such as "X9.62 prime192v2" + result = algorithm.regexpCapture("X9\\.62 .*[a-zA-Z](\\d+)[a-zA-Z].*", 1).toInt() + or + (algorithm.matches("prime%") or algorithm.matches("c2tnb%")) and //specification such as "prime192v2" + result = algorithm.regexpCapture(".*[a-zA-Z](\\d+)[a-zA-Z].*", 1).toInt() +} + +/** Taint configuration tracking flow from a key generator to a `init` method call. */ +class KeyGeneratorInitConfiguration extends TaintTracking::Configuration { + KeyGeneratorInitConfiguration() { this = "KeyGeneratorInitConfiguration" } + + override predicate isSource(DataFlow::Node source) { + source.asExpr() instanceof JavaxCryptoKeyGenerator + } + + override predicate isSink(DataFlow::Node sink) { + exists(MethodAccess ma | + ma.getMethod() instanceof KeyGeneratorInitMethod and + sink.asExpr() = ma.getQualifier() + ) + } +} + +/** Taint configuration tracking flow from a keypair generator to a `initialize` method call. */ +class KeyPairGeneratorInitConfiguration extends TaintTracking::Configuration { + KeyPairGeneratorInitConfiguration() { this = "KeyPairGeneratorInitConfiguration" } + + override predicate isSource(DataFlow::Node source) { + source.asExpr() instanceof JavaSecurityKeyPairGenerator + } + + override predicate isSink(DataFlow::Node sink) { + exists(MethodAccess ma | + ma.getMethod() instanceof KeyPairGeneratorInitMethod and + sink.asExpr() = ma.getQualifier() + ) + } +} + +/** Holds if a symmetric `KeyGenerator` implementing encryption algorithm `type` and initialized by `ma` uses an insufficient key size. `msg` provides a human-readable description of the problem. */ +bindingset[type] +predicate hasShortSymmetricKey(MethodAccess ma, string msg, string type) { + ma.getMethod() instanceof KeyGeneratorInitMethod and + exists( + JavaxCryptoKeyGenerator jcg, KeyGeneratorInitConfiguration cc, DataFlow::PathNode source, + DataFlow::PathNode dest + | + jcg.getAlgoSpec().(StringLiteral).getValue() = type and + source.getNode().asExpr() = jcg and + dest.getNode().asExpr() = ma.getQualifier() and + cc.hasFlowPath(source, dest) + ) and + ma.getArgument(0).(IntegerLiteral).getIntValue() < 128 and + msg = "Key size should be at least 128 bits for " + type + " encryption." +} + +/** Holds if an AES `KeyGenerator` initialized by `ma` uses an insufficient key size. `msg` provides a human-readable description of the problem. */ +predicate hasShortAESKey(MethodAccess ma, string msg) { hasShortSymmetricKey(ma, msg, "AES") } + +/** Holds if an asymmetric `KeyPairGenerator` implementing encryption algorithm `type` and initialized by `ma` uses an insufficient key size. `msg` provides a human-readable description of the problem. */ +bindingset[type] +predicate hasShortAsymmetricKeyPair(MethodAccess ma, string msg, string type) { + ma.getMethod() instanceof KeyPairGeneratorInitMethod and + exists( + JavaSecurityKeyPairGenerator jpg, KeyPairGeneratorInitConfiguration kc, + DataFlow::PathNode source, DataFlow::PathNode dest + | + jpg.getAlgoSpec().(StringLiteral).getValue().toUpperCase() = type and + source.getNode().asExpr() = jpg and + dest.getNode().asExpr() = ma.getQualifier() and + kc.hasFlowPath(source, dest) + ) and + ma.getArgument(0).(IntegerLiteral).getIntValue() < 2048 and + msg = "Key size should be at least 2048 bits for " + type + " encryption." +} + +/** Holds if a DSA `KeyPairGenerator` initialized by `ma` uses an insufficient key size. `msg` provides a human-readable description of the problem. */ +predicate hasShortDsaKeyPair(MethodAccess ma, string msg) { + hasShortAsymmetricKeyPair(ma, msg, "DSA") or hasShortAsymmetricKeyPair(ma, msg, "DH") +} + +/** Holds if a RSA `KeyPairGenerator` initialized by `ma` uses an insufficient key size. `msg` provides a human-readable description of the problem. */ +predicate hasShortRsaKeyPair(MethodAccess ma, string msg) { + hasShortAsymmetricKeyPair(ma, msg, "RSA") +} + +/** Holds if an EC `KeyPairGenerator` initialized by `ma` uses an insufficient key size. `msg` provides a human-readable description of the problem. */ +predicate hasShortECKeyPair(MethodAccess ma, string msg) { + ma.getMethod() instanceof KeyPairGeneratorInitMethod and + exists( + JavaSecurityKeyPairGenerator jpg, KeyPairGeneratorInitConfiguration kc, + DataFlow::PathNode source, DataFlow::PathNode dest, ClassInstanceExpr cie + | + jpg.getAlgoSpec().(StringLiteral).getValue().matches("EC%") and // ECC variants such as ECDH and ECDSA + source.getNode().asExpr() = jpg and + dest.getNode().asExpr() = ma.getQualifier() and + kc.hasFlowPath(source, dest) and + DataFlow::localExprFlow(cie, ma.getArgument(0)) and + ma.getArgument(0).getType() instanceof ECGenParameterSpec and + getECKeySize(cie.getArgument(0).(StringLiteral).getValue()) < 256 + ) and + msg = "Key size should be at least 256 bits for EC encryption." +} +// ! refactor to something like the below, +// ! need to adjust select clause then... +// ! see C# and C++ queries for ideas +// class EncryptionAlgorithm extends +// predicate hasInsufficientKeySize() { +// exists(Expr e, string msg | +// hasShortAESKey(e, msg) or +// hasShortDsaKeyPair(e, msg) or +// hasShortRsaKeyPair(e, msg) or +// hasShortECKeyPair(e, msg) +// ) +// } diff --git a/java/ql/src/Security/CWE/CWE-326/InsufficientKeySize.ql b/java/ql/src/Security/CWE/CWE-326/InsufficientKeySize.ql index 5607ec1bb87..baccf3b42c3 100644 --- a/java/ql/src/Security/CWE/CWE-326/InsufficientKeySize.ql +++ b/java/ql/src/Security/CWE/CWE-326/InsufficientKeySize.ql @@ -2,7 +2,7 @@ * @name Use of a cryptographic algorithm with insufficient key size * @description Using cryptographic algorithms with too small a key size can * allow an attacker to compromise security. - * @kind path-problem + * @kind problem * @problem.severity error * @precision high * @id java/insufficient-key-size @@ -11,139 +11,7 @@ */ import java -import semmle.code.java.security.Encryption -import semmle.code.java.dataflow.TaintTracking - -/** The Java class `java.security.spec.ECGenParameterSpec`. */ -class ECGenParameterSpec extends RefType { - ECGenParameterSpec() { this.hasQualifiedName("java.security.spec", "ECGenParameterSpec") } -} - -/** The `init` method declared in `javax.crypto.KeyGenerator`. */ -class KeyGeneratorInitMethod extends Method { - KeyGeneratorInitMethod() { - this.getDeclaringType() instanceof KeyGenerator and - this.hasName("init") - } -} - -/** The `initialize` method declared in `java.security.KeyPairGenerator`. */ -class KeyPairGeneratorInitMethod extends Method { - KeyPairGeneratorInitMethod() { - this.getDeclaringType() instanceof KeyPairGenerator and - this.hasName("initialize") - } -} - -/** Returns the key size in the EC algorithm string */ -bindingset[algorithm] -int getECKeySize(string algorithm) { - algorithm.matches("sec%") and // specification such as "secp256r1" - result = algorithm.regexpCapture("sec[p|t](\\d+)[a-zA-Z].*", 1).toInt() - or - algorithm.matches("X9.62%") and //specification such as "X9.62 prime192v2" - result = algorithm.regexpCapture("X9\\.62 .*[a-zA-Z](\\d+)[a-zA-Z].*", 1).toInt() - or - (algorithm.matches("prime%") or algorithm.matches("c2tnb%")) and //specification such as "prime192v2" - result = algorithm.regexpCapture(".*[a-zA-Z](\\d+)[a-zA-Z].*", 1).toInt() -} - -/** Taint configuration tracking flow from a key generator to a `init` method call. */ -class KeyGeneratorInitConfiguration extends TaintTracking::Configuration { - KeyGeneratorInitConfiguration() { this = "KeyGeneratorInitConfiguration" } - - override predicate isSource(DataFlow::Node source) { - source.asExpr() instanceof JavaxCryptoKeyGenerator - } - - override predicate isSink(DataFlow::Node sink) { - exists(MethodAccess ma | - ma.getMethod() instanceof KeyGeneratorInitMethod and - sink.asExpr() = ma.getQualifier() - ) - } -} - -/** Taint configuration tracking flow from a keypair generator to a `initialize` method call. */ -class KeyPairGeneratorInitConfiguration extends TaintTracking::Configuration { - KeyPairGeneratorInitConfiguration() { this = "KeyPairGeneratorInitConfiguration" } - - override predicate isSource(DataFlow::Node source) { - source.asExpr() instanceof JavaSecurityKeyPairGenerator - } - - override predicate isSink(DataFlow::Node sink) { - exists(MethodAccess ma | - ma.getMethod() instanceof KeyPairGeneratorInitMethod and - sink.asExpr() = ma.getQualifier() - ) - } -} - -/** Holds if a symmetric `KeyGenerator` implementing encryption algorithm `type` and initialized by `ma` uses an insufficient key size. `msg` provides a human-readable description of the problem. */ -bindingset[type] -predicate hasShortSymmetricKey(MethodAccess ma, string msg, string type) { - ma.getMethod() instanceof KeyGeneratorInitMethod and - exists( - JavaxCryptoKeyGenerator jcg, KeyGeneratorInitConfiguration cc, DataFlow::PathNode source, - DataFlow::PathNode dest - | - jcg.getAlgoSpec().(StringLiteral).getValue() = type and - source.getNode().asExpr() = jcg and - dest.getNode().asExpr() = ma.getQualifier() and - cc.hasFlowPath(source, dest) - ) and - ma.getArgument(0).(IntegerLiteral).getIntValue() < 128 and - msg = "Key size should be at least 128 bits for " + type + " encryption." -} - -/** Holds if an AES `KeyGenerator` initialized by `ma` uses an insufficient key size. `msg` provides a human-readable description of the problem. */ -predicate hasShortAESKey(MethodAccess ma, string msg) { hasShortSymmetricKey(ma, msg, "AES") } - -/** Holds if an asymmetric `KeyPairGenerator` implementing encryption algorithm `type` and initialized by `ma` uses an insufficient key size. `msg` provides a human-readable description of the problem. */ -bindingset[type] -predicate hasShortAsymmetricKeyPair(MethodAccess ma, string msg, string type) { - ma.getMethod() instanceof KeyPairGeneratorInitMethod and - exists( - JavaSecurityKeyPairGenerator jpg, KeyPairGeneratorInitConfiguration kc, - DataFlow::PathNode source, DataFlow::PathNode dest - | - jpg.getAlgoSpec().(StringLiteral).getValue().toUpperCase() = type and - source.getNode().asExpr() = jpg and - dest.getNode().asExpr() = ma.getQualifier() and - kc.hasFlowPath(source, dest) - ) and - ma.getArgument(0).(IntegerLiteral).getIntValue() < 2048 and - msg = "Key size should be at least 2048 bits for " + type + " encryption." -} - -/** Holds if a DSA `KeyPairGenerator` initialized by `ma` uses an insufficient key size. `msg` provides a human-readable description of the problem. */ -predicate hasShortDsaKeyPair(MethodAccess ma, string msg) { - hasShortAsymmetricKeyPair(ma, msg, "DSA") or hasShortAsymmetricKeyPair(ma, msg, "DH") -} - -/** Holds if a RSA `KeyPairGenerator` initialized by `ma` uses an insufficient key size. `msg` provides a human-readable description of the problem. */ -predicate hasShortRsaKeyPair(MethodAccess ma, string msg) { - hasShortAsymmetricKeyPair(ma, msg, "RSA") -} - -/** Holds if an EC `KeyPairGenerator` initialized by `ma` uses an insufficient key size. `msg` provides a human-readable description of the problem. */ -predicate hasShortECKeyPair(MethodAccess ma, string msg) { - ma.getMethod() instanceof KeyPairGeneratorInitMethod and - exists( - JavaSecurityKeyPairGenerator jpg, KeyPairGeneratorInitConfiguration kc, - DataFlow::PathNode source, DataFlow::PathNode dest, ClassInstanceExpr cie - | - jpg.getAlgoSpec().(StringLiteral).getValue().matches("EC%") and // ECC variants such as ECDH and ECDSA - source.getNode().asExpr() = jpg and - dest.getNode().asExpr() = ma.getQualifier() and - kc.hasFlowPath(source, dest) and - DataFlow::localExprFlow(cie, ma.getArgument(0)) and - ma.getArgument(0).getType() instanceof ECGenParameterSpec and - getECKeySize(cie.getArgument(0).(StringLiteral).getValue()) < 256 - ) and - msg = "Key size should be at least 256 bits for EC encryption." -} +import semmle.code.java.security.InsufficientKeySizeQuery from Expr e, string msg where diff --git a/java/ql/test/query-tests/security/CWE-326/InsufficientKeySize.qlref b/java/ql/test/query-tests/security/CWE-326/InsufficientKeySize.qlref index 7831abd3b6b..040c083a1a1 100644 --- a/java/ql/test/query-tests/security/CWE-326/InsufficientKeySize.qlref +++ b/java/ql/test/query-tests/security/CWE-326/InsufficientKeySize.qlref @@ -1 +1 @@ -experimental/Security/CWE/CWE-326/InsufficientKeySize.ql +Security/CWE/CWE-326/InsufficientKeySize.ql