diff --git a/java/ql/src/experimental/semmle/code/java/security/StaticInitializationVectorQuery.qll b/java/ql/lib/semmle/code/java/security/StaticInitializationVectorQuery.qll similarity index 72% rename from java/ql/src/experimental/semmle/code/java/security/StaticInitializationVectorQuery.qll rename to java/ql/lib/semmle/code/java/security/StaticInitializationVectorQuery.qll index 5d88d620c21..825a50dd63c 100644 --- a/java/ql/src/experimental/semmle/code/java/security/StaticInitializationVectorQuery.qll +++ b/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() ) } } diff --git a/java/ql/src/experimental/Security/CWE/CWE-1204/BadStaticInitializationVector.java b/java/ql/src/Security/CWE/CWE-1204/BadStaticInitializationVector.java similarity index 100% rename from java/ql/src/experimental/Security/CWE/CWE-1204/BadStaticInitializationVector.java rename to java/ql/src/Security/CWE/CWE-1204/BadStaticInitializationVector.java diff --git a/java/ql/src/experimental/Security/CWE/CWE-1204/GoodRandomInitializationVector.java b/java/ql/src/Security/CWE/CWE-1204/GoodRandomInitializationVector.java similarity index 100% rename from java/ql/src/experimental/Security/CWE/CWE-1204/GoodRandomInitializationVector.java rename to java/ql/src/Security/CWE/CWE-1204/GoodRandomInitializationVector.java diff --git a/java/ql/src/experimental/Security/CWE/CWE-1204/StaticInitializationVector.qhelp b/java/ql/src/Security/CWE/CWE-1204/StaticInitializationVector.qhelp similarity index 65% rename from java/ql/src/experimental/Security/CWE/CWE-1204/StaticInitializationVector.qhelp rename to java/ql/src/Security/CWE/CWE-1204/StaticInitializationVector.qhelp index d631dff22af..b3b17ea176a 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-1204/StaticInitializationVector.qhelp +++ b/java/ql/src/Security/CWE/CWE-1204/StaticInitializationVector.qhelp @@ -3,11 +3,10 @@

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

@@ -19,7 +18,7 @@ Use a random IV generated by SecureRandom.

-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:

diff --git a/java/ql/src/Security/CWE/CWE-1204/StaticInitializationVector.ql b/java/ql/src/Security/CWE/CWE-1204/StaticInitializationVector.ql new file mode 100644 index 00000000000..669c4e6f946 --- /dev/null +++ b/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" diff --git a/java/ql/src/change-notes/2022-07-19-static-initialization-vector.md b/java/ql/src/change-notes/2022-07-19-static-initialization-vector.md new file mode 100644 index 00000000000..011aa4d8c18 --- /dev/null +++ b/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). \ No newline at end of file diff --git a/java/ql/src/experimental/Security/CWE/CWE-1204/StaticInitializationVector.ql b/java/ql/src/experimental/Security/CWE/CWE-1204/StaticInitializationVector.ql deleted file mode 100644 index 9980f68ed80..00000000000 --- a/java/ql/src/experimental/Security/CWE/CWE-1204/StaticInitializationVector.ql +++ /dev/null @@ -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" diff --git a/java/ql/test/experimental/query-tests/security/CWE-1204/StaticInitializationVector.java b/java/ql/test/query-tests/security/CWE-1204/StaticInitializationVector.java similarity index 89% rename from java/ql/test/experimental/query-tests/security/CWE-1204/StaticInitializationVector.java rename to java/ql/test/query-tests/security/CWE-1204/StaticInitializationVector.java index f964d17239b..dca6eb261ca 100644 --- a/java/ql/test/experimental/query-tests/security/CWE-1204/StaticInitializationVector.java +++ b/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(); + } } diff --git a/java/ql/test/experimental/query-tests/security/CWE-1204/StaticInitializationVectorTest.expected b/java/ql/test/query-tests/security/CWE-1204/StaticInitializationVectorTest.expected similarity index 100% rename from java/ql/test/experimental/query-tests/security/CWE-1204/StaticInitializationVectorTest.expected rename to java/ql/test/query-tests/security/CWE-1204/StaticInitializationVectorTest.expected diff --git a/java/ql/test/experimental/query-tests/security/CWE-1204/StaticInitializationVectorTest.ql b/java/ql/test/query-tests/security/CWE-1204/StaticInitializationVectorTest.ql similarity index 89% rename from java/ql/test/experimental/query-tests/security/CWE-1204/StaticInitializationVectorTest.ql rename to java/ql/test/query-tests/security/CWE-1204/StaticInitializationVectorTest.ql index 29afc31ab04..f737847fb5e 100644 --- a/java/ql/test/experimental/query-tests/security/CWE-1204/StaticInitializationVectorTest.ql +++ b/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 {