From c414ee0e254d0a303a6fe187ef7381ea8b60ec57 Mon Sep 17 00:00:00 2001 From: Jami Cogswell Date: Thu, 6 Oct 2022 01:32:10 -0400 Subject: [PATCH] add ECC dataflow config; passes all test cases; still don't have algo name tracking --- .../security/InsufficientKeySizeQuery.qll | 337 +++--------------- .../CWE/CWE-326/InsufficientKeySize.ql | 3 +- .../CWE-326/InsufficientKeySizeTest.java | 20 +- .../CWE-326/InsufficientKeySizeTest.ql | 10 +- 4 files changed, 74 insertions(+), 296 deletions(-) diff --git a/java/ql/lib/semmle/code/java/security/InsufficientKeySizeQuery.qll b/java/ql/lib/semmle/code/java/security/InsufficientKeySizeQuery.qll index 869f3c7cb0f..4cf274f3263 100644 --- a/java/ql/lib/semmle/code/java/security/InsufficientKeySizeQuery.qll +++ b/java/ql/lib/semmle/code/java/security/InsufficientKeySizeQuery.qll @@ -2,7 +2,7 @@ import semmle.code.java.security.Encryption import semmle.code.java.dataflow.TaintTracking import semmle.code.java.dataflow.DataFlow -// ******* DATAFLOW ******************************************************************************* +// ******* DATAFLOW BELOW ************************************************************************* /** * Asymmetric (RSA, DSA, DH) key length data flow tracking configuration. */ @@ -10,16 +10,8 @@ class AsymmetricKeyTrackingConfiguration extends DataFlow::Configuration { AsymmetricKeyTrackingConfiguration() { this = "AsymmetricKeyTrackingConfiguration" } override predicate isSource(DataFlow::Node source) { - exists(IntegerLiteral integer, VarAccess var | - integer.getIntValue() < 2048 and - source.asExpr() = integer - or - // The below only handles cases when variables are used (both locally in a method and between methods) - // The above adds handling for direct use of integers as well - var.getVariable().getInitializer().getUnderlyingExpr() instanceof IntegerLiteral and - var.getVariable().getInitializer().getUnderlyingExpr().toString().toInt() < 2048 and - source.asExpr() = var.getVariable().getInitializer() - ) + source.asExpr() instanceof IntegerLiteral and // ! this works with current test cases, but reconsider IntegerLiteral when variables are used + source.toString().toInt() < 2048 } override predicate isSink(DataFlow::Node sink) { @@ -31,27 +23,22 @@ class AsymmetricKeyTrackingConfiguration extends DataFlow::Configuration { } /** - * Symmetric (AES) key length data flow tracking configuration. + * Asymmetric (RSA, DSA, DH) key length data flow tracking configuration. */ -class SymmetricKeyTrackingConfiguration extends DataFlow::Configuration { - SymmetricKeyTrackingConfiguration() { this = "SymmetricKeyTrackingConfiguration" } +class AsymmetricECCKeyTrackingConfiguration extends DataFlow::Configuration { + AsymmetricECCKeyTrackingConfiguration() { this = "AsymmetricECCKeyTrackingConfiguration" } override predicate isSource(DataFlow::Node source) { - exists(IntegerLiteral integer, VarAccess var | - integer.getIntValue() < 128 and - source.asExpr() = integer - or - // The below only handles cases when variables are used (both locally in a method and between methods) - // The above adds handling for direct use of integers as well - var.getVariable().getInitializer().getUnderlyingExpr() instanceof IntegerLiteral and - var.getVariable().getInitializer().getUnderlyingExpr().toString().toInt() < 128 and - source.asExpr() = var.getVariable().getInitializer() + exists(ClassInstanceExpr ecGenParamSpec | + getECKeySize(ecGenParamSpec.getArgument(0).(StringLiteral).getValue()) < 256 and + source.asExpr() = ecGenParamSpec ) } override predicate isSink(DataFlow::Node sink) { exists(MethodAccess ma | - ma.getMethod() instanceof KeyGeneratorInitMethod and + ma.getMethod() instanceof KeyPairGeneratorInitMethod and + ma.getArgument(0).getType() instanceof ECGenParameterSpec and sink.asExpr() = ma.getArgument(0) ) } @@ -60,11 +47,11 @@ class SymmetricKeyTrackingConfiguration extends DataFlow::Configuration { /** * Symmetric (AES) key length data flow tracking configuration. */ -class SymmetricKeyTrackingConfiguration2 extends DataFlow::Configuration { - SymmetricKeyTrackingConfiguration2() { this = "SymmetricKeyTrackingConfiguration2" } +class SymmetricKeyTrackingConfiguration extends DataFlow::Configuration { + SymmetricKeyTrackingConfiguration() { this = "SymmetricKeyTrackingConfiguration2" } override predicate isSource(DataFlow::Node source) { - source.asExpr() instanceof IntegerLiteral and + source.asExpr() instanceof IntegerLiteral and // ! this works with current test cases, but reconsider IntegerLiteral when variables are used source.toString().toInt() < 128 } @@ -76,54 +63,34 @@ class SymmetricKeyTrackingConfiguration2 extends DataFlow::Configuration { } } -class UnsafeSymmetricKeySize extends IntegerLiteral { - UnsafeSymmetricKeySize() { this.getIntValue() < 128 } +// ! below doesn't work for some reason... +predicate hasInsufficientKeySize2(DataFlow::PathNode source, DataFlow::PathNode sink) { + exists(AsymmetricKeyTrackingConfiguration config1 | config1.hasFlowPath(source, sink)) + or + exists(SymmetricKeyTrackingConfiguration config2 | config2.hasFlowPath(source, sink)) } -class UnsafeAsymmetricKeySize extends IntegerLiteral { - UnsafeAsymmetricKeySize() { this.getIntValue() < 2048 } -} - -class UnsafeKeySize extends IntegerLiteral { - UnsafeKeySize() { - this instanceof UnsafeAsymmetricKeySize and - exists(MethodAccess ma | ma.getMethod() instanceof KeyPairGeneratorInitMethod) - or - this instanceof UnsafeSymmetricKeySize and - exists(MethodAccess ma | ma.getMethod() instanceof KeyGeneratorInitMethod) - } -} - -class KeyInitMethod extends Method { - KeyInitMethod() { - this instanceof KeyGeneratorInitMethod or - this instanceof KeyPairGeneratorInitMethod - } -} - -/** - * key length data flow tracking configuration. - */ -class KeyTrackingConfiguration extends DataFlow::Configuration { - KeyTrackingConfiguration() { this = "KeyTrackingConfiguration" } - - override predicate isSource(DataFlow::Node source) { source.asExpr() instanceof UnsafeKeySize } - - override predicate isSink(DataFlow::Node sink) { - exists(MethodAccess ma | - ma.getMethod() instanceof KeyInitMethod and - sink.asExpr() = ma.getArgument(0) - ) - } -} - -// ******* DATAFLOW ******************************************************************************* +// ******** Need the below for the above ******** // ! move to Encryption.qll? /** The Java class `java.security.spec.ECGenParameterSpec`. */ private class ECGenParameterSpec extends RefType { ECGenParameterSpec() { this.hasQualifiedName("java.security.spec", "ECGenParameterSpec") } } +// ! move to Encryption.qll? +/** Returns the key size in the EC algorithm string */ +bindingset[algorithm] +private 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() +} + // ! move to Encryption.qll? /** The `init` method declared in `javax.crypto.KeyGenerator`. */ private class KeyGeneratorInitMethod extends Method { @@ -142,17 +109,19 @@ private class KeyPairGeneratorInitMethod extends Method { } } -/** Returns the key size in the EC algorithm string */ -bindingset[algorithm] -private 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() +// ******* DATAFLOW ABOVE ************************************************************************* +// ************************************************************************************************ +// ************************************************************************************************ +// ************************************************************************************************ +// ************************************************************************************************ +// ************************************************************************************************ +// ******* OLD/UNUSED OR EXPERIMENTAL CODE BELOW ************************************************** +class UnsafeSymmetricKeySize extends IntegerLiteral { + UnsafeSymmetricKeySize() { this.getIntValue() < 128 } +} + +class UnsafeAsymmetricKeySize extends IntegerLiteral { + UnsafeAsymmetricKeySize() { this.getIntValue() < 2048 } } /** Taint configuration tracking flow from a key generator to a `init` method call. */ @@ -189,215 +158,3 @@ private class KeyPairGeneratorInitConfiguration extends TaintTracking::Configura ) } } - -/** - * 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] -private predicate hasShortSymmetricKey_TEST() { - exists( - SymmetricKeyTrackingConfiguration2 cfg, DataFlow::PathNode source, DataFlow::PathNode sink - | - cfg.hasFlowPath(source, sink) - ) - // ma.getMethod() instanceof KeyGeneratorInitMethod and - // // flow needed to correctly determine algorithm type and - // // not match to ANY symmetric algorithm (although doesn't really matter since only have AES currently...) - // 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 - // ( - // // VarAccess case needed to handle FN of key-size stored in a variable - // // Note: cannot use CompileTimeConstantExpr since will miss cases when variable is not a compile-time constant - // // (e.g. not declared `final` in Java) - // exists(VarAccess var | - // var.getVariable().getInitializer().getUnderlyingExpr() instanceof IntegerLiteral and - // var.getVariable().getInitializer().getUnderlyingExpr().toString().toInt() < 128 and - // ma.getArgument(0) = var - // ) - // or - // ma.getArgument(0).(IntegerLiteral).getIntValue() < 128 - // ) and - // msg = "Key size should be at least 128 bits for " + type + " encryption." -} - -/** - * 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] -private predicate hasShortSymmetricKey(MethodAccess ma, string msg, string type) { - ma.getMethod() instanceof KeyGeneratorInitMethod and - // flow needed to correctly determine algorithm type and - // not match to ANY symmetric algorithm (although doesn't really matter since only have AES currently...) - 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 - ( - // VarAccess case needed to handle FN of key-size stored in a variable - // Note: cannot use CompileTimeConstantExpr since will miss cases when variable is not a compile-time constant - // (e.g. not declared `final` in Java) - exists(VarAccess var | - var.getVariable().getInitializer().getUnderlyingExpr() instanceof IntegerLiteral and - var.getVariable().getInitializer().getUnderlyingExpr().toString().toInt() < 128 and - ma.getArgument(0) = var - ) - or - // exists(CompileTimeConstantExpr var | - // //var.getUnderlyingExpr() instanceof IntegerLiteral and // can't include this... - // var.getIntValue() < 128 and - // ma.getArgument(0) = var - // ) - // or - 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. - */ -private 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] -private predicate hasShortAsymmetricKeyPair(MethodAccess ma, string msg, string type) { - ma.getMethod() instanceof KeyPairGeneratorInitMethod and - //ma.getQualifier() instanceof JavaSecurityKeyPairGenerator and - //ma.getQualifier().getBasicBlock() instanceof JavaSecurityKeyPairGenerator and - // * USE BELOW - ma.getQualifier().getBasicBlock().getAPredecessor() instanceof JavaSecurityKeyPairGenerator and - // * USE ABOVE - //ma.getQualifier().getBasicBlock().getNode(2) instanceof JavaSecurityKeyPairGenerator and - // ma.getQualifier() - // .getBasicBlock() - // .getANode() - // .(JavaSecurityKeyPairGenerator) - // .getAlgoSpec() - // .(StringLiteral) - // .getValue() - // .toUpperCase() = type and - //ma.getQualifier().getBasicBlock().getAPredecessor() instanceof JavaSecurityKeyPairGenerator and - // * USE BELOW - ma.getQualifier() - .getBasicBlock() - .getAPredecessor() - .(JavaSecurityKeyPairGenerator) - .getAlgoSpec() - .(StringLiteral) - .getValue() - .toUpperCase() = type and - // * USE ABOVE - // flow needed to correctly determine algorithm type and - // not match to ANY asymmetric algorithm - // * REMOVE BELOW - // 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 - // * REMOVE ABOVE - // VarAccess case needed to handle FN of key-size stored in a variable - // Note: cannot use CompileTimeConstantExpr since will miss cases when variable is not a compile-time constant - // (e.g. not declared `final` in Java) - ( - exists(VarAccess var | - var.getVariable().getInitializer().getUnderlyingExpr() instanceof IntegerLiteral and - var.getVariable().getInitializer().getUnderlyingExpr().toString().toInt() < 2048 and - ma.getArgument(0) = var - ) - or - ma.getArgument(0).(IntegerLiteral).getIntValue() < 2048 - // or - // exists( - // AsymmetricKeyTrackingConfiguration cfg, DataFlow::PathNode source, DataFlow::PathNode sink - // | - // cfg.hasFlowPath(source, sink) - // ) - ) 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. - */ -private 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. - */ -private 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. - */ -private 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 this so can use 'path-problem' select clause instead? -predicate hasInsufficientKeySize(Expr e, string msg) { - hasShortAESKey(e, msg) or - hasShortDsaKeyPair(e, msg) or - hasShortRsaKeyPair(e, msg) or - hasShortECKeyPair(e, msg) -} - -predicate hasInsufficientKeySize2(DataFlow::PathNode source, DataFlow::PathNode sink) { - exists(AsymmetricKeyTrackingConfiguration config1 | config1.hasFlowPath(source, sink)) - or - exists(SymmetricKeyTrackingConfiguration2 config2 | config2.hasFlowPath(source, sink)) -} diff --git a/java/ql/src/Security/CWE/CWE-326/InsufficientKeySize.ql b/java/ql/src/Security/CWE/CWE-326/InsufficientKeySize.ql index 2979c9116c4..93411a101b2 100644 --- a/java/ql/src/Security/CWE/CWE-326/InsufficientKeySize.ql +++ b/java/ql/src/Security/CWE/CWE-326/InsufficientKeySize.ql @@ -30,5 +30,6 @@ from DataFlow::PathNode source, DataFlow::PathNode sink where //hasInsufficientKeySize2(source, sink) exists(AsymmetricKeyTrackingConfiguration config1 | config1.hasFlowPath(source, sink)) or - exists(SymmetricKeyTrackingConfiguration2 config2 | config2.hasFlowPath(source, sink)) + exists(AsymmetricECCKeyTrackingConfiguration config2 | config2.hasFlowPath(source, sink)) or + exists(SymmetricKeyTrackingConfiguration config3 | config3.hasFlowPath(source, sink)) select sink.getNode(), source, sink, "This $@ is too small.", sink.getNode(), "key size" diff --git a/java/ql/test/query-tests/security/CWE-326/InsufficientKeySizeTest.java b/java/ql/test/query-tests/security/CWE-326/InsufficientKeySizeTest.java index 68c3e7f322e..50dc7b4637f 100644 --- a/java/ql/test/query-tests/security/CWE-326/InsufficientKeySizeTest.java +++ b/java/ql/test/query-tests/security/CWE-326/InsufficientKeySizeTest.java @@ -144,7 +144,8 @@ public class InsufficientKeySizeTest { { int size = 64; // test integer variable KeyGenerator keyGen = KeyGenerator.getInstance("AES"); // test KeyGenerator variable - testSymmetric(size, keyGen); + testSymmetric(size, keyGen); // test with variable as key size + testSymmetric2(64); // test with int constant as key size } @@ -159,7 +160,8 @@ public class InsufficientKeySizeTest { { ECGenParameterSpec ecSpec = new ECGenParameterSpec("secp112r1"); // test ECGenParameterSpec variable KeyPairGenerator keyPairGen22 = KeyPairGenerator.getInstance("EC"); // test KeyPairGenerator variable - testAsymmetricEC(ecSpec, keyPairGen22); + testAsymmetricEC(ecSpec, keyPairGen22); // test with variable as key size + testAsymmetricNonEC2(1024); // test with int constant as key size } } @@ -173,6 +175,13 @@ public class InsufficientKeySizeTest { kg.init(64); // $ hasInsufficientKeySize } + //! refactor this to use expected-value tag and combine with above method + public static void testSymmetric2(int keySize) throws java.security.NoSuchAlgorithmException, java.security.InvalidAlgorithmParameterException { + // BAD: Key size is less than 2048 + KeyGenerator keyGen = KeyGenerator.getInstance("AES"); + keyGen.init(keySize); // $ hasInsufficientKeySize + } + public static void testAsymmetricNonEC(int keySize, KeyPairGenerator kpg) throws java.security.NoSuchAlgorithmException, java.security.InvalidAlgorithmParameterException { // BAD: Key size is less than 2048 KeyPairGenerator keyPairGen = KeyPairGenerator.getInstance("RSA"); @@ -182,6 +191,13 @@ public class InsufficientKeySizeTest { kpg.initialize(1024); // $ hasInsufficientKeySize } + //! refactor this to use expected-value tag and combine with above method + public static void testAsymmetricNonEC2(int keySize) throws java.security.NoSuchAlgorithmException, java.security.InvalidAlgorithmParameterException { + // BAD: Key size is less than 2048 + KeyPairGenerator keyPairGen = KeyPairGenerator.getInstance("RSA"); + keyPairGen.initialize(keySize); // $ hasInsufficientKeySize + } + public static void testAsymmetricEC(ECGenParameterSpec spec, KeyPairGenerator kpg) throws java.security.NoSuchAlgorithmException, java.security.InvalidAlgorithmParameterException { // BAD: Key size is less than 256 KeyPairGenerator keyPairGen = KeyPairGenerator.getInstance("EC"); diff --git a/java/ql/test/query-tests/security/CWE-326/InsufficientKeySizeTest.ql b/java/ql/test/query-tests/security/CWE-326/InsufficientKeySizeTest.ql index b8133b154e4..8514fcd2098 100644 --- a/java/ql/test/query-tests/security/CWE-326/InsufficientKeySizeTest.ql +++ b/java/ql/test/query-tests/security/CWE-326/InsufficientKeySizeTest.ql @@ -9,9 +9,13 @@ class InsufficientKeySizeTest extends InlineExpectationsTest { override predicate hasActualResult(Location location, string element, string tag, string value) { tag = "hasInsufficientKeySize" and - exists(Expr e, string msg | hasInsufficientKeySize(e, msg) | - e.getLocation() = location and - element = e.toString() and + exists(DataFlow::PathNode source, DataFlow::PathNode sink | + exists(AsymmetricKeyTrackingConfiguration config1 | config1.hasFlowPath(source, sink)) or + exists(AsymmetricECCKeyTrackingConfiguration config2 | config2.hasFlowPath(source, sink)) or + exists(SymmetricKeyTrackingConfiguration config2 | config2.hasFlowPath(source, sink)) + | + sink.getNode().getLocation() = location and + element = sink.getNode().toString() and value = "" ) }