Help SonarCloud with understanding the usage of untrusted and tainted input

[FreddyGroen]

Hey all,

I’m a bit in the dark on how I can help SonarCloud understanding that we are actually validating/whitelisting user input so we can rid of some blocker issues and get a nice pretty rating on our project.

As an example, we have a C# application with some MVC controllers that delete resources from a folder managed by the application. We do a lot of validation on the input and make quite some effort on generating a path that is a valid and safe path.
Here we get a ‘Refactor this code to not construct the path from tainted, user-controlled data.’

Another example are some actions that have a redirectUrl as parameter. For example in a Login action.
SonarCloud gives me a nice ‘Refactor this code to not perform redirects based on tainted, user-controlled data.’ issue, but all code is doing checks to make sure the input we receive is safe.

How can I let SonarCloud know about this? Do any of you guys have some suggestions maybe?
Marking these issues as a false positive feels not right.

Best regards,
Freddy

[Alexandre_Gigleux]

Hello Freddy,

First, sorry for the delay to get back to you, I completely missed your message.

As of today, it’s not possible to tell to SonarCloud that you have custom methods that are there to sanitize your data flow. In the background of SonarCloud, there is our taint analyzer that is relying on configuration files to decide if your data flow is safe and that no tainted data can reach a security-sensitive API that could be used by a hacker. As of now, these configurations can’t be changed by SonarCloud users while this is possible if you are using SonarQube Enterprise Edition. See https://docs.sonarqube.org/latest/analysis/security_configuration/
Maybe we will provide this feature on SonarCloud in the future, let’s see. Meanwhile, I think the best would be to share some code samples showing how you are sanitizing your data flow so we can look at them and update if possible, the embedded/default configuration.

Regards

[FreddyGroen]

Hi @Alexandre_Gigleux,

No problem about the delay, that is somewhat expected during the summer

I have some examples, not the best. But gives an idea of the items SonarCloud found in our code:

[HttpGet]
public ActionResult ShowDocument(string plugin, string contentId)
{
	// plugin is the source, the rest propagates the value..
	var data = GetPreviewMetadata(plugin, contentId);
	..
    // logic that creates a nice model in case data is null
	..
	return View(model);
}

private PreviewMetadata GetPreviewMetadata(string plugin, string contentId)
{
	// check if plugin is known to us and contentId is valid
	var metadata = PreviewHelper.CheckPlugin(plugin, contentId);
	if (metadata == null)
	{
		return null;
	}

	// this is the culprit, the preview helper is doing some caching on the filesystem
	// it creates a directory where plugin used in the name.
	// if the plugin value was invalid, we could not get to this line
	var previewHelper = new PreviewHelper(plugin, contentId, metadata);
	return previewHelper.GetPreviewMetadata(plugin, contentId);
}

We have a few similar situations, where we validate user input, sanitize and create a temporary folder/file on the filesystem. In a few cases we show the user an error message.

I’m curious how we can help SonarCloud understanding we handled the user input?

The rule in SonarCloud says the following is complaint:

// Restrict the username to letters and digits only
if (!Regex.IsMatch(user, "^[a-zA-Z0-9]+$"))
{
    return BadRequest();
}

Is this the only pattern it detects? We have a somewhat similar piece of code, but it throws an exception.

    public static string Combine(string path1, string path2)
    {
        if (Path.IsPathRooted(path2))
        {
            throw new ArgumentException("Invalid", nameof(path2));
        }
        return Path.Combine(path1, path2);
    }

And what about logic that cannot return a BadRequest() but just serves a nice explanation to the user?

Best regards,
Freddy

[Alexandre_Gigleux]

Hello,

The fact that a method is returning a BadRequest() or an ArgumentException is not impacting the results. We are looking for potential execution path between a user input (Source) and a Sink (here the Sink is the creation of the temporary folder/file). If there is a call to a method that we consider as a Sanitizer, then we don’t raise an issue.

If I’m correct your feedback is about the rule S2083 I/O function calls should not be vulnerable to path injection attacks.
For S2083, we don’t consider Path.IsPathRooted as a Sanitizer, that’s why you have these FPs. I need to investigate if we can add it in our default list.

=> Can you provide a reproducer involving a similar code as the one that you have in the PreviewHelper.CheckPlugin?

For your feedback about Regex.IsMatch(user, "^[a-zA-Z0-9]+$"), we consider System.Text.RegularExpressions.Regex.IsMatch(string, string) as a Sanitizer for all C# rules. This Regex.IsMatch is part of the Compliant Solution we are providing in the S2083 rule description. So normally, you should not have any issue involving this piece of code as an execution step. If this is the case, we have a problem that we should investigate.

=> Can you confirm the Regex object is coming from System.Text.RegularExpressions.Regex?
=> Can you provide a simple reproducer project?

Thanks

[Alexandre_Gigleux]

Hello @FreddyGroen,

Did you manage to find a way to build a reproducer so we can work on it?

Thanks

[FreddyGroen]

Hi Alexandre,

I have an example solution, how do you want it? I’m not allowed to attach it here. Or I can paste the content here…

Best regards,
Freddy

[lrozenblyum]

Hello @Alexandre_Gigleux.
Sorry for resurrecting this old post, but here in 2021 in Java we also face a similar issue:
we execute a validation before paths usage but still SonarCloud complains about security:S2083.

Here is a code example

private Path filePath(final String fileName) { //fileName is user controlled
	Path parentPath = Paths.get(notUserControlledParameters);
	Path requestedFilePath = Paths.get(notUserControlledParameters, fileName);
	// security check against hacks like passing '../filename'
	if ( ! parentPath.equals( requestedFilePath.getParent() ) ) {
		throw new IllegalArgumentException( .... );
	}
	return requestedFilePath;
}

Is FileUtils.directoryContains the only sanitizer possible here for file operations or can others be used? In this particular case we cannot use it because the directory and the file might be not existing at the moment of this code execution.

[Hendrik_Buchwald]

Hello @lrozenblyum ,

please create a new topic for this problem, thanks!