New rule S6444: RegEx evaluation should have a time out specified

[Corniel]

See: community.sonarsource.com,

And RSPEC PR: SonarSource/rspec#1188

Description

When using System.Text.RegularExpressions to process untrusted input, pass a
timeout.
A malicious user can provide input to RegularExpressions causing a
Denial-of-Service attack.

Noncompliant Code Example

var emailPattern = new Regex(".+@.+", RegexOptions.None);
var isNumber = Regex.IsMatch(input, "[0-9]+");

Dim emailPattern As New Regex(".+@.+", RegexOptions.None)
Dim isNumber as Boolean = Regex.IsMatch(input, "[0-9]+")

Compliant Solution

var emailPattern = new Regex(".+@.+", RegexOptions.None, TimeSpan.FromMilliseconds(1));
var isNumber = Regex.IsMatch(input, "[0-9]+", RegexOptions.None, TimeSpan.FromMilliseconds(1));

Dim emailPattern As New Regex(".+@.+", RegexOptions.None, TimeSpan.FromMilliseconds(1))
Dim isNumber as Boolean = Regex.IsMatch(input, "[0-9]+", RegexOptions.None, TimeSpan.FromMilliseconds(1))

Labels: security vulnerability
Message: Pass a timeout to limit the execution time.
Default Severity: Major
Impact: Low
Likelihood: Medium
Default Quality Profiles: Sonar way
Covered Languages: C#, VB.Net
Remediation Function: ?
Constant Cost: 5min
Analysis Level: ?
Syntactic Analysis Analysis Scope: Test Sources

[martin-strecker-sonarsource]

@Corniel Thank you for the contribution. Some notes for the review.

That can be the constructor or one of the static match methods.

I reviewed the documentation, and @Corniel is right.

• Any regex instance has a fixed timeout passed in the ctor that can not be changed or overridden in the instance match methods.
• Every static method has an overload with a timeout parameter.
• The list of match methods @Corniel provided is also complete.

There is a global setting on the App Domain to set the default timeout. If not specified, the default timeout is Regex.InfiniteMatchTimeout (Source). Users can opt-out of the rule manually if they use that mechanism.

We should also test the regex compilation scenarios (CompileToAssembly, RegexOptions.Compiled) and the new source generator. I don't expect any problems here, but we should test it.

[Corniel]

There is a global setting on the App Domain to set the default timeout (..)
Unfortunately we can not detect that on assembly level. I think that people who prefer to have this set on App Domain level, should turn this rule off.

I do not see what the RegexOptions.Complied setting should change; that is a one-time penalty that has no effect on the match time-out setting, but maybe I missed something?

[Corniel]

@martin-strecker-sonarsource I consider also adding a code fix. Should not be that hard in this case.

[Corniel]

@martin-strecker-sonarsource Any timeline for progressing this?

[martin-strecker-sonarsource]

@Corniel Sorry for the late response. The team looked into this, and we are eager to see this new rule added to our suite! I'm going to work on your RSpec PR first and get it approved. I will do the review of this PR afterward. Thanks for your patience.

Can you please add a link to the RSpec PR in the PR description? SonarSource/rspec#1061

[Corniel]

@martin-strecker-sonarsource I added the detection of the Regex.NonBacktracking flag. I did not try to only detect it for .NET 7 only. I don't think adding that check is worth the added complexity.

[martin-strecker-sonarsource]

@Corniel This looks really good so far. I want you to use MethodParameterLookup to find the right ArgumentSyntax for the parameters in question. See my comments for details.

[martin-strecker-sonarsource]

Looks good now. Just some minor code style issues. One question remains though: Should we look up the parameters options and matchTimeout by name or by type? I will discuss this internally and come back to you.

[martin-strecker-sonarsource]

Some minor clean-ups, and we decided to go for the parameter.Name for the parameter lookup.

[martin-strecker-sonarsource]

/azp run Sonar.Net

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[martin-strecker-sonarsource]

/azp run Sonar.Net

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[martin-strecker-sonarsource]

Hi @Corniel,

Our continuous integration pipeline revealed, that your rule throws an System.InvalidOperationException: Nullable object must have a value. in some cases. The detailed error message is

error AD0001: Analyzer 'SonarAnalyzer.Rules.CSharp.SpecifyTimeoutOnRegex' threw an exception of type 'System.InvalidOperationException' with message 'Nullable object must have a value.'.

It seems the exception is thrown in SpecifyTimeoutOnRegexBase.cs:line 68.

The exception happens because of code somewhere in this code file:
https://github.com/SonarSource/sonar-dotnet/blob/master/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/RuleFailure/SpecialCases.cs

You can execute this test to reproduce the error:

sonar-dotnet/analyzers/tests/SonarAnalyzer.UnitTest/Common/RuleTest.cs

Lines 45 to 53 in 2906227

	[TestMethod]
	public void RulesDoNotThrow_CS()
	{
	var analyzers = RuleFinder.CreateAnalyzers(AnalyzerLanguage.CSharp, true).ToArray();
	VerifyNoExceptionThrown(@"TestCases\RuleFailure\InvalidSyntax.cs", analyzers, CompilationErrorBehavior.Ignore);
	VerifyNoExceptionThrown(@"TestCases\RuleFailure\SpecialCases.cs", analyzers, CompilationErrorBehavior.Ignore);
	VerifyNoExceptionThrown(@"TestCases\RuleFailure\PerformanceTestCases.cs", analyzers, CompilationErrorBehavior.Ignore);
	}

The same error occurs for VB too. Here you need to execute RulesDoNotThrow_VB.

Can you please have a look? If you need some help, please don't hesitate to ask. The full error message is:

Failed RulesDoNotThrow_CS [1 s]
  Error Message:
   Expected diagnostics {SpecialCases.cs(1,1): warning S1451: Add or update the header of this file., SpecialCases.cs(14,46): warning S109: Assign this magic number '42' to a well-named (variable|constant), and use the (variable|constant) instead., SpecialCases.cs(12,21): warning S1186: Add a nested comment explaining why this method is empty, throw a 'NotSupportedException' or complete the implementation., SpecialCases.cs(13,67): warning S109: Assign this magic number '42' to a well-named (variable|constant), and use the (variable|constant) instead., SpecialCases.cs(13,20): warning S2325: Make 'MethodExpressionBody' a static method., SpecialCases.cs(3,1): warning S1128: Remove this unnecessary 'using'., SpecialCases.cs(4,1): warning S1128: Remove this unnecessary 'using'., SpecialCases.cs(5,1): warning S1128: Remove this unnecessary 'using'., SpecialCases.cs(30,34): warning S4200: Make this native method private and provide a wrapper., SpecialCases.cs(10,18): warning S4050: Provide an implementation for: 'operator==', 'operator!=', 'Object.Equals' and 'Object.GetHashCode'., SpecialCases.cs(26,44): warning S4069: Implement alternative method 'Add' for the operator '+'., SpecialCases.cs(50,50): warning S4049: Consider making method 'GetDict' a property., SpecialCases.cs(14,20): warning S2325: Make 'PropertyExpressionBody' a static property., SpecialCases.cs(20,21): warning S2325: Make 'DynamicMethod' a static method., SpecialCases.cs(16,21): warning S2190: Add a way to break out of this method's recursion., SpecialCases.cs(40,28): warning S2190: Add a way to break out of this method's recursion., SpecialCases.cs(30,34): warning S4214: Make this 'P/Invoke' method private or internal., SpecialCases.cs(22,21): warning S1481: Remove the unused local variable 'local'., error AD0001: Analyzer 'SonarAnalyzer.Rules.CSharp.SpecifyTimeoutOnRegex' threw an exception of type 'System.InvalidOperationException' with message 'Nullable object must have a value.'.
Exception occurred with following context:
Compilation: project0
SyntaxTree: SpecialCases.cs
SyntaxNode: __arglist("") [InvocationExpressionSyntax]@[511..524) (17,26)-(17,39)

System.InvalidOperationException: Nullable object must have a value.
   at System.Nullable`1.get_Value()
   at SonarAnalyzer.Rules.SpecifyTimeoutOnRegexBase`1.<Initialize>b__6_1(SyntaxNodeAnalysisContext c) in C:\ProgramData\vsts-agent\1\s\analyzers\src\SonarAnalyzer.Common\Rules\SpecifyTimeoutOnRegexBase.cs:line 68
   at SonarAnalyzer.Helpers.DiagnosticAnalyzerContextHelper.<>c__DisplayClass1_0`1.<RegisterSyntaxNodeActionInNonGenerated>b__0(SyntaxNodeAnalysisContext c) in C:\ProgramData\vsts-agent\1\s\analyzers\src\SonarAnalyzer.Common\Helpers\DiagnosticAnalyzerContextHelper.cs:line 44
   at SonarAnalyzer.Helpers.SonarAnalysisContext.<>c__DisplayClass47_0`1.<RegisterContextAction>b__0(TContext c) in C:\ProgramData\vsts-agent\1\s\analyzers\src\SonarAnalyzer.Common\Helpers\SonarAnalysisContext.cs:line 245
   at Microsoft.CodeAnalysis.Diagnostics.AnalyzerExecutor.<>c__62`1.<ExecuteSyntaxNodeAction>b__62_0(ValueTuple`2 data)
   at Microsoft.CodeAnalysis.Diagnostics.AnalyzerExecutor.ExecuteAndCatchIfThrows_NoLock[TArg](DiagnosticAnalyzer analyzer, Action`1 analyze, TArg argument, Nullable`1 info)
-----

[Corniel]

I wrongly assumed that in case, there would always be an identifier token. I added a .GetValueOrDefault() to fix that.

That being said, I think it is also worth to add:
LiteralExpressionSyntax x => x.Token
To the NodeIdentifier() logic. In the case that failed (__arglist()), it might be a good idea to return the token __arglist. But I'm not sure about it, what do you think?

[martin-strecker-sonarsource]

LiteralExpressionSyntax x => x.TokenToTheNodeIdentifier() logic. In the case that failed (__arglist()), it might be a good idea to return the token __arglist. But I'm not sure about it, what do you think?

@pavel-mikula-sonarsource What is your stance on this?

You can certainly do

        public static SyntaxToken? NodeIdentifier(this SyntaxNode node) =>
            node.RemoveParentheses() switch
            {
                AttributeArgumentSyntax x => x.NameColon?.Name.Identifier,
                BaseTypeDeclarationSyntax x => x.Identifier,
                // ..
                InvocationExpressionSyntax x => x.Expression is LiteralExpressionSyntax { RawKind: (int)SyntaxKind.ArgListExpression } literal
                    ? literal.Token
                    : NodeIdentifier(x.Expression),
                // ..
                _ => null,
            };

Background:
In MethodWitoutIdentifier(__arglist("")); the __arglist("") part is parsed as invocation expression with InvocationExpression.Expression being a LiteralExpressionSyntax and the LiteralExpressionSyntax.Token is a KeywordToken. NodeIdentifier returns null for this invocation at the moment, while "__arglist" would be expected.

I like the idea to treat this obscure invocation as every other invocation. On the other hand:

• __arglist is not an identifier. It is a keyword here.
• It would be the only branch that returns something that is not an IdentifierToken which is also somewhat unexpected given the method name (NodeIdentifier).
• __arglist("") is not an invocation, even though it is parsed as such (you can tell from the IL generated).

[martin-strecker-sonarsource]

Small typo.

[martin-strecker-sonarsource]

/azp run Sonar.Net

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[martin-strecker-sonarsource]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[martin-strecker-sonarsource]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[martin-strecker-sonarsource]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[martin-strecker-sonarsource]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[martin-strecker-sonarsource]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[martin-strecker-sonarsource]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

100.0% Coverage
0.0% Duplication

[martin-strecker-sonarsource]

@Corniel Big kudos to you for implementing this great rule and for your patience. It's much appreciated.

[martin-strecker-sonarsource]

Peach validation

• About 1141 issues found (some are duplicates in the HTML file)
• Random sample check shows only true positives

csharpsquid.S6444 2022-12-16 12.40.txt <- change file extension to html