Does squid:S4721 pick up use of ProcessBuilder?

  • which versions are you using (SonarQube, Scanner, Plugin, and any relevant extension)
    SonarQube Enterprise 7.4.0.18908

  • what are you trying to achieve
    I would like to know if squid:S4721 picks up uses of ProcessBuilder.

  • what have you tried so far to achieve this
    Read the docs for squid:S4721.

Hi,

I am really not sure I understand what you mean by “pick up” here.
From the documentation : https://rules.sonarsource.com/java/RSPEC-4721

ProcessBuilder constructor and command methods will only raise issue if they are used with an argument. No issue will be raised if they are used without an argument.

This can also be seen on the test file of the analyzer : https://github.com/SonarSource/sonar-java/blob/8946e75907b61d94622217fab424c9db909f5702/java-checks/src/test/files/checks/security/ExecCallCheck.java#L39 Issues will be raised on lines with // Noncompliant comment.

Hope this helps.

1 Like

Sorry, I was quite vague in my question.

Here is a snippet of the code we were hoping would get marked as an issue:

ProcessBuilder processBuilder = new ProcessBuilder().command(command);
Process process = processBuilder.start();

command is a non-constant string.

Hi,
If I understand correctly, you do not have an issue raised on this snippet ?
I just tested this code snippet with command as String parameter of a wrapping method and an issue is raised as expected.
I am really still not sure what is the problem here.

Odd. It wasn’t raised on the pull request I tried. I’ll run another set of examples of this and see if the issue gets raised on them.

I just ran this code through and it did not flag it:


import java.io.IOException;

public interface SonarQubeSquidS4721ProcessBuilder
{
    static void main(String[] args) throws IOException, InterruptedException
    {
        ProcessBuilder processBuilder = new ProcessBuilder();
        processBuilder.command(args);
        Process process = processBuilder.start();
        int exitValue = process.waitFor();
        System.exit(exitValue);
    }
}

This line is unsafe and should be flagged:

processBuilder.command(args);

SonarQubeSquidS4721ProcessBuilder.java appears in the code scanned for the PR and is navigable in the SonarQube web UI.

I just ran your sample in our unit test suite and it is properly detected : is the rule activated in your quality profile ? can you share the log of the analysis ?
What is the sonarjava version installed on your sonarqube instance ?
The problem of false negative does not seem to be related to the rule itself but to something else.

I think it may be an issue with GitHub Enterprise merge branches in the presence of branch conflicts. I’ve been doing some more testing to isolate the issue.

squid:S4721 and squid:S2076 are activated in the quality profile used.

SonarJava 5.10.1 (build 16922) is installed on our SonarQube instance.

SonarLint also doesn’t pick it up. It picks up vulnerable JDBC usages, but not vulnerable ProcessBuilder or Runtime.getRuntime().exec usages.

I’m going to retry with a full clone (not a shallow clone) and SonarJava 5.10.2.

The Sonar Scanner log in Jenkins shows several entries like:
File '/home/jenkins/workspace/Example_example_PR-12345/com/example/SonarQubeSquidS4721ProcessBuilder.java' was detected as changed but without having changed lines

That log comes up almost 22,000 times, each time for a different file.

It also logs about missing commits:
org.eclipse.jgit.errors.MissingObjectException: Missing commit

I have been running the analysis on the merge branch, because running it on the PR branch was causing Sonar to report issues from the base branch. This seems to be working well otherwise.

The full log is massive and would be quite painful to sanitize to share here.

No luck with a full clone of the merge branch either. I have confirmed that the GitHub merge branch has the vulnerable Java examples I am testing.

The log in Jenkins only shows a single warning about encoding and shows 0 issues. Oddly the Gradle plug-in does not appear to log the scanner-cli output, so it’s very quiet relative to running the scanner-cli directly.

Here is the sanitized Scanner Context for that scan:
scanner-context.txt (46.3 KB)

Hello @AlainODea,

One of your comment rings a bell to me: “SonarQubeSquidS4721ProcessBuilder.java appears in the code scanned for the PR”.

Security Hotspot rules such as S4721 are:

  • NOT executed on PR
  • NOT executed in SonarLint

Security Hotspots should be visible only on “master” and Branches. If you don’t have the “Security Reports” menu, it is expected to not see issues related to S4721.

This is a choice we did on purpose because we designed Security Hotspots to target Security Auditors and it’s required to do a manual review before saying there is a real vulnerability.
We thought and maybe it was a mistake, that no one will take the time to review Security Hotspots on PR.

Alex

1 Like

Thank you, @Alexandre_Gigleux. That explains it. We can track Security Hotspots on our branch builds for now.

I would like to be able to get Security Hotspots running on PRs as it would help target the efforts of our security reviewers on PRs that need them.