Java: Static initialization vector #6357

artem-smotrakov · 2021-07-23T06:42:23Z

When a cipher is used in certain modes, it needs an initialization vector (IV). 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.

I'd like to add a query detecting static IVs:

The query looks for data flows from static byte array initialization to Cipher.init() calls.
Only ciphers initialized for encryption are flagged.
The query focuses only static initialization vectors. Therefore it skips cases when the IV is modified. For that reason, the query ignores cases when the IV is filled out by Random instead of SecureRandom.

At first, the query used to be much simpler. Then, I tested it on ~1000 open source projects which uncovered several FPs. Fixing those FPs made the query a bit more complex.

The query finds the following vulnerabilities:

CVE-2020-5408 in Spring Security. Please note that the issues has not been patched. They only deprecated the vulnerable methods. Therefore, the query detects the issue on the latest version of Spring Security.
OFBIZ-12281 in Apache OFBiz
PDFBOX-4191 in Apache PDFBox

- Added StaticInitializationVector.ql - Added StaticInitializationVector.qhelp - Added tests

Marcono1234

Hopefully the following review comments are useful.
The maintainers can probably provide a more in-depth review.

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

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

java/ql/src/experimental/semmle/code/java/security/StaticInitializationVectorQuery.qll

Marcono1234 · 2021-07-25T14:19:13Z

java/ql/src/experimental/semmle/code/java/security/StaticInitializationVectorQuery.qll

+ */
+private class StaticByteArrayCreation extends ArrayCreationExpr {
+  StaticByteArrayCreation() {
+    this.getType().(Array).getElementType().(PrimitiveType).getName() = "byte" and


Might be better to use getComponentType() instead of getElementType(), because the above would also hold for byte[][][] while with getComponentType() it would only hold for byte[].

This expression doesn't check the dimension. Both methods work fine here, I don't really see why getComponentType() might be better here. The docs for getElementType() says:

Gets the type of the elements used to construct this array type.

For example, the element type of Object[][] is Object.

That's exactly what is necessary here.

But will a multi-dimensional byte array (e.g. byte[][][]), or one of its sub arrays, ever be used as initialization vector (and would the other parts of this query even handle that correctly)?

Because StaticByteArrayCreation is currently matching that, e.g. new byte[5][].

I think Marcono is right, you really do only want byte[] here

I didn't consider multi-dimensional arrays. I think it may make sense to support them. For example, an application can use a set of static IVs like this:

byte[][] ivs = new byte[][] { new byte[] { 1, 2, 3 ... }, new byte[] { 4, 5, 6, ...} ... } ... GCMParameterSpec ivSpec = new GCMParameterSpec(128, ivs[i]);

I added test cases encryptWithOneOfStaticIvs01, encryptWithOneOfStaticIvs02 and encryptWithOneOfStaticZeroIvs that use multi-dimensional arrays, and updated the initializedWithConstants() predicate.

array.getInit().getAnInit().getAChildExpr() instanceof IntegerLiteral covers the following case:

byte[][] ivs = new byte[][] { new byte[8], new byte[16] };

This expression looks weird to me, but I could not find a better way to cover the case above. While debugging this case, I saw the child elements of the ArrayInits are instances of IntegerLiteral and TypeAccess. Please let me know if there is a better way.

StaticByteArrayCreation still uses getElementType() because it wants to check only the type but not dimensions. If we decide to cover only one-dimension arrays, then we can use getElementType(). I would prefer to support multi-dimensional arrays though.

While debugging this case, I saw the child elements of the ArrayInits are instances of IntegerLiteral and TypeAccess.

The VSCode extension AST viewer shows that for me as well, and manually quering ArrayCreationExpr and ArrayInit seems to confirm that.
To me that looks like a bug in the extractor, that ArrayCreationExpr inside an ArrayInit is modelled as (malformed) ArrayInit. Might be worth creating an issue for this?

To me that looks like a bug in the extractor, that ArrayCreationExpr inside an ArrayInit is modelled as (malformed) ArrayInit. Might be worth creating an issue for this?

Yes, I also expected to see ArrayCreationExpr inside. @smowton Does it look like a bug? Or, are we missing something?

Looks like a bug

Created #6552 (comment)

Marcono1234 · 2021-07-25T14:22:59Z

java/ql/src/experimental/semmle/code/java/security/StaticInitializationVectorQuery.qll

+    )
+    or
+    exists(StaticMethodAccess ma |
+      ma.getMethod().hasQualifiedName("java.util", "Arrays", "copyOf") and


This is not really updating an existing array, instead it creates a copy of it. Might be good to add a comment here explaining the rationale since otherwise the class name ArrayUpdate is rather misleading.

If you want to cover copying an existing array then this might also have to consider Arrays.copyOfRange and calls to clone() of an array type.

Also interested in the answer to this

This is not really updating an existing array, instead it creates a copy of it. Might be good to add a comment here explaining the rationale since otherwise the class name ArrayUpdate is rather misleading.

The class ArrayUpdate covers two cases:

when content of the array is updated, for example, by System.arraycopy()

when the array reference is updated, for example, by Arrays.copyOf()

I can update the qldoc for the class with the above. I can also split the class to two -- one class for each case.

I agree that ArrayUpdate may look misleading. Unfortunately, I didn't find a better name. Do you have a better name?

If you want to cover copying an existing array then this might also have to consider Arrays.copyOfRange and calls to clone() of an array type.

Sure, I can cover these methods as well.

I don't think you need to model any of these functions: they naturally interrupt dataflow from a static array to its usage by overwriting the variable or field the reference is stored in.

If a dataflow path is found anyhow then most likely there is a control-flow route around the assignment and it's genuinely possible for the static array to be used as an IV.

I don't think you need to model any of these functions: they naturally interrupt dataflow from a static array to its usage by overwriting the variable or field the reference is stored in.

If I remember correctly, while testing the query, these functions didn't interrupt dataflow. That sometimes resulted in some false-positives. But I might be missing something. I did a quick test and looks like you're right. I'll remove these functions.

java/ql/src/experimental/semmle/code/java/security/StaticInitializationVectorQuery.qll

smowton · 2021-08-10T07:45:11Z

java/ql/src/experimental/semmle/code/java/security/StaticInitializationVectorQuery.qll

+ */
+private class StaticByteArrayCreation extends ArrayCreationExpr {
+  StaticByteArrayCreation() {
+    this.getType().(Array).getElementType().(PrimitiveType).getName() = "byte" and


I think Marcono is right, you really do only want byte[] here

java/ql/src/experimental/semmle/code/java/security/StaticInitializationVectorQuery.qll

smowton · 2021-08-10T07:51:22Z

java/ql/src/experimental/semmle/code/java/security/StaticInitializationVectorQuery.qll

+    )
+    or
+    exists(StaticMethodAccess ma |
+      ma.getMethod().hasQualifiedName("java.util", "Arrays", "copyOf") and


Also interested in the answer to this

smowton · 2021-08-10T07:53:40Z

java/ql/src/experimental/semmle/code/java/security/StaticInitializationVectorQuery.qll

+  }
+
+  override predicate isSanitizer(DataFlow::Node node) {
+    exists(ArrayUpdate update | update.getArray() = node.asExpr())


Considering the existing side-condition on your source that it never flows to an array update, why this sanitizer? Can you construct any case where both sanitizer and side-condition are required? If they're redundant, it's best to drop the side-condition and just use the sanitizer.

The side-condition helps to get rid of some false-positives. For example, see encryptWithRandomIvByteByByte() test. Unfortunately, the sanitizer doesn't help here. But you're right, the sanitizer is redundant. I'll remove it.

java/ql/src/experimental/semmle/code/java/security/StaticInitializationVectorQuery.qll

artem-smotrakov · 2021-08-25T10:42:04Z

@Marcono1234 @smowton Thanks for the review. I've addressed your comments. I'm going to file a bug for this soon.

java/ql/src/experimental/semmle/code/java/security/StaticInitializationVectorQuery.qll

Co-authored-by: Chris Smowton <[email protected]>

artem-smotrakov added 2 commits July 16, 2021 19:06

Added a query for static initialization vectors in encryption

218731c

- Added StaticInitializationVector.ql - Added StaticInitializationVector.qhelp - Added tests

Use inline-expectation tests for StaticInitializationVector.ql

cfe74b5

artem-smotrakov requested a review from a team as a code owner July 23, 2021 06:42

github-actions bot added documentation Java labels Jul 23, 2021

artem-smotrakov mentioned this pull request Jul 23, 2021

Java: Static initialization vector github/securitylab#411

Closed

1 task

Marcono1234 reviewed Jul 25, 2021

View reviewed changes

Fixed a few typos

df0f9ee

smowton reviewed Aug 10, 2021

View reviewed changes

artem-smotrakov added 4 commits August 13, 2021 20:52

Support multi-dimensional arrays

4e69081

Be precise when checking for Cipher.ENCRYPT_MODE

1199240

Updated qldoc for ArrayUpdate

d218813

Covered copyOfRange() and clone() in ArrayUpdate

e2dc975

Marcono1234 reviewed Aug 14, 2021

View reviewed changes

java/ql/src/experimental/semmle/code/java/security/StaticInitializationVectorQuery.qll Outdated Show resolved Hide resolved

artem-smotrakov added 4 commits August 14, 2021 21:28

Fixed a typo in qldoc

fbac589

Don't consider copyOf() and clone() in ArrayUpdate

c80a1da

Updated qldoc for ArrayUpdate

86b7b2b

Removed sanitizer in StaticInitializationVectorConfig

f97c8bb

artem-smotrakov mentioned this pull request Aug 25, 2021

Java: More details about nested arrays #6552

Closed

smowton reviewed Aug 25, 2021

View reviewed changes

artem-smotrakov and others added 3 commits August 25, 2021 19:38

Better qldoc in StaticInitializationVectorQuery.qll

f41828e

Co-authored-by: Chris Smowton <[email protected]>

Simplify ArrayUpdate

23e2322

Co-authored-by: Chris Smowton <[email protected]>

Simplify StaticInitializationVectorSource

1dd4bf0

Co-authored-by: Chris Smowton <[email protected]>

artem-smotrakov force-pushed the static-iv branch from be4773e to 1dd4bf0 Compare August 26, 2021 07:42

smowton approved these changes Aug 26, 2021

View reviewed changes

smowton merged commit 7a0555e into github:main Aug 26, 2021

artem-smotrakov deleted the static-iv branch August 26, 2021 13:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Java: Static initialization vector #6357

Java: Static initialization vector #6357

artem-smotrakov commented Jul 23, 2021

Marcono1234 left a comment

Marcono1234 Jul 25, 2021

artem-smotrakov Aug 8, 2021

Marcono1234 Aug 8, 2021

smowton Aug 10, 2021

artem-smotrakov Aug 13, 2021

Marcono1234 Aug 13, 2021

artem-smotrakov Aug 13, 2021

smowton Aug 16, 2021

artem-smotrakov Aug 25, 2021

Marcono1234 Jul 25, 2021

smowton Aug 10, 2021

artem-smotrakov Aug 14, 2021

smowton Aug 16, 2021

artem-smotrakov Aug 25, 2021

smowton Aug 10, 2021

smowton Aug 10, 2021

smowton Aug 10, 2021

artem-smotrakov Aug 25, 2021

artem-smotrakov commented Aug 25, 2021

Java: Static initialization vector #6357

Java: Static initialization vector #6357

Conversation

artem-smotrakov commented Jul 23, 2021

Marcono1234 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

artem-smotrakov commented Aug 25, 2021