From 726889bf893edf8008561858f27246d7644f53c1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicklas=20K=C3=B6rtge?= Date: Mon, 16 Sep 2024 09:00:11 +0200 Subject: [PATCH] Fix issue #16 (#141) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix #16 Signed-off-by: Nicklas Körtge * enable issue test Signed-off-by: Hugo Queinnec --------- Signed-off-by: Nicklas Körtge Signed-off-by: Hugo Queinnec Co-authored-by: Hugo Queinnec --- .../language/java/JavaDetectionEngine.java | 44 +++++- .../files/rules/issues/Issue16TestFile.java | 14 ++ .../NextParameterDependingRulesTestFile.java | 8 +- .../ibm/plugin/rules/issues/Issue16Test.java | 143 ++++++++++++++++++ .../NextParameterDependingRulesTest.java | 28 +--- 5 files changed, 201 insertions(+), 36 deletions(-) create mode 100644 java/src/test/files/rules/issues/Issue16TestFile.java create mode 100644 java/src/test/java/com/ibm/plugin/rules/issues/Issue16Test.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 673129bb..b55f55e5 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 @@ -19,7 +19,15 @@ */ package com.ibm.engine.language.java; -import com.ibm.engine.detection.*; +import com.ibm.engine.detection.DetectionStore; +import com.ibm.engine.detection.DetectionStoreWithHook; +import com.ibm.engine.detection.Handler; +import com.ibm.engine.detection.IDetectionEngine; +import com.ibm.engine.detection.MatchContext; +import com.ibm.engine.detection.MethodDetection; +import com.ibm.engine.detection.ResolvedValue; +import com.ibm.engine.detection.TraceSymbol; +import com.ibm.engine.detection.ValueDetection; import com.ibm.engine.hooks.EnumHook; import com.ibm.engine.hooks.MethodInvocationHookWithParameterResolvement; import com.ibm.engine.hooks.MethodInvocationHookWithReturnResolvement; @@ -29,7 +37,12 @@ import com.ibm.engine.rule.DetectionRule; import com.ibm.engine.rule.MethodDetectionRule; import com.ibm.engine.rule.Parameter; -import java.util.*; +import java.util.ArrayList; +import java.util.Collections; +import java.util.LinkedList; +import java.util.List; +import java.util.Objects; +import java.util.Optional; import java.util.stream.Collectors; import javax.annotation.Nonnull; import javax.annotation.Nullable; @@ -40,7 +53,24 @@ 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.*; +import org.sonar.plugins.java.api.tree.Arguments; +import org.sonar.plugins.java.api.tree.ArrayDimensionTree; +import org.sonar.plugins.java.api.tree.AssignmentExpressionTree; +import org.sonar.plugins.java.api.tree.BaseTreeVisitor; +import org.sonar.plugins.java.api.tree.ClassTree; +import org.sonar.plugins.java.api.tree.EnumConstantTree; +import org.sonar.plugins.java.api.tree.ExpressionTree; +import org.sonar.plugins.java.api.tree.IdentifierTree; +import org.sonar.plugins.java.api.tree.ListTree; +import org.sonar.plugins.java.api.tree.MemberSelectExpressionTree; +import org.sonar.plugins.java.api.tree.MethodInvocationTree; +import org.sonar.plugins.java.api.tree.MethodTree; +import org.sonar.plugins.java.api.tree.NewArrayTree; +import org.sonar.plugins.java.api.tree.NewClassTree; +import org.sonar.plugins.java.api.tree.ReturnStatementTree; +import org.sonar.plugins.java.api.tree.Tree; +import org.sonar.plugins.java.api.tree.TypeTree; +import org.sonar.plugins.java.api.tree.VariableTree; public final class JavaDetectionEngine implements IDetectionEngine { private static final Logger LOGGER = LoggerFactory.getLogger(JavaDetectionEngine.class); @@ -691,10 +721,6 @@ private void analyseExpression( * * Check out: src/test/java/com/ibm/plugin/resolve/ResolveBuilderPatternTest.java */ - /* - * Checks if the method call itself is of interested and not in any parameter. - */ - // check if expression is a builder pattern boolean isBuilderPattern = false; if (expressionTree instanceof MethodInvocationTree methodInvocationTree @@ -795,6 +821,10 @@ private void analyseExpression( if (expression instanceof MethodInvocationTree methodInvocationTree) { // methods are part of the outer scope resolveValuesInOuterScope(methodInvocationTree, parameter); + // follow expression directly, do not find matching expression in the method + // scope + detectionStore.onDetectedDependingParameter( + parameter, methodInvocationTree, DetectionStore.Scope.EXPRESSION); } else if (expression instanceof NewClassTree newClassTree && assignedSymbol.isEmpty()) { // follow expression directly, do not find matching expression in the method diff --git a/java/src/test/files/rules/issues/Issue16TestFile.java b/java/src/test/files/rules/issues/Issue16TestFile.java new file mode 100644 index 00000000..437241a8 --- /dev/null +++ b/java/src/test/files/rules/issues/Issue16TestFile.java @@ -0,0 +1,14 @@ +import java.security.SecureRandom; +import org.bouncycastle.crypto.BlockCipher; +import org.bouncycastle.crypto.engines.AESEngine; +import org.bouncycastle.crypto.modes.GCMBlockCipher; + +public class Issue16TestFile { + + public static void test() { + // Instantiate GCMBlockCipher with newInstance() method + GCMBlockCipher newInstance = + (GCMBlockCipher) GCMBlockCipher.newInstance(AESEngine.newInstance()); // Noncompliant {{AESEngine}} {{GCMBlockCipher}} + } + +} \ No newline at end of file diff --git a/java/src/test/files/rules/issues/NextParameterDependingRulesTestFile.java b/java/src/test/files/rules/issues/NextParameterDependingRulesTestFile.java index 6dd03bdd..8d7428d0 100644 --- a/java/src/test/files/rules/issues/NextParameterDependingRulesTestFile.java +++ b/java/src/test/files/rules/issues/NextParameterDependingRulesTestFile.java @@ -5,15 +5,15 @@ public class NextParameterDependingRulesTestFile { /* Does not work: AES is not set as a child of GCM */ public GCMBlockCipher test1() { - GCMBlockCipher blockCipher = GCMBlockCipher.newInstance(AESEngine.newInstance()); // Noncompliant {{GCM}} - // Noncompliant@-1 {{AES}} + GCMBlockCipher blockCipher = GCMBlockCipher.newInstance(AESEngine.newInstance()); // Noncompliant {{GCMBlockCipher}} + // Noncompliant@-1 {{AESEngine}} return blockCipher; } /* Works: AES is set as a child of GCM */ public GCMBlockCipher test2() { - GCMBlockCipher blockCipher = new GCMBlockCipher(new AESEngine()); // Noncompliant {{GCM}} - // Noncompliant@-1 {{AES}} + GCMBlockCipher blockCipher = new GCMBlockCipher(new AESEngine()); // Noncompliant {{GCMBlockCipher}} + // Noncompliant@-1 {{AESEngine}} return blockCipher; } } diff --git a/java/src/test/java/com/ibm/plugin/rules/issues/Issue16Test.java b/java/src/test/java/com/ibm/plugin/rules/issues/Issue16Test.java new file mode 100644 index 00000000..0c326806 --- /dev/null +++ b/java/src/test/java/com/ibm/plugin/rules/issues/Issue16Test.java @@ -0,0 +1,143 @@ +/* + * SonarQube Cryptography Plugin + * Copyright (C) 2024 IBM + * + * 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.issues; + +import static org.assertj.core.api.Assertions.assertThat; + +import com.ibm.engine.detection.DetectionStore; +import com.ibm.engine.model.IValue; +import com.ibm.engine.model.ValueAction; +import com.ibm.engine.model.context.CipherContext; +import com.ibm.mapper.model.AuthenticatedEncryption; +import com.ibm.mapper.model.BlockCipher; +import com.ibm.mapper.model.BlockSize; +import com.ibm.mapper.model.INode; +import com.ibm.mapper.model.Mode; +import com.ibm.mapper.model.Oid; +import com.ibm.plugin.TestBase; +import com.ibm.plugin.rules.detection.bc.BouncyCastleJars; +import java.util.List; +import org.jetbrains.annotations.NotNull; +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; + +class Issue16Test extends TestBase { + + @Test + void test() { + CheckVerifier.newVerifier() + .onFile("src/test/files/rules/issues/Issue16TestFile.java") + .withChecks(this) + .withClassPath(BouncyCastleJars.JARS) + .verifyIssues(); + } + + @Override + public void asserts( + int findingId, + @NotNull DetectionStore detectionStore, + @NotNull List nodes) { + if (findingId == 0) { + /* + * Detection Store + */ + assertThat(detectionStore.getDetectionValues()).hasSize(1); + assertThat(detectionStore.getDetectionValueContext()).isInstanceOf(CipherContext.class); + IValue value0 = detectionStore.getDetectionValues().get(0); + assertThat(value0).isInstanceOf(ValueAction.class); + assertThat(value0.asString()).isEqualTo("GCMBlockCipher"); + + DetectionStore store_1 = + getStoreOfValueType(ValueAction.class, detectionStore.getChildren()); + assertThat(store_1.getDetectionValues()).hasSize(1); + assertThat(store_1.getDetectionValueContext()).isInstanceOf(CipherContext.class); + IValue value0_1 = store_1.getDetectionValues().get(0); + assertThat(value0_1).isInstanceOf(ValueAction.class); + assertThat(value0_1.asString()).isEqualTo("AESEngine"); + + /* + * Translation + */ + assertThat(nodes).hasSize(1); + + // AuthenticatedEncryption + INode authenticatedEncryptionNode = nodes.get(0); + assertThat(authenticatedEncryptionNode.getKind()) + .isEqualTo(AuthenticatedEncryption.class); + assertThat(authenticatedEncryptionNode.getChildren()).hasSize(3); + assertThat(authenticatedEncryptionNode.asString()).isEqualTo("AES-GCM"); + + // BlockSize under AuthenticatedEncryption + INode blockSizeNode = authenticatedEncryptionNode.getChildren().get(BlockSize.class); + assertThat(blockSizeNode).isNotNull(); + assertThat(blockSizeNode.getChildren()).isEmpty(); + assertThat(blockSizeNode.asString()).isEqualTo("128"); + + // Oid under AuthenticatedEncryption + INode oidNode = authenticatedEncryptionNode.getChildren().get(Oid.class); + assertThat(oidNode).isNotNull(); + assertThat(oidNode.getChildren()).isEmpty(); + assertThat(oidNode.asString()).isEqualTo("2.16.840.1.101.3.4.1"); + + // Mode under AuthenticatedEncryption + INode modeNode = authenticatedEncryptionNode.getChildren().get(Mode.class); + assertThat(modeNode).isNotNull(); + assertThat(modeNode.getChildren()).isEmpty(); + assertThat(modeNode.asString()).isEqualTo("GCM"); + + } else if (findingId == 1) { + /* + * Detection Store + */ + assertThat(detectionStore.getDetectionValues()).hasSize(1); + assertThat(detectionStore.getDetectionValueContext()).isInstanceOf(CipherContext.class); + IValue value0 = detectionStore.getDetectionValues().get(0); + assertThat(value0).isInstanceOf(ValueAction.class); + assertThat(value0.asString()).isEqualTo("AESEngine"); + + /* + * Translation + */ + assertThat(nodes).hasSize(1); + + // BlockCipher + INode blockCipherNode = nodes.get(0); + assertThat(blockCipherNode.getKind()).isEqualTo(BlockCipher.class); + assertThat(blockCipherNode.getChildren()).hasSize(2); + assertThat(blockCipherNode.asString()).isEqualTo("AES"); + + // BlockSize under BlockCipher + INode blockSizeNode1 = blockCipherNode.getChildren().get(BlockSize.class); + assertThat(blockSizeNode1).isNotNull(); + assertThat(blockSizeNode1.getChildren()).isEmpty(); + assertThat(blockSizeNode1.asString()).isEqualTo("128"); + + // Oid under BlockCipher + INode oidNode1 = blockCipherNode.getChildren().get(Oid.class); + assertThat(oidNode1).isNotNull(); + assertThat(oidNode1.getChildren()).isEmpty(); + assertThat(oidNode1.asString()).isEqualTo("2.16.840.1.101.3.4.1"); + } + } +} diff --git a/java/src/test/java/com/ibm/plugin/rules/issues/NextParameterDependingRulesTest.java b/java/src/test/java/com/ibm/plugin/rules/issues/NextParameterDependingRulesTest.java index 977214ce..23ba8ec8 100644 --- a/java/src/test/java/com/ibm/plugin/rules/issues/NextParameterDependingRulesTest.java +++ b/java/src/test/java/com/ibm/plugin/rules/issues/NextParameterDependingRulesTest.java @@ -25,14 +25,11 @@ import com.ibm.engine.model.IValue; import com.ibm.engine.model.ValueAction; import com.ibm.engine.model.context.CipherContext; -import com.ibm.mapper.model.AuthenticatedEncryption; import com.ibm.mapper.model.INode; -import com.ibm.mapper.model.Mode; import com.ibm.plugin.TestBase; import com.ibm.plugin.rules.detection.bc.BouncyCastleJars; import java.util.List; import org.jetbrains.annotations.NotNull; -import org.junit.jupiter.api.Disabled; import org.junit.jupiter.api.Test; import org.sonar.java.checks.verifier.CheckVerifier; import org.sonar.plugins.java.api.JavaCheck; @@ -48,11 +45,10 @@ class NextParameterDependingRulesTest extends TestBase { * rules on a parameter to capture a block cipher (AES). However, this block cipher gets * captured only when using the rule CONSTRUCTOR_1 (see `test2` in the test file) and not with * the NEW_INSTANCE_1 (see `test1` in the test file). See more details here: - * https://github.ibm.com/CryptoDiscovery/sonar-cryptography/issues/99 + * https://github.com/IBM/sonar-cryptography/issues/16 * *

The issue is here at the level of the detection store. */ - @Disabled @Test void test() { CheckVerifier.newVerifier() @@ -82,7 +78,7 @@ public void asserts( assertThat(detectionStore.getDetectionValueContext()).isInstanceOf(CipherContext.class); IValue value0 = detectionStore.getDetectionValues().get(0); assertThat(value0).isInstanceOf(ValueAction.class); - assertThat(value0.asString()).isEqualTo("GCM"); + assertThat(value0.asString()).isEqualTo("GCMBlockCipher"); DetectionStore store_1 = getStoreOfValueType(ValueAction.class, detectionStore.getChildren()); @@ -91,24 +87,6 @@ public void asserts( assertThat(store_1.getDetectionValueContext()).isInstanceOf(CipherContext.class); IValue value0_1 = store_1.getDetectionValues().get(0); assertThat(value0_1).isInstanceOf(ValueAction.class); - assertThat(value0_1.asString()).isEqualTo("AES"); - - /* - * Translation - */ - - assertThat(nodes).hasSize(1); - - // AuthenticatedEncryption - INode authenticatedEncryptionNode1 = nodes.get(0); - assertThat(authenticatedEncryptionNode1.getKind()).isEqualTo(AuthenticatedEncryption.class); - assertThat(authenticatedEncryptionNode1.getChildren()).hasSize(2); - assertThat(authenticatedEncryptionNode1.asString()).isEqualTo("AES"); - - // Mode under AuthenticatedEncryption - INode modeNode1 = authenticatedEncryptionNode1.getChildren().get(Mode.class); - assertThat(modeNode1).isNotNull(); - assertThat(modeNode1.getChildren()).isEmpty(); - assertThat(modeNode1.asString()).isEqualTo("GCM"); + assertThat(value0_1.asString()).isEqualTo("AESEngine"); } }