refactor code into InsufficientKeySize.qll

2025-12-24 04:36:35 +01:00 · 2022-10-12 15:39:57 -04:00
parent 0fc4a33d43
commit 37d85587e0
5 changed files with 165 additions and 129 deletions
--- 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() {
--- a/java/ql/lib/semmle/code/java/security/InsufficientKeySize.qll
+++ 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:
--- 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.
--- 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"
--- 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 = ""