S4817 False Positive

SonarQube Server / Community Build
,
1

Using SonarJava 5.9.2 on SonarQube 6.7.5.38563

The rule S4817 highlights all XPath usages where a method parameter is used as XPath. But the following case should not be a positive:

A private method in a class, where all callers are using constant strings as the parameter making up the XPath, e.g.:

public class Book {

  private Document doc;

  // fill doc somehow in constructor or somewhere...

  private String getXPathString(Document doc, String xpath) {
    return (String) XPathFactory.newInstance().newXPath().compile(xpath).evaluate(doc, XPathConstants.STRING); // SonarQube complains here
  }

  public String getAuthor() {
    return getXPathString(this.doc, "/book/author"); // fine, no XPath injection possible
  }

  public String getTitle() {
    return getXPathString(this.doc, "/book/title"); // fine, no XPath injection possible
  }

  // no one else calls that private method
}

Other rules, e.g. findsecbugs:XPATH_INJECTION, are more clever and do not complain here.

Although this rule is a “check if this is safe” rule, I’d expect that it frees me from always-safe situations to check, so I’d appreciate if the rule implementation was fixed.

3

Hi @falbrech-hsdg,

Thank you for your feedback.

This rule will be improved by not raising issues on hardcoded strings when they are given directly to the compile/evaluate functions (new ticket SONARJAVA-3028). However it will still raise issues when the hardcoded string is provided in a separate method. We don’t intend to have cross-procedural (“across methods”) analysis for this rule. Here is why:

SonarQube 7.3 provides two new kind of rules: Security Hotspots, such as the one you mention (S4817), and Vulnerability rules using a taint analysis engine.

  • Vulnerability rules using a taint analysis engine raise issues when they are quite sure that there is a vulnerability. Those rules avoid as much as possible false positives, which means that they will also miss some real vulnerabilities. For example rule S2091 does cross-procedural analysis and would not raise any issue in your example.
  • Security Hotspot rules are raising issues whenever a piece of code needs to be reviewed, it covers cases where the Vulnerability rules will not raise issues. These rules are simpler and do not do cross-procedural analysis. We are however working at making the review process as easy as possible.

Security Hotspots are not supposed to impact the quality gate. They are only meant to be reviewed and turned to Vulnerabilities if need be. However the review process is only available since SonarQube 7.3. In SonarQube 6.7, security hotspot rules exist as Vulnerability rules but are not activated in the “SonarWay” Quality Profile. This was necessary for backward compatibility.

If detecting XPATH/SQL/… injections is your priority I would suggest to update your SonarQube instance. Note that the taint analysis engine is not available in the Community Edition. Security Hotspots are however available in every SonarQube edition and will not impact your quality gate. You will be free to review them at your leisure.

If you prefer to remain on SonarQube 6.7 I would recommend to disable the rule as it will create false positives in your case.

Don’t hesitate to share more feedbacks or ask questions.

Cheers,
Nicolas