refactor tests and code, update help file

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

@@ -2,12 +2,12 @@ import semmle.code.java.security.Encryption
 import semmle.code.java.dataflow.TaintTracking
 
 /** The Java class `java.security.spec.ECGenParameterSpec`. */
-class ECGenParameterSpec extends RefType {
+private class ECGenParameterSpec extends RefType {
   ECGenParameterSpec() { this.hasQualifiedName("java.security.spec", "ECGenParameterSpec") }
 }
 
 /** The `init` method declared in `javax.crypto.KeyGenerator`. */
-class KeyGeneratorInitMethod extends Method {
+private class KeyGeneratorInitMethod extends Method {
   KeyGeneratorInitMethod() {
     this.getDeclaringType() instanceof KeyGenerator and
     this.hasName("init")
@@ -15,7 +15,7 @@ class KeyGeneratorInitMethod extends Method {
 }
 
 /** The `initialize` method declared in `java.security.KeyPairGenerator`. */
-class KeyPairGeneratorInitMethod extends Method {
+private class KeyPairGeneratorInitMethod extends Method {
   KeyPairGeneratorInitMethod() {
     this.getDeclaringType() instanceof KeyPairGenerator and
     this.hasName("initialize")
@@ -24,7 +24,7 @@ class KeyPairGeneratorInitMethod extends Method {
 
 /** Returns the key size in the EC algorithm string */
 bindingset[algorithm]
-int getECKeySize(string 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
@@ -36,7 +36,7 @@ int getECKeySize(string algorithm) {
 }
 
 /** Taint configuration tracking flow from a key generator to a `init` method call. */
-class KeyGeneratorInitConfiguration extends TaintTracking::Configuration {
+private class KeyGeneratorInitConfiguration extends TaintTracking::Configuration {
   KeyGeneratorInitConfiguration() { this = "KeyGeneratorInitConfiguration" }
 
   override predicate isSource(DataFlow::Node source) {
@@ -51,8 +51,11 @@ class KeyGeneratorInitConfiguration extends TaintTracking::Configuration {
   }
 }
 
-/** Taint configuration tracking flow from a keypair generator to a `initialize` method call. */
-class KeyPairGeneratorInitConfiguration extends TaintTracking::Configuration {
+/**
+ * Taint configuration tracking flow from a keypair generator to
+ * an `initialize` method call.
+ */
+private class KeyPairGeneratorInitConfiguration extends TaintTracking::Configuration {
   KeyPairGeneratorInitConfiguration() { this = "KeyPairGeneratorInitConfiguration" }
 
   override predicate isSource(DataFlow::Node source) {
@@ -67,9 +70,14 @@ class KeyPairGeneratorInitConfiguration extends TaintTracking::Configuration {
   }
 }
 
-/** Holds if a symmetric `KeyGenerator` implementing encryption algorithm `type` and initialized by `ma` uses an insufficient key size. `msg` provides a human-readable description of the problem. */
+/**
+ * Holds if a symmetric `KeyGenerator` implementing encryption algorithm
+ * `type` and initialized by `ma` uses an insufficient key size.
+ *
+ * `msg` provides a human-readable description of the problem.
+ */
 bindingset[type]
-predicate hasShortSymmetricKey(MethodAccess ma, string msg, string type) {
+private predicate hasShortSymmetricKey(MethodAccess ma, string msg, string type) {
   ma.getMethod() instanceof KeyGeneratorInitMethod and
   exists(
     JavaxCryptoKeyGenerator jcg, KeyGeneratorInitConfiguration cc, DataFlow::PathNode source,
@@ -84,12 +92,22 @@ predicate hasShortSymmetricKey(MethodAccess ma, string msg, string type) {
   msg = "Key size should be at least 128 bits for " + type + " encryption."
 }
 
-/** Holds if an AES `KeyGenerator` initialized by `ma` uses an insufficient key size. `msg` provides a human-readable description of the problem. */
-predicate hasShortAESKey(MethodAccess ma, string msg) { hasShortSymmetricKey(ma, msg, "AES") }
+/**
+ * Holds if an AES `KeyGenerator` initialized by `ma` uses an insufficient key size.
+ * `msg` provides a human-readable description of the problem.
+ */
+private predicate hasShortAESKey(MethodAccess ma, string msg) {
+  hasShortSymmetricKey(ma, msg, "AES")
+}
 
-/** Holds if an asymmetric `KeyPairGenerator` implementing encryption algorithm `type` and initialized by `ma` uses an insufficient key size. `msg` provides a human-readable description of the problem. */
+/**
+ * Holds if an asymmetric `KeyPairGenerator` implementing encryption algorithm
+ * `type` and initialized by `ma` uses an insufficient key size.
+ *
+ * `msg` provides a human-readable description of the problem.
+ */
 bindingset[type]
-predicate hasShortAsymmetricKeyPair(MethodAccess ma, string msg, string type) {
+private predicate hasShortAsymmetricKeyPair(MethodAccess ma, string msg, string type) {
   ma.getMethod() instanceof KeyPairGeneratorInitMethod and
   exists(
     JavaSecurityKeyPairGenerator jpg, KeyPairGeneratorInitConfiguration kc,
@@ -104,18 +122,31 @@ predicate hasShortAsymmetricKeyPair(MethodAccess ma, string msg, string type) {
   msg = "Key size should be at least 2048 bits for " + type + " encryption."
 }
 
-/** Holds if a DSA `KeyPairGenerator` initialized by `ma` uses an insufficient key size. `msg` provides a human-readable description of the problem. */
-predicate hasShortDsaKeyPair(MethodAccess ma, string msg) {
-  hasShortAsymmetricKeyPair(ma, msg, "DSA") or hasShortAsymmetricKeyPair(ma, msg, "DH")
+/**
+ * Holds if a DSA `KeyPairGenerator` initialized by `ma` uses an insufficient key size.
+ *
+ * `msg` provides a human-readable description of the problem.
+ */
+private predicate hasShortDsaKeyPair(MethodAccess ma, string msg) {
+  hasShortAsymmetricKeyPair(ma, msg, "DSA") or
+  hasShortAsymmetricKeyPair(ma, msg, "DH")
 }
 
-/** Holds if a RSA `KeyPairGenerator` initialized by `ma` uses an insufficient key size. `msg` provides a human-readable description of the problem. */
-predicate hasShortRsaKeyPair(MethodAccess ma, string msg) {
+/**
+ * Holds if a RSA `KeyPairGenerator` initialized by `ma` uses an insufficient key size.
+ *
+ * `msg` provides a human-readable description of the problem.
+ */
+private predicate hasShortRsaKeyPair(MethodAccess ma, string msg) {
   hasShortAsymmetricKeyPair(ma, msg, "RSA")
 }
 
-/** Holds if an EC `KeyPairGenerator` initialized by `ma` uses an insufficient key size. `msg` provides a human-readable description of the problem. */
-predicate hasShortECKeyPair(MethodAccess ma, string msg) {
+/**
+ * Holds if an EC `KeyPairGenerator` initialized by `ma` uses an insufficient key size.
+ *
+ * `msg` provides a human-readable description of the problem.
+ */
+private predicate hasShortECKeyPair(MethodAccess ma, string msg) {
   ma.getMethod() instanceof KeyPairGeneratorInitMethod and
   exists(
     JavaSecurityKeyPairGenerator jpg, KeyPairGeneratorInitConfiguration kc,
@@ -131,15 +162,11 @@ predicate hasShortECKeyPair(MethodAccess ma, string msg) {
   ) and
   msg = "Key size should be at least 256 bits for EC encryption."
 }
-// ! refactor to something like the below,
-// ! need to adjust select clause then...
-// ! see C# and C++ queries for ideas
-// class EncryptionAlgorithm extends
-// predicate hasInsufficientKeySize() {
-//   exists(Expr e, string msg |
-//     hasShortAESKey(e, msg) or
-//     hasShortDsaKeyPair(e, msg) or
-//     hasShortRsaKeyPair(e, msg) or
-//     hasShortECKeyPair(e, msg)
-//   )
-// }
+
+// ! refactor this so can use 'path-problem' select clause instead?
+predicate hasInsufficientKeySize(Expr e, string msg) {
+  hasShortAESKey(e, msg) or
+  hasShortDsaKeyPair(e, msg) or
+  hasShortRsaKeyPair(e, msg) or
+  hasShortECKeyPair(e, msg)
+}

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

@@ -1,29 +1,78 @@
-<!DOCTYPE qhelp PUBLIC "-//Semmle//qhelp//EN" "qhelp.dtd">
+<!DOCTYPE qhelp PUBLIC
+    "-//Semmle//qhelp//EN"
+    "qhelp.dtd">
 <qhelp>
+
     <overview>
-        <p>This rule finds uses of encryption algorithms with too small a key size. Encryption algorithms
-are vulnerable to brute force attack when too small a key size is used.</p>
+        <p>Modern encryption relies on the computational infeasibility of breaking a cipher and decoding its
+        message without the key. As computational power increases, the ability to break ciphers grows, and key
+        sizes need to become larger as a result. Encryption algorithms that use too small of a key size are
+        vulnerable to brute force attacks, which can reveal sensitive data.</p>
     </overview>
 
     <recommendation>
-        <p>The key should be at least 2048 bits long when using RSA and DSA encryption, 256 bits long when using EC encryption, and 128 bits long when using
-symmetric encryption.</p>
+        <p>Use a key of the recommended size or larger. The key size should be at least 2048 bits for RSA or
+        DSA encryption, 256 bits for elliptic curve (EC) encryption, and 128 bits for symmetric encryption,
+        such as AES.</p>
     </recommendation>
 
+    <example>
+
+          <p>
+               The following code uses encryption with insufficient key sizes.
+          </p>
+
+          <sample src="InsufficientKeySizeBad.java" />
+
+           <p>
+               To fix the code, change the key sizes to be the recommended size or
+               larger for each algorithm.
+          </p>
+
+          <!-- <p>
+               In the example below, the key sizes are set correctly.
+          </p>
+
+          <sample src="InsufficientKeySizeGood.java" /> -->
+
+     </example>
+
     <references>
-
         <li>
-            Wikipedia.
-            <a href="http://en.wikipedia.org/wiki/Key_size">Key size</a>
+            Wikipedia:
+            <a href="http://en.wikipedia.org/wiki/Key_size">Key size</a>.
         </li>
         <li>
-            SonarSource.
-            <a href="https://rules.sonarsource.com/java/type/Vulnerability/RSPEC-4426">Cryptographic keys should be robust</a>
+            Wikipedia: <a href="https://en.wikipedia.org/wiki/Strong_cryptography">Strong cryptography</a>.
+        </li>
+        <!-- <li>
+            Wikipedia:
+            <a href="https://en.wikipedia.org/wiki/RSA_(cryptosystem)">RSA (cryptosystem)</a>.
         </li>
         <li>
-            CWE.
-            <a href="https://cwe.mitre.org/data/definitions/326.html">CWE-326: Inadequate Encryption Strength</a>
+            Wikipedia:
+            <a href="https://en.wikipedia.org/wiki/Digital_Signature_Algorithm">Digital Signature Algorithm</a>.
+        </li>
+        <li>
+            Wikipedia:
+            <a href="https://en.wikipedia.org/wiki/Elliptic-curve_cryptography">Elliptic-curve cryptography</a>.
+        </li>
+        <li>
+            Wikipedia:
+            <a href="https://en.wikipedia.org/wiki/Advanced_Encryption_Standard">Advanced Encryption Standard</a>.
+        </li> -->
+        <li>
+            OWASP: <a href="https://cheatsheetseries.owasp.org/cheatsheets/Cryptographic_Storage_Cheat_Sheet.html#algorithms">
+            Cryptographic Storage Cheat Sheet</a>.
+        </li>
+        <li>
+            OWASP: <a href="https://owasp.org/www-project-web-security-testing-guide/stable/4-Web_Application_Security_Testing/09-Testing_for_Weak_Cryptography/04-Testing_for_Weak_Encryption">
+            Testing for Weak Encryption</a>.
+        </li>
+        <li>
+            NIST:
+            <a href="https://nvlpubs.nist.gov/nistpubs/SpecialPublications/NIST.SP.800-131Ar2.pdf">
+            Transitioning the Use of Cryptographic Algorithms and Key Lengths</a>.
         </li>
-
     </references>
 </qhelp>

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

@@ -1,6 +1,6 @@
 /**
- * @name Use of a cryptographic algorithm with insufficient key size
- * @description Using cryptographic algorithms with too small a key size can
+ * @name Insufficient key size used with a cryptographic algorithm
+ * @description Using cryptographic algorithms with too small of a key size can
  *              allow an attacker to compromise security.
  * @kind problem
  * @problem.severity error
@@ -14,9 +14,5 @@ import java
 import semmle.code.java.security.InsufficientKeySizeQuery
 
 from Expr e, string msg
-where
-  hasShortAESKey(e, msg) or
-  hasShortDsaKeyPair(e, msg) or
-  hasShortRsaKeyPair(e, msg) or
-  hasShortECKeyPair(e, msg)
+where hasInsufficientKeySize(e, msg)
 select e, msg

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

@@ -0,0 +1,16 @@
+    KeyPairGenerator keyPairGen1 = KeyPairGenerator.getInstance("RSA");
+    // BAD: Key size is less than 2048
+    keyPairGen1.initialize(1024);
+
+    KeyPairGenerator keyPairGen2 = KeyPairGenerator.getInstance("DSA");
+    // BAD: Key size is less than 2048
+    keyPairGen2.initialize(1024);
+
+    KeyPairGenerator keyPairGen3 = KeyPairGenerator.getInstance("EC");
+    // BAD: Key size is less than 256
+    ECGenParameterSpec ecSpec1 = new ECGenParameterSpec("secp112r1");
+    keyPairGen3.initialize(ecSpec1);
+
+    KeyGenerator keyGen = KeyGenerator.getInstance("AES");
+    // BAD: Key size is less than 128
+    keyGen.init(64);

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

@@ -0,0 +1,16 @@
+    KeyPairGenerator keyPairGen1 = KeyPairGenerator.getInstance("RSA");
+    // GOOD: Key size is no less than 2048
+    keyPairGen1.initialize(2048);
+
+    KeyPairGenerator keyPairGen2 = KeyPairGenerator.getInstance("DSA");
+    // GOOD: Key size is no less than 2048
+    keyPairGen2.initialize(2048);
+
+    KeyPairGenerator keyPairGen3 = KeyPairGenerator.getInstance("EC");
+    // GOOD: Key size is no less than 256
+    ECGenParameterSpec ecSpec = new ECGenParameterSpec("secp256r1");
+    keyPairGen3.initialize(ecSpec);
+
+    KeyGenerator keyGen = KeyGenerator.getInstance("AES");
+    // GOOD: Key size is no less than 128
+    keyGen.init(128);

java/ql/test/query-tests/security/CWE-326/InsufficientKeySize.qlref

@@ -1 +0,0 @@
-Security/CWE/CWE-326/InsufficientKeySize.ql

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

@@ -2,92 +2,92 @@ import java.security.KeyPairGenerator;
 import java.security.spec.ECGenParameterSpec;
 import javax.crypto.KeyGenerator;
 
-public class InsufficientKeySize {
+public class InsufficientKeySizeTest {
     public void CryptoMethod() throws java.security.NoSuchAlgorithmException, java.security.InvalidAlgorithmParameterException {
         KeyGenerator keyGen1 = KeyGenerator.getInstance("AES");
         // BAD: Key size is less than 128
-        keyGen1.init(64);
+        keyGen1.init(64); // $ hasInsufficientKeySize
 
         KeyGenerator keyGen2 = KeyGenerator.getInstance("AES");
         // GOOD: Key size is no less than 128
-        keyGen2.init(128);
+        keyGen2.init(128); // Safe
 
         KeyPairGenerator keyPairGen1 = KeyPairGenerator.getInstance("RSA");
         // BAD: Key size is less than 2048
-        keyPairGen1.initialize(1024);
+        keyPairGen1.initialize(1024); // $ hasInsufficientKeySize
 
         KeyPairGenerator keyPairGen2 = KeyPairGenerator.getInstance("RSA");
         // GOOD: Key size is no less than 2048
-        keyPairGen2.initialize(2048);
+        keyPairGen2.initialize(2048); // Safe
 
         KeyPairGenerator keyPairGen3 = KeyPairGenerator.getInstance("DSA");
         // BAD: Key size is less than 2048
-        keyPairGen3.initialize(1024);
+        keyPairGen3.initialize(1024); // $ hasInsufficientKeySize
 
         KeyPairGenerator keyPairGen4 = KeyPairGenerator.getInstance("DSA");
         // GOOD: Key size is no less than 2048
-        keyPairGen4.initialize(2048);
+        keyPairGen4.initialize(2048); // Safe
 
         KeyPairGenerator keyPairGen5 = KeyPairGenerator.getInstance("EC");
         // BAD: Key size is less than 256
         ECGenParameterSpec ecSpec1 = new ECGenParameterSpec("secp112r1");
-        keyPairGen5.initialize(ecSpec1);
+        keyPairGen5.initialize(ecSpec1); // $ hasInsufficientKeySize
 
         KeyPairGenerator keyPairGen6 = KeyPairGenerator.getInstance("EC");
         // BAD: Key size is less than 256
-        keyPairGen6.initialize(new ECGenParameterSpec("secp112r1"));
+        keyPairGen6.initialize(new ECGenParameterSpec("secp112r1")); // $ hasInsufficientKeySize
 
         KeyPairGenerator keyPairGen7 = KeyPairGenerator.getInstance("EC");
         // GOOD: Key size is no less than 256
         ECGenParameterSpec ecSpec2 = new ECGenParameterSpec("secp256r1");
-        keyPairGen7.initialize(ecSpec2);
+        keyPairGen7.initialize(ecSpec2); // Safe
 
         KeyPairGenerator keyPairGen8 = KeyPairGenerator.getInstance("EC");
         // BAD: Key size is less than 256
         ECGenParameterSpec ecSpec3 = new ECGenParameterSpec("X9.62 prime192v2");
-        keyPairGen8.initialize(ecSpec3);
+        keyPairGen8.initialize(ecSpec3); // $ hasInsufficientKeySize
 
         KeyPairGenerator keyPairGen9 = KeyPairGenerator.getInstance("EC");
         // BAD: Key size is less than 256
         ECGenParameterSpec ecSpec4 = new ECGenParameterSpec("X9.62 c2tnb191v3");
-        keyPairGen9.initialize(ecSpec4);
+        keyPairGen9.initialize(ecSpec4); // $ hasInsufficientKeySize
 
         KeyPairGenerator keyPairGen10 = KeyPairGenerator.getInstance("EC");
         // BAD: Key size is less than 256
         ECGenParameterSpec ecSpec5 = new ECGenParameterSpec("sect163k1");
-        keyPairGen10.initialize(ecSpec5);
+        keyPairGen10.initialize(ecSpec5); // $ hasInsufficientKeySize
 
         KeyPairGenerator keyPairGen11 = KeyPairGenerator.getInstance("EC");
         // GOOD: Key size is no less than 256
         ECGenParameterSpec ecSpec6 = new ECGenParameterSpec("X9.62 c2tnb359v1");
-        keyPairGen11.initialize(ecSpec6);
+        keyPairGen11.initialize(ecSpec6); // Safe
 
         KeyPairGenerator keyPairGen12 = KeyPairGenerator.getInstance("EC");
         // BAD: Key size is less than 256
         ECGenParameterSpec ecSpec7 = new ECGenParameterSpec("prime192v2");
-        keyPairGen12.initialize(ecSpec7);
+        keyPairGen12.initialize(ecSpec7); // $ hasInsufficientKeySize
 
         KeyPairGenerator keyPairGen13 = KeyPairGenerator.getInstance("EC");
-        // BAD: Key size is no less than 256
+        // BAD: Key size is no less than 256 // ! I think this comment is wrong - double-check
         ECGenParameterSpec ecSpec8 = new ECGenParameterSpec("prime256v1");
-        keyPairGen13.initialize(ecSpec8);
+        keyPairGen13.initialize(ecSpec8); // Safe
 
         KeyPairGenerator keyPairGen14 = KeyPairGenerator.getInstance("EC");
         // BAD: Key size is less than 256
         ECGenParameterSpec ecSpec9 = new ECGenParameterSpec("c2tnb191v1");
-        keyPairGen14.initialize(ecSpec9);
+        keyPairGen14.initialize(ecSpec9); // $ hasInsufficientKeySize
 
         KeyPairGenerator keyPairGen15 = KeyPairGenerator.getInstance("EC");
-        // BAD: Key size is no less than 256
+        // BAD: Key size is no less than 256 // ! I think this comment is wrong - double-check
         ECGenParameterSpec ecSpec10 = new ECGenParameterSpec("c2tnb431r1");
-        keyPairGen15.initialize(ecSpec10);
+        keyPairGen15.initialize(ecSpec10); // Safe
 
         KeyPairGenerator keyPairGen16 = KeyPairGenerator.getInstance("dh");
         // BAD: Key size is less than 2048
-        keyPairGen16.initialize(1024);
+        keyPairGen16.initialize(1024); // $ hasInsufficientKeySize
 
         KeyPairGenerator keyPairGen17 = KeyPairGenerator.getInstance("DH");
         // GOOD: Key size is no less than 2048
-        keyPairGen17.initialize(2048);
+        keyPairGen17.initialize(2048); // Safe
     }
 }

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

@@ -0,0 +1,18 @@
+import java
+import TestUtilities.InlineExpectationsTest
+import semmle.code.java.security.InsufficientKeySizeQuery
+
+class InsufficientKeySizeTest extends InlineExpectationsTest {
+  InsufficientKeySizeTest() { this = "InsufficientKeySize" }
+
+  override string getARelevantTag() { result = "hasInsufficientKeySize" }
+
+  override predicate hasActualResult(Location location, string element, string tag, string value) {
+    tag = "hasInsufficientKeySize" and
+    exists(Expr e, string msg | hasInsufficientKeySize(e, msg) |
+      e.getLocation() = location and
+      element = e.toString() and
+      value = ""
+    )
+  }
+}