From f0f3e0cf22d04d91ba239c0bb38105ccbfa6a7ac Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicklas=20K=C3=B6rtge?= Date: Mon, 31 Aug 2026 13:37:51 +0200 Subject: [PATCH] fix: stop endless recursion in resolveValues on assignment cycles Fixes #525. resolveValues follows a variable's assignments and its initializer to find its value. It had no cycle check. Code like String algorithm = "AES"; String copy = algorithm; ... algorithm = copy; made it recurse forever and crash the scan with a StackOverflowError. The existing selections.size() > 15 guard never fires here, because identifier-to-identifier hops do not grow the selections list. The bug is old, but 1.6.1 exposed it: since e1fdab3b the engine resolves the arguments of every method call (for detached call records), not only the arguments of matched crypto calls. So a cycle anywhere in the scanned code now reaches resolveValues. The fix moves the variable branch into resolveVariableValues and tracks which variables are being resolved on the current path. A variable that is already on the path is not followed again. The variable is released again when its resolution is done, so two sibling branches may still resolve through the same variable. A new red-green test reproduces the exact alternating stack trace from the issue and checks that the value still resolves through the cycle to the constant initializer. --- .../language/java/JavaDetectionEngine.java | 153 ++++++++++++------ ...erGetInstanceCyclicAssignmentTestFile.java | 17 ++ ...CipherGetInstanceCyclicAssignmentTest.java | 78 +++++++++ 3 files changed, 202 insertions(+), 46 deletions(-) create mode 100644 java/src/test/files/rules/detection/jca/cipher/JcaCipherGetInstanceCyclicAssignmentTestFile.java create mode 100644 java/src/test/java/com/ibm/plugin/rules/detection/jca/cipher/JcaCipherGetInstanceCyclicAssignmentTest.java diff --git a/engine/src/main/java/com/ibm/engine/language/java/JavaDetectionEngine.java b/engine/src/main/java/com/ibm/engine/language/java/JavaDetectionEngine.java index 99e800e46..6a7cfdaca 100644 --- a/engine/src/main/java/com/ibm/engine/language/java/JavaDetectionEngine.java +++ b/engine/src/main/java/com/ibm/engine/language/java/JavaDetectionEngine.java @@ -48,10 +48,12 @@ import com.ibm.engine.rule.Parameter; import java.util.ArrayList; import java.util.Collections; +import java.util.HashSet; import java.util.LinkedList; import java.util.List; import java.util.Objects; import java.util.Optional; +import java.util.Set; import java.util.stream.Collectors; import javax.annotation.Nonnull; import javax.annotation.Nullable; @@ -301,7 +303,8 @@ public List> resolveValuesInInnerScope( @Nonnull Tree expression, @Nullable IValueFactory valueFactory) { if (expression instanceof ExpressionTree expressionTree) { - return resolveValues(clazz, expressionTree, valueFactory, new LinkedList<>()); + return resolveValues( + clazz, expressionTree, valueFactory, new LinkedList<>(), new HashSet<>()); } return Collections.emptyList(); } @@ -312,49 +315,16 @@ private List> resolveValues( @Nonnull Class clazz, @Nonnull ExpressionTree tree, @Nullable IValueFactory valueFactory, - @Nonnull LinkedList selections) { + @Nonnull LinkedList selections, + @Nonnull Set resolvingVariables) { if (selections.size() > 15) { return Collections.emptyList(); } else if (tree.is(Tree.Kind.IDENTIFIER)) { IdentifierTree identifierTree = (IdentifierTree) tree; if (identifierTree.symbol().isVariableSymbol()) { // variable - VariableTree variableTree = (VariableTree) identifierTree.symbol().declaration(); - if (variableTree != null) { - LinkedList> result = new LinkedList<>(); - - List usages = new ArrayList<>(variableTree.symbol().usages()); - usages.remove(identifierTree); - // not only initialization, also other declarations - if (!usages.isEmpty()) { - for (IdentifierTree usage : usages) { - Tree parent = usage.parent(); - if (parent != null && parent.is(Tree.Kind.ASSIGNMENT)) { - AssignmentExpressionTree assignment = - (AssignmentExpressionTree) parent; - if (assignment.expression() != usage) { - result.addAll( - resolveValues( - clazz, - assignment.expression(), - valueFactory, - selections)); - } - } - } - } - - ExpressionTree initializer = variableTree.initializer(); - if (initializer != null) { - Optional value = resolveConstant(clazz, initializer); - if (value.isPresent()) { - result.addFirst(new ResolvedValue<>(value.get(), initializer)); - } else { - return resolveValues(clazz, initializer, valueFactory, selections); - } - } - return result; - } + return resolveVariableValues( + clazz, identifierTree, valueFactory, selections, resolvingVariables); } else if (identifierTree.symbol().isEnum()) { ClassTree enumClassTree = (ClassTree) identifierTree.symbol().declaration(); if (enumClassTree != null && !selections.isEmpty()) { @@ -385,17 +355,30 @@ private List> resolveValues( (MemberSelectExpressionTree) tree; selections.addFirst(memberSelectExpressionTree); return resolveValues( - clazz, memberSelectExpressionTree.expression(), valueFactory, selections); + clazz, + memberSelectExpressionTree.expression(), + valueFactory, + selections, + resolvingVariables); } return List.of(new ResolvedValue<>(value.get(), tree)); } else if (tree.is(Tree.Kind.METHOD_INVOCATION)) { MethodInvocationTree methodInvocationTree = (MethodInvocationTree) tree; selections.addFirst(methodInvocationTree); final List> resolvedValues = - resolveJavaProperties(clazz, methodInvocationTree, valueFactory, selections); + resolveJavaProperties( + clazz, + methodInvocationTree, + valueFactory, + selections, + resolvingVariables); if (resolvedValues.isEmpty()) { return resolveValues( - clazz, methodInvocationTree.methodSelect(), valueFactory, selections); + clazz, + methodInvocationTree.methodSelect(), + valueFactory, + selections, + resolvingVariables); } else { return resolvedValues; } @@ -409,7 +392,12 @@ private List> resolveValues( ArrayDimensionTree dimensionTree = dimensionTrees.get(0); ExpressionTree dimensionDefinition = dimensionTree.expression(); if (dimensionDefinition != null) { - return resolveValues(clazz, dimensionDefinition, valueFactory, selections); + return resolveValues( + clazz, + dimensionDefinition, + valueFactory, + selections, + resolvingVariables); } } else if (dimensionTrees.size() > 1) { LOGGER.info( @@ -419,7 +407,13 @@ private List> resolveValues( ListTree initializers = newArrayTree.initializers(); final List> values = new ArrayList<>(); for (ExpressionTree initializer : initializers) { - values.addAll(resolveValues(clazz, initializer, valueFactory, selections)); + values.addAll( + resolveValues( + clazz, + initializer, + valueFactory, + selections, + resolvingVariables)); } return values; } @@ -428,7 +422,8 @@ private List> resolveValues( selections.addFirst(newClassTree); if (newClassTree.arguments().size() == 1) { ExpressionTree expressionTree = newClassTree.arguments().get(0); - return resolveValues(clazz, expressionTree, valueFactory, selections); + return resolveValues( + clazz, expressionTree, valueFactory, selections, resolvingVariables); } else if (newClassTree.arguments().size() > 1) { LOGGER.info( "Detected constructor definition has more then one argument to resolve. Redefine the rule to explicitly define the param to resolve"); @@ -442,11 +437,73 @@ private List> resolveValues( return Collections.emptyList(); } + /** + * Resolves the possible values of a variable by following the assignments to it and its + * initializer. The assignment graph can contain cycles (e.g. {@code String a = b;} together + * with {@code b = a;}): a variable that is already being resolved further up the call chain is + * not followed again, since it cannot contribute a new value and following it would recurse + * forever (issue #525). + */ + @Nonnull + private List> resolveVariableValues( + @Nonnull Class clazz, + @Nonnull IdentifierTree identifierTree, + @Nullable IValueFactory valueFactory, + @Nonnull LinkedList selections, + @Nonnull Set resolvingVariables) { + final Symbol symbol = identifierTree.symbol(); + if (!resolvingVariables.add(symbol)) { + // cycle: this variable is already being resolved further up the call chain + return Collections.emptyList(); + } + try { + VariableTree variableTree = (VariableTree) symbol.declaration(); + if (variableTree == null) { + return Collections.emptyList(); + } + LinkedList> result = new LinkedList<>(); + + List usages = new ArrayList<>(variableTree.symbol().usages()); + usages.remove(identifierTree); + // not only initialization, also other declarations + for (IdentifierTree usage : usages) { + Tree parent = usage.parent(); + if (parent != null && parent.is(Tree.Kind.ASSIGNMENT)) { + AssignmentExpressionTree assignment = (AssignmentExpressionTree) parent; + if (assignment.expression() != usage) { + result.addAll( + resolveValues( + clazz, + assignment.expression(), + valueFactory, + selections, + resolvingVariables)); + } + } + } + + ExpressionTree initializer = variableTree.initializer(); + if (initializer != null) { + Optional value = resolveConstant(clazz, initializer); + if (value.isPresent()) { + result.addFirst(new ResolvedValue<>(value.get(), initializer)); + } else { + return resolveValues( + clazz, initializer, valueFactory, selections, resolvingVariables); + } + } + return result; + } finally { + resolvingVariables.remove(symbol); + } + } + private List> resolveJavaProperties( @Nonnull Class clazz, @Nonnull MethodInvocationTree methodInvocationTree, @Nullable IValueFactory valueFactory, - @Nonnull LinkedList selections) { + @Nonnull LinkedList selections, + @Nonnull Set resolvingVariables) { final MatchContext matchContext = new MatchContext(false, false, List.of()); final MethodMatcher javaPropertyWithDefaultValueMatcher = new MethodMatcher<>( @@ -460,7 +517,11 @@ private List> resolveJavaProperties( return Collections.emptyList(); } return resolveValues( - clazz, methodInvocationTree.arguments().get(1), valueFactory, selections); + clazz, + methodInvocationTree.arguments().get(1), + valueFactory, + selections, + resolvingVariables); } return Collections.emptyList(); } diff --git a/java/src/test/files/rules/detection/jca/cipher/JcaCipherGetInstanceCyclicAssignmentTestFile.java b/java/src/test/files/rules/detection/jca/cipher/JcaCipherGetInstanceCyclicAssignmentTestFile.java new file mode 100644 index 000000000..f31c3ebde --- /dev/null +++ b/java/src/test/files/rules/detection/jca/cipher/JcaCipherGetInstanceCyclicAssignmentTestFile.java @@ -0,0 +1,17 @@ +import javax.crypto.Cipher; + +public class JcaCipherGetInstanceCyclicAssignmentTestFile { + + private String algorithm = "AES/ECB/PKCS5Padding"; + private String copy = algorithm; + + public void swap() { + // Cycle in the assignment graph: resolving `copy` follows its initializer to + // `algorithm`, and resolving `algorithm` follows this assignment back to `copy`. + algorithm = copy; + } + + public void cipher() throws Exception { + Cipher c = Cipher.getInstance(copy); // Noncompliant {{(BlockCipher) AES-128-ECB-PKCS5}} + } +} diff --git a/java/src/test/java/com/ibm/plugin/rules/detection/jca/cipher/JcaCipherGetInstanceCyclicAssignmentTest.java b/java/src/test/java/com/ibm/plugin/rules/detection/jca/cipher/JcaCipherGetInstanceCyclicAssignmentTest.java new file mode 100644 index 000000000..3872faf17 --- /dev/null +++ b/java/src/test/java/com/ibm/plugin/rules/detection/jca/cipher/JcaCipherGetInstanceCyclicAssignmentTest.java @@ -0,0 +1,78 @@ +/* + * Sonar Cryptography Plugin + * Copyright (C) 2024 PQCA + * + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to you under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package com.ibm.plugin.rules.detection.jca.cipher; + +import static org.assertj.core.api.Assertions.assertThat; + +import com.ibm.engine.detection.DetectionStore; +import com.ibm.engine.model.Algorithm; +import com.ibm.engine.model.IValue; +import com.ibm.engine.model.context.CipherContext; +import com.ibm.mapper.model.BlockCipher; +import com.ibm.mapper.model.INode; +import com.ibm.plugin.TestBase; +import java.util.List; +import javax.annotation.Nonnull; +import org.junit.jupiter.api.Test; +import org.sonar.java.checks.verifier.CheckVerifier; +import org.sonar.plugins.java.api.JavaCheck; +import org.sonar.plugins.java.api.JavaFileScannerContext; +import org.sonar.plugins.java.api.semantic.Symbol; +import org.sonar.plugins.java.api.tree.Tree; + +/** + * Regression test for issue #525: a cycle in the assignment graph (a field initialized from + * another field, which is in turn assigned back from the first) must not send {@code + * resolveValues} into infinite recursion (StackOverflowError). The value must still resolve + * through the cycle to the constant initializer. + */ +class JcaCipherGetInstanceCyclicAssignmentTest extends TestBase { + + @Test + void test() { + CheckVerifier.newVerifier() + .onFile( + "src/test/files/rules/detection/jca/cipher/JcaCipherGetInstanceCyclicAssignmentTestFile.java") + .withChecks(this) + .verifyIssues(); + } + + @Override + public void asserts( + int findingId, + @Nonnull DetectionStore detectionStore, + @Nonnull List nodes) { + /* + * Detection Store + */ + assertThat(detectionStore.getDetectionValues()).hasSize(1); + IValue value = detectionStore.getDetectionValues().get(0); + assertThat(detectionStore.getDetectionValueContext()).isInstanceOf(CipherContext.class); + assertThat(value).isInstanceOf(Algorithm.class); + assertThat(value.asString()).isEqualTo("AES/ECB/PKCS5Padding"); + /* + * Translation + */ + assertThat(nodes).hasSize(1); + INode node = nodes.get(0); + assertThat(node).isInstanceOf(BlockCipher.class); + assertThat(node.asString()).isEqualTo("AES-128-ECB-PKCS5"); + } +}