clean up code

Author: Jami Cogswell

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") }

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,55 +106,57 @@ 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
     this.getConstructedType() instanceof DhGenParameterSpec
   }
 }
 
-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

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. */

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"