SONARJAVA-4895: Fix S3329 FPs when random IV is generated in separate function #5325

anton-haubner-sonarsource · 2025-10-20T15:13:39Z

This implementation potentially slightly deviates from the ticket requirements:
In the case where we can not trace a safe IV generating method because it is not defined in the same class, or because it uses too deeply nested calls, the FP will still be raised.
I placed a comment in the implementation that explains when this happens.
By contrast, the ticket description demands

If it is not possible to follow the flow for the IV generation, no issue should be raised.

However, if we apply this, we will lose too many TPs that are covered by the current implementation.

The reproducer from the ticket and many adjacent cases are still covered by the changes made in this PR.
If we want to improve detection beyond this point, I would recommend switching to an analysis engine that supports cross-procedural/cross-file dataflow (i.e. DBD, or maybe a taint analysis since this is a security issue.).

Reviewing commit-by-commit is strongly recommended.

… IV to applicable to isolated byte vectors This will be needed to handle the initialization of IV byte arrays in separate methods.

…oss re-assignments

…byte arrays into separate class This will allow us to extend the logic in follow up commits so that user-defined safe factory methods are taken into account.

anton-haubner-sonarsource · 2025-10-22T12:03:44Z

java-checks/src/main/java/org/sonar/java/checks/security/CipherBlockChainingCheck.java

-      if (!mitVisit.secureRandomFound) {
-        reportIssue(newClassTree, "Use a dynamically-generated, random IV.");
-      }
+    var mTree = ExpressionUtils.getEnclosingMethod(newClassTree);


This commit does not change any functionality, yet
It only adds the ability to check for the secure initialization of arbitrary expression trees (we will need in a later commit).
(Also, I did some renaming, to make the meaning of some classes/fields a little bit clearer)

anton-haubner-sonarsource · 2025-10-22T12:05:53Z

java-checks/src/main/java/org/sonar/java/checks/security/CipherBlockChainingCheck.java


-  private static boolean isSecureRandomGenerateSeed(@Nullable ExpressionTree tree) {
-    return tree != null && tree.is(Tree.Kind.METHOD_INVOCATION) && SECURE_RANDOM_GENERATE_SEED.matches((MethodInvocationTree) tree);
+  private static Stream<MethodInvocationTree> findConstructingMethods(ExpressionTree expressionTree) {


isDynamicallyGenerated contained some logic to find any methods that have been used to construct a given identifier.
This commit factors this logic out into a separate method, as we will need it in a later commit.

Otherwise, no functionality is yet changed by this commit.

… IV byte arrays

…ray generator detection

…S3329 violations

dorian-burihabwa-sonarsource

Thanks for documenting the cases we do and do not cover clearly that will help when sorting out future feedback on the rule. There is a single case where I am not sure if it should explicitly be marked as a FP.

In addition, if we can find a way to join the finder and detector in a way that avoids having them cycle in their relationship that would help with future maintenance.

dorian-burihabwa-sonarsource · 2025-10-22T12:57:27Z

...les/non-compiling/checks/security/CipherBlockChainingCheckShouldDetectCustomIVFactories.java

So this file contains 1 TPs and 2 FPs

Can you clarify the concern / the action?
I.e. should I make it more clear in the comments that this test file is about edge cases for non-compiling code, causing possible FPs?

dorian-burihabwa-sonarsource · 2025-10-22T13:38:33Z

...ult/src/main/java/checks/security/CipherBlockChainingCheckShouldDetectCustomIVFactories.java

+      final byte[] iv = initIvWithByteBuffer(12);
+      cipher.init(ENCRYPT_MODE, secretKey, new IvParameterSpec(iv)); // Compliant


This is FN, isn't it? If so, we should mark it the same way we marked the FPs below

Not necessarily. It is unclear here, how the byte buffer is initialized and as far as I can tell, the previous implementation explicitly does not raise when a byte buffer is used for initialization (perhaps to avoid FPs?).
I.e. I tried to adhere to the existing behaviour here, and I would say we can neither say its necessarily a FN or TN because the byte buffer contents are unknown.

I can add comment about this, though.

dorian-burihabwa-sonarsource · 2025-10-23T08:10:57Z

java-checks/src/main/java/org/sonar/java/checks/security/CipherBlockChainingCheck.java

  }
+
+  private static class SecureByteArrayGeneratorDetector {
+    private @Nullable SecureByteArrayFactoryFinder secureByteArrayFactoryFinder = null;


There seems to be a bi-directonial connection between the finder and the detector. Is there a way we can simplify this?

The bi-directional connection is somewhat intentional, and an artifact of alternating between method-local analysis and cross-method analysis. I.e.

SecureByteArrayGeneratorDetector detects local secure dynamic initialization, and refers to SecureByteArrayFactoryFinder when it needs to go cross-method. It also implicitly decides when to stop the cycle (i.e. at call depth 1 - when the reference to the finder is null).

SecureByteArrayFactoryFinder detects which methods are insecure by invoking SecureByteArrayGeneratorDetector to analyze their local context.

So indeed, the relation is cyclic, and the cycle can be resolved by joining the components. At the same time, their current implementation separates their responsibilities, and the cycle only exists between private nested classes.

Still, if you would strongly prefer to avoid this cycle, I'll try to spend some more time refactoring this.

I introduced some interfaces that abstract from the direct cyclic relation, and that make the dependency between the components more clear, and where we cut the cycle:
23474de

At the same time, I would like to keep the two components and their responsibilities separated, so I decided against joining them.

Please let me know what you think about this tradeoff.

…n intra- and cross-procedural components

sonarqube-next · 2025-10-24T15:18:19Z

Quality Gate passed

Issues
0 New issues
0 Fixed issues
0 Accepted issues

Measures
0 Security Hotspots
0 Dependency risks
91.7% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube

dorian-burihabwa-sonarsource

Thanks for taking a second look at this. The setup is more complex but I think we gained overall with cleaner method and object names. I think we can merge as-is

dorian-burihabwa-sonarsource · 2025-10-24T17:19:54Z

java-checks/src/main/java/org/sonar/java/checks/security/CipherBlockChainingCheck.java

-      this.secureByteArrayFactoryFinder = secureByteArrayFactoryFinder;
-      return this;
+    static IvFactoryFinder disabled() {
+      return methodInvocation -> false;


I got a little confused by the notation here but it should work OK

anton-haubner-sonarsource force-pushed the SONARJAVA-4895 branch 3 times, most recently from bd1715e to 4a121f6 Compare October 22, 2025 11:50

anton-haubner-sonarsource added 7 commits October 22, 2025 13:58

SONARJAVA-4895: Add reproducer

f4dd058

SONARJAVA-4895: Add control case to new test suite file

7e3990c

SONARJAVA-4895: Add tests for basic IV generating methods

e9751c7

SONARJAVA-4895: Add tests for non-compiling cases

56d4071

SONARJAVA-4895: Prepare visitor for checking secure initialization of…

67e3bd4

… IV to applicable to isolated byte vectors This will be needed to handle the initialization of IV byte arrays in separate methods.

SONARJAVA-4895: Factor out logic to find constructing invocations acr…

8ba2de4

…oss re-assignments

SONARJAVA-4895: Factor out logic for detecting dynamically generated …

2b8782e

…byte arrays into separate class This will allow us to extend the logic in follow up commits so that user-defined safe factory methods are taken into account.

anton-haubner-sonarsource force-pushed the SONARJAVA-4895 branch from 4a121f6 to dafb0d2 Compare October 22, 2025 11:59

anton-haubner-sonarsource commented Oct 22, 2025

View reviewed changes

anton-haubner-sonarsource added 3 commits October 22, 2025 14:09

SONARJAVA-4895: Add visitor for finding methods that securely produce…

f05d60f

… IV byte arrays

SONARJAVA-4895: Integrate custom IV byte array factories into byte ar…

ac186b8

…ray generator detection

SONARJAVA-4895: Find secure IV byte array factories before detecting …

92a9f13

…S3329 violations

anton-haubner-sonarsource force-pushed the SONARJAVA-4895 branch from dafb0d2 to 92a9f13 Compare October 22, 2025 12:09

anton-haubner-sonarsource marked this pull request as ready for review October 22, 2025 12:10

anton-haubner-sonarsource requested a review from dorian-burihabwa-sonarsource October 22, 2025 12:29

dorian-burihabwa-sonarsource requested changes Oct 23, 2025

View reviewed changes

anton-haubner-sonarsource added 2 commits October 24, 2025 13:41

SONARJAVA-4895: Review feedback: Clarify ByteBuffer case

59616a0

SONARJAVA-4895: Review feedback: Abstract from cyclic relation betwee…

23474de

…n intra- and cross-procedural components

anton-haubner-sonarsource requested a review from dorian-burihabwa-sonarsource October 24, 2025 15:21

dorian-burihabwa-sonarsource approved these changes Oct 24, 2025

View reviewed changes

anton-haubner-sonarsource merged commit 1c24c62 into master Oct 27, 2025
18 checks passed

anton-haubner-sonarsource deleted the SONARJAVA-4895 branch October 27, 2025 08:06

		final byte[] iv = initIvWithByteBuffer(12);
		cipher.init(ENCRYPT_MODE, secretKey, new IvParameterSpec(iv)); // Compliant

SONARJAVA-4895: Fix S3329 FPs when random IV is generated in separate function #5325

SONARJAVA-4895: Fix S3329 FPs when random IV is generated in separate function #5325

Uh oh!

Conversation

anton-haubner-sonarsource commented Oct 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

anton-haubner-sonarsource Oct 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dorian-burihabwa-sonarsource left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anton-haubner-sonarsource Oct 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sonarqube-next bot commented Oct 24, 2025

Quality Gate passed

Uh oh!

dorian-burihabwa-sonarsource left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

anton-haubner-sonarsource commented Oct 20, 2025 •

edited

Loading

anton-haubner-sonarsource Oct 22, 2025 •

edited

Loading

anton-haubner-sonarsource Oct 24, 2025 •

edited

Loading