The language support for Python was written before some general changes were introduced.
It should therefore be updated to take advantage of these changes, to have a more coherent and maintainable implementation.

• Rewrite how values are resolved to make it closer to the Java implementation
  • Use ValueActionFactory to capture the function names
• Refactor the translation similarly to what was done with JavaTranslationProcess
  • Add a reorganization layer if necessary

Extend logging capabilities for the scanner:

• log total amount of detected cryptographic assest after the scan
• ...

Context

When writing a rule using the DetectionRuleBuilder, one can use the shouldBeDetectedAs function to resolve directly a specific parameter, and one can use the addDependingDetectionRules function to resolve a parameter with another rule (for example when this method is a method invocation, using a depending detection rule allows to resolve a specific parameter of this function call).
This issue is about the behaviour of the static analysis when resolving the following case (written here in Python, but the issue is also valid for Java):

result = func1(func2("RSA")) # We want to resolve "RSA"

To do so, one should write a rule R1 to detect func1, with a depending detection rule R2 on its parameter to detect func2. Then R2 resolves the content of the parameter of func2, here "RSA", using shouldBeDetectedAs.
Currently (at main 0cd69ef79984b2c91bd0e06b07fb5488a2ea6473 and feature/python-support f5cdfe445fd4e5ea87ba6d0e886b84f02ad0c7cd), this type of resolution implies a complex mechanism that is quite limited.

Current mechanism

The detection process starts by visiting all method invocations (and class instantiations). For each invocation, a detection executive is started for each registered "entry" rule: R1 in our case.
We will therefore look for all functions func1 in the current scope. Once we have a match, the function analyseExpression is called, and there are several cases:

• If the rule is only interested in the function without its parameter, it is handled in a special way that is not relevant for this issue.
• If the rule has a DetectableParameter (coming from shouldBeDetectedAs), meaning that we want to resolve a parameter of the detected function call, we name the subsequent handling logic case of a detectable parameter.
• If the rule has no DetectableParameter despite having parameters, it is probably a rule written for an intermediary function that does directly not carry information, but may have depending detections rules. We name the subsequent handling logic case of an intermediary parameter.

In our example, func1 brings not information other than through the content of its parameter func2("RSA"). R1 is written as a rule with a depending detection rule R2 on its parameter, but without a shouldBeDetectedAs part.
We therefore enter the case of an intermediary parameter in the function analyseExpression.

Case of an intermediary parameter

In this case, depending on the implementation, we have the choice to call onDetectedDependingParameter with two possible scopes: EXPRESSION or ENCLOSED_METHOD. Basically, it will resolve the depending detection rule(s) by visiting either the parameter tree (func2("RSA") in our case) or the enclosing method of this parameter (the function in which this expression is).

In our example, we want to resolve this particular func2("RSA") call, so using the EXPRESSION scope makes sense. func2("RSA") will be the only match of rule R2, which will correctly resolve the parameter content "RSA". This resolution is will go through the case of a detectable parameter explained below. This example worked as expected!

Problem 1: This example worked because func2("RSA") was a function call. Here is a second example with an intermediary variable var:

var = func2("RSA")
result = func1(var) # We want to resolve "RSA"

In this case, because var is not a function call, visiting its tree will not return any results. Finding the function call associated to var is not that trivial, because there may be several intermediary assignments, results of function calls, dictionary values, etc.

Case of a detectable parameter

Let's look at a new example:

var = ec.ECDSA("SHA_256")
other_var = ec.ECDSA("SHA_512") # Some other var we are not interested in
result = func1(var) # We want to resolve "ECDSA" and "SHA_256"

In this example, func1 still does not bring information other than through the content of its parameter var = ec.ECDSA("SHA_256").
R1' (to detect func1) is written as a rule with a depending detection rule R2' for its parameter, but has now also a shouldBeDetectedAs part aiming at resolving the value "ECDSA" (name of the function used as parameter of func1).
R2' (to detect ec.ECDSA) (with shouldBeDetectedAs), resolves the content of the parameter of ec.ECDSA, here "SHA_256", using shouldBeDetectedAs.
Upon detection of func1 with rule R1', because the parameter of func1 is a detectable parameter, we enter the case of a detectable parameter.

Let's say that resolution works as expected in this case ("ECDSA" has been detected). Then, analyseExpression will call onReceivingNewDetection, that will call the following rules (here R2') with followNextRules. For now, there is no choice of scope, so it is necessarily the scope of the enclosing method. This means that it will look for a call of ECDSA in the entire scope of the method: how to make sure that this call is linked to our parameter var of func1?
To do so, the current implementation uses a TraceSymbol. onReceivingNewDetection will obtain the symbol of the parameter of func1, here SYMBOL, var, and will pass it as an argument to followNextRules.

Then, all function calls in the scope of the enclosing method will be matched against R2', which will detect the two ECDSA function calls, one assigned to var and the other to other_var. Using the previous TraceSymbol, we filter the findings to only keep the finding linked to var.
In our case, this works and this example will correctly resolve "SHA_256".

Problem 2: This example worked because ec.ECDSA("SHA_256") was directly assigned to var. Here is a second example intermediary variable intermediary_var:

var = ec.ECDSA("SHA_256")
intermediary_var = var
other_var = ec.ECDSA("SHA_512") # Some other var we are not interested in
result = func1(intermediary_var) # We want to resolve "ECDSA" and "SHA_256"

In this case, SYMBOL, intermediary_var is the TraceSymbol obtained at the end of R1' and passed to followNextRules. But the symbol to which ec.ECDSA("SHA_256") is assigned is var. Because those two symbols don't match, there will be no detection. Again, correctly linking intermediary_var to ec.ECDSA("SHA_256") is not that trivial, because there may be several intermediary assignments, results of function calls, dictionary values, etc.

Problem 3: What if there is no intermediary variable at all:

other_var = ec.ECDSA("SHA_512") # Some other var we are not interested in
other_func(ec.ECDSA("SHA_512")) # Some other function call we are not interested in
result = func1(ec.ECDSA("SHA_256")) # We want to resolve "ECDSA" and "SHA_256"

In this case, NO_SYMBOL is the TraceSymbol obtained at the end of R1' and passed to followNextRules, because the parameter of func1 is not a variable. Then, all function calls in the scope of the enclosing method will be matched against R2', which will detect the three ECDSA function calls, one assigned to other_var and the two others assigned to nothing (TraceSymbol NO_SYMBOL). The filtering step can then remove the ECDSA linked to the TraceSymbol SYMBOL, other_var, but it cannot distinguish between the two NO_SYMBOL. It will therefore resolve both SHA_512 and SHA_256.
To avoid this case, we need to ensure that only the parameter of func1 gets resolved. This could be done by limiting the scope to this parameter expression, as we can do in the case of an intermediary parameter (but we then need to solve Problem 1).

A better approach

Could a better and simpler approach be found and replace this complex mechanism (currently based on limited TraceSymbols and varying scopes of resolution)?

TODO: think about it.

Let's look at the example below, where our rule NEW_CIPHER (actual rule from Python's CryptographyCipher.java file) has two parameters that need to be detected:

private static final IDetectionRule<Tree> NEW_CIPHER = new DetectionRuleBuilder<Tree>()
    .createDetectionRule()
    .forObjectTypes("cryptography.hazmat.primitives.ciphers")
    .forMethods("Cipher")
    .withMethodParameter("cryptography.hazmat.primitives.ciphers.algorithms.*")
        .shouldBeDetectedAs(new AlgorithmFactory())
    .withMethodParameter("cryptography.hazmat.primitives.ciphers.modes.*")
        .shouldBeDetectedAs(new AlgorithmFactory())
    .buildForContext(new CipherContext())
    .withDependingDetectionRules(followingNewCipherRules);

This rule has a set of depending detection rules followingNewCipherRules at the level of the method (and not related to a parameter).
With the current handling of depending detection rules, once a parameter is detected, the function analyseExpression is called, which will call onReceivingNewDetection.
Here, both "method parameter related detection rules" and "invoked object related detection rules" are handled using followNextRules.
In our particular case, onReceivingNewDetection will be called twice, once for each parameter, which will therefore duplicate "invoked object related detection rules" and their related findings.

Therefore, calling "invoked object related detection rules" should not be done at this point. This may be fixed in the more general refactoring of entry points described in issue #42 or in the more general refactoring of subrules handling described in issue #34.

Evaluate requirements and submit request to add the sonar cryptography plugin to the SonarQube marketplace.

Sources:

• https://community.sonarsource.com/t/deploying-to-the-marketplace/35236

• Investigate if capturing elliptic curves is possible
• Handle cases where crypto assets have multiple nodes of the same type at the same level (like two digests)
• Handle all static fields
• Better handle enrichment: move all the enriched content currently in translation to enrichment
• Look into missing classes in org.bouncycastle.crypto: is there missing information?

After installation we received the following error during the Scanning Process:

org.springframework.beans.factory.BeanCreationException: Error creating bean with name 'org.sonarsource.scanner.api.internal.IsolatedClassloader@67f639d3-org.sonar.scanner.bootstrap.ScannerPluginRepository': Initialization of bean failed; nested exception is java.lang.IllegalStateException: The plugin [crypto] does not support Java 11.0.20
[60](https://gitlab.lgt.com/psb/sb-frontend-dev/-/jobs/17551597#L60)

The translation of the Python example CryptographyRSASign1Test returns the following result without using the default enricher (com.ibm.enricher.Enricher).

DEBUG [translation] (PrivateKey) RSA
DEBUG [translation]    └─ (Algorithm) RSA
DEBUG [translation]       └─ (KeyGeneration) KEYGENERATION
DEBUG [translation]       └─ (KeyLength) 2048
DEBUG [translation]    └─ (KeyLength) 2048
DEBUG [translation]    └─ (Signature) RSASSA-PSS
DEBUG [translation]       └─ (MessageDigest) SHA384
DEBUG [translation]       └─ (Sign) SIGN
DEBUG [translation]       └─ (Algorithm) RSA
DEBUG [translation]          └─ (KeyLength) 2048
DEBUG [translation]       └─ (ProbabilisticSignatureScheme) PSS
DEBUG [translation]          └─ (MaskGenerationFunction) MGF1
DEBUG [translation]             └─ (MessageDigest) SHA256

When adding the default enricher, the result becomes:

DEBUG [translation] (PrivateKey) RSA
DEBUG [translation]    └─ (Algorithm) RSA
DEBUG [translation]       └─ (KeyGeneration) KEYGENERATION
DEBUG [translation]       └─ (KeyLength) 2048
DEBUG [translation]       └─ (Oid) 1.2.840.113549.1.1.1
DEBUG [translation]    └─ (KeyLength) 2048
DEBUG [translation]    └─ (Signature) RSASSA-PSS
DEBUG [translation]       └─ (MessageDigest) SHA384
DEBUG [translation]       └─ (Sign) SIGN
DEBUG [translation]       └─ (Algorithm) RSA
DEBUG [translation]          └─ (KeyLength) 2048
DEBUG [translation]          └─ (Oid) 1.2.840.113549.1.1.1
DEBUG [translation]       └─ (ProbabilisticSignatureScheme) PSS
DEBUG [translation]          └─ (MaskGenerationFunction) MGF1
DEBUG [translation]             └─ (MessageDigest) SHA256
DEBUG [translation]       └─ (SaltLength) 160  <––––––––––––––––––
DEBUG [translation]       └─ (MaskGenerationFunction) MGF1  <–––––
DEBUG [translation]          └─ (MessageDigest) SHA-1 <–––––––––––
DEBUG [translation]             └─ (BlockSize) 512  <–––––––––––––
DEBUG [translation]             └─ (KeyLength) 512  <–––––––––––––
DEBUG [translation]             └─ (DigestSize) 160  <––––––––––––
DEBUG [translation]             └─ (Oid) 1.3.14.3.2.26  <–––––––––
DEBUG [translation]       └─ (Oid) 1.2.840.113549.1.1.10

As expected, various OIDs have been added. However, the lines above with arrows show some JCA-specific information which has been added and is not necessarily true in the Python implementation.

The default enricher should be restrained to enrich translations with OIDs, and the remaining enrichments should be moved to a language-specific enricher class (the Python implementation uses PythonEnricher.java for example).

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).

JCA uses a provider-based architecture, where one can specify a cryptography provider (like BouncyCastle) which will provide the implementations of JCA APIs.

• Investigate if we can often identify the provider when tracking JCA crypto calls
• If so, restructure the JCA rules to make them depend on the provider
• Add BouncyCastle as a cryptography provider for JCA

Example

Take for example CBCBlockCipher which implements the interface BlockCipher.
To instantiate such an object, one can use the static method CBCModeCipher.newInstance(BlockCipher cipher). Relevant information is contained into the cipher parameter, and we aim to capture it using depending detection rules.

However, because the cipher parameter is of type BlockCipher, nothings prevents the developer from having a call CBCModeCipher.newInstance(CBCModeCipher.newInstance(CBCModeCipher.newInstance(...))) with un unbounded succession of calls.
When writing the rules, this means that if we add all the BlockCipher rules as depending detection rules for the parameter of CBCModeCipher.newInstance, we will include the CBCModeCipher.newInstance rule, which will create an infinite chain of depending detection rules.

Looking deeper into how these classes are supposed to be used, it looks like all classes implementing BlockCipher are divided into two categories: ___Engine (taking no parameters to instantiate) and ___BlockCipher (taking a BlockCipher parameter to instantiate) classes, and that ___Engine classes are supposed to be the only one used as parameter of ___BlockCipher classes. For example, CBCBlockCipher cipher = CBCBlockCipher.newInstance(DESEngine.newInstance()).

Generalisation

This case seems more general than just the BlockCipher interface, for example it also happens with the AsymmetricBlockCipher interface, and seems to be a recurring pattern in how BouncyCastle is coded.

Why this matters

Therefore, to avoid the case of infinite chains of rules described in the example, we assume in our rules that only ___Engine classes are used as parameter of the other classes of the same interface. This assumption seems to be confirmed by code examples, in which I always observed these kinds of instantiations.

Example inspired by this code.

_curveTable = {
    b'ecdsa-sha2-nistp256': ec.SECP256R1(),
    b'ecdsa-sha2-nistp384': ec.SECP384R1(),
    b'ecdsa-sha2-nistp521': ec.SECP521R1(),
}

def _fromECComponents(cls, x, y, curve, privateValue=None):
        publicNumbers = ec.EllipticCurvePublicNumbers(
            x=x, y=y, curve=_curveTable[curve])
        #....

Key._fromECComponents(..., ..., ..., b'ecdsa-sha2-nistp256', ...) # Noncompliant {{SECP256R1}} (desired behaviour)

In this example, we want to resolve a curve value by looking into a dictionary. While this resolution is already implemented, this is a particular case where the subscription index curve is a parameter of the enclosing function.
Here, curve is correctly resolved (using outer scope resolution) to b'ecdsa-sha2-nistp256', but this resolved value is not later used to look into the dictionary.
Therefore, the captured value is currently b'ecdsa-sha2-nistp256' instead of SECP256R1.

Context

In Python, here is an example of how a sign function is called (case of elliptic curves):

private_key = ec.generate_private_key(ec.SECP384R1())
sig = private_key.sign(digest, ec.ECDSA(hashes.SHA256()))

And here is how a verify function is called:

public_key = private_key.public_key()
public_key.verify(signature, data, ec.ECDSA(hashes.SHA256()))

We see that some crypto information is located at the level of the private key generation (here the chosen curve), and some other information, more closely related to the sign/verify algorithm , is located in the sign/verify function call (here the signing algorithm and its hash function).

In the case of verify, in most cases we will not even generate the private key, but we will receive the public key from someone else. In this case, we cannot use generate_private_key as an entry point for a rule identifying verify, and we may lack the curve information completely.

General problem: Because crypto information is distributed among several function calls, that could be located in different files, it is challenging to link them to aggregate all the information in one place. It may even be impossible to obtain all the information in some cases.

Connecting multiple files

Currently, the SonarQube Python Plugin seems to create an AST for each Python file, without linking symbols that were imported from another file.
For now, let's consider that only generate_private_key is the entry point of our sign detection rule which contains a depending detection rule to detect the sign function.

Obtaining arguments values

# file1.py
from file2 import custom_sign1
hash = hashes.SHA256()
sig = custom_sign1(digest, hash)

# file2.py
def custom_sign1(digest, hash):
   private_key = ec.generate_private_key(ec.SECP384R1())
   return private_key.sign(digest, ec.ECDSA(hash))

In this example, our rule correctly detects both generate_private_key and sign in file2.py. It can retrieve all crypto information easily, except for the hash variable that is a parameter of the enclosing wrapper function custom_sign1.
This case is already handled at the scale of one file: we look for all the calls of custom_sign1 in the file, and when we find one, we resolve its hash argument.

Problem 1: However, here, custom_sign1 is called from a different file file1.py, that we don't see when we go over the AST of file2.py to search for function calls. In this case, the value of hash will not be resolved.

Depending rule in a subfunction

# file1.py
from file2 import custom_sign2
private_key = ec.generate_private_key(ec.SECP384R1())
sig = custom_sign2(private_key, digest)

# file2.py
def custom_sign2(private_key , digest):
   return private_key.sign(digest, ec.ECDSA(hashes.SHA256()))

In this example, our rule detects the entry point generate_private_key in file1.py. The depending detection rule then look for a sign function call, but because the AST does not contain information about the content of file2.py, it cannot resolve its associated crypto values.
Note that this problem would probably also happen if the calls to generate_private_key and sign were inside different functions in the same file.

Problem 2: The scope in which a depending detection rule looks for a match is too limited, as it does not look into the content of other called functions, whether these functions are imported from another file or not.

This example could be even more complicated: we could imagine a codebase where all crypto calls would be inside custom wrappers, so here both generate_private_key and sign would be in different functions in different files, but would still need to be linked somehow.

Importing an external value

# file1.py
from file2 import crypto_dict
private_key = ec.generate_private_key(crypto_dict['intermediate'])

# file2.py
crypto_dict = {'beginner': ec.SECP384R1(),
               'intermediate': ec.BrainpoolP256R1(),
               'advanced': ec.SECT233K1()}

In this example, we only look at the detection of generate_private_key. Our rule will identify the function call, and will then try to resolve the value of the argument to obtain the curve.

Problem 3: The AST contains no information about the content of crypto_dict, so we cannot resolve the value of the curve. Currently, we will resolve the value of string index, in this case intermediate.

When even connecting files is not sufficient

Let's suppose now that we have successfully "connected" all of our files, hence the 3 problems above are solved.
Let's go back to our initial verify example:

public_key.verify(signature, data, ec.ECDSA(hashes.SHA256()))

If this public_key is generated by our code, like below, then we may manage to resolve everything.

private_key = ec.generate_private_key(ec.SECP384R1())
public_key = private_key.public_key()

However, as explained previously, this may be rarely the case: in the cases of verifying a signature or encrypting a message with a public key, we mostly expect the public key to come from another protocol participant. Therefore, the generation of the private and public key will be probably unreachable by static analysis, as there will be a phase of communication between the key generation (by Alice) and the signature verification or public key encryption (by Bob).

In this case, we can abandon the idea of obtaining information linked to the key generation (like the curve) when detecting the verify function. However, we would still want to obtain information linked to the verify function call (like the signature algorithm and hash function), which we currently do not get as we didn't detect the entry point generate_private_key.

Problem 4: Because verify is not an entry point (it is only a depending detection rules that is applied upon a detection of generate_private_key), we currently cannot resolve any crypto information from the verify function call. The problem is more complex than just making verify an entry point rule. Indeed, we do not know whether we will detect a generate_private_key entry point or not, which lead to a problem in all cases:

• Having sign as a depending detection rule of generate_private_key: case described above.
• Having sign only as an entry point: we will correctly identify the crypto information related to the sign function call, but we will never get information related to the key generation (like the curve).
• Having sign as an entry point and as a depending detection rule of generate_private_key: we will get all the crypto information, but when generate_private_key detects something, the sign rule will be applied twice and we will get duplicate results.
• Having sign as an entry point with generate_private_key as a depending detection rule: this would solve our problem if these were the only rules, however we have several other cases were we need generate_private_key to be an entry point. Then if sign detects something, generate_private_key will be applied twice (one as depending detection rule of sign and once as the entry point for the other rules).

Draft ideas for improvement

• Make scanning connect multiple files instead of scanning each file independently (to create an AST bigger than just one file) [to help in problems 1, 2, 3]
  • Possibility 1: Work at the SonarQube Python Plugin level to change the scanning process (probably long/hard work)
  • Possibility 2: Find an "easy" fix which does not require changing the scanning process, but that could be a pre-processing (merging several files into one?) or post-processing (aggregating and linking all ASTs?)
• Improve the way we look for depending detection rules to look into the content of called functions and more [to help in problem 2]
• Revamp how depending detection rules and entry points work [to solve problem 4]