From 37d85587e007472b976d2416c5a36a5485d20ecd Mon Sep 17 00:00:00 2001 From: Jami Cogswell Date: Wed, 12 Oct 2022 15:39:57 -0400 Subject: [PATCH] refactor code into InsufficientKeySize.qll --- .../semmle/code/java/security/Encryption.qll | 40 ++++++ .../java/security/InsufficientKeySize.qll | 100 +++++++++++++ .../security/InsufficientKeySizeQuery.qll | 133 ++---------------- .../CWE/CWE-326/InsufficientKeySize.ql | 11 +- .../CWE-326/InsufficientKeySizeTest.ql | 10 +- 5 files changed, 165 insertions(+), 129 deletions(-) create mode 100644 java/ql/lib/semmle/code/java/security/InsufficientKeySize.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..a1535a438b5 100644 --- a/java/ql/lib/semmle/code/java/security/Encryption.qll +++ b/java/ql/lib/semmle/code/java/security/Encryption.qll @@ -83,11 +83,27 @@ class KeyGenerator extends RefType { KeyGenerator() { this.hasQualifiedName("javax.crypto", "KeyGenerator") } } +/** The `init` method declared in `javax.crypto.KeyGenerator`. */ +class KeyGeneratorInitMethod extends Method { + KeyGeneratorInitMethod() { + this.getDeclaringType() instanceof KeyGenerator and + this.hasName("init") + } +} + /** The Java class `java.security.KeyPairGenerator`. */ class KeyPairGenerator extends RefType { KeyPairGenerator() { this.hasQualifiedName("java.security", "KeyPairGenerator") } } +/** The `initialize` method declared in `java.security.KeyPairGenerator`. */ +class KeyPairGeneratorInitMethod extends Method { + KeyPairGeneratorInitMethod() { + this.getDeclaringType() instanceof KeyPairGenerator and + this.hasName("initialize") + } +} + /** The `verify` method of the class `javax.net.ssl.HostnameVerifier`. */ class HostnameVerifierVerify extends Method { HostnameVerifierVerify() { @@ -307,6 +323,12 @@ class JavaxCryptoSecretKey extends JavaxCryptoAlgoSpec { } } +// TODO: consider extending JavaxCryptoAlgoSpec as above does; will need to override getAlgoSpec() method +/** The Java class `javax.crypto.spec.DHGenParameterSpec`. */ +class DhGenParameterSpec extends RefType { + DhGenParameterSpec() { this.hasQualifiedName("javax.crypto.spec", "DHGenParameterSpec") } +} + class JavaxCryptoKeyGenerator extends JavaxCryptoAlgoSpec { JavaxCryptoKeyGenerator() { exists(Method m | m.getAReference() = this | @@ -367,6 +389,24 @@ class JavaSecuritySignature extends JavaSecurityAlgoSpec { override Expr getAlgoSpec() { result = this.(ConstructorCall).getArgument(0) } } +// TODO: consider extending JavaSecurityAlgoSpec as above does; will need to override getAlgoSpec() method +/** The Java class `java.security.spec.ECGenParameterSpec`. */ +class EcGenParameterSpec extends RefType { + EcGenParameterSpec() { this.hasQualifiedName("java.security.spec", "ECGenParameterSpec") } +} + +// TODO: consider extending JavaSecurityAlgoSpec as above does; will need to override getAlgoSpec() method +/** The Java class `java.security.spec.RSAKeyGenParameterSpec`. */ +class RsaKeyGenParameterSpec extends RefType { + RsaKeyGenParameterSpec() { this.hasQualifiedName("java.security.spec", "RSAKeyGenParameterSpec") } +} + +// TODO: consider extending JavaSecurityAlgoSpec as above does; will need to override getAlgoSpec() method +/** The Java class `java.security.spec.DSAGenParameterSpec`. */ +class DsaGenParameterSpec extends RefType { + DsaGenParameterSpec() { this.hasQualifiedName("java.security.spec", "DSAGenParameterSpec") } +} + /** A method call to the Java class `java.security.KeyPairGenerator`. */ class JavaSecurityKeyPairGenerator extends JavaxCryptoAlgoSpec { JavaSecurityKeyPairGenerator() { diff --git a/java/ql/lib/semmle/code/java/security/InsufficientKeySize.qll b/java/ql/lib/semmle/code/java/security/InsufficientKeySize.qll new file mode 100644 index 00000000000..4aa4bbe7a44 --- /dev/null +++ b/java/ql/lib/semmle/code/java/security/InsufficientKeySize.qll @@ -0,0 +1,100 @@ +/** Provides classes and predicates related to insufficient key sizes in Java. */ + +private import semmle.code.java.security.Encryption +private import semmle.code.java.dataflow.DataFlow + +/** A source for an insufficient key size. */ +abstract class InsufficientKeySizeSource extends DataFlow::Node { } + +/** A sink for an insufficient key size. */ +abstract class InsufficientKeySizeSink extends DataFlow::Node { } + +// TODO: Consider if below 3 sources should be private and if it's possible to only use InsufficientKeySizeSource in the configs +// TODO: add QLDocs if keeping non-private +class AsymmetricNonECSource extends InsufficientKeySizeSource { + AsymmetricNonECSource() { getNodeIntValue(this) < 2048 } +} + +class AsymmetricECSource extends InsufficientKeySizeSource { + AsymmetricECSource() { + getNodeIntValue(this) < 256 or + getECKeySize(this.asExpr().(StringLiteral).getValue()) < 256 // need this for the cases when the key size is embedded in the curve name. + } +} + +class SymmetricSource extends InsufficientKeySizeSource { + SymmetricSource() { getNodeIntValue(this) < 128 } +} + +private int getNodeIntValue(DataFlow::Node node) { + result = node.asExpr().(IntegerLiteral).getIntValue() +} + +/** 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() +} + +class AsymmetricNonECSink extends InsufficientKeySizeSink { + AsymmetricNonECSink() { + exists(MethodAccess ma, JavaSecurityKeyPairGenerator jpg | + ma.getMethod() instanceof KeyPairGeneratorInitMethod and + jpg.getAlgoSpec().(StringLiteral).getValue().toUpperCase().matches(["RSA", "DSA", "DH"]) and + DataFlow::localExprFlow(jpg, ma.getQualifier()) and + this.asExpr() = ma.getArgument(0) + ) + or + exists(ClassInstanceExpr genParamSpec | + genParamSpec.getConstructedType() instanceof AsymmetricNonECSpec and + this.asExpr() = genParamSpec.getArgument(0) + ) + } +} + +// TODO: move to Encryption.qll? or keep here since specific to this query? +private class AsymmetricNonECSpec extends RefType { + AsymmetricNonECSpec() { + this instanceof RsaKeyGenParameterSpec or + this instanceof DsaGenParameterSpec or + this instanceof DhGenParameterSpec + } +} + +class AsymmetricECSink extends InsufficientKeySizeSink { + AsymmetricECSink() { + exists(MethodAccess ma, JavaSecurityKeyPairGenerator jpg | + ma.getMethod() instanceof KeyPairGeneratorInitMethod and + jpg.getAlgoSpec().(StringLiteral).getValue().toUpperCase().matches("EC%") and + DataFlow::localExprFlow(jpg, ma.getQualifier()) and + this.asExpr() = ma.getArgument(0) + ) + or + exists(ClassInstanceExpr ecGenParamSpec | + ecGenParamSpec.getConstructedType() instanceof EcGenParameterSpec and + this.asExpr() = ecGenParamSpec.getArgument(0) + ) + } +} + +class SymmetricSink extends InsufficientKeySizeSink { + SymmetricSink() { + exists(MethodAccess ma, JavaxCryptoKeyGenerator jcg | + ma.getMethod() instanceof KeyGeneratorInitMethod and + jcg.getAlgoSpec().(StringLiteral).getValue().toUpperCase() = "AES" and + DataFlow::localExprFlow(jcg, ma.getQualifier()) and + this.asExpr() = ma.getArgument(0) + ) + } +} +// TODO: +// todo #0: look into use of specs without keygen objects; should spec not be a sink in these cases? +// todo #3: make list of algo names more easily reusable (either as constant-type variable at top of file, or model as own class to share, etc.) +// todo #5: diff --git a/java/ql/lib/semmle/code/java/security/InsufficientKeySizeQuery.qll b/java/ql/lib/semmle/code/java/security/InsufficientKeySizeQuery.qll index c141cd04529..51800aa8df9 100644 --- a/java/ql/lib/semmle/code/java/security/InsufficientKeySizeQuery.qll +++ b/java/ql/lib/semmle/code/java/security/InsufficientKeySizeQuery.qll @@ -1,148 +1,43 @@ -/** Provides classes and predicates related to insufficient key sizes in Java. */ +/** Provides data flow configurations to be used in queries related to insufficient key sizes. */ import semmle.code.java.security.Encryption import semmle.code.java.dataflow.DataFlow import semmle.code.java.dataflow.TaintTracking +import semmle.code.java.security.InsufficientKeySize /** - * An Asymmetric (RSA, DSA, DH) key length data flow tracking configuration. + * A data flow configuration for tracking non-elliptic curve asymmetric algorithms + * (RSA, DSA, and DH) key sizes. */ class AsymmetricNonECKeyTrackingConfiguration extends DataFlow::Configuration { AsymmetricNonECKeyTrackingConfiguration() { this = "AsymmetricNonECKeyTrackingConfiguration" } - override predicate isSource(DataFlow::Node source) { - source.asExpr().(IntegerLiteral).getIntValue() < 2048 - } + override predicate isSource(DataFlow::Node source) { source instanceof AsymmetricNonECSource } - override predicate isSink(DataFlow::Node sink) { - exists(MethodAccess ma, JavaSecurityKeyPairGenerator jpg | - ma.getMethod() instanceof KeyPairGeneratorInitMethod and - jpg.getAlgoSpec().(StringLiteral).getValue().toUpperCase().matches(["RSA", "DSA", "DH"]) and - DataFlow::localExprFlow(jpg, ma.getQualifier()) and - sink.asExpr() = ma.getArgument(0) - ) - or - // TODO: combine below three for less duplicated code - exists(ClassInstanceExpr rsaKeyGenParamSpec | - rsaKeyGenParamSpec.getConstructedType() instanceof RsaKeyGenParameterSpec and - sink.asExpr() = rsaKeyGenParamSpec.getArgument(0) - ) - or - exists(ClassInstanceExpr dsaGenParamSpec | - dsaGenParamSpec.getConstructedType() instanceof DsaGenParameterSpec and - sink.asExpr() = dsaGenParamSpec.getArgument(0) - ) - or - exists(ClassInstanceExpr dhGenParamSpec | - dhGenParamSpec.getConstructedType() instanceof DhGenParameterSpec and - sink.asExpr() = dhGenParamSpec.getArgument(0) - ) - } + override predicate isSink(DataFlow::Node sink) { sink instanceof AsymmetricNonECSink } } /** - * An Asymmetric (EC) key length data flow tracking configuration. + * A data flow configuration for tracking elliptic curve (EC) asymmetric + * algorithm key sizes. */ class AsymmetricECKeyTrackingConfiguration extends DataFlow::Configuration { AsymmetricECKeyTrackingConfiguration() { this = "AsymmetricECKeyTrackingConfiguration" } - override predicate isSource(DataFlow::Node source) { - source.asExpr().(IntegerLiteral).getIntValue() < 256 or - getECKeySize(source.asExpr().(StringLiteral).getValue()) < 256 // need this for the cases when the key size is embedded in the curve name. - } + override predicate isSource(DataFlow::Node source) { source instanceof AsymmetricECSource } - override predicate isSink(DataFlow::Node sink) { - exists(MethodAccess ma, JavaSecurityKeyPairGenerator jpg | - ma.getMethod() instanceof KeyPairGeneratorInitMethod and - jpg.getAlgoSpec().(StringLiteral).getValue().toUpperCase().matches("EC%") and - DataFlow::localExprFlow(jpg, ma.getQualifier()) and - sink.asExpr() = ma.getArgument(0) - ) - or - exists(ClassInstanceExpr ecGenParamSpec | - ecGenParamSpec.getConstructedType() instanceof EcGenParameterSpec and - sink.asExpr() = ecGenParamSpec.getArgument(0) - ) - } + override predicate isSink(DataFlow::Node sink) { sink instanceof AsymmetricECSink } } -/** - * A Symmetric (AES) key length data flow tracking configuration. - */ +/** A data flow configuration for tracking symmetric algorithm (AES) key sizes. */ class SymmetricKeyTrackingConfiguration extends DataFlow::Configuration { SymmetricKeyTrackingConfiguration() { this = "SymmetricKeyTrackingConfiguration" } - override predicate isSource(DataFlow::Node source) { - source.asExpr().(IntegerLiteral).getIntValue() < 128 - } + override predicate isSource(DataFlow::Node source) { source instanceof SymmetricSource } - override predicate isSink(DataFlow::Node sink) { - exists(MethodAccess ma, JavaxCryptoKeyGenerator jcg | - ma.getMethod() instanceof KeyGeneratorInitMethod and - jcg.getAlgoSpec().(StringLiteral).getValue().toUpperCase() = "AES" and - DataFlow::localExprFlow(jcg, ma.getQualifier()) and - sink.asExpr() = ma.getArgument(0) - ) - } + override predicate isSink(DataFlow::Node sink) { sink instanceof SymmetricSink } } - -// ********************** Need the below models for the above configs ********************** -// todo: move some/all of below to Encryption.qll or elsewhere? -/** The Java class `java.security.spec.ECGenParameterSpec`. */ -private class EcGenParameterSpec extends RefType { - EcGenParameterSpec() { this.hasQualifiedName("java.security.spec", "ECGenParameterSpec") } -} - -/** The Java class `java.security.spec.RSAKeyGenParameterSpec`. */ -private class RsaKeyGenParameterSpec extends RefType { - RsaKeyGenParameterSpec() { this.hasQualifiedName("java.security.spec", "RSAKeyGenParameterSpec") } -} - -/** The Java class `java.security.spec.DSAGenParameterSpec`. */ -private class DsaGenParameterSpec extends RefType { - DsaGenParameterSpec() { this.hasQualifiedName("java.security.spec", "DSAGenParameterSpec") } -} - -/** The Java class `javax.crypto.spec.DHGenParameterSpec`. */ -private class DhGenParameterSpec extends RefType { - DhGenParameterSpec() { this.hasQualifiedName("javax.crypto.spec", "DHGenParameterSpec") } -} - -/** The `init` method declared in `javax.crypto.KeyGenerator`. */ -private class KeyGeneratorInitMethod extends Method { - KeyGeneratorInitMethod() { - this.getDeclaringType() instanceof KeyGenerator and - this.hasName("init") - } -} - -/** The `initialize` method declared in `java.security.KeyPairGenerator`. */ -private class KeyPairGeneratorInitMethod extends Method { - KeyPairGeneratorInitMethod() { - this.getDeclaringType() instanceof KeyPairGenerator and - this.hasName("initialize") - } -} - -/** 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 ************************************************************************* -// TODO: -// todo #0: look into use of specs without keygens; should spec not be a sink in these cases? -// todo #1: make representation of source that can be shared across the configs -// todo #2: make representation of sink that can be shared across the configs -// todo #3: make list of algo names more easily reusable (either as constant-type variable at top of file, or model as own class to share, etc.) -// todo #4: refactor to be more like the Python (or C#) version? (or not possible because of lack of DataFlow::Node for void method in Java?) +// ******* 3 DATAFLOW CONFIGS ABOVE ************************************************************************* // ******* SINGLE CONFIG ATTEMPT BELOW ************************************************************************* // /** // * A key length data flow tracking configuration. diff --git a/java/ql/src/Security/CWE/CWE-326/InsufficientKeySize.ql b/java/ql/src/Security/CWE/CWE-326/InsufficientKeySize.ql index 040f17abfc0..cb560aa1f34 100644 --- a/java/ql/src/Security/CWE/CWE-326/InsufficientKeySize.ql +++ b/java/ql/src/Security/CWE/CWE-326/InsufficientKeySize.ql @@ -16,9 +16,10 @@ import semmle.code.java.security.InsufficientKeySizeQuery import DataFlow::PathGraph from DataFlow::PathNode source, DataFlow::PathNode sink -where exists(KeyTrackingConfiguration config1 | config1.hasFlowPath(source, sink)) -//or -// exists(AsymmetricNonECKeyTrackingConfiguration config1 | config1.hasFlowPath(source, sink)) or -// exists(AsymmetricECKeyTrackingConfiguration config2 | config2.hasFlowPath(source, sink)) or -// exists(SymmetricKeyTrackingConfiguration config3 | config3.hasFlowPath(source, sink)) +where + //exists(KeyTrackingConfiguration config1 | config1.hasFlowPath(source, sink)) + //or + exists(AsymmetricNonECKeyTrackingConfiguration config1 | config1.hasFlowPath(source, sink)) or + exists(AsymmetricECKeyTrackingConfiguration config2 | config2.hasFlowPath(source, sink)) or + exists(SymmetricKeyTrackingConfiguration config3 | config3.hasFlowPath(source, sink)) select sink.getNode(), source, sink, "This $@ is too small.", source.getNode(), "key size" 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 6d0d83f25fe..869831242f9 100644 --- a/java/ql/test/query-tests/security/CWE-326/InsufficientKeySizeTest.ql +++ b/java/ql/test/query-tests/security/CWE-326/InsufficientKeySizeTest.ql @@ -11,12 +11,12 @@ class InsufficientKeySizeTest extends InlineExpectationsTest { override predicate hasActualResult(Location location, string element, string tag, string value) { tag = "hasInsufficientKeySize" and exists(DataFlow::PathNode source, DataFlow::PathNode sink | - exists(KeyTrackingConfiguration config1 | config1.hasFlowPath(source, sink)) - | + //exists(KeyTrackingConfiguration config1 | config1.hasFlowPath(source, sink)) //or - // exists(AsymmetricNonECKeyTrackingConfiguration config1 | config1.hasFlowPath(source, sink)) or - // exists(AsymmetricECKeyTrackingConfiguration config2 | config2.hasFlowPath(source, sink)) or - //exists(SymmetricKeyTrackingConfiguration config3 | config3.hasFlowPath(source, sink)) + exists(AsymmetricNonECKeyTrackingConfiguration config1 | config1.hasFlowPath(source, sink)) or + exists(AsymmetricECKeyTrackingConfiguration config2 | config2.hasFlowPath(source, sink)) or + exists(SymmetricKeyTrackingConfiguration config3 | config3.hasFlowPath(source, sink)) + | sink.getNode().getLocation() = location and element = sink.getNode().toString() and value = ""