diff --git a/.gitignore b/.gitignore index dbcf57e71..7ca76c60c 100644 --- a/.gitignore +++ b/.gitignore @@ -3,6 +3,14 @@ target/ !**/src/main/**/target/ !**/src/test/**/target/ +### Generated scan output ### +cbom.json +maven_fast/ +*.tokens +*.interp +generated-sources/ +.antlr/ + ### IntelliJ IDEA ### .idea *.iws diff --git a/engine/src/main/java/com/ibm/engine/language/python/PythonDetectionEngine.java b/engine/src/main/java/com/ibm/engine/language/python/PythonDetectionEngine.java index c56513dba..7bd9dad09 100644 --- a/engine/src/main/java/com/ibm/engine/language/python/PythonDetectionEngine.java +++ b/engine/src/main/java/com/ibm/engine/language/python/PythonDetectionEngine.java @@ -77,6 +77,7 @@ public Tree extractArgumentFromMethodCaller( && methodParameterIdentifier instanceof Name nameTree) { // Check that we have the expected number of parameters + @Nonnull Optional> parameters = Optional.ofNullable(methodTree) .map(FunctionDef::parameters) @@ -110,11 +111,7 @@ public Tree extractArgumentFromMethodCaller( Optional name = parameters .filter(parameterList -> finalIndex < parameterList.size()) - .map( - parameterList -> - Optional.ofNullable(parameterList.get(finalIndex))) - // .filter(Objects::nonNull) - .map(Optional::get) + .map(parameterList -> parameterList.get(finalIndex)) .map(org.sonar.plugins.python.api.tree.Parameter::name) .map(Name::name); @@ -126,8 +123,9 @@ public Tree extractArgumentFromMethodCaller( return null; } + @Nonnull @Override - public @Nonnull List> resolveValuesInInnerScope( + public List> resolveValuesInInnerScope( @Nonnull Class clazz, @Nonnull Tree expression, @Nullable IValueFactory valueFactory) { @@ -154,13 +152,14 @@ public void resolveValuesInOuterScope( if (optionalMethodTree.isEmpty()) { return; } - Tree methodTree = optionalMethodTree.get(); + @Nonnull Tree methodTree = optionalMethodTree.get(); // If we cannot resolve the expression, it shoud be because it is an argument of the // enclosing function. We therefore create a hook, but we need to get the argument to // which `expressionTree` resolves to. // To do so, we call `resolveValues` with the special parameter // `returningEnclosingParam` set to true. + @Nonnull List> resolvedValues = PythonSemantic.resolveValues( Object.class, expressionTree, new LinkedList<>(), null, true, this); @@ -168,7 +167,7 @@ public void resolveValuesInOuterScope( if (resolvedValues.size() != 1) { return; } - final Tree resolvedParameter = resolvedValues.get(0).tree(); + @Nonnull final Tree resolvedParameter = resolvedValues.get(0).tree(); createAMethodHook(methodTree, resolvedParameter, detectableParameter); // Note that compared to the Java implementation, there is no case where we call @@ -227,16 +226,19 @@ public void resolveMethodReturnValues( throw new UnsupportedOperationException("Unimplemented method 'resolveMethodReturnValues'"); } - @Override + @Nullable @Override public ResolvedValue resolveEnumValue( - Class clazz, Tree enumClassDefinition, LinkedList selections) { + @Nonnull Class clazz, + @Nonnull Tree enumClassDefinition, + @Nonnull LinkedList selections) { // TODO: Enums are not a major part of Pythonm, it is left for later // https://docs.python.org/3/library/enum.html throw new UnsupportedOperationException("Unimplemented method 'resolveEnumValue'"); } + @Nonnull @Override - public Optional> getAssignedSymbol(Tree expression) { + public Optional> getAssignedSymbol(@Nonnull Tree expression) { // When the expression is an assignment like `43` in `global_var = 43`, it will return the // symbol of the Name `global_var`. // In Java, `getAssignedSymbol` seem to return the symbol of the *parent* of the expression. @@ -274,20 +276,24 @@ public Optional> getAssignedSymbol(Tree expression) { throw new UnsupportedOperationException("Unimplemented case."); } + @Nonnull @Override public Optional> getMethodInvocationParameterSymbol( - Tree methodInvocationTree, Parameter parameter) { + @Nonnull Tree methodInvocationTree, @Nonnull Parameter parameter) { if (methodInvocationTree instanceof CallExpression callExpression) { - return getTraceSymbol(parameter, callExpression.arguments()); + @Nonnull List arguments = callExpression.arguments(); + return getTraceSymbol(parameter, arguments); } return Optional.empty(); } + @Nonnull @Override public Optional> getNewClassParameterSymbol( - Tree newClass, Parameter parameter) { + @Nonnull Tree newClass, @Nonnull Parameter parameter) { if (newClass instanceof CallExpression callExpression) { - return getTraceSymbol(parameter, callExpression.arguments()); + @Nonnull List arguments = callExpression.arguments(); + return getTraceSymbol(parameter, arguments); } return Optional.empty(); } @@ -296,45 +302,52 @@ public Optional> getNewClassParameterSymbol( private Optional> getTraceSymbol( @Nonnull Parameter parameter, @Nonnull List arguments) { if (parameter.getIndex() >= arguments.size()) { - return Optional.of(TraceSymbol.createWithStateDifferent()); + @Nonnull + Optional> res = Optional.of(TraceSymbol.createWithStateDifferent()); + return res; } Argument arg = arguments.get(parameter.getIndex()); if (arg instanceof RegularArgument regularArg) { Expression expressionArg = regularArg.expression(); if (expressionArg.is(Tree.Kind.NAME)) { Name nameArg = (Name) expressionArg; - return Optional.of(TraceSymbol.createFrom(nameArg.symbol())); + @Nonnull + Optional> res = + Optional.of(TraceSymbol.createFrom(nameArg.symbol())); + return res; } } - return Optional.of(TraceSymbol.createWithStateNoSymbol()); + @Nonnull + Optional> res = Optional.of(TraceSymbol.createWithStateNoSymbol()); + return res; } @Override public boolean isInvocationOnVariable( - Tree methodInvocation, TraceSymbol variableSymbol) { + @Nonnull Tree methodInvocation, @Nonnull TraceSymbol variableSymbol) { if (methodInvocation instanceof CallExpression callExpression) { if (!variableSymbol.is(TraceSymbol.State.SYMBOL)) { return false; } Symbol variable = variableSymbol.getSymbol(); - Expression callee = callExpression.callee(); - if (variable == null || !callee.is(Tree.Kind.QUALIFIED_EXPR)) { + if (variable == null) { return false; } - QualifiedExpression qualifiedExpression = (QualifiedExpression) callee; - if (qualifiedExpression.qualifier() instanceof Name name) { - Optional nameString = Optional.of(name).map(Name::symbol).map(Symbol::name); - return nameString.isPresent() && nameString.get().equals(variable.name()); + if (callExpression.callee() instanceof QualifiedExpression qualifiedExpression) { + Expression qualifier = qualifiedExpression.qualifier(); + if (qualifier instanceof Name name) { + Symbol qualifierSymbol = name.symbol(); + return variable.equals(qualifierSymbol); + } } - - return false; } return false; } @Override - public boolean isInitForVariable(Tree newClass, TraceSymbol variableSymbol) { + public boolean isInitForVariable( + @Nonnull Tree newClass, @Nonnull TraceSymbol variableSymbol) { if (!variableSymbol.is(TraceSymbol.State.SYMBOL)) { return false; } @@ -351,6 +364,20 @@ public boolean isInitForVariable(Tree newClass, TraceSymbol variableSymb private void analyseExpression( @Nonnull TraceSymbol traceSymbol, @Nonnull CallExpression expressionTree) { + boolean isInvocation = + isInvocationOnVariable(expressionTree, traceSymbol) + || isInitForVariable(expressionTree, traceSymbol); + + // Check if the variable symbols for the method (if applicable) are connected + Optional assignedSymbol = + getAssignedSymbol(expressionTree).map(TraceSymbol::getSymbol); + + if (traceSymbol.is(TraceSymbol.State.DIFFERENT) + || (traceSymbol.is(TraceSymbol.State.SYMBOL) && !isInvocation) + || (traceSymbol.is(TraceSymbol.State.NO_SYMBOL) && assignedSymbol.isPresent())) { + return; + } + if (detectionStore.getDetectionRule().is(MethodDetectionRule.class)) { MethodDetection methodDetection = new MethodDetection<>(expressionTree, null); detectionStore.onReceivingNewDetection(methodDetection); @@ -364,22 +391,16 @@ private void analyseExpression( } // Extracts the arguments for the provided expression - List arguments = expressionTree.arguments(); - boolean isInvocation = - isInvocationOnVariable(expressionTree, traceSymbol) - || isInitForVariable(expressionTree, traceSymbol); - // TODO: It would be better to have a case disjunction to use either - // isInvocationOnVariable or isInitForVariable, but it is difficult in Python + @Nonnull List arguments = expressionTree.arguments(); int index = 0; for (Parameter parameter : detectionRule.parameters()) { - if (!checkCurrentIndexState( - index, arguments, isInvocation, traceSymbol, expressionTree)) { + if (arguments.size() <= index) { index++; continue; } // the expression tree of the parameter - Tree expression = arguments.get(index); // this is an Argument tree + @Nonnull Tree expression = arguments.get(index); // this is an Argument tree if (expression instanceof RegularArgument regularArgument) { expression = regularArgument.expression(); } @@ -392,6 +413,7 @@ private void analyseExpression( DetectableParameter detectableParameter = (DetectableParameter) parameter; // try to resolve value in inner scope + @Nonnull List> resolvedValues = resolveValuesInInnerScope( Object.class, expression, detectableParameter.getiValueFactory()); @@ -401,12 +423,14 @@ private void analyseExpression( } else { resolvedValues.stream() .map( - resolvedValue -> - new ValueDetection<>( - resolvedValue, - detectableParameter, - expressionTree, - expressionTree)) + resolvedValue -> { + @Nonnull ResolvedValue val = resolvedValue; + return new ValueDetection<>( + val, + detectableParameter, + expressionTree, + expressionTree); + }) .forEach(detectionStore::onReceivingNewDetection); } } else if (!parameter.getDetectionRules().isEmpty()) { @@ -417,40 +441,12 @@ private void analyseExpression( * In this case, we resolve the parameter with the depending detection rule with an EXPRESSION scope, * this way we ensure to only resolve the right parameter content and not similar calls in the same function scope. */ + @Nonnull Tree expr = expression; detectionStore.onDetectedDependingParameter( - parameter, expression, DetectionStore.Scope.EXPRESSION); + parameter, expr, DetectionStore.Scope.EXPRESSION); } index++; } } - - private boolean checkCurrentIndexState( - int index, - List arguments, - boolean isInvocation, - @Nonnull TraceSymbol traceSymbol, - @Nonnull CallExpression expressionTree) { - /* - * Check if the matched method does have equal or less number of arguments compared to the index - * of interested defined in the detection rule. - * This will prevent an index out of bound - */ - if (arguments.size() <= index) { - return false; - } - - // Check if the variable symbols for the method (if applicable) are connected - Optional assignedSymbol = - getAssignedSymbol(expressionTree).map(ts -> ts.getSymbol()); - - return !(traceSymbol.is(TraceSymbol.State.DIFFERENT) - || - // checks if a symbol is set and therefore expected, then check if the symbols - // match. - (traceSymbol.is(TraceSymbol.State.SYMBOL) && !isInvocation) - || - // checks if no symbol is expected, but the matched method has one. - (traceSymbol.is(TraceSymbol.State.NO_SYMBOL) && assignedSymbol.isPresent())); - } } diff --git a/engine/src/main/java/com/ibm/engine/language/python/PythonLanguageTranslation.java b/engine/src/main/java/com/ibm/engine/language/python/PythonLanguageTranslation.java index e644f4360..4c863be9c 100644 --- a/engine/src/main/java/com/ibm/engine/language/python/PythonLanguageTranslation.java +++ b/engine/src/main/java/com/ibm/engine/language/python/PythonLanguageTranslation.java @@ -68,7 +68,7 @@ public Optional getInvokedObjectTypeString( // well as the "invoked object type" (`cryptography.hazmat.primitives.asymmetric.dsa`) // used in the rule's `forObjectTypes` if (callExpression.callee() instanceof QualifiedExpression qualifiedExpression) { - return PythonSemantic.resolveTreeType(qualifiedExpression.name()); + return PythonSemantic.resolveTreeType(qualifiedExpression.qualifier()); } else if (callExpression.callee() instanceof Name functionName) { return PythonSemantic.resolveTreeType(functionName); } diff --git a/engine/src/main/java/com/ibm/engine/language/python/PythonSemantic.java b/engine/src/main/java/com/ibm/engine/language/python/PythonSemantic.java index 49d85e209..8e2f0c2ca 100644 --- a/engine/src/main/java/com/ibm/engine/language/python/PythonSemantic.java +++ b/engine/src/main/java/com/ibm/engine/language/python/PythonSemantic.java @@ -131,8 +131,8 @@ public static Optional resolveTreeType(@Nonnull Tree tree) { Objects.equals(nameClassSymbol.fullyQualifiedName(), string)); } } - // Otherwise, we accept all types - return Optional.of((String string) -> true); + // Otherwise, we do not accept any type + return Optional.of((String string) -> false); } // Obtain the type from the content @@ -261,7 +261,7 @@ private static boolean resolveFullyQualifiedNameStringType( if (wantedStringType.endsWith(".*")) { result = fullyQualifiedNameStringType.startsWith( - wantedStringType.substring(0, wantedStringType.length() - 2)) + wantedStringType.substring(0, wantedStringType.length() - 2)) || shortenedFullyQualifiedNameStringType.startsWith( wantedStringType.substring(0, wantedStringType.length() - 2)); } else { @@ -369,10 +369,10 @@ private static List> resolveValues( usages.removeIf( usage -> (usage.tree() instanceof Name usageNameTree - && (usageNameTree.equals(nameTree) - || (usageNameTree.firstToken() != null - && usageNameTree.firstToken().line() - > currentLine)))); + && (usageNameTree.equals(nameTree) + || (usageNameTree.firstToken() != null + && usageNameTree.firstToken().line() + > currentLine)))); } // If the current Name has no assignment, function declaration or parameter usage, then @@ -478,7 +478,7 @@ private static List> resolveValues( if (statementTree instanceof ReturnStatement returnStatementTree && !returnStatementTree.expressions().isEmpty()) { if (subscriptionIndex - instanceof Integer intSubscriptionIndex + instanceof Integer intSubscriptionIndex && intSubscriptionIndex < returnStatementTree .expressions() @@ -557,9 +557,9 @@ private static List> resolveValues( // Continue the inner resolution if we have a non-empty // `argsMappingList` if (usageNameTree.parent() - instanceof - org.sonar.plugins.python.api.tree.Parameter - parameterTree + instanceof + org.sonar.plugins.python.api.tree.Parameter + parameterTree && !argsMappingList.isEmpty()) { Argument argument = argsMappingList.pollLast().get(parameterTree); if (argument instanceof RegularArgument regularArgument) { diff --git a/python/src/test/files/rules/detection/keyagreement/PycaFalsePositiveTestFile.py b/python/src/test/files/rules/detection/keyagreement/PycaFalsePositiveTestFile.py new file mode 100644 index 000000000..65932039a --- /dev/null +++ b/python/src/test/files/rules/detection/keyagreement/PycaFalsePositiveTestFile.py @@ -0,0 +1,16 @@ +class Model: + def generate(self, x): + return x + +model = Model() +# This should NOT be detected as X448 or X25519 key generation +ids = model.generate(10) + +def foo(): + # Another generic call + x = some_other_object.generate() + return x + +from cryptography.hazmat.primitives.asymmetric import x448 +# This SHOULD be detected +private_key = x448.X448PrivateKey.generate() # Noncompliant diff --git a/python/src/test/java/com/ibm/plugin/rules/detection/keyagreement/PycaFalsePositiveTest.java b/python/src/test/java/com/ibm/plugin/rules/detection/keyagreement/PycaFalsePositiveTest.java new file mode 100644 index 000000000..64b63b220 --- /dev/null +++ b/python/src/test/java/com/ibm/plugin/rules/detection/keyagreement/PycaFalsePositiveTest.java @@ -0,0 +1,61 @@ +/* + * Sonar Cryptography Plugin + * Copyright (C) 2026 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.keyagreement; + +import static org.assertj.core.api.Assertions.assertThat; + +import com.ibm.engine.detection.DetectionStore; +import com.ibm.mapper.model.INode; +import com.ibm.mapper.model.KeyAgreement; +import com.ibm.plugin.TestBase; +import java.util.List; +import javax.annotation.Nonnull; +import org.junit.jupiter.api.Test; +import org.sonar.plugins.python.api.PythonCheck; +import org.sonar.plugins.python.api.PythonVisitorContext; +import org.sonar.plugins.python.api.symbols.Symbol; +import org.sonar.plugins.python.api.tree.Tree; +import org.sonar.python.checks.utils.PythonCheckVerifier; + +class PycaFalsePositiveTest extends TestBase { + + @Test + void test() { + // This will trigger the asserts method for each finding + PythonCheckVerifier.verify( + "src/test/files/rules/detection/keyagreement/PycaFalsePositiveTestFile.py", this); + } + + @Override + public void asserts( + int findingId, + @Nonnull DetectionStore detectionStore, + @Nonnull List nodes) { + + // We expect only ONE finding (findingId == 0) which is the legitimate x448.generate() + // The generic model.generate() should NOT trigger any finding. + + assertThat(findingId).isEqualTo(0); + + INode keyAgreementNode = nodes.get(0); + assertThat(keyAgreementNode.getKind()).isEqualTo(KeyAgreement.class); + assertThat(keyAgreementNode.asString()).isEqualTo("x448"); + } +}