16

I am validating date using regex in JavaScript but when I run SonarQube for code analysis. It is showing the regex as a security vulnerability.

Example 1:

Below is the regex pattern (link to source of regex https://stackoverflow.com/a/15504877/13353721):

^(?:(?:31(\/|-|\.)(?:0?[13578]|1[02]))\1|(?:(?:29|30)(\/|-|\.)(?:0?[13-9]|1[0-2])\2))(?:(?:1[6-9]|[2-9]\d)?\d{2})$|^(?:29(\/|-|\.)0?2\3(?:(?:(?:1[6-9]|[2-9]\d)?(?:0[48]|[2468][048]|[13579][26])|(?:(?:16|[2468][048]|[3579][26])00))))$|^(?:0?[1-9]|1\d|2[0-8])(\/|-|\.)(?:(?:0?[1-9])|(?:1[0-2]))\4(?:(?:1[6-9]|[2-9]\d)?\d{2})$

Example 2:

For floating value I have used the below regex 

^\d{1,5}(?:\.\d{1,5})?$

SonarQube is throwing the same security error, I tried various different regex patterns, but it is not working.

sepp2k
  • 363,768
  • 54
  • 674
  • 675
rahul
  • 406
  • 1
  • 5
  • 14
  • 1
    As per [this Sonarsource documenation](https://rules.sonarsource.com/java/RSPEC-4784), *This rule flags any execution of a hardcoded regular expression which has at least 3 characters and at least two instances of any of the following characters: `*+{.`*. So, you must make sure your pattern complies with the rule. – Wiktor Stribiżew Apr 28 '20 at 11:23
  • @WiktorStribiżew For date I used momentjs it is working now, but float not working,what changes I have to make can you please suggest? – rahul Apr 28 '20 at 13:45
  • 1
    Well, judging by the error description try building it via variables, `var d = "\\d{1,5}"; var float_rx = new RegExp("^" + d + "(?:\\." + d + ")?$")` – Wiktor Stribiżew Apr 28 '20 at 13:48
  • @WiktorStribiżew Thank you for the reply, If I create like this var d = "^\d{1,5}(?:\.\d{1,5})?$"; var float_rx = new RegExp(d) also it is working I am not getting how this is working. – rahul Apr 28 '20 at 15:12

1 Answers1

35

Hotspot vs. Vulnerability

First of all note that SonarQube is informing you about a security hotspot, not a vulnerability. What that means is (quoting from the docs):

A Security Hotspot highlights a security-sensitive piece of code that the developer needs to review. Upon review, you'll either find there is no threat or you need to apply a fix to secure the code.

[...]

With a Hotspot, a security-sensitive piece of code is highlighted, but the overall application security may not be impacted. It's up to the developer to review the code to determine whether or not a fix is needed to secure the code.

The important takeaway here is that SonarQube is not telling you that there is something wrong. It's telling you that you should look carefully at the code to determine whether something is wrong.

In other words it's telling you that your regex may be vulnerable to ReDoS attacks, not that it actually is. If you review the code and determine that there is no vulnerability, it is perfectly fine to just dismiss the issue without changing anything.

So why exactly is SonarQube telling you to review this code?

SonarQube doesn't actually detect whether a regular expression is vulnerable to ReDoS or not (that's why it's labelled as a security hotspot, not a vulnerability). Instead it flags all non-trivial regular expressions and reminds you to review them to determine whether they're vulnerable or not. As explained in the documentation of the rule, it considers any regex as non-trivial that contains more than one occurrence of any of the characters *+{.

Since both of your regular expressions are non-trivial by that criteria, both are flagged.

Update: The above applies to the ReDoS rule that was around when this answer was written. This rule has been deprecated in the mean time and replaced with a new rule that should only complain about regular expressions that actually have a superlinear runtime. The new rule does not complain about the regex in this question.

So is your code vulnerable?

No, neither of your regular expressions are vulnerable. In fact the only repetition operator used in either expression is {} and since you provide an upper bound in all cases, there isn't even any unbounded repetition.

However, I'd say your first regex is complicated enough to be a readability and maintenance nightmare. So you should consider replacing it with another approach (such as splitting the string into individual numbers and checking that each number is in the desired range).

So what should you do?

Having determined that the regular expressions are not vulnerable, you should dismiss the hotspot.

In the comments it was pointed out, that the message will go away if you split the regex string into multiple concatenated strings or move it into a variable. The reason that works is simply that it tricks SonarQube into not finding the regex. So that change wouldn't make your code any better or safer, it would just confuse SonarQube and is in no way preferable to just dismissing the message. It is generally not advisable to obfuscate your code just to make your static analysis tools shut up.

sepp2k
  • 363,768
  • 54
  • 674
  • 675