Code seems to be wrong in compliant solution

SonarCloud
, ,
#1

Template for a good bug report, formatted with Markdown:

  • Versions used (SonarQube, Scanner, Plugin, and any relevant extension)
  • Error observed (wrap logs/code around triple quote ``` for proper formatting)
  • Steps to reproduce
  • Potential workaround
  • Scanner command used when applicable (private details masked)
  • In case of SonarCloud:
    • ALM used (GitHub, Bitbucket Cloud, Azure DevOps)
    • CI system used (Bitbucket Cloud, Azure DevOps, Travis CI, Circle CI, Jenkins, other)

Why is this an issue →
I/O function calls should not be vulnerable to path injection attacks

 public class RSPEC2083IOInjectionCompliantController : Controller
    {
        public IActionResult Index()
        {
            return View();
        }

        public IActionResult DeleteFile(string fileName)
        {
            string destDirectory = "~/CustomersData/";

            string destFileName = Path.GetFullPath(System.IO.Path.Combine(destDirectory, fileName));
            string fullDestDirPath = Path.GetFullPath(destDirectory + Path.DirectorySeparatorChar);

            if (!destFileName.StartsWith(fullDestDirPath, StringComparison.Ordinal)) <--- Should remove ! in front of destFileName
            {
                System.IO.File.Delete(destFileName); // Compliant
                return Content("File " + fileName + " deleted");
            } else
            {
                return BadRequest();
            }
        }

    }
#2

Hi @jeff-yaeger ,

Welcome to the community!

You copied the template for a good bug report, but then did not fill in any of the details. It’s important for us to know as much as possible about your environment to help with relevant solutions.

As for your code: what happens when you pass a filename such as “…/…/foo.sys”? Now you’re outside of the application directory and can delete any file you wish, assuming the application runs with enough privileges to delete that file.

#3

Hi @cba ,

I’m pretty sure this should read:

if (destFileName.StartsWith(fullDestDirPath, StringComparison.Ordinal)) 
            {
                System.IO.File.Delete(destFileName); // Compliant
                return Content("File " + fileName + " deleted");
            } else
            {
                return BadRequest();
            }

Because you want to validate that the path you are creating starts with the destination path you expect.
The way it’s written currently, it would return a BadRequest for having the correct file path.
Just wanted to point that out.

#4

Hi @jeff-yaeger ,

Are you 100% sure that Path.GetFullPath will always collapse a path like C:\app\CustomersData\..\..\Windows\foo.sys into C:\Windows\foo.sys ? Even if someone has fun e.g. with UTF8 characters for directory traversal?

In a string comparison, C:\app\CustomersData\..\..\Windows\foo.sys still starts with the correct path, so will still delete the system file. That’s all that SonarQube is trying to tell you - review your code as there may be unintended side effects. If you’re confident that your code is secure, then you can resolve this issue as “Won’t fix”.

#5

Hi @cba, that’s not what I’m trying to say at all. I understand the reason for checking the full path. I’m saying with the example code put out by sonar cube, it sends you to the wrong result. If the path is correct, it returns a BadRequest, if the path doesn’t start with the fullDestDirPath(i.e. the path is wrong), it deletes and returns a message saying the file is deleted.

#6

Ah I see. That’s where filling out the template details would have helped :wink:

Which version of SonarQube is this? Or is this SonarCloud? And what is the rule name? I’ll flag this post so that some of the more experienced folks can take a closer look.

#7

@cba Your right, I should have used the template. This is sonar cloud. The name of the rule is “I/O function calls should not be vulnerable to path injection attacks”.

#10

Hi @jeff-yaeger and welcome to the community.

Thanks for reported this problem.
The change should be visible in a few weeks.

#12

This topic was automatically closed 7 days after the last reply. New replies are no longer allowed.