Improve Error Messages related to data flows from forbidden types

[Lonzak]

Nice work so far! CogiCrypt is quite helpful but in certain aspects some improvements can be done.
In general most of the error message have to be improved. For instance the following code

KeyStore ks = KeyStore.getInstance("pkcs12", "BC");
ks.load(fileInputStream, keyPassword.toCharArray());  <- Error

triggers the following error:

Second parameter should never be of type java.lang.String

Ok. What exactly does it mean? The password was originally a string. ok. But how can it be fixed? What can go wrong if I use Strings in the first place? One thing is to detect security related bugs, misuses etc. But the other thing is to educate and to help developers improve.

The sonar guys have done it quite nicely:
https://rules.sonarsource.com/java/type/Vulnerability/RSPEC-4426

1. Security / Bug explanation
2. Noncompliant Code Example
3. Compliant Code Example
4. See (Mitre, OWASP ...)

[kruegers]

Hey Lonzak,

We totally agree CogniCrypt's error messages could do with some improvement. Thanks for raising this issue here. That said, I want to mention two things.

1. Most of our error messages do already give suggestions for how to correct the underlying problem. We are also planning to integrate quick fixes to relieve users from having to go through the trouble of having to fix their code themselves.
2. In this particular case, the question of what is correct is really hard to answer as the actual general answer is just the negative CogniCrypt does tell the user: "Do not make this object a String." Everything else you mention is correct and important, of course, but highly depending on the given context, which we cannot reliably determine at this point. This has to do with what we check for. CogniCrypt's analysis makes use of usage rules in the specification language CrySL. The rule for KeyStore requires that none of the passwords must have data flows from a String object. The analysis detects such a flow for your code snippet above and reports it.

This is not to say, there is nothing to be done about the way it is being reported currently, even now. I'll be leaving this issue open for suggestions on how to improve this particular error message. If you have any suggestions on how to rephrase it, please let us know.

[Lonzak]

Regarding (1): Ok maybe I didn't see or find that suggestion. Where can I see it? I tried a right click on the error but there is no sub-menu. I also didn't find a CogniCrypt view or perspective. Maybe you can point me in the right direction?
Regarding (2): That is understood. But let me draw a comparison. If you look at Spotbugs (formerly findbugs) or Sonar you'll see that you have the possibility to get additional information to a detected bug. Because developers don't have a clue what to do with some of the reported bugs (rules).

Let me make an example. E.g. Spotbugs reports:

Bug: Method foo() uses a FileInputStream or FileOutputStream constructor.

So far so good. But what is the problem with that? Without any information I would not fix that problem and just ignore it. That is why this short bug info is followed by an explanation:

This method creates and uses a java.io.FileInputStream or java.io.FileOutputStream object. Unfortunately both of these classes implement a finalize method, which means that objects created will likely hang around until a full garbage collection occurs, which will leave excessive garbage on the heap for longer, and potentially much longer than expected. Java 7 introduced two ways to create streams for reading and writing files that do not have this concern. You should consider switching from these above classes to InputStream is = java.nio.file.Files.newInputStream(myfile.toPath()); OutputStream os = java.nio.file.Files.newOutputStream(myfile.toPath());

Ah nice - now I know where, why and how I can improve my code and if I have the time I am gonna do it.
So in comparison CogniCrypt currently provides the headline but no explanation!

Second parameter should never be of type java.lang.String.

Why is using "password".toCharArray() bad? How can I improve or change it? Since most passwords are strings or byte arrays anyway how do I fix it? You said that most of the bugs have a suggestion, but the ones I see non has any additional information. But since you report that problem you obviously have some good reason to do so - so please tell us and enlighten us ignorant folks ;-)

Let me show what what do I mean with the help of an example:

CogniCrypt NOW: Operation on object of type java.security.MessageDigest object not completed. Expected call to digest, update [sic]

CogniCrypt IMPROVED:
Bug: Incomplete usage of a java.security.MessageDigest object. Expected a call to digest(...) or update(...).
Explanation:

In the analyzed code there are potential code paths which could result in not calling one of the below mentioned methods. Please verify the number of calls and ensure that their ordering is correct.
The valid generation of a message digest always contains three steps:

//(1) Creating an instance of the digest method
MessageDigest digest = MessageDigest.getInstance(YourHashMethod);

//(2) Adding or updating the actual data (can be called multiple times)
digest.update("Some data which should be hashed.".getBytes());

//(3) Call digest() to complete the digest operation. Afterwards the digest is reset!
byte[] hash = digest.digest();

//(4) The digest can be reused. For the sake of clarity it is recommended to explicitly indicate that
digest.reset(); //<optional> goto (2) again

And if you are really nice you also tell abour your limitations here:

Please note, that cryptSL currently can not detect update(...) calls within loops. (more information here on False-Positives)

for(int i=0; i<1; i++) {
	md.update(ba);
}