feat(csharp): add C# ChaCha20Poly1305 detection rules

[Kaldesyvon]

Hello team,

After restoring C# I am adding new detection rules for ChaCha20Poly1305 algorithm.

This is my first PR in this project so I would appreciate any comments or help.

Thank you for reviewing.

Best,
Martin

[fynnth]

Hi Martin,

That looks good structure wise but we need to refine the detection rule a bit according to the comments above.
Take a look at the structure in the AES-Branch: https://github.com/fynnth/sonar-cryptography-csharp/tree/aes-related-crypto
The detection rules for csharp currently in https://github.com/cbomkit/sonar-cryptography are just dummies that need to be refined to not only detect only the constructor but also all operations that then depend on that instantiation. For chacha20poly1305 this should be quick like i outlined in the comments :)
If something is unclear, just ask.

Best regards,
fynn

[fynnth]

Hi Martin,

that looks way better now, very good job! Also the unit test is comprehensive and covers properly.
@cbomkit/cbomkit-maintainers:
could you also have a look, if it fits for you? and should we merge this as a first proper detection rule for csharp or wait till the whole library will be covered?

Best regards,
Fynn

[san-zrl]

If you detect this as

.shouldBeDetectedAs(new CipherActionFactory<>(CipherAction.Action.ENCRYPT))

you would not have to mix this into the handling of the value actions in the translator.

[fynnth]

Thats a good change thanks, will also implement that in my local rules :)

[san-zrl]

This depends on how specific you want to be. withAnyParameters() means what is says - anything is ok. If I read the C# docs correctly this Encrypt requires 5 parameters. Your test now uses 4 which is OK if you have ANY here, but is this correct C#?

[fynnth]

This is correct c# code because AAD in this function has a default value that will be taken if only 4 parameters are there. That is also the reason for that specific test case and the test before that covers the 5 parameter option.

HOWEVER i have tried to compile the file and it seems that new ReadOnlySpan<byte>[12]; creates an array of ReadOnlySpan<byte> which then fails once it gets used as parameter where it becomes non-convertible.
Also the file needs using System; for compilation, which doesnt matter for the test but just to not let it out.

For me this works better: Span<byte> nonce = stackalloc byte[12];
Did it compile for you Martin?

[san-zrl]

Hi @Kaldesyvon , hi @fynnth,

I just learned that C# has the same concept of named parameters like python. Named parameters in a function call can occur in arbitrary order. Some of them are optional which makes the number of variants a valid function call can have even higher. Writing detection rules for pycryptodome was thus very difficult because you need a lot of rules to cover the most common call variants. To this end, I suggested to add another call to the detection rule interface

.withNamedMethodParameter(String name, String, boolean optional);

withNamedMethodParameter can be used the same way as the original wtihNamedParameter call to match method parameters. It just uses the parameter names (if existing) as the first selection criterion (falling back to positional matching if there is no name). The named parameters can occur in any order. They can be combined with positional parameters (preceeding the named ones). Please have a look, this is implemented in PR #509.

For your reference, there is currently one detection rule in python/…/detection/pycrypto/hash/PycryptoCryptoHash.java for SHA512.new(data=None, truncate=None) as a sample rule for a function with named optional parameters. There is also extensive testing of edge cases which gives you an idea how this rules behaves, what matches and what doesn't. I asked @n1ckl0sk0rtge to review the PR. As i mentioned, this is a byproduct of #495 which is on hold until #509 is decided.

[fynnth]

Hi @san-zrl,

Thats true, I have not thought of that. But for csharp the issue is even worse because the parameter name was not in the parser interface that I have implemented, so i have to rework that too before anything like that works. (info is just not there for parameter names)

My plan would be to finish detection rules like it is now and then afterwards start reworking the engine and implementing named parameters, then update the rules and also extend parameter detection in general (for example trying to detect certain key sizes that were instantiated in a variable before the method call)
Would that fit?

[san-zrl]

@fynnth - Sure. For the moment the new feature is python specific. We have to see where else it would make sense.