javasecurity:S5145 unclear example compliant solution

[patrik.jetzer]

Hi, We need further information about fixing issues reported by the rule

javasecurity:S5145 Change this code to not log user-controlled data.
Logging should not be vulnerable to injection attacks**

The example compliant solution shows some replace call on the string
data = data.replaceAll(“[\n\r]”, “_”);
but what is the rule exactly looking for accepting input to be sanitized or validated?
To understand what’s going on, I was looking for the source code of the rule, but could not find it.

Can you therefore

• Provide more information what exactly the rules is looking for accepting input to be sanitized or validated
• Provide relevant source code of the rule

Thanks

• What language is this for: java
• Which rule: javasecurity:S5145 Change this code to not log user-controlled data.
• We are using
  • SonarQube: Enterprise Edition Version 9.9.1 (build 69595)

[Pierre-Loup_Tristant]

Hi @patrik.jetzer,

@patrik.jetzer

what is the rule exactly looking for accepting input to be sanitized or validated?

The rule expects the inputs to be free of newline characters before being logged.

@patrik.jetzer

Can you therefore

• Provide more information what exactly the rules is looking for accepting input to be sanitized or validated
• Provide relevant source code of the rule

Can I ask you to explain what use case you expect the rule to cover instead?

[Thomas_Mauch]

Hi

As part of a security analysis of our application we investigate whether the provided Sonar security rules can be applied.
The concept of

javasecurity:S5145 Change this code to not log user-controlled data

sounds promising, but it seems that the provided implementation only focuse on the replacement of newlines.

It also seems that a compliant solution must use the slow replaceAll() call which constructs a regex on each call,
but more performant solutions are not accepted:

accepted:
str = str.replaceAll(“[\n\r]”, “_”);
not accepted:
str = StringUtils.replaceChars(str, “\n\r”, “__”);

What exact replacement calls are accepted by the rule?

Where can I find the source code so we could eventually come up with a custom rule which suits our needs better?

Many thanks,
Thomas

[Pierre-Loup_Tristant]

Hi @Thomas_Mauch,

Are you facing the same problem @patrik.jetzer reported?

@Thomas_Mauch

not accepted:
str = StringUtils.replaceChars(str, “\n\r”, “__”);

If StringUtils is org.apache.commons.lang3.StringUtils then yes this should be part of the methods we consider as sanitizers for this rule. I created an improvement ticket so that it’s fixed in a future version of SonarQube.

@Thomas_Mauch

Where can I find the source code so we could eventually come up with a custom rule which suits our needs better?

You can’t this component is closed source as it’s part of our commercial editions of the products.
But you can customize what the rule considers a sanitizer for this rule using a custom configuration.

[Thomas_Mauch]

Thanks the prompt response, I will check the custom configuration you mentioned.

[Thomas_Mauch]

I have looked now at the
Java JSON file example
in

Security engine custom configuration

Unfortunately, the format of the JSON file seems not complete.

• can you provide a formal definition of methodId?
  (it seems to be a mixture of textual and bytecode signature, but how would e.g. a non-top level class be referenced, with ‘.’ or ‘$’)

• The following keys are not documented:

“isWhitelist”: true
“isMethodPrefix”: true
“interval”: {
“fromIndex”: 1
}

• The description of “args” is referring to “function call” - what are you considering a function call, IMHO Java just has methods

“The args is the index of the parameter that can receive a tainted variable. Index starts:
1 for a function call.”

Many thanks,
Thomas

[Pierre-Loup_Tristant]

Hi @Thomas_Mauch,

Something like this should work:

    {
      "methodId": "org.apache.commons.lang.StringUtils#replaceChars((Ljava/lang/String;Ljava/lang/String;",
      "isMethodPrefix": true,
      "args": [
        1
      ]
    },
    {
      "methodId": "org.apache.commons.lang3.StringUtils#replaceChars((Ljava/lang/String;Ljava/lang/String;",
      "isMethodPrefix": true,
      "args": [
        1
      ]
    }

[Thomas_Mauch]

Are you sure about the double parentheses?

"org.apache.commons.lang.StringUtils#replaceChars(("

can you explain what’s the meaning of isMethodPrefix?
and does this JSON object has to appear twice identically in the file?

Thanks

As said, a complete description of the format would help?

[Pierre-Loup_Tristant]

Hi Thomasm

@Thomas_Mauch

Are you sure about the double parentheses?

Sorry, the double parenthesis was a typo. Here is the right configuration to make replaceChars a sanitizer for S5145:

{
  "S5145": {
    "sanitizers": [
      {
      "methodId": "org.apache.commons.lang.StringUtils#replaceChars(Ljava/lang/String;Ljava/lang/String;",
      "isMethodPrefix": true,
      "args": [
        1
      ]
    },
    {
      "methodId": "org.apache.commons.lang3.StringUtils#replaceChars(Ljava/lang/String;Ljava/lang/String;",
      "isMethodPrefix": true,
      "args": [
        1
      ]
    }
    ]
  }
}

@Thomas_Mauch

can you explain what’s the meaning of isMethodPrefix?

When this is set to true it will match any method signature that starts with what is specified in the methodId field. if not set, or set to false it will match the exact method signature instead.

@Thomas_Mauch

and does this JSON object has to appear twice identically in the file?

They are not identical. If you look carefully you will see that the package name is slightly different.

@Thomas_Mauch

As said, a complete description of the format would help?

Tthe Security engine custom configuration documentation is the only resource I can share. If you encounter more problems please let us know.

Regards,