From da218fdbf1adf1a96ff43c33bdd13a9831fdbd50 Mon Sep 17 00:00:00 2001 From: Jami Cogswell Date: Fri, 14 Oct 2022 13:03:34 -0400 Subject: [PATCH] clean up code --- .../semmle/code/java/security/Encryption.qll | 14 ++- .../java/security/InsufficientKeySize.qll | 85 ++++++++++--------- .../security/InsufficientKeySizeQuery.qll | 2 - .../CWE/CWE-326/InsufficientKeySize.ql | 9 +- 4 files changed, 51 insertions(+), 59 deletions(-) diff --git a/java/ql/lib/semmle/code/java/security/Encryption.qll b/java/ql/lib/semmle/code/java/security/Encryption.qll index a1535a438b5..adb2dc74691 100644 --- a/java/ql/lib/semmle/code/java/security/Encryption.qll +++ b/java/ql/lib/semmle/code/java/security/Encryption.qll @@ -83,6 +83,11 @@ class KeyGenerator extends RefType { KeyGenerator() { this.hasQualifiedName("javax.crypto", "KeyGenerator") } } +/** The Java class `java.security.KeyPairGenerator`. */ +class KeyPairGenerator extends RefType { + KeyPairGenerator() { this.hasQualifiedName("java.security", "KeyPairGenerator") } +} + /** The `init` method declared in `javax.crypto.KeyGenerator`. */ class KeyGeneratorInitMethod extends Method { KeyGeneratorInitMethod() { @@ -91,11 +96,6 @@ class KeyGeneratorInitMethod extends Method { } } -/** 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() { @@ -323,7 +323,6 @@ 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") } @@ -389,19 +388,16 @@ 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") } diff --git a/java/ql/lib/semmle/code/java/security/InsufficientKeySize.qll b/java/ql/lib/semmle/code/java/security/InsufficientKeySize.qll index b34a9e0da21..16f960f1b6d 100644 --- a/java/ql/lib/semmle/code/java/security/InsufficientKeySize.qll +++ b/java/ql/lib/semmle/code/java/security/InsufficientKeySize.qll @@ -15,36 +15,40 @@ abstract class InsufficientKeySizeSink extends DataFlow::Node { predicate hasState(DataFlow::FlowState state) { state instanceof DataFlow::FlowStateEmpty } } -/** A source for an insufficient key size (asymmetric-non-ec: RSA, DSA, DH). */ +// *********************************** SOURCES *********************************** +/** A source for an insufficient key size used in RSA, DSA, and DH algorithms. */ private class AsymmetricNonEcSource extends InsufficientKeySizeSource { AsymmetricNonEcSource() { getNodeIntValue(this) < 2048 } override predicate hasState(DataFlow::FlowState state) { state = "2048" } } -/** A source for an insufficient key size (asymmetric-ec: EC%). */ +/** A source for an insufficient key size used in elliptic curve (EC) algorithms. */ private class AsymmetricEcSource extends InsufficientKeySizeSource { AsymmetricEcSource() { - getNodeIntValue(this) < 256 or - getEcKeySize(this.asExpr().(StringLiteral).getValue()) < 256 // for cases when the key size is embedded in the curve name + getNodeIntValue(this) < 256 + or + // the below is needed for cases when the key size is embedded in the curve name + getEcKeySize(this.asExpr().(StringLiteral).getValue()) < 256 } override predicate hasState(DataFlow::FlowState state) { state = "256" } } -/** A source for an insufficient key size (symmetric: AES). */ +/** A source for an insufficient key size used in AES algorithms. */ private class SymmetricSource extends InsufficientKeySizeSource { SymmetricSource() { getNodeIntValue(this) < 128 } override predicate hasState(DataFlow::FlowState state) { state = "128" } } +// ************************** SOURCES HELPER PREDICATES ************************** +/** Returns the integer value of a given Node. */ private int getNodeIntValue(DataFlow::Node node) { result = node.asExpr().(IntegerLiteral).getIntValue() } -// TODO: check if any other regex should be added based on some MRVA results. -/** Returns the key size in the EC algorithm string */ +/** Returns the key size from an EC algorithm curve name string */ bindingset[algorithm] private int getEcKeySize(string algorithm) { algorithm.matches("sec%") and // specification such as "secp256r1" @@ -57,31 +61,26 @@ private int getEcKeySize(string algorithm) { result = algorithm.regexpCapture(".*[a-zA-Z](\\d+)[a-zA-Z].*", 1).toInt() } -/** A sink for an insufficient key size (asymmetric-non-ec). */ +// ************************************ SINKS ************************************ +/** A sink for an insufficient key size used in RSA, DSA, and DH algorithms. */ private class AsymmetricNonEcSink extends InsufficientKeySizeSink { AsymmetricNonEcSink() { - // hasKeySizeInInitMethod(this, "asymmetric-non-ec") - // or - //hasKeySizeInSpec(this, "asymmetric-non-ec") - exists(AsymmInitMethodAccess ma, AsymmKeyGen kg | + exists(AsymmetricInitMethodAccess ma, AsymmetricKeyGenerator kg | kg.getAlgoName().matches(["RSA", "DSA", "DH"]) and DataFlow::localExprFlow(kg, ma.getQualifier()) and this.asExpr() = ma.getKeySizeArg() ) or - exists(AsymmetricNonEcSpec s | this.asExpr() = s.getKeySizeArg()) + exists(AsymmetricNonEcSpec spec | this.asExpr() = spec.getKeySizeArg()) } override predicate hasState(DataFlow::FlowState state) { state = "2048" } } -/** A sink for an insufficient key size (asymmetric-ec). */ +/** A sink for an insufficient key size used in elliptic curve (EC) algorithms. */ private class AsymmetricEcSink extends InsufficientKeySizeSink { AsymmetricEcSink() { - // hasKeySizeInInitMethod(this, "asymmetric-ec") - // or - //hasKeySizeInSpec(this, "asymmetric-ec") - exists(AsymmInitMethodAccess ma, AsymmKeyGen kg | + exists(AsymmetricInitMethodAccess ma, AsymmetricKeyGenerator kg | kg.getAlgoName().matches("EC%") and DataFlow::localExprFlow(kg, ma.getQualifier()) and this.asExpr() = ma.getKeySizeArg() @@ -93,11 +92,11 @@ private class AsymmetricEcSink extends InsufficientKeySizeSink { override predicate hasState(DataFlow::FlowState state) { state = "256" } } -/** A sink for an insufficient key size (symmetric). */ +/** A sink for an insufficient key size used in AES algorithms. */ private class SymmetricSink extends InsufficientKeySizeSink { SymmetricSink() { //hasKeySizeInInitMethod(this, "symmetric") - exists(SymmInitMethodAccess ma, SymmKeyGen kg | + exists(SymmetricInitMethodAccess ma, SymmetricKeyGenerator kg | kg.getAlgoName() = "AES" and DataFlow::localExprFlow(kg, ma.getQualifier()) and this.asExpr() = ma.getKeySizeArg() @@ -107,43 +106,49 @@ private class SymmetricSink extends InsufficientKeySizeSink { override predicate hasState(DataFlow::FlowState state) { state = "128" } } -abstract class InitMethodAccess extends MethodAccess { +// ********************** SINKS HELPER CLASSES & PREDICATES ********************** +/** A call to a method that initializes a key generator. */ +abstract class KeyGenInitMethodAccess extends MethodAccess { + /** Gets the `keysize` argument of this call. */ Argument getKeySizeArg() { result = this.getArgument(0) } } -class AsymmInitMethodAccess extends InitMethodAccess { - AsymmInitMethodAccess() { this.getMethod() instanceof KeyPairGeneratorInitMethod } +/** A call to the `initialize` method declared in `java.security.KeyPairGenerator`. */ +private class AsymmetricInitMethodAccess extends KeyGenInitMethodAccess { + AsymmetricInitMethodAccess() { this.getMethod() instanceof KeyPairGeneratorInitMethod } } -class SymmInitMethodAccess extends InitMethodAccess { - SymmInitMethodAccess() { this.getMethod() instanceof KeyGeneratorInitMethod } +/** A call to the `init` method declared in `javax.crypto.KeyGenerator`. */ +private class SymmetricInitMethodAccess extends KeyGenInitMethodAccess { + SymmetricInitMethodAccess() { this.getMethod() instanceof KeyGeneratorInitMethod } } -abstract class KeyGen extends JavaxCryptoAlgoSpec { +/** An instance of a key generator. */ +abstract class KeyGeneratorObject extends JavaxCryptoAlgoSpec { string getAlgoName() { result = this.getAlgoSpec().(StringLiteral).getValue().toUpperCase() } } -class AsymmKeyGen extends KeyGen { - AsymmKeyGen() { this instanceof JavaSecurityKeyPairGenerator } +/** An instance of a `java.security.KeyPairGenerator`. */ +private class AsymmetricKeyGenerator extends KeyGeneratorObject { + AsymmetricKeyGenerator() { this instanceof JavaSecurityKeyPairGenerator } - // ! this override is repetitive... override Expr getAlgoSpec() { result = this.(MethodAccess).getArgument(0) } } -class SymmKeyGen extends KeyGen { - SymmKeyGen() { this instanceof JavaxCryptoKeyGenerator } +/** An instance of a `javax.crypto.KeyGenerator`. */ +private class SymmetricKeyGenerator extends KeyGeneratorObject { + SymmetricKeyGenerator() { this instanceof JavaxCryptoKeyGenerator } - // ! this override is repetitive... override Expr getAlgoSpec() { result = this.(MethodAccess).getArgument(0) } } -// ! use below instead of/in above?? (actually I don't think I need any of this, can just use AsymmetricNonEcSpec and EcGenParameterSpec directly???) -// Algo spec -abstract class AsymmetricAlgoSpec extends ClassInstanceExpr { +/** An instance of an algorithm specification. */ +abstract class AlgoSpec extends ClassInstanceExpr { Argument getKeySizeArg() { result = this.getArgument(0) } } -class AsymmetricNonEcSpec extends AsymmetricAlgoSpec { +/** An instance of an RSA, DSA, or DH algorithm specification. */ +private class AsymmetricNonEcSpec extends AlgoSpec { AsymmetricNonEcSpec() { this.getConstructedType() instanceof RsaKeyGenParameterSpec or this.getConstructedType() instanceof DsaGenParameterSpec or @@ -151,11 +156,7 @@ class AsymmetricNonEcSpec extends AsymmetricAlgoSpec { } } -class AsymmetricEcSpec extends AsymmetricAlgoSpec { +/** An instance of an elliptic curve (EC) algorithm specification. */ +private class AsymmetricEcSpec extends AlgoSpec { AsymmetricEcSpec() { this.getConstructedType() instanceof EcGenParameterSpec } } -// 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: add barrier guard for !=0 conditional case -// todo: only update key sizes (and key size strings) in one place in the code diff --git a/java/ql/lib/semmle/code/java/security/InsufficientKeySizeQuery.qll b/java/ql/lib/semmle/code/java/security/InsufficientKeySizeQuery.qll index eb8e5441747..eb416d9647b 100644 --- a/java/ql/lib/semmle/code/java/security/InsufficientKeySizeQuery.qll +++ b/java/ql/lib/semmle/code/java/security/InsufficientKeySizeQuery.qll @@ -1,8 +1,6 @@ /** 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 /** A data flow configuration for tracking key sizes used in cryptographic algorithms. */ diff --git a/java/ql/src/Security/CWE/CWE-326/InsufficientKeySize.ql b/java/ql/src/Security/CWE/CWE-326/InsufficientKeySize.ql index 8f0f54c7d9b..b82104db79a 100644 --- a/java/ql/src/Security/CWE/CWE-326/InsufficientKeySize.ql +++ b/java/ql/src/Security/CWE/CWE-326/InsufficientKeySize.ql @@ -16,9 +16,6 @@ import semmle.code.java.security.InsufficientKeySizeQuery import DataFlow::PathGraph from DataFlow::PathNode source, DataFlow::PathNode sink -where exists(KeySizeConfiguration config1 | config1.hasFlowPath(source, sink)) -//or -// exists(AsymmetricNonECKeyTrackingConfiguration cfg | cfg.hasFlowPath(source, sink)) or -// exists(AsymmetricECKeyTrackingConfiguration cfg | cfg.hasFlowPath(source, sink)) or -// exists(SymmetricKeyTrackingConfiguration cfg | cfg.hasFlowPath(source, sink)) -select sink.getNode(), source, sink, "This $@ is too small.", source.getNode(), "key size" +where exists(KeySizeConfiguration cfg | cfg.hasFlowPath(source, sink)) +select sink.getNode(), source, sink, "This $@ is less than the recommended key size.", + source.getNode(), "key size"