From 218731ca0aca5c3810bcfd42c9df6655104bc0e3 Mon Sep 17 00:00:00 2001 From: Artem Smotrakov Date: Sun, 11 Jul 2021 17:35:26 +0200 Subject: [PATCH 01/14] Added a query for static initialization vectors in encryption - Added StaticInitializationVector.ql - Added StaticInitializationVector.qhelp - Added tests --- .../BadStaticInitializationVector.java | 4 + .../GoodRandomInitializationVector.java | 6 + .../CWE-1204/StaticInitializationVector.qhelp | 46 +++++ .../CWE-1204/StaticInitializationVector.ql | 190 ++++++++++++++++++ .../StaticInitializationVector.expected | 15 ++ .../CWE-1204/StaticInitializationVector.java | 118 +++++++++++ .../CWE-1204/StaticInitializationVector.qlref | 1 + 7 files changed, 380 insertions(+) create mode 100644 java/ql/src/experimental/Security/CWE/CWE-1204/BadStaticInitializationVector.java create mode 100644 java/ql/src/experimental/Security/CWE/CWE-1204/GoodRandomInitializationVector.java create mode 100644 java/ql/src/experimental/Security/CWE/CWE-1204/StaticInitializationVector.qhelp create mode 100644 java/ql/src/experimental/Security/CWE/CWE-1204/StaticInitializationVector.ql create mode 100644 java/ql/test/experimental/query-tests/security/CWE-1204/StaticInitializationVector.expected create mode 100644 java/ql/test/experimental/query-tests/security/CWE-1204/StaticInitializationVector.java create mode 100644 java/ql/test/experimental/query-tests/security/CWE-1204/StaticInitializationVector.qlref diff --git a/java/ql/src/experimental/Security/CWE/CWE-1204/BadStaticInitializationVector.java b/java/ql/src/experimental/Security/CWE/CWE-1204/BadStaticInitializationVector.java new file mode 100644 index 000000000000..85e8be6d8ce2 --- /dev/null +++ b/java/ql/src/experimental/Security/CWE/CWE-1204/BadStaticInitializationVector.java @@ -0,0 +1,4 @@ +byte[] iv = new byte[16]; // all zeroes +GCMParameterSpec params = new GCMParameterSpec(128, iv); +Cipher cipher = Cipher.getInstance("AES/GCM/PKCS5PADDING"); +cipher.init(Cipher.ENCRYPT_MODE, key, params); \ No newline at end of file diff --git a/java/ql/src/experimental/Security/CWE/CWE-1204/GoodRandomInitializationVector.java b/java/ql/src/experimental/Security/CWE/CWE-1204/GoodRandomInitializationVector.java new file mode 100644 index 000000000000..faceb119d643 --- /dev/null +++ b/java/ql/src/experimental/Security/CWE/CWE-1204/GoodRandomInitializationVector.java @@ -0,0 +1,6 @@ +byte[] iv = new byte[16]; +SecureRandom random = SecureRandom.getInstanceStrong(); +random.nextBytes(iv); +GCMParameterSpec params = new GCMParameterSpec(128, iv); +Cipher cipher = Cipher.getInstance("AES/GCM/PKCS5PADDING"); +cipher.init(Cipher.ENCRYPT_MODE, key, params); \ No newline at end of file diff --git a/java/ql/src/experimental/Security/CWE/CWE-1204/StaticInitializationVector.qhelp b/java/ql/src/experimental/Security/CWE/CWE-1204/StaticInitializationVector.qhelp new file mode 100644 index 000000000000..49f5862f3ded --- /dev/null +++ b/java/ql/src/experimental/Security/CWE/CWE-1204/StaticInitializationVector.qhelp @@ -0,0 +1,46 @@ + + + + +

+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 transfered or stored, +or this can help the attacker run a dictionary attack. +

+ + + +

+Use a random IV generated by SecureRandom. +

+ + + +

+The following example initializes a cipher with a static IV which is unsafe: +

+ + +

+The next example initializes a cipher with a random IV: +

+ + + + +
  • + Wikipedia: + Initialization vector. +
    • +
  • + National Institute of Standards and Technology: + Recommendation for Block Cipher Modes of Operation. +
    • +
  • + National Institute of Standards and Technology: + FIPS 140-2: Security Requirements for Cryptographic Modules. +
    • +     +     \ 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 new file mode 100644 index 000000000000..0d5dcf4ff4d3 --- /dev/null +++ b/java/ql/src/experimental/Security/CWE/CWE-1204/StaticInitializationVector.ql @@ -0,0 +1,190 @@ +/** + * @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 transfered 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 semmle.code.java.dataflow.TaintTracking +import semmle.code.java.dataflow.TaintTracking2 +import DataFlow::PathGraph + +/** + * Holds if `array` is initialized only with constants, for example, + * `new byte[8]` or `new byte[] { 1, 2, 3, 4, 5, 6, 7, 8 }`. + */ +private predicate initializedWithConstants(ArrayCreationExpr array) { + not exists(array.getInit()) + or + forex(Expr element | element = array.getInit().getAChildExpr() | + element instanceof CompileTimeConstantExpr + ) +} + +/** + * An expression that creates a byte array that is initialized with constants. + */ +private class StaticByteArrayCreation extends ArrayCreationExpr { + StaticByteArrayCreation() { + this.getType().(Array).getElementType().(PrimitiveType).getName() = "byte" and + initializedWithConstants(this) + } +} + +/** Defines a sub-set of expressions that update an array. */ +private class ArrayUpdate extends Expr { + Expr array; + + ArrayUpdate() { + exists(Assignment assign, ArrayAccess arrayAccess | arrayAccess = assign.getDest() | + assign = this and + arrayAccess.getArray() = array and + not assign.getSource() instanceof CompileTimeConstantExpr + ) + or + exists(StaticMethodAccess ma | + ma.getMethod().hasQualifiedName("java.lang", "System", "arraycopy") and + ma = this and + ma.getArgument(2) = array + ) + or + exists(StaticMethodAccess ma | + ma.getMethod().hasQualifiedName("java.util", "Arrays", "copyOf") and + ma = this and + ma = array + ) + or + exists(MethodAccess ma, Method m | + m = ma.getMethod() and + ma = this and + ma.getArgument(0) = array + | + m.hasQualifiedName("java.io", "InputStream", "read") or + m.hasQualifiedName("java.nio", "ByteBuffer", "get") or + m.hasQualifiedName("java.security", "SecureRandom", "nextBytes") or + m.hasQualifiedName("java.util", "Random", "nextBytes") + ) + } + + /** Returns the updated array. */ + Expr getArray() { result = array } +} + +/** + * A config that tracks dataflow from creating an array to an operation that updates it. + */ +private class ArrayUpdateConfig extends TaintTracking2::Configuration { + ArrayUpdateConfig() { this = "ArrayUpdateConfig" } + + override predicate isSource(DataFlow::Node source) { + source.asExpr() instanceof StaticByteArrayCreation + } + + override predicate isSink(DataFlow::Node sink) { + exists(ArrayUpdate update | update.getArray() = sink.asExpr()) + } +} + +/** + * A source that defines an array that doesn't get updated. + */ +private class StaticInitializationVectorSource extends DataFlow::Node { + StaticInitializationVectorSource() { + exists(StaticByteArrayCreation array | array = this.asExpr() | + not exists(ArrayUpdate update, ArrayUpdateConfig config | + config.hasFlow(DataFlow2::exprNode(array), DataFlow2::exprNode(update.getArray())) + ) + ) + } +} + +/** + * 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().(VarAccess).getVariable().hasName("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() + ) + } +} + +/** + * A sink that initializes a cipher for encryption with unsafe parameters. + */ +private class EncryptionInitializationSink extends DataFlow::Node { + EncryptionInitializationSink() { + exists(MethodAccess ma, Method m, EncryptionModeConfig config | 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)) + ) + } +} + +/** + * Holds if `fromNode` to `toNode` is a dataflow step + * that creates cipher's parameters with initialization vector. + */ +private predicate createInitializationVectorSpecStep(DataFlow::Node fromNode, DataFlow::Node toNode) { + exists(ConstructorCall cc, RefType type | + cc = toNode.asExpr() and type = cc.getConstructedType() + | + type.hasQualifiedName("javax.crypto.spec", "IvParameterSpec") and + cc.getArgument(0) = fromNode.asExpr() + or + type.hasQualifiedName("javax.crypto.spec", ["GCMParameterSpec", "RC2ParameterSpec"]) and + cc.getArgument(1) = fromNode.asExpr() + or + type.hasQualifiedName("javax.crypto.spec", "RC5ParameterSpec") and + cc.getArgument(3) = fromNode.asExpr() + ) +} + +/** + * A config that tracks dataflow to initializing a cipher with a static initialization vector. + */ +private class StaticInitializationVectorConfig extends TaintTracking::Configuration { + StaticInitializationVectorConfig() { this = "StaticInitializationVectorConfig" } + + override predicate isSource(DataFlow::Node source) { + source instanceof StaticInitializationVectorSource + } + + override predicate isSink(DataFlow::Node sink) { sink instanceof EncryptionInitializationSink } + + override predicate isAdditionalTaintStep(DataFlow::Node fromNode, DataFlow::Node toNode) { + createInitializationVectorSpecStep(fromNode, toNode) + } + + override predicate isSanitizer(DataFlow::Node node) { + exists(ArrayUpdate update | update.getArray() = node.asExpr()) + } +} + +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.expected b/java/ql/test/experimental/query-tests/security/CWE-1204/StaticInitializationVector.expected new file mode 100644 index 000000000000..a26dc0b26ae2 --- /dev/null +++ b/java/ql/test/experimental/query-tests/security/CWE-1204/StaticInitializationVector.expected @@ -0,0 +1,15 @@ +edges +| StaticInitializationVector.java:13:21:13:81 | new byte[] : byte[] | StaticInitializationVector.java:19:51:19:56 | ivSpec | +| StaticInitializationVector.java:26:21:26:32 | new byte[] : byte[] | StaticInitializationVector.java:32:51:32:56 | ivSpec | +| StaticInitializationVector.java:39:21:39:32 | new byte[] : byte[] | StaticInitializationVector.java:48:51:48:56 | ivSpec | +nodes +| StaticInitializationVector.java:13:21:13:81 | new byte[] : byte[] | semmle.label | new byte[] : byte[] | +| StaticInitializationVector.java:19:51:19:56 | ivSpec | semmle.label | ivSpec | +| StaticInitializationVector.java:26:21:26:32 | new byte[] : byte[] | semmle.label | new byte[] : byte[] | +| StaticInitializationVector.java:32:51:32:56 | ivSpec | semmle.label | ivSpec | +| StaticInitializationVector.java:39:21:39:32 | new byte[] : byte[] | semmle.label | new byte[] : byte[] | +| StaticInitializationVector.java:48:51:48:56 | ivSpec | semmle.label | ivSpec | +#select +| StaticInitializationVector.java:19:51:19:56 | ivSpec | StaticInitializationVector.java:13:21:13:81 | new byte[] : byte[] | StaticInitializationVector.java:19:51:19:56 | ivSpec | A $@ should not be used for encryption. | StaticInitializationVector.java:13:21:13:81 | new byte[] | static initialization vector | +| StaticInitializationVector.java:32:51:32:56 | ivSpec | StaticInitializationVector.java:26:21:26:32 | new byte[] : byte[] | StaticInitializationVector.java:32:51:32:56 | ivSpec | A $@ should not be used for encryption. | StaticInitializationVector.java:26:21:26:32 | new byte[] | static initialization vector | +| StaticInitializationVector.java:48:51:48:56 | ivSpec | StaticInitializationVector.java:39:21:39:32 | new byte[] : byte[] | StaticInitializationVector.java:48:51:48:56 | ivSpec | A $@ should not be used for encryption. | StaticInitializationVector.java:39:21:39:32 | new byte[] | static initialization vector | diff --git a/java/ql/test/experimental/query-tests/security/CWE-1204/StaticInitializationVector.java b/java/ql/test/experimental/query-tests/security/CWE-1204/StaticInitializationVector.java new file mode 100644 index 000000000000..fe02f276bacf --- /dev/null +++ b/java/ql/test/experimental/query-tests/security/CWE-1204/StaticInitializationVector.java @@ -0,0 +1,118 @@ +import javax.crypto.Cipher; +import javax.crypto.spec.GCMParameterSpec; +import javax.crypto.spec.IvParameterSpec; +import javax.crypto.spec.SecretKeySpec; + +import java.security.SecureRandom; +import java.util.Arrays; + +public class StaticInitializationVector { + + // BAD: AES-GCM with static IV from a byte array + public byte[] encryptWithStaticIvByteArrayWithInitializer(byte[] key, byte[] plaintext) throws Exception { + byte[] iv = new byte[] { 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 0, 1, 2, 3, 4, 5 }; + + GCMParameterSpec ivSpec = new GCMParameterSpec(128, iv); + SecretKeySpec keySpec = new SecretKeySpec(key, "AES"); + + Cipher cipher = Cipher.getInstance("AES/GCM/PKCS5PADDING"); + cipher.init(Cipher.ENCRYPT_MODE, keySpec, ivSpec); + cipher.update(plaintext); + return cipher.doFinal(); + } + + // BAD: AES-GCM with static IV from zero-initialized byte array + public byte[] encryptWithZeroStaticIvByteArray(byte[] key, byte[] plaintext) throws Exception { + byte[] iv = new byte[16]; + + GCMParameterSpec ivSpec = new GCMParameterSpec(128, iv); + SecretKeySpec keySpec = new SecretKeySpec(key, "AES"); + + Cipher cipher = Cipher.getInstance("AES/GCM/PKCS5PADDING"); + cipher.init(Cipher.ENCRYPT_MODE, keySpec, ivSpec); + cipher.update(plaintext); + return cipher.doFinal(); + } + + // BAD: AES-CBC with static IV from zero-initialized byte array + public byte[] encryptWithStaticIvByteArray(byte[] key, byte[] plaintext) throws Exception { + byte[] iv = new byte[16]; + for (byte i = 0; i < iv.length; i++) { + iv[i] = 1; + } + + 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(); + } + + // GOOD: AES-GCM with a random IV + public byte[] encryptWithRandomIv(byte[] key, byte[] plaintext) throws Exception { + byte[] iv = new byte[16]; + + SecureRandom random = SecureRandom.getInstanceStrong(); + random.nextBytes(iv); + + GCMParameterSpec ivSpec = new GCMParameterSpec(128, iv); + SecretKeySpec keySpec = new SecretKeySpec(key, "AES"); + + Cipher cipher = Cipher.getInstance("AES/GCM/PKCS5PADDING"); + cipher.init(Cipher.ENCRYPT_MODE, keySpec, ivSpec); + cipher.update(plaintext); + return cipher.doFinal(); + } + + // GOOD: AES-GCM with a random IV + public byte[] encryptWithRandomIvByteByByte(byte[] key, byte[] plaintext) throws Exception { + SecureRandom random = SecureRandom.getInstanceStrong(); + byte[] iv = new byte[16]; + for (int i = 0; i < iv.length; i++) { + iv[i] = (byte) random.nextInt(); + } + + GCMParameterSpec ivSpec = new GCMParameterSpec(128, iv); + SecretKeySpec keySpec = new SecretKeySpec(key, "AES"); + + Cipher cipher = Cipher.getInstance("AES/GCM/PKCS5PADDING"); + cipher.init(Cipher.ENCRYPT_MODE, keySpec, ivSpec); + cipher.update(plaintext); + return cipher.doFinal(); + } + + // GOOD: AES-GCM with a random IV + public byte[] encryptWithRandomIvWithSystemArrayCopy(byte[] key, byte[] plaintext) throws Exception { + byte[] randomBytes = new byte[16]; + SecureRandom.getInstanceStrong().nextBytes(randomBytes); + + byte[] iv = new byte[16]; + System.arraycopy(randomBytes, 0, iv, 0, 16); + + GCMParameterSpec ivSpec = new GCMParameterSpec(128, iv); + SecretKeySpec keySpec = new SecretKeySpec(key, "AES"); + + Cipher cipher = Cipher.getInstance("AES/GCM/PKCS5PADDING"); + cipher.init(Cipher.ENCRYPT_MODE, keySpec, ivSpec); + cipher.update(plaintext); + return cipher.doFinal(); + } + + // GOOD: AES-GCM with a random IV + public byte[] encryptWithRandomIvWithArraysCopy(byte[] key, byte[] plaintext) throws Exception { + byte[] randomBytes = new byte[16]; + SecureRandom.getInstanceStrong().nextBytes(randomBytes); + + byte[] iv = Arrays.copyOf(randomBytes, 16); + + GCMParameterSpec ivSpec = new GCMParameterSpec(128, iv); + SecretKeySpec keySpec = new SecretKeySpec(key, "AES"); + + Cipher cipher = Cipher.getInstance("AES/GCM/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/StaticInitializationVector.qlref b/java/ql/test/experimental/query-tests/security/CWE-1204/StaticInitializationVector.qlref new file mode 100644 index 000000000000..18df4ffb8d3e --- /dev/null +++ b/java/ql/test/experimental/query-tests/security/CWE-1204/StaticInitializationVector.qlref @@ -0,0 +1 @@ +experimental/Security/CWE/CWE-1204/StaticInitializationVector.ql \ No newline at end of file From cfe74b527a9bdbaf9c79e461fe62f3a7b9a5b115 Mon Sep 17 00:00:00 2001 From: Artem Smotrakov Date: Sat, 17 Jul 2021 01:04:52 +0200 Subject: [PATCH 02/14] Use inline-expectation tests for StaticInitializationVector.ql --- .../CWE-1204/StaticInitializationVector.ql | 166 +----------------- .../StaticInitializationVectorQuery.qll | 166 ++++++++++++++++++ .../StaticInitializationVector.expected | 15 -- .../CWE-1204/StaticInitializationVector.java | 6 +- .../CWE-1204/StaticInitializationVector.qlref | 1 - .../StaticInitializationVectorTest.expected | 0 .../StaticInitializationVectorTest.ql | 20 +++ 7 files changed, 190 insertions(+), 184 deletions(-) create mode 100644 java/ql/src/experimental/semmle/code/java/security/StaticInitializationVectorQuery.qll delete mode 100644 java/ql/test/experimental/query-tests/security/CWE-1204/StaticInitializationVector.expected delete mode 100644 java/ql/test/experimental/query-tests/security/CWE-1204/StaticInitializationVector.qlref create mode 100644 java/ql/test/experimental/query-tests/security/CWE-1204/StaticInitializationVectorTest.expected create mode 100644 java/ql/test/experimental/query-tests/security/CWE-1204/StaticInitializationVectorTest.ql diff --git a/java/ql/src/experimental/Security/CWE/CWE-1204/StaticInitializationVector.ql b/java/ql/src/experimental/Security/CWE/CWE-1204/StaticInitializationVector.ql index 0d5dcf4ff4d3..f041919ed93a 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-1204/StaticInitializationVector.ql +++ b/java/ql/src/experimental/Security/CWE/CWE-1204/StaticInitializationVector.ql @@ -17,173 +17,9 @@ */ import java -import semmle.code.java.dataflow.TaintTracking -import semmle.code.java.dataflow.TaintTracking2 +import experimental.semmle.code.java.security.StaticInitializationVectorQuery import DataFlow::PathGraph -/** - * Holds if `array` is initialized only with constants, for example, - * `new byte[8]` or `new byte[] { 1, 2, 3, 4, 5, 6, 7, 8 }`. - */ -private predicate initializedWithConstants(ArrayCreationExpr array) { - not exists(array.getInit()) - or - forex(Expr element | element = array.getInit().getAChildExpr() | - element instanceof CompileTimeConstantExpr - ) -} - -/** - * An expression that creates a byte array that is initialized with constants. - */ -private class StaticByteArrayCreation extends ArrayCreationExpr { - StaticByteArrayCreation() { - this.getType().(Array).getElementType().(PrimitiveType).getName() = "byte" and - initializedWithConstants(this) - } -} - -/** Defines a sub-set of expressions that update an array. */ -private class ArrayUpdate extends Expr { - Expr array; - - ArrayUpdate() { - exists(Assignment assign, ArrayAccess arrayAccess | arrayAccess = assign.getDest() | - assign = this and - arrayAccess.getArray() = array and - not assign.getSource() instanceof CompileTimeConstantExpr - ) - or - exists(StaticMethodAccess ma | - ma.getMethod().hasQualifiedName("java.lang", "System", "arraycopy") and - ma = this and - ma.getArgument(2) = array - ) - or - exists(StaticMethodAccess ma | - ma.getMethod().hasQualifiedName("java.util", "Arrays", "copyOf") and - ma = this and - ma = array - ) - or - exists(MethodAccess ma, Method m | - m = ma.getMethod() and - ma = this and - ma.getArgument(0) = array - | - m.hasQualifiedName("java.io", "InputStream", "read") or - m.hasQualifiedName("java.nio", "ByteBuffer", "get") or - m.hasQualifiedName("java.security", "SecureRandom", "nextBytes") or - m.hasQualifiedName("java.util", "Random", "nextBytes") - ) - } - - /** Returns the updated array. */ - Expr getArray() { result = array } -} - -/** - * A config that tracks dataflow from creating an array to an operation that updates it. - */ -private class ArrayUpdateConfig extends TaintTracking2::Configuration { - ArrayUpdateConfig() { this = "ArrayUpdateConfig" } - - override predicate isSource(DataFlow::Node source) { - source.asExpr() instanceof StaticByteArrayCreation - } - - override predicate isSink(DataFlow::Node sink) { - exists(ArrayUpdate update | update.getArray() = sink.asExpr()) - } -} - -/** - * A source that defines an array that doesn't get updated. - */ -private class StaticInitializationVectorSource extends DataFlow::Node { - StaticInitializationVectorSource() { - exists(StaticByteArrayCreation array | array = this.asExpr() | - not exists(ArrayUpdate update, ArrayUpdateConfig config | - config.hasFlow(DataFlow2::exprNode(array), DataFlow2::exprNode(update.getArray())) - ) - ) - } -} - -/** - * 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().(VarAccess).getVariable().hasName("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() - ) - } -} - -/** - * A sink that initializes a cipher for encryption with unsafe parameters. - */ -private class EncryptionInitializationSink extends DataFlow::Node { - EncryptionInitializationSink() { - exists(MethodAccess ma, Method m, EncryptionModeConfig config | 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)) - ) - } -} - -/** - * Holds if `fromNode` to `toNode` is a dataflow step - * that creates cipher's parameters with initialization vector. - */ -private predicate createInitializationVectorSpecStep(DataFlow::Node fromNode, DataFlow::Node toNode) { - exists(ConstructorCall cc, RefType type | - cc = toNode.asExpr() and type = cc.getConstructedType() - | - type.hasQualifiedName("javax.crypto.spec", "IvParameterSpec") and - cc.getArgument(0) = fromNode.asExpr() - or - type.hasQualifiedName("javax.crypto.spec", ["GCMParameterSpec", "RC2ParameterSpec"]) and - cc.getArgument(1) = fromNode.asExpr() - or - type.hasQualifiedName("javax.crypto.spec", "RC5ParameterSpec") and - cc.getArgument(3) = fromNode.asExpr() - ) -} - -/** - * A config that tracks dataflow to initializing a cipher with a static initialization vector. - */ -private class StaticInitializationVectorConfig extends TaintTracking::Configuration { - StaticInitializationVectorConfig() { this = "StaticInitializationVectorConfig" } - - override predicate isSource(DataFlow::Node source) { - source instanceof StaticInitializationVectorSource - } - - override predicate isSink(DataFlow::Node sink) { sink instanceof EncryptionInitializationSink } - - override predicate isAdditionalTaintStep(DataFlow::Node fromNode, DataFlow::Node toNode) { - createInitializationVectorSpecStep(fromNode, toNode) - } - - override predicate isSanitizer(DataFlow::Node node) { - exists(ArrayUpdate update | update.getArray() = node.asExpr()) - } -} - 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(), diff --git a/java/ql/src/experimental/semmle/code/java/security/StaticInitializationVectorQuery.qll b/java/ql/src/experimental/semmle/code/java/security/StaticInitializationVectorQuery.qll new file mode 100644 index 000000000000..f190153385e2 --- /dev/null +++ b/java/ql/src/experimental/semmle/code/java/security/StaticInitializationVectorQuery.qll @@ -0,0 +1,166 @@ +import java +import semmle.code.java.dataflow.TaintTracking +import semmle.code.java.dataflow.TaintTracking2 + +/** + * Holds if `array` is initialized only with constants, for example, + * `new byte[8]` or `new byte[] { 1, 2, 3, 4, 5, 6, 7, 8 }`. + */ +private predicate initializedWithConstants(ArrayCreationExpr array) { + not exists(array.getInit()) + or + forex(Expr element | element = array.getInit().getAChildExpr() | + element instanceof CompileTimeConstantExpr + ) +} + +/** + * An expression that creates a byte array that is initialized with constants. + */ +private class StaticByteArrayCreation extends ArrayCreationExpr { + StaticByteArrayCreation() { + this.getType().(Array).getElementType().(PrimitiveType).getName() = "byte" and + initializedWithConstants(this) + } +} + +/** Defines a sub-set of expressions that update an array. */ +private class ArrayUpdate extends Expr { + Expr array; + + ArrayUpdate() { + exists(Assignment assign, ArrayAccess arrayAccess | arrayAccess = assign.getDest() | + assign = this and + arrayAccess.getArray() = array and + not assign.getSource() instanceof CompileTimeConstantExpr + ) + or + exists(StaticMethodAccess ma | + ma.getMethod().hasQualifiedName("java.lang", "System", "arraycopy") and + ma = this and + ma.getArgument(2) = array + ) + or + exists(StaticMethodAccess ma | + ma.getMethod().hasQualifiedName("java.util", "Arrays", "copyOf") and + ma = this and + ma = array + ) + or + exists(MethodAccess ma, Method m | + m = ma.getMethod() and + ma = this and + ma.getArgument(0) = array + | + m.hasQualifiedName("java.io", "InputStream", "read") or + m.hasQualifiedName("java.nio", "ByteBuffer", "get") or + m.hasQualifiedName("java.security", "SecureRandom", "nextBytes") or + m.hasQualifiedName("java.util", "Random", "nextBytes") + ) + } + + /** Returns the updated array. */ + Expr getArray() { result = array } +} + +/** + * A config that tracks dataflow from creating an array to an operation that updates it. + */ +private class ArrayUpdateConfig extends TaintTracking2::Configuration { + ArrayUpdateConfig() { this = "ArrayUpdateConfig" } + + override predicate isSource(DataFlow::Node source) { + source.asExpr() instanceof StaticByteArrayCreation + } + + override predicate isSink(DataFlow::Node sink) { + exists(ArrayUpdate update | update.getArray() = sink.asExpr()) + } +} + +/** + * A source that defines an array that doesn't get updated. + */ +private class StaticInitializationVectorSource extends DataFlow::Node { + StaticInitializationVectorSource() { + exists(StaticByteArrayCreation array | array = this.asExpr() | + not exists(ArrayUpdate update, ArrayUpdateConfig config | + config.hasFlow(DataFlow2::exprNode(array), DataFlow2::exprNode(update.getArray())) + ) + ) + } +} + +/** + * 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().(VarAccess).getVariable().hasName("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() + ) + } +} + +/** + * A sink that initializes a cipher for encryption with unsafe parameters. + */ +private class EncryptionInitializationSink extends DataFlow::Node { + EncryptionInitializationSink() { + exists(MethodAccess ma, Method m, EncryptionModeConfig config | 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)) + ) + } +} + +/** + * Holds if `fromNode` to `toNode` is a dataflow step + * that creates cipher's parameters with initialization vector. + */ +private predicate createInitializationVectorSpecStep(DataFlow::Node fromNode, DataFlow::Node toNode) { + exists(ConstructorCall cc, RefType type | + cc = toNode.asExpr() and type = cc.getConstructedType() + | + type.hasQualifiedName("javax.crypto.spec", "IvParameterSpec") and + cc.getArgument(0) = fromNode.asExpr() + or + type.hasQualifiedName("javax.crypto.spec", ["GCMParameterSpec", "RC2ParameterSpec"]) and + cc.getArgument(1) = fromNode.asExpr() + or + type.hasQualifiedName("javax.crypto.spec", "RC5ParameterSpec") and + cc.getArgument(3) = fromNode.asExpr() + ) +} + +/** + * A config that tracks dataflow to initializing a cipher with a static initialization vector. + */ +class StaticInitializationVectorConfig extends TaintTracking::Configuration { + StaticInitializationVectorConfig() { this = "StaticInitializationVectorConfig" } + + override predicate isSource(DataFlow::Node source) { + source instanceof StaticInitializationVectorSource + } + + override predicate isSink(DataFlow::Node sink) { sink instanceof EncryptionInitializationSink } + + override predicate isAdditionalTaintStep(DataFlow::Node fromNode, DataFlow::Node toNode) { + createInitializationVectorSpecStep(fromNode, toNode) + } + + override predicate isSanitizer(DataFlow::Node node) { + exists(ArrayUpdate update | update.getArray() = node.asExpr()) + } +} diff --git a/java/ql/test/experimental/query-tests/security/CWE-1204/StaticInitializationVector.expected b/java/ql/test/experimental/query-tests/security/CWE-1204/StaticInitializationVector.expected deleted file mode 100644 index a26dc0b26ae2..000000000000 --- a/java/ql/test/experimental/query-tests/security/CWE-1204/StaticInitializationVector.expected +++ /dev/null @@ -1,15 +0,0 @@ -edges -| StaticInitializationVector.java:13:21:13:81 | new byte[] : byte[] | StaticInitializationVector.java:19:51:19:56 | ivSpec | -| StaticInitializationVector.java:26:21:26:32 | new byte[] : byte[] | StaticInitializationVector.java:32:51:32:56 | ivSpec | -| StaticInitializationVector.java:39:21:39:32 | new byte[] : byte[] | StaticInitializationVector.java:48:51:48:56 | ivSpec | -nodes -| StaticInitializationVector.java:13:21:13:81 | new byte[] : byte[] | semmle.label | new byte[] : byte[] | -| StaticInitializationVector.java:19:51:19:56 | ivSpec | semmle.label | ivSpec | -| StaticInitializationVector.java:26:21:26:32 | new byte[] : byte[] | semmle.label | new byte[] : byte[] | -| StaticInitializationVector.java:32:51:32:56 | ivSpec | semmle.label | ivSpec | -| StaticInitializationVector.java:39:21:39:32 | new byte[] : byte[] | semmle.label | new byte[] : byte[] | -| StaticInitializationVector.java:48:51:48:56 | ivSpec | semmle.label | ivSpec | -#select -| StaticInitializationVector.java:19:51:19:56 | ivSpec | StaticInitializationVector.java:13:21:13:81 | new byte[] : byte[] | StaticInitializationVector.java:19:51:19:56 | ivSpec | A $@ should not be used for encryption. | StaticInitializationVector.java:13:21:13:81 | new byte[] | static initialization vector | -| StaticInitializationVector.java:32:51:32:56 | ivSpec | StaticInitializationVector.java:26:21:26:32 | new byte[] : byte[] | StaticInitializationVector.java:32:51:32:56 | ivSpec | A $@ should not be used for encryption. | StaticInitializationVector.java:26:21:26:32 | new byte[] | static initialization vector | -| StaticInitializationVector.java:48:51:48:56 | ivSpec | StaticInitializationVector.java:39:21:39:32 | new byte[] : byte[] | StaticInitializationVector.java:48:51:48:56 | ivSpec | A $@ should not be used for encryption. | StaticInitializationVector.java:39:21:39:32 | new byte[] | static initialization vector | diff --git a/java/ql/test/experimental/query-tests/security/CWE-1204/StaticInitializationVector.java b/java/ql/test/experimental/query-tests/security/CWE-1204/StaticInitializationVector.java index fe02f276bacf..95a0d4b49f88 100644 --- a/java/ql/test/experimental/query-tests/security/CWE-1204/StaticInitializationVector.java +++ b/java/ql/test/experimental/query-tests/security/CWE-1204/StaticInitializationVector.java @@ -16,7 +16,7 @@ public byte[] encryptWithStaticIvByteArrayWithInitializer(byte[] key, byte[] pla SecretKeySpec keySpec = new SecretKeySpec(key, "AES"); Cipher cipher = Cipher.getInstance("AES/GCM/PKCS5PADDING"); - cipher.init(Cipher.ENCRYPT_MODE, keySpec, ivSpec); + cipher.init(Cipher.ENCRYPT_MODE, keySpec, ivSpec); // $staticInitializationVector cipher.update(plaintext); return cipher.doFinal(); } @@ -29,7 +29,7 @@ public byte[] encryptWithZeroStaticIvByteArray(byte[] key, byte[] plaintext) thr SecretKeySpec keySpec = new SecretKeySpec(key, "AES"); Cipher cipher = Cipher.getInstance("AES/GCM/PKCS5PADDING"); - cipher.init(Cipher.ENCRYPT_MODE, keySpec, ivSpec); + cipher.init(Cipher.ENCRYPT_MODE, keySpec, ivSpec); // $staticInitializationVector cipher.update(plaintext); return cipher.doFinal(); } @@ -45,7 +45,7 @@ public byte[] encryptWithStaticIvByteArray(byte[] key, byte[] plaintext) throws SecretKeySpec keySpec = new SecretKeySpec(key, "AES"); Cipher cipher = Cipher.getInstance("AES/CBC/PKCS5PADDING"); - cipher.init(Cipher.ENCRYPT_MODE, keySpec, ivSpec); + cipher.init(Cipher.ENCRYPT_MODE, keySpec, ivSpec); // $staticInitializationVector cipher.update(plaintext); return cipher.doFinal(); } diff --git a/java/ql/test/experimental/query-tests/security/CWE-1204/StaticInitializationVector.qlref b/java/ql/test/experimental/query-tests/security/CWE-1204/StaticInitializationVector.qlref deleted file mode 100644 index 18df4ffb8d3e..000000000000 --- a/java/ql/test/experimental/query-tests/security/CWE-1204/StaticInitializationVector.qlref +++ /dev/null @@ -1 +0,0 @@ -experimental/Security/CWE/CWE-1204/StaticInitializationVector.ql \ No newline at end of file diff --git a/java/ql/test/experimental/query-tests/security/CWE-1204/StaticInitializationVectorTest.expected b/java/ql/test/experimental/query-tests/security/CWE-1204/StaticInitializationVectorTest.expected new file mode 100644 index 000000000000..e69de29bb2d1 diff --git a/java/ql/test/experimental/query-tests/security/CWE-1204/StaticInitializationVectorTest.ql b/java/ql/test/experimental/query-tests/security/CWE-1204/StaticInitializationVectorTest.ql new file mode 100644 index 000000000000..29afc31ab045 --- /dev/null +++ b/java/ql/test/experimental/query-tests/security/CWE-1204/StaticInitializationVectorTest.ql @@ -0,0 +1,20 @@ +import java +import experimental.semmle.code.java.security.StaticInitializationVectorQuery +import TestUtilities.InlineExpectationsTest + +class StaticInitializationVectorTest extends InlineExpectationsTest { + StaticInitializationVectorTest() { this = "StaticInitializationVectorTest" } + + override string getARelevantTag() { result = "staticInitializationVector" } + + override predicate hasActualResult(Location location, string element, string tag, string value) { + tag = "staticInitializationVector" and + exists(DataFlow::Node src, DataFlow::Node sink, StaticInitializationVectorConfig conf | + conf.hasFlow(src, sink) + | + sink.getLocation() = location and + element = sink.toString() and + value = "" + ) + } +} From df0f9ee3a552e283eb5538f3cc82cf618d9bdb30 Mon Sep 17 00:00:00 2001 From: Fosstars Date: Sun, 8 Aug 2021 12:50:04 +0200 Subject: [PATCH 03/14] Fixed a few typos --- .../Security/CWE/CWE-1204/StaticInitializationVector.qhelp | 2 +- .../Security/CWE/CWE-1204/StaticInitializationVector.ql | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/java/ql/src/experimental/Security/CWE/CWE-1204/StaticInitializationVector.qhelp b/java/ql/src/experimental/Security/CWE/CWE-1204/StaticInitializationVector.qhelp index 49f5862f3ded..d631dff22aff 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-1204/StaticInitializationVector.qhelp +++ b/java/ql/src/experimental/Security/CWE/CWE-1204/StaticInitializationVector.qhelp @@ -6,7 +6,7 @@ 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 transfered or stored, +This lets an attacker learn if the same data pieces are transferred or stored, or this can help the attacker run a dictionary attack.

    diff --git a/java/ql/src/experimental/Security/CWE/CWE-1204/StaticInitializationVector.ql b/java/ql/src/experimental/Security/CWE/CWE-1204/StaticInitializationVector.ql index f041919ed93a..9980f68ed80f 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-1204/StaticInitializationVector.ql +++ b/java/ql/src/experimental/Security/CWE/CWE-1204/StaticInitializationVector.ql @@ -5,7 +5,7 @@ * 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 transfered or stored, + * 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 From 4e69081c220a053940ab5ca3c3b67047faee6595 Mon Sep 17 00:00:00 2001 From: Fosstars Date: Fri, 13 Aug 2021 20:52:27 +0200 Subject: [PATCH 04/14] Support multi-dimensional arrays --- .../StaticInitializationVectorQuery.qll | 10 ++-- .../CWE-1204/StaticInitializationVector.java | 48 +++++++++++++++++++ 2 files changed, 55 insertions(+), 3 deletions(-) diff --git a/java/ql/src/experimental/semmle/code/java/security/StaticInitializationVectorQuery.qll b/java/ql/src/experimental/semmle/code/java/security/StaticInitializationVectorQuery.qll index f190153385e2..40860075f885 100644 --- a/java/ql/src/experimental/semmle/code/java/security/StaticInitializationVectorQuery.qll +++ b/java/ql/src/experimental/semmle/code/java/security/StaticInitializationVectorQuery.qll @@ -3,13 +3,17 @@ import semmle.code.java.dataflow.TaintTracking import semmle.code.java.dataflow.TaintTracking2 /** - * Holds if `array` is initialized only with constants, for example, - * `new byte[8]` or `new byte[] { 1, 2, 3, 4, 5, 6, 7, 8 }`. + * Holds if `array` is initialized only with constants. */ private predicate initializedWithConstants(ArrayCreationExpr array) { + // creating an array without an initializer, for example `new byte[8]` not exists(array.getInit()) or - forex(Expr element | element = array.getInit().getAChildExpr() | + // creating a multidimensional array with an initializer like `{ new byte[8], new byte[16] }` + 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() | element instanceof CompileTimeConstantExpr ) } diff --git a/java/ql/test/experimental/query-tests/security/CWE-1204/StaticInitializationVector.java b/java/ql/test/experimental/query-tests/security/CWE-1204/StaticInitializationVector.java index 95a0d4b49f88..05ae7e4dd8b8 100644 --- a/java/ql/test/experimental/query-tests/security/CWE-1204/StaticInitializationVector.java +++ b/java/ql/test/experimental/query-tests/security/CWE-1204/StaticInitializationVector.java @@ -50,6 +50,54 @@ public byte[] encryptWithStaticIvByteArray(byte[] key, byte[] plaintext) throws return cipher.doFinal(); } + // BAD: AES-GCM with static IV from a multidimensional byte array + public byte[] encryptWithOneOfStaticIvs01(byte[] key, byte[] plaintext) throws Exception { + byte[][] staticIvs = new byte[][] { + { 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 0, 1, 2, 3, 4, 5 }, + { 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 0, 1, 2, 3, 4, 42 } + }; + + GCMParameterSpec ivSpec = new GCMParameterSpec(128, staticIvs[1]); + SecretKeySpec keySpec = new SecretKeySpec(key, "AES"); + + Cipher cipher = Cipher.getInstance("AES/GCM/PKCS5PADDING"); + cipher.init(Cipher.ENCRYPT_MODE, keySpec, ivSpec); // $staticInitializationVector + cipher.update(plaintext); + return cipher.doFinal(); + } + + // BAD: AES-GCM with static IV from a multidimensional byte array + public byte[] encryptWithOneOfStaticIvs02(byte[] key, byte[] plaintext) throws Exception { + byte[][] staticIvs = new byte[][] { + new byte[] { 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 0, 1, 2, 3, 4, 5 }, + new byte[] { 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 0, 1, 2, 3, 4, 42 } + }; + + GCMParameterSpec ivSpec = new GCMParameterSpec(128, staticIvs[1]); + SecretKeySpec keySpec = new SecretKeySpec(key, "AES"); + + Cipher cipher = Cipher.getInstance("AES/GCM/PKCS5PADDING"); + cipher.init(Cipher.ENCRYPT_MODE, keySpec, ivSpec); // $staticInitializationVector + cipher.update(plaintext); + return cipher.doFinal(); + } + + // BAD: AES-GCM with static IV from a multidimensional byte array + public byte[] encryptWithOneOfStaticZeroIvs(byte[] key, byte[] plaintext) throws Exception { + byte[][] ivs = new byte[][] { + new byte[8], + new byte[16] + }; + + GCMParameterSpec ivSpec = new GCMParameterSpec(128, ivs[1]); + SecretKeySpec keySpec = new SecretKeySpec(key, "AES"); + + Cipher cipher = Cipher.getInstance("AES/GCM/PKCS5PADDING"); + cipher.init(Cipher.ENCRYPT_MODE, keySpec, ivSpec); // $staticInitializationVector + cipher.update(plaintext); + return cipher.doFinal(); + } + // GOOD: AES-GCM with a random IV public byte[] encryptWithRandomIv(byte[] key, byte[] plaintext) throws Exception { byte[] iv = new byte[16]; From 11992404ec3bb368a4e3455d260532819a8defd4 Mon Sep 17 00:00:00 2001 From: Fosstars Date: Sat, 14 Aug 2021 12:18:02 +0200 Subject: [PATCH 05/14] Be precise when checking for Cipher.ENCRYPT_MODE --- .../code/java/security/StaticInitializationVectorQuery.qll | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/java/ql/src/experimental/semmle/code/java/security/StaticInitializationVectorQuery.qll b/java/ql/src/experimental/semmle/code/java/security/StaticInitializationVectorQuery.qll index 40860075f885..66ebb38a5216 100644 --- a/java/ql/src/experimental/semmle/code/java/security/StaticInitializationVectorQuery.qll +++ b/java/ql/src/experimental/semmle/code/java/security/StaticInitializationVectorQuery.qll @@ -102,7 +102,11 @@ private class EncryptionModeConfig extends TaintTracking2::Configuration { EncryptionModeConfig() { this = "EncryptionModeConfig" } override predicate isSource(DataFlow::Node source) { - source.asExpr().(VarAccess).getVariable().hasName("ENCRYPT_MODE") + source + .asExpr() + .(FieldRead) + .getField() + .hasQualifiedName("javax.crypto", "Cipher", "ENCRYPT_MODE") } override predicate isSink(DataFlow::Node sink) { From d218813320dd26113a9fee6f5c20aaec64d011bc Mon Sep 17 00:00:00 2001 From: Fosstars Date: Sat, 14 Aug 2021 13:09:14 +0200 Subject: [PATCH 06/14] Updated qldoc for ArrayUpdate --- .../code/java/security/StaticInitializationVectorQuery.qll | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/java/ql/src/experimental/semmle/code/java/security/StaticInitializationVectorQuery.qll b/java/ql/src/experimental/semmle/code/java/security/StaticInitializationVectorQuery.qll index 66ebb38a5216..fe75c1df1fb2 100644 --- a/java/ql/src/experimental/semmle/code/java/security/StaticInitializationVectorQuery.qll +++ b/java/ql/src/experimental/semmle/code/java/security/StaticInitializationVectorQuery.qll @@ -28,7 +28,12 @@ private class StaticByteArrayCreation extends ArrayCreationExpr { } } -/** Defines a sub-set of expressions that update an array. */ +/** + * Defines a sub-set of expressions that update either content of an array or an array reference. + * There sub-set covers only methods that are likely to set a non-static IV. + * For example, `java.util.Arrays.fill()` is not covered because it assigns the same value + * to each element of the array. + */ private class ArrayUpdate extends Expr { Expr array; From e2dc9753ac767b9110e33ea2d7d241a572410e71 Mon Sep 17 00:00:00 2001 From: Fosstars Date: Sat, 14 Aug 2021 13:25:46 +0200 Subject: [PATCH 07/14] Covered copyOfRange() and clone() in ArrayUpdate --- .../code/java/security/StaticInitializationVectorQuery.qll | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/java/ql/src/experimental/semmle/code/java/security/StaticInitializationVectorQuery.qll b/java/ql/src/experimental/semmle/code/java/security/StaticInitializationVectorQuery.qll index fe75c1df1fb2..128f40b8c888 100644 --- a/java/ql/src/experimental/semmle/code/java/security/StaticInitializationVectorQuery.qll +++ b/java/ql/src/experimental/semmle/code/java/security/StaticInitializationVectorQuery.qll @@ -51,7 +51,7 @@ private class ArrayUpdate extends Expr { ) or exists(StaticMethodAccess ma | - ma.getMethod().hasQualifiedName("java.util", "Arrays", "copyOf") and + ma.getMethod().hasQualifiedName("java.util", "Arrays", ["copyOf", "copyOfRange"]) and ma = this and ma = array ) @@ -66,6 +66,10 @@ private class ArrayUpdate extends Expr { m.hasQualifiedName("java.security", "SecureRandom", "nextBytes") or m.hasQualifiedName("java.util", "Random", "nextBytes") ) + or + exists(MethodAccess ma, Method m | m = ma.getMethod() | + m.getDeclaringType().hasName("byte[]") and m.hasName("clone") and ma = this and ma = array + ) } /** Returns the updated array. */ From fbac5891b874d1fa411f1e4e3a1f056529b9b241 Mon Sep 17 00:00:00 2001 From: Fosstars Date: Sat, 14 Aug 2021 21:28:30 +0200 Subject: [PATCH 08/14] Fixed a typo in qldoc --- .../code/java/security/StaticInitializationVectorQuery.qll | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/java/ql/src/experimental/semmle/code/java/security/StaticInitializationVectorQuery.qll b/java/ql/src/experimental/semmle/code/java/security/StaticInitializationVectorQuery.qll index 128f40b8c888..c5f43de99b64 100644 --- a/java/ql/src/experimental/semmle/code/java/security/StaticInitializationVectorQuery.qll +++ b/java/ql/src/experimental/semmle/code/java/security/StaticInitializationVectorQuery.qll @@ -30,7 +30,7 @@ private class StaticByteArrayCreation extends ArrayCreationExpr { /** * Defines a sub-set of expressions that update either content of an array or an array reference. - * There sub-set covers only methods that are likely to set a non-static IV. + * This sub-set covers only methods that are likely to set a non-static IV. * For example, `java.util.Arrays.fill()` is not covered because it assigns the same value * to each element of the array. */ From c80a1da48366d54471da7a7d1a7fcfbe327c478e Mon Sep 17 00:00:00 2001 From: Fosstars Date: Wed, 25 Aug 2021 12:11:34 +0200 Subject: [PATCH 09/14] Don't consider copyOf() and clone() in ArrayUpdate --- .../java/security/StaticInitializationVectorQuery.qll | 10 ---------- .../security/CWE-1204/StaticInitializationVector.java | 3 ++- 2 files changed, 2 insertions(+), 11 deletions(-) diff --git a/java/ql/src/experimental/semmle/code/java/security/StaticInitializationVectorQuery.qll b/java/ql/src/experimental/semmle/code/java/security/StaticInitializationVectorQuery.qll index c5f43de99b64..cbddf2b64502 100644 --- a/java/ql/src/experimental/semmle/code/java/security/StaticInitializationVectorQuery.qll +++ b/java/ql/src/experimental/semmle/code/java/security/StaticInitializationVectorQuery.qll @@ -50,12 +50,6 @@ private class ArrayUpdate extends Expr { ma.getArgument(2) = array ) or - exists(StaticMethodAccess ma | - ma.getMethod().hasQualifiedName("java.util", "Arrays", ["copyOf", "copyOfRange"]) and - ma = this and - ma = array - ) - or exists(MethodAccess ma, Method m | m = ma.getMethod() and ma = this and @@ -66,10 +60,6 @@ private class ArrayUpdate extends Expr { m.hasQualifiedName("java.security", "SecureRandom", "nextBytes") or m.hasQualifiedName("java.util", "Random", "nextBytes") ) - or - exists(MethodAccess ma, Method m | m = ma.getMethod() | - m.getDeclaringType().hasName("byte[]") and m.hasName("clone") and ma = this and ma = array - ) } /** Returns the updated array. */ diff --git a/java/ql/test/experimental/query-tests/security/CWE-1204/StaticInitializationVector.java b/java/ql/test/experimental/query-tests/security/CWE-1204/StaticInitializationVector.java index 05ae7e4dd8b8..f964d17239bb 100644 --- a/java/ql/test/experimental/query-tests/security/CWE-1204/StaticInitializationVector.java +++ b/java/ql/test/experimental/query-tests/security/CWE-1204/StaticInitializationVector.java @@ -153,7 +153,8 @@ public byte[] encryptWithRandomIvWithArraysCopy(byte[] key, byte[] plaintext) th byte[] randomBytes = new byte[16]; SecureRandom.getInstanceStrong().nextBytes(randomBytes); - byte[] iv = Arrays.copyOf(randomBytes, 16); + byte[] iv = new byte[16]; + iv = Arrays.copyOf(randomBytes, 16); GCMParameterSpec ivSpec = new GCMParameterSpec(128, iv); SecretKeySpec keySpec = new SecretKeySpec(key, "AES"); From 86b7b2b86dccd4517e32b94e19c44c361073c513 Mon Sep 17 00:00:00 2001 From: Fosstars Date: Wed, 25 Aug 2021 12:14:36 +0200 Subject: [PATCH 10/14] Updated qldoc for ArrayUpdate --- .../code/java/security/StaticInitializationVectorQuery.qll | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/java/ql/src/experimental/semmle/code/java/security/StaticInitializationVectorQuery.qll b/java/ql/src/experimental/semmle/code/java/security/StaticInitializationVectorQuery.qll index cbddf2b64502..5e192ee326e7 100644 --- a/java/ql/src/experimental/semmle/code/java/security/StaticInitializationVectorQuery.qll +++ b/java/ql/src/experimental/semmle/code/java/security/StaticInitializationVectorQuery.qll @@ -28,12 +28,7 @@ private class StaticByteArrayCreation extends ArrayCreationExpr { } } -/** - * Defines a sub-set of expressions that update either content of an array or an array reference. - * This sub-set covers only methods that are likely to set a non-static IV. - * For example, `java.util.Arrays.fill()` is not covered because it assigns the same value - * to each element of the array. - */ +/** Defines a sub-set of expressions that update an array. */ private class ArrayUpdate extends Expr { Expr array; From f97c8bb049fb4607da97bf632997dbfef9b08269 Mon Sep 17 00:00:00 2001 From: Fosstars Date: Wed, 25 Aug 2021 12:40:48 +0200 Subject: [PATCH 11/14] Removed sanitizer in StaticInitializationVectorConfig --- .../code/java/security/StaticInitializationVectorQuery.qll | 4 ---- 1 file changed, 4 deletions(-) diff --git a/java/ql/src/experimental/semmle/code/java/security/StaticInitializationVectorQuery.qll b/java/ql/src/experimental/semmle/code/java/security/StaticInitializationVectorQuery.qll index 5e192ee326e7..c41cc045f5fc 100644 --- a/java/ql/src/experimental/semmle/code/java/security/StaticInitializationVectorQuery.qll +++ b/java/ql/src/experimental/semmle/code/java/security/StaticInitializationVectorQuery.qll @@ -161,8 +161,4 @@ class StaticInitializationVectorConfig extends TaintTracking::Configuration { override predicate isAdditionalTaintStep(DataFlow::Node fromNode, DataFlow::Node toNode) { createInitializationVectorSpecStep(fromNode, toNode) } - - override predicate isSanitizer(DataFlow::Node node) { - exists(ArrayUpdate update | update.getArray() = node.asExpr()) - } } From f41828e5dbf8ae99fd33bc20ceee94e23ee321fb Mon Sep 17 00:00:00 2001 From: Artem Smotrakov Date: Wed, 25 Aug 2021 19:38:33 +0200 Subject: [PATCH 12/14] Better qldoc in StaticInitializationVectorQuery.qll Co-authored-by: Chris Smowton --- .../code/java/security/StaticInitializationVectorQuery.qll | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/java/ql/src/experimental/semmle/code/java/security/StaticInitializationVectorQuery.qll b/java/ql/src/experimental/semmle/code/java/security/StaticInitializationVectorQuery.qll index c41cc045f5fc..b442277cf6e0 100644 --- a/java/ql/src/experimental/semmle/code/java/security/StaticInitializationVectorQuery.qll +++ b/java/ql/src/experimental/semmle/code/java/security/StaticInitializationVectorQuery.qll @@ -10,6 +10,9 @@ private predicate initializedWithConstants(ArrayCreationExpr array) { 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 }` @@ -28,7 +31,7 @@ private class StaticByteArrayCreation extends ArrayCreationExpr { } } -/** Defines a sub-set of expressions that update an array. */ +/** An expression that updates `array`. */ private class ArrayUpdate extends Expr { Expr array; From 23e23226358bf5276ee6d9f8d6e6f0e570155bea Mon Sep 17 00:00:00 2001 From: Artem Smotrakov Date: Wed, 25 Aug 2021 19:43:43 +0200 Subject: [PATCH 13/14] Simplify ArrayUpdate Co-authored-by: Chris Smowton --- .../code/java/security/StaticInitializationVectorQuery.qll | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/java/ql/src/experimental/semmle/code/java/security/StaticInitializationVectorQuery.qll b/java/ql/src/experimental/semmle/code/java/security/StaticInitializationVectorQuery.qll index b442277cf6e0..119e457bb3fb 100644 --- a/java/ql/src/experimental/semmle/code/java/security/StaticInitializationVectorQuery.qll +++ b/java/ql/src/experimental/semmle/code/java/security/StaticInitializationVectorQuery.qll @@ -36,9 +36,9 @@ private class ArrayUpdate extends Expr { Expr array; ArrayUpdate() { - exists(Assignment assign, ArrayAccess arrayAccess | arrayAccess = assign.getDest() | + exists(Assignment assign | assign = this and - arrayAccess.getArray() = array and + assign.getDest().(ArrayAccess).getArray() = array and not assign.getSource() instanceof CompileTimeConstantExpr ) or From 1dd4bf00acf99deee3b2d9d7ca92eb80f7476a05 Mon Sep 17 00:00:00 2001 From: Fosstars Date: Thu, 26 Aug 2021 09:41:05 +0200 Subject: [PATCH 14/14] Simplify StaticInitializationVectorSource Co-authored-by: Chris Smowton --- .../code/java/security/StaticInitializationVectorQuery.qll | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/java/ql/src/experimental/semmle/code/java/security/StaticInitializationVectorQuery.qll b/java/ql/src/experimental/semmle/code/java/security/StaticInitializationVectorQuery.qll index 119e457bb3fb..5d88d620c214 100644 --- a/java/ql/src/experimental/semmle/code/java/security/StaticInitializationVectorQuery.qll +++ b/java/ql/src/experimental/semmle/code/java/security/StaticInitializationVectorQuery.qll @@ -11,7 +11,7 @@ private predicate initializedWithConstants(ArrayCreationExpr array) { 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 + // 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 @@ -85,9 +85,7 @@ private class ArrayUpdateConfig extends TaintTracking2::Configuration { private class StaticInitializationVectorSource extends DataFlow::Node { StaticInitializationVectorSource() { exists(StaticByteArrayCreation array | array = this.asExpr() | - not exists(ArrayUpdate update, ArrayUpdateConfig config | - config.hasFlow(DataFlow2::exprNode(array), DataFlow2::exprNode(update.getArray())) - ) + not exists(ArrayUpdateConfig config | config.hasFlow(DataFlow2::exprNode(array), _)) ) } }