squid:S2068 false positives

[Andreas Mandel]

Hi,

I'd say that squid:S2068 makes it's job to easy. Not every constant with "PASSWORD" in its name stores a password. The finder should be much more sensitive IMHO.

    /** Property to read the password. */
    // this triggers squid:S2068 but should not (FP)
    public static final String DB_PASSWORD = "com.example.sonarqube.DB_PASSWORD";

    public void test() {
        // the default value "tiger" could trigger squid:S2068, but is likely hard to find      
        final char[] secret = System.getProperty(DB_PASSWORD, "tiger").toCharArray();
        // ...
        PasswordAuthentication pa = new PasswordAuthentication("scott", secret);
    }

    public void testFail() {
        // this should trigger squid:S2068 and is easy to find.
        PasswordAuthentication pa = new PasswordAuthentication("user", "mySecretPassword".toCharArray());
    }

Since severity is BLOCKER it should do it's best not to produce any FP.

What do you think?


SQ 5.3 / Java 3.13.1

Kind Regards,
Andreas.

[Victor Noël]

I think for this kind of problem, it's maybe better to take the time to verify each of these issue and mark them as FP in the SonarQube UI than risking missing them!

It quasi impossible to properly detect this kind of problems, so I think the choice was to make it very liberal in the way it detects issues.
You can also decide to disable that rule… but I'm pretty sure it would be better not too ;)

And actually, I would maybe make it even more strict and have it detect constants named PWD too :)

[Andreas Mandel]

Hi Victor,

hmmm. False Positives always decrease the value of the other real issues found especially in the area of the BLOCKER severity. The rule could and should do better than the current implementation. Might be it should be split in 2 separate rules - one that works based on the field and constant naming pattern, the other that checks the API usage with constant strings (eg the PasswordAuthentication sample or the DriverManager.getConnection(*) sample given in the Rule description. These should get different severities based on the different confidence that it is not a FP.

Filtering via UI is a different topic - actually I can not use it because the lack of branch support (https://groups.google.com/forum/#!topicsearchin/sonarqube/filter/sonarqube/8PRayoHIK2c).

Kind Regards,

Andreas.

BTW: AFAIK - It already also checks for PWD.

(*) if the detection is based on the created URL and not on the fact that the name of the field.

[Victor Noël]

Well, I agree with you, it would be best to have a better rule yes.

In the case it is not possible, maybe you overestimate the problems introduced by FPs: this kind of FP are there because a human HAS to check the problem, and once it's done, it's done, so it's not polluting other issues.

The main problem actually, as you highlighted it, is filtering: if it was working as desired, these FPs would not be a problem to you I think…
it can clearly be improved! On a side note, have you looked at using annotations directly in the code to cover you needs for now?

[Andreas Mandel]

Hmmm, I gave a example how to improve the rule, by not simply checking the name of a field but the use of constants at known apis that consume passwords.

Manual work should be required as little as possible...

For the filtering I need a central revision controlled place where the filters are documented with a reasoning. In case of a (external) review this information is used as input for the reviewer. The inline filtering does not support this. One experience with this also is that you end up with source code that is filled up with //NOSONAR or similar annotation all over the place.

Kind Regards,

Andreas.

[Nicolas Peru]

Hi Andreas and Victor, 

A bit late to the party but just to explain that, as explained by Victor, we are a bit more tolerant regarding FP when it comes down to security rules as you want to be alerted to any potential flaws and let a human check things and that is why, yes this one has some FPs but we're fine with it (and for the anecdot, this rules detects itself :) ) .

As for filtering, there were already answers on other threads about the support of branching that is coming to the platform at one point, but that is not related to the rule, so I am not the one to speak properly here ;) (and that was answered on other threads). 

HTH, 

Cheers, 

--
You received this message because you are subscribed to the Google Groups "SonarQube" group.
To unsubscribe from this group and stop receiving emails from it, send an email to sonarqube+...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/sonarqube/30e8c110-1207-412c-898f-5438640594c4%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

--

Nicolas PERU | SonarSource

Senior Developer
http://sonarsource.com

https://twitter.com/benzonico

[Andreas Mandel]

Hi Nicolas,

thanks for joining! I'm on the other side here - you have recognized this. :-) IMHO it is very important to reduce the amount of possible false positives especially with increasing severity . The acceptance of static code analysis decreases very fast when you block a delivery because of blocker issues - and then need to admit that the case was just the naming of some variables which contained the substring "PASSWORD". 

In my initial post I gave a example of a hard coded password which is not detected at all today, might be an idea for a new Rule? Don't you agree that the Rule should better check the usage of a static string rather than the name of a variable pointing to it?

Kind Regards,

Andreas.

[Nicolas Peru]

Hi Andreas, 

 IMHO it is very important to reduce the amount of possible false positives especially with increasing severity . 

Let me just clarify one thing : we are 100% aligned on this and this is what we are doing daily : we definitely value false negative over false positives for our rules.

However, that being said : security rules are a bit different in this essence and we tolerate a bit more noise from those. 

Cheers, 

To view this discussion on the web visit https://groups.google.com/d/msgid/sonarqube/69599a85-16de-449d-a228-72121e3d395e%40googlegroups.com.

For more options, visit https://groups.google.com/d/optout.

[Andreas Mandel]

Hi Nicolas,

ok - I would not do this differentiation - IMHO, every issue can be security critical and lead to a vulnerability, even just wrongly formatted unreadable code. But not every security critical topic is otherwise a issue in the code.

That aside, did you consider the "checkFail()" sample in my initial post for a new rule - there are different APIs that consume passwords. 

Kind Regards,

Andreas.

[Nicolas Peru]

Hi Andreas, 

RSPEC updated and ticket created to handle the mentioned case : https://jira.sonarsource.com/browse/RSPEC-2069 and https://jira.sonarsource.com/browse/SONARJAVA-1782

Cheers, 

To view this discussion on the web visit https://groups.google.com/d/msgid/sonarqube/05ce560b-b1a3-4152-bc48-4d686d4796a7%40googlegroups.com.

For more options, visit https://groups.google.com/d/optout.

[Andreas Mandel]

Hi Nicolas,
thanks,
Andreas.