Skip to content

Commit

Permalink
Fix issue #16 (#141)
Browse files Browse the repository at this point in the history
* fix #16

Signed-off-by: Nicklas Körtge <[email protected]>

* enable issue test

Signed-off-by: Hugo Queinnec <[email protected]>

---------

Signed-off-by: Nicklas Körtge <[email protected]>
Signed-off-by: Hugo Queinnec <[email protected]>
Co-authored-by: Hugo Queinnec <[email protected]>
  • Loading branch information
n1ckl0sk0rtge and hugoqnc authored Sep 16, 2024
1 parent 586910a commit 726889b
Show file tree
Hide file tree
Showing 5 changed files with 201 additions and 36 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand All @@ -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<Tree, Symbol> {
private static final Logger LOGGER = LoggerFactory.getLogger(JavaDetectionEngine.class);
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
14 changes: 14 additions & 0 deletions java/src/test/files/rules/issues/Issue16TestFile.java
Original file line number Diff line number Diff line change
@@ -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}}
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
}
143 changes: 143 additions & 0 deletions java/src/test/java/com/ibm/plugin/rules/issues/Issue16Test.java
Original file line number Diff line number Diff line change
@@ -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<JavaCheck, Tree, Symbol, JavaFileScannerContext> detectionStore,
@NotNull List<INode> nodes) {
if (findingId == 0) {
/*
* Detection Store
*/
assertThat(detectionStore.getDetectionValues()).hasSize(1);
assertThat(detectionStore.getDetectionValueContext()).isInstanceOf(CipherContext.class);
IValue<Tree> value0 = detectionStore.getDetectionValues().get(0);
assertThat(value0).isInstanceOf(ValueAction.class);
assertThat(value0.asString()).isEqualTo("GCMBlockCipher");

DetectionStore<JavaCheck, Tree, Symbol, JavaFileScannerContext> store_1 =
getStoreOfValueType(ValueAction.class, detectionStore.getChildren());
assertThat(store_1.getDetectionValues()).hasSize(1);
assertThat(store_1.getDetectionValueContext()).isInstanceOf(CipherContext.class);
IValue<Tree> 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<Tree> 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");
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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
*
* <p>The issue is here at the level of the detection store.
*/
@Disabled
@Test
void test() {
CheckVerifier.newVerifier()
Expand Down Expand Up @@ -82,7 +78,7 @@ public void asserts(
assertThat(detectionStore.getDetectionValueContext()).isInstanceOf(CipherContext.class);
IValue<Tree> value0 = detectionStore.getDetectionValues().get(0);
assertThat(value0).isInstanceOf(ValueAction.class);
assertThat(value0.asString()).isEqualTo("GCM");
assertThat(value0.asString()).isEqualTo("GCMBlockCipher");

DetectionStore<JavaCheck, Tree, Symbol, JavaFileScannerContext> store_1 =
getStoreOfValueType(ValueAction.class, detectionStore.getChildren());
Expand All @@ -91,24 +87,6 @@ public void asserts(
assertThat(store_1.getDetectionValueContext()).isInstanceOf(CipherContext.class);
IValue<Tree> 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");
}
}

0 comments on commit 726889b

Please sign in to comment.