sonar-cryptography icon indicating copy to clipboard operation
sonar-cryptography copied to clipboard

Next parameter depending rules may not be handled

Open n1ckl0sk0rtge opened this issue 1 year ago • 0 comments

The problem

Let's consider this code line example:

GCMBlockCipher blockCipher = GCMBlockCipher.newInstance(AESEngine.newInstance())

Suppose we have a rule R1 to detect the call GCMBlockCipher.newInstance, and it has a depending detection rule R2 on its parameter to detect the call AESEngine.newInstance. When R1 makes a finding, the following code gets ultimately executed to handle depending detection rules (where expression is the argument, so here AESEngine.newInstance()): https://github.ibm.com/CryptoDiscovery/sonar-cryptography/blob/51d3f15a18ac3cea5903206c5502da2fe2ea568f/engine/src/main/java/com/ibm/engine/language/java/JavaDetectionEngine.java#L746-L759

However, because AESEngine.newInstance() is here a MethodInvocationTree, it will execute the first case and try to resolveValuesInOuterScope, without handling depending rules.

Note that if our code uses a intermediary variable engine instead (like below), this problem does not occur and depending rules are correctly handled.

AESEngine engine = AESEngine.newInstance();
GCMBlockCipher blockCipher = GCMBlockCipher.newInstance(engine)

Solution?

A trivial solution idea would be to just handle depending rules in any case, so removing the last line from the else statement, giving the following code:

if (expression instanceof MethodInvocationTree methodInvocationTree) {
    // methods are part of the outer scope
    resolveValuesInOuterScope(methodInvocationTree, parameter);
} else if (expression instanceof NewClassTree newClassTree
        && assignedSymbol.isEmpty()) {
    // follow expression directly, do not find matching expression in the method
    // scope
    detectionStore.onDetectedDependingParameter(
            parameter, newClassTree, DetectionStore.Scope.EXPRESSION);
}
// handle next rules
detectionStore.onDetectedDependingParameter(
        parameter, expressionTree, DetectionStore.Scope.ENCLOSED_METHOD);

However, while this easy fix correctly fixes our example above, it breaks 3 JCA unit tests (for reasons I have not investigated).

n1ckl0sk0rtge avatar Jun 12 '24 12:06 n1ckl0sk0rtge