Merge pull request #9853 from joefarebrother/static-init-vec

Java: Promote Static Initialization Vector query

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

@@ -1,3 +1,5 @@
+/** Definitions for the Static Initialization Vector query. */
+
 import java
 import semmle.code.java.dataflow.TaintTracking
 import semmle.code.java.dataflow.TaintTracking2
@@ -9,15 +11,14 @@ private predicate initializedWithConstants(ArrayCreationExpr array) {
   // creating an array without an initializer, for example `new byte[8]`
   not exists(array.getInit())
   or
-  // creating a multidimensional array with an initializer like `{ new byte[8], new byte[16] }`
-  // This works around https://github.com/github/codeql/issues/6552 -- change me once there is
-  // a better way to distinguish nested initializers that create zero-filled arrays
-  // (e.g. `new byte[1]`) from those with an initializer list (`new byte[] { 1 }` or just `{ 1 }`)
-  array.getInit().getAnInit().getAChildExpr() instanceof IntegerLiteral
-  or
-  // creating an array wit an initializer like `new byte[] { 1, 2 }`
-  forex(Expr element | element = array.getInit().getAnInit() |
+  initializedWithConstantsHelper(array.getInit())
+}
+
+private predicate initializedWithConstantsHelper(ArrayInit arInit) {
+  forex(Expr element | element = arInit.getAnInit() |
     element instanceof CompileTimeConstantExpr
+    or
+    initializedWithConstantsHelper(element)
   )
 }
 
@@ -74,9 +75,7 @@ private class ArrayUpdateConfig extends TaintTracking2::Configuration {
     source.asExpr() instanceof StaticByteArrayCreation
   }
 
-  override predicate isSink(DataFlow::Node sink) {
-    exists(ArrayUpdate update | update.getArray() = sink.asExpr())
-  }
+  override predicate isSink(DataFlow::Node sink) { sink.asExpr() = any(ArrayUpdate upd).getArray() }
 }
 
 /**
@@ -85,29 +84,12 @@ private class ArrayUpdateConfig extends TaintTracking2::Configuration {
 private class StaticInitializationVectorSource extends DataFlow::Node {
   StaticInitializationVectorSource() {
     exists(StaticByteArrayCreation array | array = this.asExpr() |
-      not exists(ArrayUpdateConfig config | config.hasFlow(DataFlow2::exprNode(array), _))
-    )
-  }
-}
-
-/**
- * A config that tracks initialization of a cipher for encryption.
- */
-private class EncryptionModeConfig extends TaintTracking2::Configuration {
-  EncryptionModeConfig() { this = "EncryptionModeConfig" }
-
-  override predicate isSource(DataFlow::Node source) {
-    source
-        .asExpr()
-        .(FieldRead)
-        .getField()
-        .hasQualifiedName("javax.crypto", "Cipher", "ENCRYPT_MODE")
-  }
-
-  override predicate isSink(DataFlow::Node sink) {
-    exists(MethodAccess ma, Method m | m = ma.getMethod() |
-      m.hasQualifiedName("javax.crypto", "Cipher", "init") and
-      ma.getArgument(0) = sink.asExpr()
+      not exists(ArrayUpdateConfig config | config.hasFlow(DataFlow2::exprNode(array), _)) and
+      // Reduce FPs from utility methods that return an empty array in an exceptional case
+      not exists(ReturnStmt ret |
+        array.getADimension().(CompileTimeConstantExpr).getIntValue() = 0 and
+        DataFlow::localExprFlow(array, ret.getResult())
+      )
     )
   }
 }
@@ -117,13 +99,14 @@ private class EncryptionModeConfig extends TaintTracking2::Configuration {
  */
 private class EncryptionInitializationSink extends DataFlow::Node {
   EncryptionInitializationSink() {
-    exists(MethodAccess ma, Method m, EncryptionModeConfig config | m = ma.getMethod() |
+    exists(MethodAccess ma, Method m, FieldRead fr | m = ma.getMethod() |
       m.hasQualifiedName("javax.crypto", "Cipher", "init") and
       m.getParameterType(2)
           .(RefType)
           .hasQualifiedName("java.security.spec", "AlgorithmParameterSpec") and
-      ma.getArgument(2) = this.asExpr() and
-      config.hasFlowToExpr(ma.getArgument(0))
+      fr.getField().hasQualifiedName("javax.crypto", "Cipher", "ENCRYPT_MODE") and
+      DataFlow::localExprFlow(fr, ma.getArgument(0)) and
+      ma.getArgument(2) = this.asExpr()
     )
   }
 }

java/ql/src/Security/CWE/CWE-1204/StaticInitializationVector.qhelp

@@ -3,11 +3,10 @@
 
 <overview>
 <p>
-A cipher needs an initialization vector (IV) when it is used in certain modes
-such as CBC or GCM. Under the same secret key, IVs should be unique and ideally unpredictable.
-Given a secret key, if the same IV is used for encryption, the same plaintexts result in the same ciphertexts.
-This lets an attacker learn if the same data pieces are transferred or stored,
-or this can help the attacker run a dictionary attack.
+When a cipher is used in certain modes such as CBC or GCM, it requires an initialization vector (IV).
+Under the same secret key, IVs should be unique and ideally unpredictable.
+If the same IV is used with the same secret key, then the same plaintext results in the same ciphertext.
+This can let an attacker learn if the same data pieces are transferred or stored, or help the attacker run a dictionary attack.
 </p>
 </overview>
 
@@ -19,7 +18,7 @@ Use a random IV generated by <code>SecureRandom</code>.
 
 <example>
 <p>
-The following example initializes a cipher with a static IV which is unsafe:
+The following example initializes a cipher with a static IV, which is unsafe:
 </p>
 <sample src="BadStaticInitializationVector.java" />
 

java/ql/src/Security/CWE/CWE-1204/StaticInitializationVector.ql

@@ -0,0 +1,21 @@
+/**
+ * @name Using a static initialization vector for encryption
+ * @description An initialization vector (IV) used for ciphers of certain modes (such as CBC or GCM) should be unique and unpredictable, to maximize encryption and prevent dictionary attacks.
+ * @kind path-problem
+ * @problem.severity warning
+ * @security-severity 7.5
+ * @precision high
+ * @id java/static-initialization-vector
+ * @tags security
+ *       external/cwe/cwe-329
+ *       external/cwe/cwe-1204
+ */
+
+import java
+import semmle.code.java.security.StaticInitializationVectorQuery
+import DataFlow::PathGraph
+
+from DataFlow::PathNode source, DataFlow::PathNode sink, StaticInitializationVectorConfig conf
+where conf.hasFlowPath(source, sink)
+select sink.getNode(), source, sink, "A $@ should not be used for encryption.", source.getNode(),
+  "static initialization vector"

java/ql/src/change-notes/2022-07-19-static-initialization-vector.md

@@ -0,0 +1,4 @@
+---
+category: newQuery
+---
+* The query "Using a static initialization vector for encryption" (`java/static-initialization-vector`) has been promoted from experimental to the main query pack. This query was originally [submitted as an experimental query by @artem-smotrakov](https://github.com/github/codeql/pull/6357).

java/ql/src/experimental/Security/CWE/CWE-1204/StaticInitializationVector.ql

@@ -1,26 +0,0 @@
-/**
- * @name Using a static initialization vector for encryption
- * @description A cipher needs an initialization vector (IV) in some cases,
- *              for example, when CBC or GCM modes are used. IVs are used to randomize the encryption,
- *              therefore they should be unique and ideally unpredictable.
- *              Otherwise, the same plaintexts result in same ciphertexts under a given secret key.
- *              If a static IV is used for encryption, this lets an attacker learn
- *              if the same data pieces are transferred or stored,
- *              or this can help the attacker run a dictionary attack.
- * @kind path-problem
- * @problem.severity warning
- * @precision high
- * @id java/static-initialization-vector
- * @tags security
- *       external/cwe/cwe-329
- *       external/cwe/cwe-1204
- */
-
-import java
-import experimental.semmle.code.java.security.StaticInitializationVectorQuery
-import DataFlow::PathGraph
-
-from DataFlow::PathNode source, DataFlow::PathNode sink, StaticInitializationVectorConfig conf
-where conf.hasFlowPath(source, sink)
-select sink.getNode(), source, sink, "A $@ should not be used for encryption.", source.getNode(),
-  "static initialization vector"

java/ql/test/query-tests/security/CWE-1204/StaticInitializationVector.java

@@ -164,4 +164,26 @@ public class StaticInitializationVector {
         cipher.update(plaintext);
         return cipher.doFinal();
     }
+
+    public byte[] generate(int size) throws Exception { 
+        if (size == 0) { 
+            return new byte[0];
+        }
+        byte[] randomBytes = new byte[size];
+        SecureRandom.getInstanceStrong().nextBytes(randomBytes);
+        return randomBytes;
+    }
+
+    // GOOD: AES-CBC with a random IV
+    public byte[] encryptWithGeneratedIvByteArray(byte[] key, byte[] plaintext) throws Exception {
+        byte[] iv = generate(16);
+
+        IvParameterSpec ivSpec = new IvParameterSpec(iv);
+        SecretKeySpec keySpec = new SecretKeySpec(key, "AES");
+
+        Cipher cipher = Cipher.getInstance("AES/CBC/PKCS5PADDING");
+        cipher.init(Cipher.ENCRYPT_MODE, keySpec, ivSpec); 
+        cipher.update(plaintext);
+        return cipher.doFinal();
+    }
 }

java/ql/test/query-tests/security/CWE-1204/StaticInitializationVectorTest.ql

@@ -1,5 +1,5 @@
 import java
-import experimental.semmle.code.java.security.StaticInitializationVectorQuery
+import semmle.code.java.security.StaticInitializationVectorQuery
 import TestUtilities.InlineExpectationsTest
 
 class StaticInitializationVectorTest extends InlineExpectationsTest {