From f8f21c7ee60d22539ddc05179914ff4f6e3550d0 Mon Sep 17 00:00:00 2001 From: Joe Farebrother Date: Wed, 13 Jul 2022 15:36:39 +0100 Subject: [PATCH 01/10] Move static init vector query and tests from experimental to main --- .../code/java/security/StaticInitializationVectorQuery.qll | 0 .../Security/CWE/CWE-1204/BadStaticInitializationVector.java | 0 .../Security/CWE/CWE-1204/GoodRandomInitializationVector.java | 0 .../Security/CWE/CWE-1204/StaticInitializationVector.qhelp | 0 .../Security/CWE/CWE-1204/StaticInitializationVector.ql | 2 +- .../security/CWE-1204/StaticInitializationVector.java | 0 .../security/CWE-1204/StaticInitializationVectorTest.expected | 0 .../security/CWE-1204/StaticInitializationVectorTest.ql | 2 +- 8 files changed, 2 insertions(+), 2 deletions(-) rename java/ql/{src/experimental => lib}/semmle/code/java/security/StaticInitializationVectorQuery.qll (100%) rename java/ql/src/{experimental => }/Security/CWE/CWE-1204/BadStaticInitializationVector.java (100%) rename java/ql/src/{experimental => }/Security/CWE/CWE-1204/GoodRandomInitializationVector.java (100%) rename java/ql/src/{experimental => }/Security/CWE/CWE-1204/StaticInitializationVector.qhelp (100%) rename java/ql/src/{experimental => }/Security/CWE/CWE-1204/StaticInitializationVector.ql (93%) rename java/ql/test/{experimental => }/query-tests/security/CWE-1204/StaticInitializationVector.java (100%) rename java/ql/test/{experimental => }/query-tests/security/CWE-1204/StaticInitializationVectorTest.expected (100%) rename java/ql/test/{experimental => }/query-tests/security/CWE-1204/StaticInitializationVectorTest.ql (89%) 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 100% rename from java/ql/src/experimental/semmle/code/java/security/StaticInitializationVectorQuery.qll rename to java/ql/lib/semmle/code/java/security/StaticInitializationVectorQuery.qll 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 100% rename from java/ql/src/experimental/Security/CWE/CWE-1204/StaticInitializationVector.qhelp rename to java/ql/src/Security/CWE/CWE-1204/StaticInitializationVector.qhelp diff --git a/java/ql/src/experimental/Security/CWE/CWE-1204/StaticInitializationVector.ql b/java/ql/src/Security/CWE/CWE-1204/StaticInitializationVector.ql similarity index 93% rename from java/ql/src/experimental/Security/CWE/CWE-1204/StaticInitializationVector.ql rename to java/ql/src/Security/CWE/CWE-1204/StaticInitializationVector.ql index 9980f68ed80..24e24b0f41c 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-1204/StaticInitializationVector.ql +++ b/java/ql/src/Security/CWE/CWE-1204/StaticInitializationVector.ql @@ -17,7 +17,7 @@ */ import java -import experimental.semmle.code.java.security.StaticInitializationVectorQuery +import semmle.code.java.security.StaticInitializationVectorQuery import DataFlow::PathGraph from DataFlow::PathNode source, DataFlow::PathNode sink, StaticInitializationVectorConfig conf 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 100% 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 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 { From c0a1300955481ac9262cc0df6cc087c7e783fcfb Mon Sep 17 00:00:00 2001 From: Joe Farebrother Date: Thu, 14 Jul 2022 17:22:54 +0100 Subject: [PATCH 02/10] Improve initializedWthConstants to no longer need a workaround --- .../security/StaticInitializationVectorQuery.qll | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/java/ql/lib/semmle/code/java/security/StaticInitializationVectorQuery.qll b/java/ql/lib/semmle/code/java/security/StaticInitializationVectorQuery.qll index 5d88d620c21..73c2a4bc389 100644 --- a/java/ql/lib/semmle/code/java/security/StaticInitializationVectorQuery.qll +++ b/java/ql/lib/semmle/code/java/security/StaticInitializationVectorQuery.qll @@ -9,15 +9,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) ) } From 4d0957711b060398708818feda6e59eccade7e45 Mon Sep 17 00:00:00 2001 From: Joe Farebrother Date: Tue, 19 Jul 2022 11:22:33 +0100 Subject: [PATCH 03/10] Reduce FPs from empty arrays --- .../code/java/security/StaticInitializationVectorQuery.qll | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/java/ql/lib/semmle/code/java/security/StaticInitializationVectorQuery.qll b/java/ql/lib/semmle/code/java/security/StaticInitializationVectorQuery.qll index 73c2a4bc389..ce8db741336 100644 --- a/java/ql/lib/semmle/code/java/security/StaticInitializationVectorQuery.qll +++ b/java/ql/lib/semmle/code/java/security/StaticInitializationVectorQuery.qll @@ -84,7 +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), _)) + 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()) + ) ) } } From c152a27a68668ac0961616fc98c789f719d59b3f Mon Sep 17 00:00:00 2001 From: Joe Farebrother Date: Tue, 19 Jul 2022 12:09:50 +0100 Subject: [PATCH 04/10] Reword docs --- .../CWE/CWE-1204/StaticInitializationVector.qhelp | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/java/ql/src/Security/CWE/CWE-1204/StaticInitializationVector.qhelp b/java/ql/src/Security/CWE/CWE-1204/StaticInitializationVector.qhelp index d631dff22af..acb3caf2403 100644 --- a/java/ql/src/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.

From 960a4e58a0af8a304558b496c3603c0507d25601 Mon Sep 17 00:00:00 2001 From: Joe Farebrother Date: Tue, 19 Jul 2022 13:08:02 +0100 Subject: [PATCH 05/10] Add change note --- .../change-notes/2022-07-19-static-initialization-vector.md | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 java/ql/src/change-notes/2022-07-19-static-initialization-vector.md 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..5b84368362b --- /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 From a62bb8e11594ce2974221017c13dc080583f5470 Mon Sep 17 00:00:00 2001 From: Joe Farebrother Date: Tue, 19 Jul 2022 13:47:14 +0100 Subject: [PATCH 06/10] Add additional test case --- .../CWE-1204/StaticInitializationVector.java | 22 +++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/java/ql/test/query-tests/security/CWE-1204/StaticInitializationVector.java b/java/ql/test/query-tests/security/CWE-1204/StaticInitializationVector.java index f964d17239b..dca6eb261ca 100644 --- a/java/ql/test/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(); + } } From bf32b5a8fd58e1447cb9686eaff849d5ee358dee Mon Sep 17 00:00:00 2001 From: Joe Farebrother Date: Tue, 19 Jul 2022 14:50:12 +0100 Subject: [PATCH 07/10] Reiview suggestions - add doc comment, reword description, simplify a part --- .../java/security/StaticInitializationVectorQuery.qll | 6 +++--- .../Security/CWE/CWE-1204/StaticInitializationVector.ql | 9 ++------- 2 files changed, 5 insertions(+), 10 deletions(-) diff --git a/java/ql/lib/semmle/code/java/security/StaticInitializationVectorQuery.qll b/java/ql/lib/semmle/code/java/security/StaticInitializationVectorQuery.qll index ce8db741336..bea53097e83 100644 --- a/java/ql/lib/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 @@ -73,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() } } /** diff --git a/java/ql/src/Security/CWE/CWE-1204/StaticInitializationVector.ql b/java/ql/src/Security/CWE/CWE-1204/StaticInitializationVector.ql index 24e24b0f41c..3a30d670e08 100644 --- a/java/ql/src/Security/CWE/CWE-1204/StaticInitializationVector.ql +++ b/java/ql/src/Security/CWE/CWE-1204/StaticInitializationVector.ql @@ -1,12 +1,7 @@ /** * @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. + * @description An initialization vector (IV) used for ciphers of certain modes (such as CBC or GCM) should be unique and unpredicateble. + * Otherwise, if the same IV is used with a the same secret key then the same plaintext results in same ciphertext, which weakens the encryption. * @kind path-problem * @problem.severity warning * @precision high From 5afc0b0c15f81c280e5d04d4ec4da1e61df393c7 Mon Sep 17 00:00:00 2001 From: Joe Farebrother Date: Tue, 19 Jul 2022 15:17:53 +0100 Subject: [PATCH 08/10] Add security severity --- java/ql/src/Security/CWE/CWE-1204/StaticInitializationVector.ql | 1 + 1 file changed, 1 insertion(+) diff --git a/java/ql/src/Security/CWE/CWE-1204/StaticInitializationVector.ql b/java/ql/src/Security/CWE/CWE-1204/StaticInitializationVector.ql index 3a30d670e08..d01a820abff 100644 --- a/java/ql/src/Security/CWE/CWE-1204/StaticInitializationVector.ql +++ b/java/ql/src/Security/CWE/CWE-1204/StaticInitializationVector.ql @@ -4,6 +4,7 @@ * Otherwise, if the same IV is used with a the same secret key then the same plaintext results in same ciphertext, which weakens the encryption. * @kind path-problem * @problem.severity warning + * @security-severity 7.5 * @precision high * @id java/static-initialization-vector * @tags security From 7989ba33914fd6ecafc9d925cc256dd233340039 Mon Sep 17 00:00:00 2001 From: Joe Farebrother Date: Fri, 22 Jul 2022 11:04:20 +0100 Subject: [PATCH 09/10] Replace a tainttracking instance with local flow --- .../StaticInitializationVectorQuery.qll | 29 +++---------------- 1 file changed, 4 insertions(+), 25 deletions(-) diff --git a/java/ql/lib/semmle/code/java/security/StaticInitializationVectorQuery.qll b/java/ql/lib/semmle/code/java/security/StaticInitializationVectorQuery.qll index bea53097e83..825a50dd63c 100644 --- a/java/ql/lib/semmle/code/java/security/StaticInitializationVectorQuery.qll +++ b/java/ql/lib/semmle/code/java/security/StaticInitializationVectorQuery.qll @@ -94,40 +94,19 @@ private class StaticInitializationVectorSource extends DataFlow::Node { } } -/** - * 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() - ) - } -} - /** * 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() | + 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() ) } } From 7c188a6b962db77f780aba33dcf220fb85dfe096 Mon Sep 17 00:00:00 2001 From: Joe Farebrother Date: Tue, 16 Aug 2022 11:26:18 +0100 Subject: [PATCH 10/10] Apply doc suggestions --- .../src/Security/CWE/CWE-1204/StaticInitializationVector.qhelp | 2 +- .../ql/src/Security/CWE/CWE-1204/StaticInitializationVector.ql | 3 +-- .../change-notes/2022-07-19-static-initialization-vector.md | 2 +- 3 files changed, 3 insertions(+), 4 deletions(-) diff --git a/java/ql/src/Security/CWE/CWE-1204/StaticInitializationVector.qhelp b/java/ql/src/Security/CWE/CWE-1204/StaticInitializationVector.qhelp index acb3caf2403..b3b17ea176a 100644 --- a/java/ql/src/Security/CWE/CWE-1204/StaticInitializationVector.qhelp +++ b/java/ql/src/Security/CWE/CWE-1204/StaticInitializationVector.qhelp @@ -18,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 index d01a820abff..669c4e6f946 100644 --- a/java/ql/src/Security/CWE/CWE-1204/StaticInitializationVector.ql +++ b/java/ql/src/Security/CWE/CWE-1204/StaticInitializationVector.ql @@ -1,7 +1,6 @@ /** * @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 unpredicateble. - * Otherwise, if the same IV is used with a the same secret key then the same plaintext results in same ciphertext, which weakens the 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 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 index 5b84368362b..011aa4d8c18 100644 --- 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 @@ -1,4 +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 +* 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