add local algo name tracking, still need to add ability to track algo name when KeyGen obj is param to other method

java/ql/lib/semmle/code/java/security/InsufficientKeySizeQuery.qll

@@ -2,6 +2,11 @@ import semmle.code.java.security.Encryption
 import semmle.code.java.dataflow.TaintTracking
 import semmle.code.java.dataflow.DataFlow
 
+// TODO:
+// 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: finish adding tracking for algo type/name... need flow/taint-tracking for across methods??
+// todo #3a: 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.)
 // ******* DATAFLOW BELOW *************************************************************************
 /**
  * Asymmetric (RSA, DSA, DH) key length data flow tracking configuration.
@@ -10,13 +15,27 @@ class AsymmetricKeyTrackingConfiguration extends DataFlow::Configuration {
   AsymmetricKeyTrackingConfiguration() { this = "AsymmetricKeyTrackingConfiguration" }
 
   override predicate isSource(DataFlow::Node source) {
-    source.asExpr() instanceof IntegerLiteral and // ! this works with current test cases, but reconsider IntegerLiteral when variables are used
-    source.toString().toInt() < 2048
+    exists(ClassInstanceExpr rsaGenParamSpec |
+      rsaGenParamSpec.getConstructedType() instanceof RSAGenParameterSpec and // ! double-check if should just use getType() instead
+      rsaGenParamSpec.getArgument(0).(IntegerLiteral).getIntValue() < 2048 and
+      source.asExpr() = rsaGenParamSpec
+    )
+    or
+    source.asExpr().(IntegerLiteral).getIntValue() < 2048
   }
 
   override predicate isSink(DataFlow::Node sink) {
-    exists(MethodAccess ma |
+    exists(MethodAccess ma, VarAccess va |
       ma.getMethod() instanceof KeyPairGeneratorInitMethod and
+      va.getVariable()
+          .getAnAssignedValue()
+          .(JavaSecurityKeyPairGenerator)
+          .getAlgoSpec()
+          .(StringLiteral)
+          .getValue()
+          .toUpperCase()
+          .matches(["RSA", "DSA", "DH"]) and
+      ma.getQualifier() = va and
       sink.asExpr() = ma.getArgument(0)
     )
   }
@@ -30,15 +49,26 @@ class AsymmetricECCKeyTrackingConfiguration extends DataFlow::Configuration {
 
   override predicate isSource(DataFlow::Node source) {
     exists(ClassInstanceExpr ecGenParamSpec |
-      getECKeySize(ecGenParamSpec.getArgument(0).(StringLiteral).getValue()) < 256 and
+      getECKeySize(ecGenParamSpec.getArgument(0).(StringLiteral).getValue()) < 256 and // ! can generate EC with just the keysize and not the curve apparently... (based on netty/netty FP example)
       source.asExpr() = ecGenParamSpec
     )
+    or
+    source.asExpr().(IntegerLiteral).getIntValue() < 256
   }
 
   override predicate isSink(DataFlow::Node sink) {
-    exists(MethodAccess ma |
+    exists(MethodAccess ma, VarAccess va |
       ma.getMethod() instanceof KeyPairGeneratorInitMethod and
-      ma.getArgument(0).getType() instanceof ECGenParameterSpec and
+      //ma.getArgument(0).getType() instanceof ECGenParameterSpec and // ! can generate EC with just the keysize and not the curve apparently... (based on netty/netty FP example)
+      va.getVariable()
+          .getAnAssignedValue()
+          .(JavaSecurityKeyPairGenerator)
+          .getAlgoSpec()
+          .(StringLiteral)
+          .getValue()
+          .toUpperCase()
+          .matches(["EC%"]) and
+      ma.getQualifier() = va and
       sink.asExpr() = ma.getArgument(0)
     )
   }
@@ -51,13 +81,21 @@ class SymmetricKeyTrackingConfiguration extends DataFlow::Configuration {
   SymmetricKeyTrackingConfiguration() { this = "SymmetricKeyTrackingConfiguration2" }
 
   override predicate isSource(DataFlow::Node source) {
-    source.asExpr() instanceof IntegerLiteral and // ! this works with current test cases, but reconsider IntegerLiteral when variables are used
-    source.toString().toInt() < 128
+    source.asExpr().(IntegerLiteral).getIntValue() < 128
   }
 
   override predicate isSink(DataFlow::Node sink) {
-    exists(MethodAccess ma |
+    exists(MethodAccess ma, VarAccess va |
       ma.getMethod() instanceof KeyGeneratorInitMethod and
+      va.getVariable()
+          .getAnAssignedValue()
+          .(JavaxCryptoKeyGenerator)
+          .getAlgoSpec()
+          .(StringLiteral)
+          .getValue()
+          .toUpperCase()
+          .matches(["AES"]) and
+      ma.getQualifier() = va and
       sink.asExpr() = ma.getArgument(0)
     )
   }
@@ -77,6 +115,11 @@ private class ECGenParameterSpec extends RefType {
   ECGenParameterSpec() { this.hasQualifiedName("java.security.spec", "ECGenParameterSpec") }
 }
 
+/** The Java class `java.security.spec.ECGenParameterSpec`. */
+private class RSAGenParameterSpec extends RefType {
+  RSAGenParameterSpec() { this.hasQualifiedName("java.security.spec", "RSAKeyGenParameterSpec") }
+}
+
 // ! move to Encryption.qll?
 /** Returns the key size in the EC algorithm string */
 bindingset[algorithm]

java/ql/src/Security/CWE/CWE-326/InsufficientKeySize.ql

@@ -28,7 +28,6 @@ import DataFlow::PathGraph
 //   sink.getNode(), "size"
 from DataFlow::PathNode source, DataFlow::PathNode sink
 where
-  //hasInsufficientKeySize2(source, sink)
   exists(AsymmetricKeyTrackingConfiguration config1 | config1.hasFlowPath(source, sink)) or
   exists(AsymmetricECCKeyTrackingConfiguration config2 | config2.hasFlowPath(source, sink)) or
   exists(SymmetricKeyTrackingConfiguration config3 | config3.hasFlowPath(source, sink))

java/ql/test/query-tests/security/CWE-326/InsufficientKeySizeTest.java

@@ -1,5 +1,6 @@
 import java.security.KeyPairGenerator;
 import java.security.spec.ECGenParameterSpec;
+import java.security.spec.RSAKeyGenParameterSpec;
 import javax.crypto.KeyGenerator;
 
 public class InsufficientKeySizeTest {
@@ -27,6 +28,16 @@ public class InsufficientKeySizeTest {
             // GOOD: Key size is no less than 2048
             KeyPairGenerator keyPairGen2 = KeyPairGenerator.getInstance("RSA");
             keyPairGen2.initialize(2048); // Safe
+
+            // test with spec
+            // BAD: Key size is less than 2048
+            KeyPairGenerator keyPairGen3 = KeyPairGenerator.getInstance("RSA");
+            RSAKeyGenParameterSpec rsaSpec = new RSAKeyGenParameterSpec(1024, null);
+            keyPairGen3.initialize(rsaSpec); // $ hasInsufficientKeySize
+
+            // BAD: Key size is less than 2048
+            KeyPairGenerator keyPairGen4 = KeyPairGenerator.getInstance("RSA");
+            keyPairGen4.initialize(new RSAKeyGenParameterSpec(1024, null)); // $ hasInsufficientKeySize
         }
 
         // DSA (Asymmetric)
@@ -145,7 +156,7 @@ public class InsufficientKeySizeTest {
             int size = 64; // test integer variable
             KeyGenerator keyGen = KeyGenerator.getInstance("AES"); // test KeyGenerator variable
             testSymmetric(size, keyGen); // test with variable as key size
-            testSymmetric2(64); // test with int constant as key size
+            testSymmetric2(64); // test with int literal as key size
          }
 
 
@@ -153,15 +164,16 @@ public class InsufficientKeySizeTest {
          {
             int size = 1024; // test integer variable
             KeyPairGenerator keyPairGen21 = KeyPairGenerator.getInstance("RSA"); // test KeyPairGenerator variable
-            testAsymmetricNonEC(size, keyPairGen21);
+            testAsymmetricNonEC(size, keyPairGen21); // test with variable as key size
+            testAsymmetricNonEC2(1024); // test with int literal as key size
          }
 
          // Test variable passed to other method(s) - Asymmetric, EC
          {
             ECGenParameterSpec ecSpec = new ECGenParameterSpec("secp112r1"); // test ECGenParameterSpec variable
             KeyPairGenerator keyPairGen22 = KeyPairGenerator.getInstance("EC"); // test KeyPairGenerator variable
-            testAsymmetricEC(ecSpec, keyPairGen22); // test with variable as key size
-            testAsymmetricNonEC2(1024); // test with int constant as key size
+            testAsymmetricEC(ecSpec, keyPairGen22);
+
          }
 
     }